Support for mips/mipsel #99
Conversation
Fixed swap_data function (sam.c): Four bytes that hold the value of number of elements in an auxiliary data array have to be swapped at different times for reading/writing. In cram/os.h system endianness wasn't being properly detected. In cram/cram_encode.c (function cram_encode_aux) 'count' variable bytes have to be swapped on big endian machines. In test/sam.c reference byte array for checking has to be in big-endian format on big-endian machines.
Avoid unaligned memory access on architectures that don't support it.
| if ((p = check_bam_aux_get(aln, "XB", 'B')) && memcmp(p, "Bc\3\0\0\0\xfe\x00\x02", 9) != 0) | ||
| #elif defined SP_BIG_ENDIAN | ||
| if ((p = check_bam_aux_get(aln, "XB", 'B')) && memcmp(p, "Bc\0\0\0\3\xfe\x00\x02", 9) != 0) | ||
| #endif |
There was a problem hiding this comment.
This change would mean that the data in the file became dependent on host endianness, which is clearly wrong -- for interchange, the file contents are the same regardless of host endianness.
There was a problem hiding this comment.
It looks like that it is not data from file that is being compared here, but data from an internal buffer previously stored in host byte order.
To expand on this, function check_bam_aux_get returns a pointer to auxiliary data which is stored in aln->data (aln is a variable of type bam1_t). This structure has previously been filled with data from input file in function sam_parse1 during reading of input file.
This particular 4 byte integer value is copied to the internal buffer in sam.c:830
830 kputc_('B', &str); kputc_(type, &str); kputsn_(&n, 4, &str);
kputsn_ is defined as (htslib/kstring.h:178):
static inline int kputsn_(const void *p, int l, kstring_t *s)
{
if (s->l + l > s->m) {
char *tmp;
s->m = s->l + l;
kroundup32(s->m);
if ((tmp = (char*)realloc(s->s, s->m)))
s->s = tmp;
else
return EOF;
}
memcpy(s->s + s->l, p, l);
s->l += l;
return l;
}
Four bytes of integer variable n are copied to (str->s+str->l) which is the address inside the internal buffer. The value is stored in host byte order, which is big endian in case of big endian mips.
Finally, at the end of sam_parse1, b->data is assigned to be str->s.
842 b->data = (uint8_t*)str.s; b->l_data = str.l; b->m_data = str.m;
Also, I came upon a comment in function sam_parse1 placed before the parsing of aux data (sam.c:778) that states:
// Note that (like the bam1_core_t fields) this aux data in b->data is
// stored in host endianness; so there is no byte swapping needed here.
As far as I can see, this practice is also followed for other formats, e.g. when data is read from input BAM files and/or written to output BAM files, function swap_data (sam.c:268) is used for converting multibyte values from little endian to big endian format and vice versa, depending on the operation. Again, aux data is kept in host byte order.
Taking this into account, the change from "Bc\3\0\0\0\xfe\x00\x02" to "Bc\0\0\0\3\xfe\x00\x02" reflects the difference in data endianness (depending on the host), not a change in the input file itself.
As I am not an expert on the subject, I could be wrong, so any suggestions on how to correctly support htslib for mips/mipsel are most welcome.
There was a problem hiding this comment.
Agreed that perhaps it is the internal memory which is in host order rather than the file.
For CRAM I finally managed to build (yawn) Staden io_lib-1.13.7 on an emulated big endian machine. It worked out of the box and produces both BAM and CRAM which I can decode on an Intel linux box. I tried the opposite direction and that also works. So the on-disk formats are working fine there.
Obviously io_lib isn't the same as htslib (in particular the BAM component is totally different), but the CRAM source has a common ancestry and will be synced again shortly after the 1.0 release (once CRAM v3.0 becomes finalised). So I'm wary of changes to cram_encode.c given they apparently aren't necessary within io_lib.
On little endian machines the in-memory data structure and on-disk data structure is identical. There are clearly two approaches here. 1) Keep the in-memory data structure the same as the on-disk structure and byte swap just-in-time when accessing. 2) Byte swap the in-memory data structures on reading so manipulation is easy, and then byte swap back again just before writing. Given the need to prevent unaligned accesses too, option 1 is often easier, although it can be cheated a bit (in my own implementation I deliberately pad out the read name with nuls to make the CIGAR field word aligned again, as a faster alternative to byte-by-byte loading).
I suspect it is differences in choices of method 1 vs 2 between io_lib and htslib that causes the CRAM code to not work properly on big endian machines within htslib, but I haven't yet tested that.
There was a problem hiding this comment.
Apologies, you are correct re this code: the data is in host-endianness but still packed for wire-alignment... despite having added that comment, I had forgotten just how baroque this code is! So test/sam.c does indeed need fixing, though the right fix is to check p[2..5] as a host int (modulo alignment concerns) rather than as a string.
There was a problem hiding this comment.
There are still unaligned memory accesses in sam.c. Eg line 729:
cigar[i] = strtol(q, &q, 10)<<BAM_CIGAR_SHIFT;
I'll experiment some more.
|
Hi, We distribute tophat [0] in Debian, and currently it fails to build from source on mips/mipsel architectures - this is the build log for mips: Can you see what is the problem there? Thank you, [0] http://anonscm.debian.org/gitweb/?p=debian-med/tophat.git |
|
Hi, Alex. Best Regards |
|
Hi Alex, as Aleksandar was prevented, I took a look at tophat issue. More you can find here: Regards, |
|
Where are we with solving this issue? |
|
It was rewritten by myself and then furthered by Rob: https://github.com/daviesrob/htslib/tree/SPARC However I don't think there was any inclination to incorporate it, so it got dropped and is now way out of date. |
|
Looking at https://buildd.debian.org/status/package.php?p=htslib, 1.3.1 is fine on little-endian platforms (including mipsel), but fails on big-endian. I think big-endian support is partly present in htslib, so it might be possible to revive it. Not sure how much work it would be though. |
|
Hi, |
|
On Sun, Dec 11, 2016 at 12:16:13AM -0800, Andreas Tille wrote:
the initialy suggested patch for the problem was not accepted since more than two years. Do you have any other solution to fix the build issue on armhf?
I think for now it's fair to say there has been little appetite in
supporting less well used architectures, in favour of spending time on
more serious issues and wish-list items (of which there are still
plenty to work through!) and I don't really see that changing any time
soon.
It's unfortunate that we didn't create a PR of the rewrite of this
(https://github.com/daviesrob/htslib/tree/SPARC by Rob and myself) and
merge it promptly as that too is now so old it would be a large amount
of work to integrate (and so unlikely to happen).
Can you make a real case for these platforms being important to the
bioinformatics community? If so it may be possible to reevaluate the
priorities.
James
--
James Bonfield ([email protected]) | 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.
|
|
On Mon, Dec 12, 2016 at 01:36:56AM -0800, James Bonfield wrote:
Can you make a real case for these platforms being important to the
bioinformatics community? If so it may be possible to reevaluate the
priorities.
No, I can't except that in Debian we try to get all software that is
possible released on all architectures. Considering that there was a
patch that should enable this I was simply checking the status of this
issue. If you explicitly state that htslib is not and will not be
supported on armhf/mips/mipsel I would add this to the package metadata and close
the bug in the Debian bug tracking system. Its just not sensible to
leave a bug with a patch open for years and do nothing. :-)
Kind regards
Andreas.
…
|
|
Actually armhf is little-endian, so it should in theory work fine. And in fact when I last tried it using qemu (so not a real armhf machine) it worked for me. See samtools/samtools#612 If we can get into your build system and can have a poke around we might be able to find out what's going on. Without that, we're a bit stuck until we can get something to test on. |
|
When it comes to alignment issues qemu favours performance over correctness. So code with alignment issues that will fail on real hardware will often work in qemu. |
|
On Mon, Dec 12, 2016 at 03:13:28PM -0800, daviesrob wrote:
Actually armhf is little-endian, so it should in theory work fine. And in fact when I last tried it using qemu (so not a _real_ armhf machine) it worked for me. See samtools/samtools#612
If we can get into your build system and can have a poke around we might be able to find out what's going on. Without that, we're a bit stuck until we can get something to test on.
Hi,
I have asked how to gain access to a real armhf box. Hope this helps:
On Tue, Dec 13, 2016 at 7:12 PM, peter green ***@***.***> wrote:
On 13/12/16 07:37, Andreas Tille wrote:
> when discussing bug #800469 upstream thinks that armhf should work and
> the question came up whether there is some chance to access an armhf
> machine that is comparable to our autobuilders.
>
The porterbox abel.debian.org runs the same hardware as the buildds and has
both armel and armhf chroots available.
There is a procedure for guest accounts on porterboxes, though I haven't
used it myself so I don't know how well it works in practice.
The process has simplified, and just visit: https://nm.debian.org/wizard/
and click "Request guest account".
Just curious that this issue cannot be reproduced by pbuilder or
similar on self arm boxes?
Cheers,
--
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1
…
|
|
I'm not in the office at the moment, but I can try digging out a local Debian developer when I get back if that helps. Assuming there's any left one week before Christmas... I thought modern arm cpus could manage unaligned access. This could be a problem if not. |
|
On 13/12/16 23:41, daviesrob wrote:
I thought modern arm cpus could manage unaligned access. This could be
a problem if not.
They can for integer but not for floating point.
|
|
The issue can be reproduced on a Raspberry PI. |
Fix for issue #98
Avoid unaligned memory access on architectures that don't support it.
Fixed endianness related issues.