On 11/5/21 12:54 PM, Andrew Burgess wrote:
> * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 10:43:38 +0100]:
> 
>> On 11/5/21 10:33 AM, Andrew Burgess wrote:
>>> * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-11-04 14:55:59 +0100]:
>>>
>>>> When running test-case gdb.arch/i386-avx.exp with clang I ran into:
>>>> ...
>>>> (gdb) PASS: gdb.arch/i386-avx.exp: set first breakpoint in main
>>>> continue^M
>>>> Continuing.^M
>>>> ^M
>>>> Program received signal SIGSEGV, Segmentation fault.^M
>>>> 0x000000000040052b in main (argc=1, argv=0x7fffffffd3c8) at i386-avx.c:54^M
>>>> 54        asm ("vmovaps 0(%0), %%ymm0\n\t"^M
>>>> (gdb) FAIL: gdb.arch/i386-avx.exp: continue to breakpoint: \
>>>>   continue to first breakpoint in main
>>>> ...
>>>>
>>>> The problem is that the vmovaps insn requires an 256-bit (or 32-byte aligned
>>>> address), and it's only 16-byte aligned:
>>>> ...
>>>> (gdb) p /x $rax
>>>> $1 = 0x601030
>>>> ...
>>>>
>>>> Fix this by copying to a sufficiently aligned address.
>>>>
>>>> Tested on x86_64-linux, with both gcc and clang.
>>>> ---
>>>>  gdb/testsuite/gdb.arch/i386-avx.c | 28 +++++++++++++++++++++++++++-
>>>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/gdb/testsuite/gdb.arch/i386-avx.c b/gdb/testsuite/gdb.arch/i386-avx.c
>>>> index 4e938399a24..9b5323f9f76 100644
>>>> --- a/gdb/testsuite/gdb.arch/i386-avx.c
>>>> +++ b/gdb/testsuite/gdb.arch/i386-avx.c
>>>> @@ -18,6 +18,9 @@
>>>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>>>  
>>>>  #include <stdio.h>
>>>> +#include <stdint.h>
>>>> +#include <assert.h>
>>>> +#include <string.h>
>>>>  #include "nat/x86-cpuid.h"
>>>>  
>>>>  typedef struct {
>>>> @@ -25,7 +28,7 @@ typedef struct {
>>>>  } v8sf_t;
>>>>  
>>>>  
>>>> -v8sf_t data[] =
>>>> +v8sf_t data_orig[] =
>>>
>>> I see the same problem.  Did you consider using:
>>>
>>>   /* Some useful comment ....  */
>>>   v8sf_t data[] __attribute__ ((aligned(32))) = ....
>>>
>>> this seems to fix the problem on clang for me, and still works fine
>>> with gcc.
>>
>> I did consider this, and decided against it because it's not
>> portable.
>>
>> Note btw that there is no other usage of this:
>> ...
>> $ find gdb/testsuite/ -type f | xargs grep attribute.*align
>> $
> 
> 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.

> I'd hope most production level compilers would, if they don't support
> 'aligned' have something similar/equivalent.
> 
> Personally, I'd go with a compatibility macro, and let folk who care
> about other compilers figure out what they need when they hit the
> problem.  But I'm not blocking your proposed solution if you feel
> strongly about it.

I prefer using portable constructs if possible, and reasonably maintainable.

Anyway, the solution I've implemented has one further benefit: it
provides precise alignment, such that accidentally specifying a too low
alignment cannot be compensated by accidental surplus alignment.  [ I've
also filed a related gcc PR (
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103095 ). ]

This version also fixes i386-sse.c.

Any further comments?

Thanks,
- Tom