[Insight-developers] found sane.c offending line

Thu Mar 4 11:55:52 EST 2010

Hi,

Thanks very much for the patch.

Yes, this should be committed to Sourceforge's NrrdIO too.

Gordon

On Mar 4, 2010, at 10:17 AM, Bradley Lowekamp wrote:

> Sounds good!
>
> I have attached the fix as a patch to make things easier.
>
> <nrrd-endian.patch>
>
> 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
>
>