[Mesa-dev] [PATCH] gallium: Fix blend color alignment for 32-bit systems

[Nicolai Hähnle]

On 12.07.2016 15:44, Roland Scheidegger wrote:
> Am 12.07.2016 um 13:40 schrieb Nicolai Hähnle:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> This fixes a regression introduced by commit d8d6091a8.
>>
>> Heap allocations may be only 8-byte aligned on 32-bit system, and so having
>> members with 16-byte alignment (such as in the case where pipe_blend_color is
>> embedded in radeonsi's si_context) is undefined behavior which indeed causes
>> crashes when compiled with gcc -O3.
>>
>> Rather than track down and fix all allocation sites where a pipe_blend_color
>> may be embedded, assume that the original compiler bug only affects 64-bits.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96835
>> Cc: Tim Rowley <timothy.o.rowley at intel.com>
>> Cc: Chuck Atkins <chuck.atkins at kitware.com>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> --
>> This should fix the linked bug report. The big question is whether the
>> assumption about the original compiler problem is correct?
>> ---
>>   src/gallium/include/pipe/p_state.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
>> index a73a771..1986495 100644
>> --- a/src/gallium/include/pipe/p_state.h
>> +++ b/src/gallium/include/pipe/p_state.h
>> @@ -335,8 +335,13 @@ struct pipe_blend_color
>>       * unaligned accessors resulting in a segfault.  Specifically several
>>       * versions of the Intel compiler are known to be affected but it's likely
>>       * others are as well.
>> +    *
>> +    * This only applies on 64-bit architectures, and adding the alignment on
>> +    * 32-bit architectures causes bugs because heap allocations are not
>> +    * sufficiently aligned.
>>       */
>> -   PIPE_ALIGN_VAR(16) float color[4];
>> +   PIPE_ALIGN_VAR(sizeof(void *) >= 8 ? 16 : 4)
>> +   float color[4];
>>   };
>>
>
> Honestly, I'd rather get rid of it, this gets really hacky. (If the
> compiler bug was happening in the driver, it could easily do alignment
> on its own, even if possibly at the cost of a copy).
>
> Note that technically, malloc allocations aren't guaranteed to be 16
> byte on a 64bit arch. Rather, malloc() has to honor biggest alignment
> for a standard type (and no, __mm128 doesn't count), which on x64 linux
> is long double (128bit). No idea if that's the case everywhere (even if
> it's not, there's of course some possibility it will return 16 byte
> aligned addresses anyway).

Yeah, I'm really not happy about this either. Tim, Chuck, given that the 
original fix created a new bug, is this something you could just fix in 
the affected driver instead?

Nicolai

>
> Roland
>
>