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

Submitted by Nicolai Hähnle on July 12, 2016, 11:40 a.m.

Details

Message ID 1468323616-10514-1-git-send-email-nhaehnle@gmail.com
State New
Headers show
Series "gallium: Fix blend color alignment for 32-bit systems" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Nicolai Hähnle July 12, 2016, 11:40 a.m.
From: Nicolai Hähnle <nicolai.haehnle@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@intel.com>
Cc: Chuck Atkins <chuck.atkins@kitware.com>
Cc: <mesa-stable@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(-)

Patch hide | download patch | download mbox

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];
 };
 
 

Comments

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek

On Tue, Jul 12, 2016 at 1:40 PM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
> From: Nicolai Hähnle <nicolai.haehnle@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@intel.com>
> Cc: Chuck Atkins <chuck.atkins@kitware.com>
> Cc: <mesa-stable@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];
>  };
>
>
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Am 12.07.2016 um 13:40 schrieb Nicolai Hähnle:
> From: Nicolai Hähnle <nicolai.haehnle@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@intel.com>
> Cc: Chuck Atkins <chuck.atkins@kitware.com>
> Cc: <mesa-stable@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).

Roland
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@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@intel.com>
>> Cc: Chuck Atkins <chuck.atkins@kitware.com>
>> Cc: <mesa-stable@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
>
>
(I've been out on vacation for the past week and a half so sorry it took so
long for me to get to this)

I'm okay with reverting the initial commit entirely.  At the time I was
thinking about the alignment in terms of C++ memory allocation, in which
case the struct would be allocated with new and the alignment attribute on
the member would ensure that it gets aligned properly, regardless of the
underlying cpu arch.  However, since it's actually being allocated with
malloc and cast to the struct pointer, the alignment attribute instead is
just asserting that the member is aligned, regardless of whether or not it
actually is.  Given that, the implementation is definitely the wrong way to
go about trying to address this issue.

Please revert.

- Chuck

On Wed, Jul 13, 2016 at 4:15 AM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:

> 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@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@intel.com>
>>> Cc: Chuck Atkins <chuck.atkins@kitware.com>
>>> Cc: <mesa-stable@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
>>
>>
>>