control: forwarded -1 James Bonfield <j...@sanger.ac.uk>
control: tags -1 upstream
Hi James,
There is a bug report against the Debian package of the latest
io_lib version. See the full bug log here:
https://bugs.debian.org/876839
Below you can read about a suggested solution.
Kind regards
Andreas.
On Tue, Sep 26, 2017 at 10:31:01PM +0200, Christian Seiler wrote:
> On 09/26/2017 10:06 PM, Andreas Tille wrote:
> >> ...
> >> In file included from bgzip.c:56:0:
> >> bgzip.c: In function 'gzi_index_dump':
> >> ../io_lib/os.h:127:10: error: invalid operands to binary & (have 'uint64_t
> >> * {aka long long unsigned int *}' and 'long long int')
> >> (((x & 0x00000000000000ffLL) << 56) + \
> >> ^
> >> ../io_lib/os.h:185:20: note: in expansion of macro 'iswap_int8'
> >> #define le_int8(x) iswap_int8((x))
> >> ^~~~~~~~~~
> >> bgzip.c:190:16: note: in expansion of macro 'le_int8'
> >> if (fwrite(le_int8(&n), sizeof(n), 1, idx_f) != 1)
> >> ^~~~~~~
>
> This code is completely wrong.
>
> le_int8 appears to do a 64 bit byte order swap to adjust the
> endianness of a quantity. What bgzip.c does at this point is the
> following (removed if() for clarity):
>
> uint64_t n = idx->n;
> fwrite(le_int8(&n), sizeof(n), 1, idx_f);
>
> &n is the pointer to the 'n' variable, but you really don't want
> to byte swap a pointer to a local variable before passing it to
> a function that reads that pointer (also note that the pointer
> could be 32 bit on 32 bit systems!).
>
> What you presumably want to do is byte swap the _contents_ of the
> pointer (especially since n is a 64 bit integer). If you look at
> the read code in the same file this is actually what happens:
>
> if (8 != fread(&n, 1, 8, fp))
> goto err;
> n = le_int8(n);
>
> So what you'd rather want to have is the following code:
>
> uint64_t n = le_int8(idx->n);
> fwrite(&n, sizeof(n), 1, idx_f);
>
> Or, if I adjust the entirety of that section in the original write
> code:
>
> int i;
> uint64_t n = le_int8(idx->n);
> if (fwrite(&n), sizeof(n), 1, idx_f) != 1)
> goto fail;
> for (i=0; i<idx->n; i++) {
> uint64_t nn;
> nn = le_int8(idx->c_off[i]);
> if (fwrite(&nn, sizeof(nn), 1, idx_f) != 1)
> goto fail;
> nn = le_int8(idx->u_off[i]);
> if (fwrite(&nn, sizeof(nn), 1, idx_f) != 1)
> goto fail;
> }
>
> That should fix the compiler error you're seeing.
>
> The only reason that doesn't fail on Little Endian is because the
> le_int8(x) function is a no-op on those systems and just passes
> through the original pointer.
>
> Regards,
> Christian
>
--
http://fam-tille.de
