[pooma-dev] Re: [PATCH] Fix FileSetReader/Writer

[Jeffrey D. Oldham]

Richard Guenther wrote:

> Jeffrey D. Oldham wrote:
>
>> Richard Guenther wrote:
>>
>>> On Wed, 1 Sep 2004, Richard Guenther wrote:
>>>
>>>  
>>>
>>>> On Wed, 1 Sep 2004 oldham at sirius.codesourcery.com wrote:
>>>>
>>>>  
>>>>
>>>>>  regressions.io.filesetreadertest1             : FAIL
>>>>>    Unexpected exit_code, standard error.
>>>>>
>>>>>  regressions.io.filesetreadertest2             : FAIL
>>>>>    Unexpected exit_code, standard error.
>>>>>     
>>>>
>>>>
>>>> I finally figured out why these fail on ia32 (but not on amd64 and 
>>>> ia64).
>>>> The test data was appearantly generated on a 64bit big-endian host 
>>>> and the
>>>> reader just reads bytes and expects a C++ long to be 64bit everywhere
>>>> (which is not true obviously).
>>>>
>>>> There is also the POOMA_HAS_LONG_LONG define which is set nowhere
>>>> and used only in the FileSetReader/Writer and ElementProperties.
>>>>
>>>> We could check for an appropriate 64bit type during configure and
>>>> use that or just ignore the issue.
>>>>   
>>>
>>>
>>>
>>> Yay, and of course this is not enough, as required alignment for 64bit
>>> datatypes is of course different.  We should shoot the one that came up
>>> with
>>>
>>>  template <class T>
>>>  struct OffsetData
>>>  {
>>>    void reverseBytes();
>>>
>>>    int nodedata[6*Dim];  // domain data (same format as .layout)
>>>    bool isCompressed;    // Is the data compressed
>>>    Offset_t offset;      // offset in sizeof(T) units
>>>    T compressedValue;    // Data value, if compressed
>>>  };
>>>
>>> as possibly "portable" structure to write byte-for-byte to a file.
>>> Placing bool between int and long long is surely not a good idea.
>>>
>>> Changing the above to
>>>
>>>    int nodedata[6*Dim];  // domain data (same format as .layout)
>>>    union {
>>>      bool isCompressed;  // Is the data compressed
>>>      char pad[8];
>>>    } u;
>>>    Offset_t offset;      // offset in sizeof(T) units
>>>
>>> _seems_ to "fix" the problem.
>>>
>>> So, is the following patch ok?  Tested on ia32, amd64 and ia64 linux.
>>>
>>>  
>>>
>> This patch seems to contain two ideas: 1) add POOMA_INT64 and 2) 
>> changing OffsetData.  Ensuring use of 64-bit values seems to be a 
>> good idea.  I am unsure why the order of structure data members is 
>> important.  C++ compilers should obey the C++ ABI 
>> (http://www.codesourcery.com/cxx-abi/) so the structure will be laid 
>> out in the same way on all machines.
>
>
> I don't think so.  Fact is, that different compilers obey different 
> C++ ABIs, so the structure may be layed out differently on IRIX with 
> CC than on ia32 with g++.  I'm not trying to fix all possible 
> problems, but just the fact that all 64bit ABIs I know of force 
> alignment of 8-byte types
> (like Offset_t) on 8-byte boundaries.  The 32bit g++ ABI on ia32 though
> requires only 4-byte boundary for the 8-byte long long (or maybe it's a
> bug in g++).
>
> Of course properly fixing it for all cases would need a
> packed structure for the read/write with a byte-wise copy to a properly
> aligned structure for further use.  But that is beyond the patch. 
> Rather than doing that I'd bring over parts of my HDF5 support.
>
>> Does using just (1) solve the problem?
>
>
> No.  You'll get sizeof(OffsetData) of 96 for ia64 and 92 for ia32 with
> offset of offset being 80 for the one case and 76 for the other.
>
> I just tested the patch on a 32bit big-endian machine (ppc) and it 
> works there, too.
>
> Richard.

It is possible to change your patch to use an Offset_t instead of 
char[8]?  This seems a more portable way to align to 64 bits.

Regardless, please commit your patch.  I guess these two items will no 
longer fail in the nightly tests starting 03Sep.

Thanks for fixing this.

-- 
Jeffrey D. Oldham
oldham at codesourcery.com