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

[Richard Guenther <rguenth@xxxxxxxxxxxxxxxxxxxxxxxxxxx>]

Jeffrey D. Oldham wrote:
> Richard Guenther wrote:
>
> > On Wed, 1 Sep 2004, Richard Guenther wrote:
> >
> >  
> >
> > > On Wed, 1 Sep 2004 oldham@xxxxxxxxxxxxxxxxxxxxxxx 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.