[Insight-developers] found sane.c offending line

[Michel Audette]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100304/41f04c7f/attachment.htm>