[Insight-developers] found sane.c offending line

Thu Mar 4 11:17:18 EST 2010

Sounds good!

I have attached the fix as a patch to make things easier.

Thanks!
Brad

On Mar 4, 2010, at 11:07 AM, Michel Audette wrote:

> Hi gents, 
> 
> I'll have a look at Brad's suggested code, and look into committing it today or tomorrow. I need to put into Sourceforge as well, right? 
> 
> Okay by everyone?
> 
> Cheers,
> 
> Michel
> 
> On Thu, Mar 4, 2010 at 11:04 AM, Gordon L. Kindlmann <glk at uchicago.edu> wrote:
> Hi-
> 
> On Mar 4, 2010, at 9:11 AM, Bradley Lowekamp wrote:
> 
> Hello,
> 
> 
> I am glad we got on the same page!
> 
> indeed!
> 
> 
> The CMake variable uses a TRY_RUN command to determine the endianness. This does now work because the single executable contains instructions for multiple architectures. The solution is to remove the command line defines from the CMakeLists.txt file and add the following based on itkConfigure.h.in to NrrdConfigure.h.in
> 
> 
> /* what byte order */
> /* All compilers that support Mac OS X define either __BIG_ENDIAN__ or
>   __LITTLE_ENDIAN__ to match the endianness of the architecture being
>   compiled for. This is not necessarily the same as the architecture of
>   the machine doing the building. In order to support Universal Binaries on
>   Mac OS X, we prefer those defines to decide the endianness.
>   On other platform, we use the result of the TRY_RUN. */
> #if !defined(__APPLE__)
>  #if @CMAKE_WORDS_BIGENDIAN@
>    #define TEEM_ENDIAN 4321
>  #else
>    #define TEEM_ENDIAN 1234
>  #endif
> #else
>  #if defined(__BIG_ENDIAN__)
>    #define TEEM_ENDIAN 4321
>  #else
>    #define TEEM_ENDIAN 1234
>  #endif
> #endif
> 
> Ok, great, this looks like the right kind of clever.  It seems like this should be boilerplate for lots of things in ITK, right?
> 
> Michel, if you aren't sick of this, can you commit this fix to Insight/Utilities/NrrdIO's CMakeLists.txt?
> 
> 
> I have tested the above and it solves the bug, and there are no new itk test failures on my apple. The one complication in writing the code was the caution for defining "CMAKE_WORDS_BIGENDIAN". This is due to the fact that it may be defined else where (or in other libraries), and I didn't want to introduce any conflicts.
> 
> Overall this is a minor bug on one platform. But I always build my apple releases with support for all the apple architectures. It just makes it easy to deal and distribute when need, one binary or library.
> 
> I agree with this.
> 
> 
> I do not know the implication of this fix for the rest of the TEEM library or how best to integrate it.
> 
> All you're doing is improving the logic of how to set the TEEM_ENDIAN compile-time #define, in the context of compiling NrrdIO in ITK.  Teem has its own top-level CMakeLists.txt, but that's irrelevant for the NrrdIO update.  If Teem gets to the point of being compiled in this cross-platform way, then this fix should be propagated to that CMakeLists.txt, but not doing so will not cause any problems.
> 
> Thanks for this help,
> 
> Gordon
> 
> 
> 
> Thanks,
> Brad
> 
> 
> 
> 
> 
> On Mar 2, 2010, at 7:12 PM, Gordon L. Kindlmann wrote:
> 
> Hello,
> 
> On Mar 2, 2010, at 5:41 PM, Bradley Lowekamp wrote:
> 
> Hello,
> 
> Thanks for the further explanation. I have notice the Nan
> performance issue before but never looked into it.
> 
> What would you like to be done with about the following bug?
> 
> When a universal binary is build on an intel and then
> itkNrrdImageReadWriteTest11 is run on a ppc the following is at the
> end of the output:
> 
> victoria:InsightRelease blowek1$ arch -ppc /scratch/blowek1/build/
> InsightRelease/bin/itkIOTests --compare /nfs/mead/Users/blowek1/src/
> Insight/Testing/Data/Baseline/IO/vol-ascii.nrrd /scratch/blowek1/
> build/InsightRelease/Testing/Temporary/vol11.nrrd
> itkNrrdImageReadWriteTest /nfs/mead/Users/blowek1/src/Insight/
> Testing/Data/Input/vol-gzip-little.nhdr /scratch/blowek1/build/
> InsightRelease/Testing/Temporary/vol11.nrrd
> exception in file reader
> 
> itk::ExceptionObject (0x1e6f280)
> Location: "virtual void itk::NrrdImageIO::ReadImageInformation()"
> File: /nfs/mead/Users/blowek1/src/Insight/Code/IO/itkNrrdImageIO.cxx
> Line: 264
> Description: itk::ERROR: NrrdImageIO(0x1e6ee40):
> ReadImageInformation: Error reading /nfs/mead/Users/blowek1/src/
> Insight/Testing/Data/Input/vol-gzip-little.nhdr:
> [nrrd] nrrdLoad: trouble reading "/nfs/mead/Users/blowek1/src/
> Insight/Testing/Data/Input/vol-gzip-little.nhdr"
> [nrrd] nrrdRead: trouble
> [nrrd] _nrrdRead: sanity check FAILED: have to fix and re-compile
> [nrrd] nrrdSanity: airSanity() failed: TEEM_ENDIAN is wrong
> 
> Ok, I didn't know that this was an existing bug in NrrdIO; I only
> thought the mac cross-build issue had to do with the NaN-related
> airSanity() check; sorry.  How long has this bug been known?
> 
> I guess the answer will come down to understanding how, in CMake, is
> the platform endienness being 1) learned, and 2) communicated to
> whatever asks to know it.
> 
> The solution will have to make sure that the endienness communicated
> to the compiler is the endienness of the intended execution platform,
> rather than the endienness of whatever happens to be compiling the
> code.  For some reason, in the case of NrrdIO, these two endiennesses
> are getting confused.
> 
> Gordon
> 
> 
> 
> 
> Exception detected while reading /nfs/mead/Users/blowek1/src/Insight/
> Testing/Data/Baseline/IO/vol-ascii.nrrd : itk::ERROR:
> NrrdImageIO(0x1e6eae0): ReadImageInformation: Error reading /nfs/
> mead/Users/blowek1/src/Insight/Testing/Data/Baseline/IO/vol-
> ascii.nrrd:
> [nrrd] nrrdLoad: trouble reading "/nfs/mead/Users/blowek1/src/
> Insight/Testing/Data/Baseline/IO/vol-ascii.nrrd"
> [nrrd] nrrdRead: trouble
> [nrrd] _nrrdRead: sanity check FAILED: have to fix and re-compile
> [nrrd] nrrdSanity: airSanity() failed: TEEM_ENDIAN is wrong
> Exception detected while reading /nfs/mead/Users/blowek1/src/Insight/
> Testing/Data/Baseline/IO/vol-ascii.nrrd : itk::ERROR:
> NrrdImageIO(0x1e6f700): ReadImageInformation: Error reading /nfs/
> mead/Users/blowek1/src/Insight/Testing/Data/Baseline/IO/vol-
> ascii.nrrd:
> [nrrd] nrrdLoad: trouble reading "/nfs/mead/Users/blowek1/src/
> Insight/Testing/Data/Baseline/IO/vol-ascii.nrrd"
> [nrrd] nrrdRead: trouble
> [nrrd] _nrrdRead: sanity check FAILED: have to fix and re-compile
> [nrrd] nrrdSanity: airSanity() failed: TEEM_ENDIAN is wrong
> <DartMeasurement name="BaselineImageName" type="text/string">vol-
> ascii.nrrd</DartMeasurement>
> 
> Brad
> 
> On Mar 2, 2010, at 6:18 PM, Gordon L. Kindlmann wrote:
> 
> Hello,
> 
> On Mar 1, 2010, at 9:12 AM, Bradley Lowekamp wrote:
> 
> Hello Gordon,
> 
> I agree that the endien-ness is was not the cause of the issue
> related to the nan. However, the handling of endien-ness is not
> absolutely correct for some builds on Apple systems. Specifically,
> when configuring ITK for a universal binary, the run time check
> performed to detect endian-ness may be wrong. As the latest version
> of OSX does not support the ppc architecture, the universal binaries
> may fall into obscurity shortly. However, is is still the best way
> to build portable releases for the apple. None the less, this issue
> should be able to be easily addressed.
> 
> http://www.cmake.org/Wiki/CMake_FAQ#How_do_I_build_universal_binaries_on_Mac_OS_X.3F
> http://en.wikipedia.org/wiki/Universal_binary
> 
> Basically, an apple universal binary is a single executable, library
> or bundle that supports multiple architectures. It is implemented as
> a argument to gcc which can specify multiple architectures i.e.
> ppc;i386;ppc64;x86_64 That is a single compilation will be building
> both big endian an little endian at the same time. In this case the
> command line definition is going to be wrong for one of the
> architecture types types.
> 
> To actually demonstrate this error one needs to build a universal
> binary on one architecture, and then run it on another. I have not
> figured away to do this with the nightly CDash system though.
> 
> Given that no one has reported this as a bug before, it is worth
> asking: Does the TEEM_ENDIAN matter for nrrdIO, aside from the
> sanity check? It is possible that the file intrinsically knows the
> endian-ness and all corrections correctly happen. In which case this
> issue is similar to the NAN issue does not actually matter for
> correct behavior of the nrrdIO library.
> 
> The airSanity() sanity check includes a check on endienness because
> lacking that check, nrrdIO will generate incorrect results on multi-
> byte values that were written on an architecture of one endienness,
> and read into an with architecture with a different endienness.
> 
> So yes, this absolutely matters for NrrdIO, because even though the
> file knows the endienness with which it was written, it is only the
> difference between the file's recorded endienness and the reading
> architectures endienness which triggers the byte ordering swap at
> read-
> time.  Corrections won't "happen" unless the reading archicture knows
> its endienness, and for the time being, that information is learned
> once at compile-time, rather than being re-learned at run-time for
> all
> the situations where endienness matters.
> 
> The issue with signalling-vs-quiet NaNs arises only because I've
> tried
> to be conscientious about only using quiet NaNs in Teem, to avoid any
> unexpected overhead triggered by signalling NaNs causing an OS-level
> exception.  Lacking an exception handler for this, operation would
> probably continued as usual.  On architectures (like Windows) that
> don't seem to consistently respect the signalling-ness of signalling
> NaNs, this aspect of the airSanity() check is turned off.  But this
> is
> at most a performance issue, and only in unusual circumstances (i.e.,
> the majority of a large dataset is all NaNs).  The endien-ness issue,
> in contrast, is a total show-stopper if its wrong.
> 
> Let me know if this raises more concerns or confusion than it
> answers;
> I'd be happy to try to explain further.
> 
> Gordon
> 
> 
> Brad
> 
> On Feb 26, 2010, at 12:13 PM, Gordon L. Kindlmann wrote:
> 
> Hello,
> 
> Endian-ness and qnan-hi-bit-ness are separate aspects of hardware,
> and
> right now the endien-ness is being correctly handled, and
> explicitly
> checked by airSanity().
> 
> In any case,, "if defined(__APPLE__)" seems to be a legit way of
> blocking out this code for now.
> 
> Gordon
> 
> On Feb 26, 2010, at 9:24 AM, Bradley Lowekamp wrote:
> 
> I believe the solution to the problem involves using code which is
> similar to that in itkConfigure.h.in into teemEndian.c.
> 
> From NrrdIO CMakeList:
> 
> # Set compiler flags for endian-ness.
> IF(CMAKE_WORDS_BIGENDIAN)
> ADD_DEFINITIONS(-DTEEM_ENDIAN=4321)
> ELSE(CMAKE_WORDS_BIGENDIAN)
> ADD_DEFINITIONS(-DTEEM_ENDIAN=1234)
> ENDIF(CMAKE_WORDS_BIGENDIAN)
> 
> from teemEndian.h:
> #ifndef TEEM_ENDIAN
> #  error TEEM_ENDIAN not defined, see architecture-specific .mk
> file
> or check compilation options
> #elif TEEM_ENDIAN == 1234
> #  /* okay, its little endian */
> #elif TEEM_ENDIAN == 4321
> #  /* okay, its big endian */
> #else
> #  error TEEM_ENDIAN not set to 1234 (little endian) or 4321 (big
> endian), see architecture-specific .mk file or check compilatio\
> n options
> #endif
> 
> From itkConfigure.h.in:
> /* what byte order */
> /* All compilers that support Mac OS X define either
> __BIG_ENDIAN__ or
> __LITTLE_ENDIAN__ to match the endianness of the architecture
> being
> compiled for. This is not necessarily the same as the
> architecture of
> the machine doing the building. In order to support Universal
> Binaries on
> Mac OS X, we prefer those defines to decide the endianness.
> On other platform, we use the result of the TRY_RUN. */
> #if !defined(__APPLE__)
> #cmakedefine CMAKE_WORDS_BIGENDIAN
> #ifdef CMAKE_WORDS_BIGENDIAN
>  #define ITK_WORDS_BIGENDIAN
> #endif
> #elif defined(__BIG_ENDIAN__)
> #define CMAKE_WORDS_BIGENDIAN
> #define ITK_WORDS_BIGENDIAN
> #endif
> 
> 
> 
> 
> 
> Thanks for investigating this.
> 
> If there isn't a way for the preprocessor to know "am I a cross-
> compiled mac", for the time being we can just use whatever pre-
> processor means is used to say "am I a mac", and then fix it a
> more
> refined way later.
> 
> Gordon
> 
> On Feb 25, 2010, at 3:54 PM, Michel Audette wrote:
> 
> Hi Brad,
> 
> as Gordon suspected, the line in sane.c which was causing your
> tests
> to fail is the one with #ifdef /#endif section within
> if (!( airFP_QNAN == airFPClass_f(AIR_NAN)
>     && airFP_QNAN == airFPClass_f(AIR_QNAN)
> #if !defined(_MSC_VER) || _MSC_VER < 1400 /* VS2005 converts
> SNAN to
> QNAN */
>     && airFP_SNAN == airFPClass_f(AIR_SNAN)
> #endif
>     && airFP_QNAN == airFPClass_d(AIR_NAN)
>     && airFP_QNAN == airFPClass_d(AIR_QNAN) )) {
> 
> return airInsane_AIR_NAN;
> }
> 
> On the cross-compiled Mac, I did a printf:
> airFP_QNAN 2 airFPClass_f(AIR_NAN) 2
> airFP_QNAN 2 airFPClass_f(AIR_QNAN) 2
> !ifdef airFP_SNAN 1 airFPClass_f(AIR_NAN) 2
> airFP_QNAN 2 airFPClass_f(AIR_NAN) 2
> airFP_QNAN 2 airFPClass_f(AIR_QNAN) 2
> 
> On Linux, I get:
> airFP_QNAN 2 airFPClass_f(AIR_NAN) 2
> airFP_QNAN 2 airFPClass_f(AIR_QNAN) 2
> !ifdef airFP_SNAN 1 airFPClass_f(AIR_NAN) 1
> airFP_QNAN 2 airFPClass_f(AIR_NAN) 2
> airFP_QNAN 2 airFPClass_f(AIR_QNAN) 2
> 
> Question is then: what is an appropriate preprocessor flag for
> cross-
> compiled Macs?
> 
> Best wishes,
> 
> Michel
> -- 
> Michel Audette, Ph.D.
> R & D Engineer,
> Kitware Inc.,
> Chapel Hill, N.C.
> 
> 
> 
> 
> 
> ========================================================
> Bradley Lowekamp
> Lockheed Martin Contractor for
> Office of High Performance Computing and Communications
> National Library of Medicine
> blowekamp at mail.nih.gov
> 
> 
> 
> 
> ========================================================
> Bradley Lowekamp
> Lockheed Martin Contractor for
> Office of High Performance Computing and Communications
> National Library of Medicine
> blowekamp at mail.nih.gov
> 
> 
> 
> 
> ========================================================
> Bradley Lowekamp
> Lockheed Martin Contractor for
> Office of High Performance Computing and Communications
> National Library of Medicine
> blowekamp at mail.nih.gov
> 
> 
> 
> 
> 
> 
> -- 
> Michel Audette, Ph.D. 
> R & D Engineer, 
> Kitware Inc.,
> Chapel Hill, N.C. 
> 

========================================================
Bradley Lowekamp  
Lockheed Martin Contractor for
Office of High Performance Computing and Communications
National Library of Medicine 
blowekamp at mail.nih.gov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100304/400edb7d/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nrrd-endian.patch
Type: application/octet-stream
Size: 1532 bytes
Desc: not available
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100304/400edb7d/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100304/400edb7d/attachment-0001.htm>