Re: [PATCH] [gdb/testsuite] Fix gdb.arch/i386-avx.exp with clang

[Tom de Vries <tdevries@suse.de>]

On 11/5/21 2:54 PM, Pedro Alves wrote:
> On 2021-11-05 13:35, Tom de Vries wrote:
>> On 11/5/21 2:20 PM, Pedro Alves wrote:
>>> On 2021-11-05 13:15, Tom de Vries wrote:
>>>> On 11/5/21 1:55 PM, Pedro Alves wrote:
>>>>> On 2021-11-05 12:23, Tom de Vries via Gdb-patches wrote:
>>>>>
>>>>>>> No, but in gdb/testsuite/lib/attribute.h we do setup a compatibility
>>>>>>> macro for 'noclone', so there's definitely precedent for using
>>>>>>> attributes that might not be supported everywhere.
>>>>>>>
>>>>>>
>>>>>> Right, I'm aware of this, but that's a typical case where we have no
>>>>>> portable alternative.
>>>>>
>>>>> We actually do -- _Alignas is standard C11.  This fixes the test as well:
>>>>>
>>>>>   _Alignas(32) v8sf_t data[] =
>>>>>
>>>>
>>>> I was referring to the noclone, but ok, I was not aware of the _Alignas,
>>>> good to know, thanks.
>>>>
>>>> Anyway, in the latest version this is not relevant anymore, since the
>>>> precise alignment implementation has an extra benefit, as explained in
>>>> the post.
>>>>
>>>
>>> OOC, is that benefit important here?
>>>
>>
>> So, this is the post I mentioned (
>> https://sourceware.org/pipermail/gdb-patches/2021-November/183183.html ).
>>
>> Well, the benefit is that it prevents accidental overalignment, which is
>> the reason that this problem escaped detection and/or fixing for so long.
>>
>> Without that, I could do a thinko and specify too small an alignment and
>> have the test passing accidentally, only to fail in a different setup.
>>
> 
> The code made no effort at all to align the object, which is I think the main reason
> why it went missed.

Well, yes, but "no effort at all to align the object" still translates
to some minimal required alignment, which was too small, and which was
not detected because of accidental over-alignment.  So AFAICT, my
reasoning is sound here.

> As soon as you write some explicit alignment, I don't think
> that your proposal helps that much.

It helps me by giving me confidence that I hardcoded a large enough
alignment.

> The new allocator won't help _finding_ the places
> that miss alignment directives.

Correct, I'm not claiming that.  It will help me though in case the
instruction requires an alignment of 32 and I misread the documentation
and understand and use 16 instead.

> I'm honestly not finding the benefit compelling enough to
> justify the complication, compared to using alignas/_Alignas, which is what actual user
> code will be using.  My .2c, anyhow.
> 

Thanks for sharing your opinion on this.  I understand what you're
saying, but I do think the benefit is compelling enough.  I spent a lot
of my time trying to reproduce, analyze and fix problems that are only
seen on some systems, or intermittently, and consequently I very much
value anything that makes test-cases behave the same on different systems.

I've now split the patch in two, and committed:
- 1 patch implementing the fix with _Alignas
  https://sourceware.org/pipermail/gdb-patches/2021-December/184249.html
- 1 patch following up to use precise align
  https://sourceware.org/pipermail/gdb-patches/2021-December/184250.html

This should help focus the rationale for the second part.  It also means
that in case of problems, it can be reverted easily without
re-introducing the error fixed by the first part.

Thanks,
- Tom