Re: Bug#889147: abyss: PairedDBG_LoadAlgorithm test fails on sparc64 due to strict alignment violation

[James Clarke <jrtc27@debian.org>]

On 2 Feb 2018, at 15:49, David Matthew Mattli <dmm@mattli.us> wrote:
> 
> James Clarke <jrtc27@debian.org> writes:
> 
>> user debian-sparc@lists.debian.org
>> usertags sparc64
>> thanks
>> 
>> (You missed the User: pseudoheader)
>> 
>>> On 2 Feb 2018, at 14:46, David Matthew Mattli <dmm@mattli.us> wrote:
>>> 
>>> Package: abyss
>>> Severity: normal
>>> Tags: patch upstream
>>> Usertags: sparc64
>>> 
>>> Dear Maintainer,
>>> 
>>> This package currently FTBFS on sparc64 due to the
>>> PairedDBG_LoadAlgorithm test failing with a SIGBUS. The Kmer.load and
>>> Kmer.storeReverse methods in the Common/Kmer.cpp file cast a uint8_t*
>>> to a size_t* without ensuring the pointer value has the proper
>>> alignment.
>>> 
>>> To fix this I added an aligned stack allocated buffer and memcpy to
>>> that. Stack allocated is appropriate because the buffer size is
>>> small(32 bytes) and known at compile time.
>> 
>> Hi,
>> As a sparc64 porter, thanks for fixing packages! Just a couple of
>> comments on the patch.
>> 
>>> --- a/Common/Kmer.cpp
>>> +++ b/Common/Kmer.cpp
>>> @@ -188,9 +188,10 @@
>>> 	Seq seq;
>>> #if MAX_KMER > 96
>>> # if WORDS_BIGENDIAN
>>> -	const size_t *s = reinterpret_cast<const size_t*>(src);
>>> +	size_t buf[Kmer::NUM_BYTES];
>> 
>> Should be divided by sizeof(size_t). Also, why not call it s? That should
>> reduce the number of changes.
>> 
>>> +	memcpy(buf, src, Kmer::NUM_BYTES);
>>> 	size_t *d = reinterpret_cast<size_t*>(&seq + 1);
>>> -	copy(s, s + Kmer::NUM_BYTES/sizeof(size_t), reverse_iterator<size_t*>(d));
>>> +	copy(buf, buf + Kmer::NUM_BYTES/sizeof(size_t), reverse_iterator<size_t*>(d));
>>> # else
>>> 	uint8_t *d = reinterpret_cast<uint8_t*>(&seq);
>>> 	memcpy(d, src, sizeof seq);
>>> @@ -235,9 +236,10 @@
>>> #if MAX_KMER > 96
>>> # if WORDS_BIGENDIAN
>>> 	const size_t *s = reinterpret_cast<const size_t*>(&seq);
>>> -	size_t *d = reinterpret_cast<size_t*>(dest);
>>> +	size_t d[Kmer::NUM_BYTES];
>> 
>> Ditto for the size (this time you used the same name as before).
>> 
>>> 	copy(s, s + Kmer::NUM_BYTES/sizeof(size_t),
>>> 			reverse_iterator<size_t*>(d +  Kmer::NUM_BYTES/sizeof(size_t)));
>>> +	memcpy(dest, d, Kmer::NUM_BYTES);
>>> 	reverse(dest, dest + Kmer::NUM_BYTES);
>>> # else
>>> 	memcpy(dest, &seq, Kmer::NUM_BYTES);
>> 
>> Regards,
>> James
> 
> Thanks for the quick feedback! How does this revised patch look?

Looks sensible as a non-maintainer. Let's wait for a debian-med person to
review (technically I have commit rights, but I know nothing about this
package).

James