Re: [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 address@hidden 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
address@hidden