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

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

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

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

[-- Attachment #2: 0001-gdb-testsuite-Fix-data-alignment-in-gdb.arch-i386-avx-sse-.exp.patch --]
[-- Type: text/x-patch, Size: 5762 bytes --]

[gdb/testsuite] Fix data alignment in gdb.arch/i386-{avx,sse}.exp

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.

Likewise in gdb.arch/i386-sse.exp.

Tested on x86_64-linux, with both gcc and clang.

---
 gdb/testsuite/gdb.arch/i386-avx.c         |  9 +++-
 gdb/testsuite/gdb.arch/i386-sse.c         | 10 +++-
 gdb/testsuite/lib/precise-aligned-alloc.c | 89 +++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/i386-avx.c b/gdb/testsuite/gdb.arch/i386-avx.c
index 4e938399a24..255ff5ee6f5 100644
--- a/gdb/testsuite/gdb.arch/i386-avx.c
+++ b/gdb/testsuite/gdb.arch/i386-avx.c
@@ -25,7 +25,7 @@ typedef struct {
 } v8sf_t;
 
 
-v8sf_t data[] =
+v8sf_t data_orig[] =
   {
     { {  0.0,  0.125,  0.25,  0.375,  0.50,  0.625,  0.75,  0.875 } },
     { {  1.0,  1.125,  1.25,  1.375,  1.50,  1.625,  1.75,  1.875 } },
@@ -47,10 +47,15 @@ v8sf_t data[] =
 #endif
   };
 
+#include "../lib/precise-aligned-alloc.c"
 
 int
 main (int argc, char **argv)
 {
+  void *allocated_ptr;
+  v8sf_t *data
+    = precise_aligned_dup (32, sizeof (data_orig), &allocated_ptr, data_orig);
+
   asm ("vmovaps 0(%0), %%ymm0\n\t"
        "vmovaps 32(%0), %%ymm1\n\t"
        "vmovaps 64(%0), %%ymm2\n\t"
@@ -107,5 +112,7 @@ main (int argc, char **argv)
 
   puts ("Bye!"); /* second breakpoint here */
 
+  free (allocated_ptr);
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.arch/i386-sse.c b/gdb/testsuite/gdb.arch/i386-sse.c
index a5941a4071e..c78a510c1a7 100644
--- a/gdb/testsuite/gdb.arch/i386-sse.c
+++ b/gdb/testsuite/gdb.arch/i386-sse.c
@@ -25,7 +25,7 @@ typedef struct {
 } v4sf_t;
 
 
-v4sf_t data[] =
+v4sf_t data_orig[] =
   {
     { {  0.0,  0.25,  0.50,  0.75 } },
     { {  1.0,  1.25,  1.50,  1.75 } },
@@ -62,9 +62,15 @@ have_sse (void)
     return 0;
 }
 
+#include "../lib/precise-aligned-alloc.c"
+
 int
 main (int argc, char **argv)
 {
+  void *allocated_ptr;
+  v4sf_t *data
+    = precise_aligned_dup (16, sizeof (data_orig), &allocated_ptr, data_orig);
+
   if (have_sse ())
     {
       asm ("movaps 0(%0), %%xmm0\n\t"
@@ -124,5 +130,7 @@ main (int argc, char **argv)
       puts ("Bye!"); /* second breakpoint here */
     }
 
+  free (allocated_ptr);
+
   return 0;
 }
diff --git a/gdb/testsuite/lib/precise-aligned-alloc.c b/gdb/testsuite/lib/precise-aligned-alloc.c
new file mode 100644
index 00000000000..67b6f2bc618
--- /dev/null
+++ b/gdb/testsuite/lib/precise-aligned-alloc.c
@@ -0,0 +1,89 @@
+/* This test file is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdlib.h>
+#include <assert.h>
+#include <string.h>
+#include <stdint.h>
+
+/* Allocate SIZE memory with ALIGNMENT, and return it.  If FREE_POINTER,
+   return in it the corresponding pointer to be passed to free.
+
+   Do the alignment precisely, in other words, if an alignment of 4 is
+   requested, make sure the pointer is 4-byte aligned, but not 8-byte
+   aligned.  In other words, make sure the pointer is not overaligned.
+
+   The benefit of using precise alignment is that accidentally specifying
+   a too low alignment will not be compensated by accidental
+   overalignment.  */
+
+static void *
+precise_aligned_alloc (size_t alignment, size_t size, void **free_pointer)
+{
+  size_t used_alignment = 2 * alignment;
+  size_t used_size = size + used_alignment;
+
+  void *p = malloc (used_size);
+  assert (p != NULL);
+  void *p_end = p + used_size;
+
+  if (free_pointer != NULL)
+    *free_pointer = p;
+
+  void *p_orig = p;
+
+  /* Align to used_alignment.  */
+  size_t mask = (used_alignment - 1);
+  if (((uintptr_t)p & mask) == 0)
+    ;
+  else
+    {
+      p = (void*)((uintptr_t)p & ~mask);
+      p += used_alignment;
+    }
+
+  p += alignment;
+
+  /* Verify p is without bounds, and points to large enough area.  */
+  assert (p >= p_orig);
+  assert (p + size <= p_end);
+
+  /* Verify required alignment.  */
+  mask = (alignment - 1);
+  assert (((uintptr_t)p & mask) == 0);
+
+  /* Verify required alignment is precise.  */
+  mask = (used_alignment - 1);
+  assert (((uintptr_t)p & mask) != 0);
+
+  return p;
+}
+
+/* Duplicate data SRC of size SIZE to a newly allocated, precisely aligned
+   location with alignment ALIGNMENT.  */
+
+static void *
+precise_aligned_dup (size_t alignment, size_t size, void **free_pointer,
+		     void *src)
+{
+  void *p = precise_aligned_alloc (alignment, size, free_pointer);
+
+  memcpy (p, src, size);
+
+  return p;
+}