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

[David Matthew Mattli <dmm@mattli.us>]

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?

Thanks,
David

--- a/Common/Kmer.cpp
+++ b/Common/Kmer.cpp
@@ -188,7 +188,8 @@
        Seq seq;
 #if MAX_KMER > 96
 # if WORDS_BIGENDIAN
-       const size_t *s = reinterpret_cast<const size_t*>(src);
+       size_t s[Kmer::NUM_BYTES/sizeof(size_t)];
+       memcpy(s, 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));
 # else
@@ -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/sizeof(size_t)];
        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);