On Wed, Jun 04, 2014 at 06:57:23AM -0700, John Marshall wrote:
> > -					(((unsigned char *)aux)[7]<<24));
> > +	    uint32_t count;
> > +	    if(is_big_endian())
> > +	    {
> > +	        count = (uint32_t)((((unsigned char *)aux)[7]<< 0) +
> > +				(((unsigned char *)aux)[6]<< 8) +
> > +				(((unsigned char *)aux)[5]<<16) +
> > +				(((unsigned char *)aux)[4]<<24));
> > +	    }
> > +	    else
> > +	    {
> > +	        count = (uint32_t)((((unsigned char *)aux)[4]<< 0) +
> > +				(((unsigned char *)aux)[5]<< 8) +
> > +				(((unsigned char *)aux)[6]<<16) +
> > +				(((unsigned char *)aux)[7]<<24));
> > +	    }
> 
> If `aux` points to data in host endianness, then the way to copy it
> to `count` is with `count = *(uint32_t *)&aux[4]`, or less appallingly
> with `memcpy`. 

`count = *(uint32_t *)&aux[4]` could do an uninitialised memory access
so it would need to be verbatim loaded.

The main question here is over how best to deal with byte swapping.
For io_lib I byte swap the main structure (ie bam1_core_t) during
loading (and back again just before saving) as there are lots of
things which historically access these directly rather than using
access functions/macros. I also byte swap the cigar array.

I don't however make any attempt to byte swap the auxiliary data as
it's complex and potentially unnecessary.  It could perhaps be
said that there are also going to be code which munges this directly,
but I obviously had the luxury of not caring. :-)

Htslib here differs as it calls swap_data which byte swaps the entire
auxiliary data too. (This is where my CRAM code failed as it wasn't
dealing with this correctly.) Arguably htslib's approach is correct
given that there will be code out there that munges aux manually, and
may not be as inefficient as it ininitially sounds[1].

My original code CRAM was carefully constructing the integer byte by
byte specifcally take make it endianness agnostic, but obviously
doesn't work due to the differences in in-memory representations.
Clearly this needs fixing, and I agree the fix is appropriate to put
into cram_decode.c.

It's probably best done with something like:

count = le_int4(ua_int4((uint32_t *)&aux[4]));

with:

le_int4 from os.h or

#ifdef BIG_ENDIAN
static inline le_int4(uint32_t x) {
    return (((x & 0x000000ff) << 24) + \
            ((x & 0x0000ff00) <<  8) + \
	    ((x & 0x00ff0000) >>  8) + \
	    ((x & 0xff000000) >> 24));
}
#else
static inline le_int4(int32_t x) { return x; }
#endif

#ifdef ALLOW_UAC
uint32_t ua_int4(uint32_t *x) { return *x; }
#else
uint32_t ua_int4(uint32_t *x) {
    unsigned char *c = (unsigned char *)x;
    // correct endianness?
    return c[0] + (c[1]<<8) + (c[2]<<16) + (c[3]<<24);
#endif

Ie we can have some standard functions for memory fetching in cases
where we *may* be unaligned, and use these throughout. The systems
that support unaligned access just inline it to the obvious memory
fetch while other systems do the appropriate magic to fetch the data.

Of course le_int4(ua_int4(...)) combined could probably be written
more efficiently so perhaps we want a compound function too.  I think
there are also various gcc primitives now to do some of these byte
swapping operations so again it's best done in a macro or static
inline function (ideally) so they can all be rewritten in the optimal
way for the current build host.

James

[1] Long term, once we add support for specifying what fields are
required for decoding (very useful for CRAM as it can significantly
speed up reading), we can also use that same hint to avoid byte
swapping data we won't be reading. This neatly avoids the worries
about inefficiently byte swapping auxiliary data when we're only, say,
doing a samtools flagstat.

> The original bit-shift code is the portable way to convert unaligned
> wire-endian data to a host integer.  So if `aux` points to data in
> wire endianness, then the original code is correct.

Yes, alas it doesn't for htslib which is where the bug lies. My fault
in not checking.

James

-- 
James Bonfield (jkb@sanger.ac.uk) | Hora aderat briligi. Nunc et Slythia Tova
                                  | Plurima gyrabant gymbolitare vabo;
  A Staden Package developer:     | Et Borogovorum mimzebant undique formae,
https://sf.net/projects/staden/   | Momiferique omnes exgrabure Rathi. 


-- 
 The Wellcome Trust Sanger Institute is operated by Genome Research 
 Limited, a charity registered in England with number 1021457 and a 
 company registered in England with number 2742969, whose registered 
 office is 215 Euston Road, London, NW1 2BE.