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

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

[Tom de Vries]

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[] =
   {
     { {  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 +50,33 @@ v8sf_t data[] =
 #endif
   };
 
+float data_buf[sizeof (data_orig) * 2 / sizeof (float)];
 
 int
 main (int argc, char **argv)
 {
+  v8sf_t *data;
+
+  /* Initialize data pointer.  */
+  {
+    float *p = data_buf;
+    float *data_buf_end = &data_buf[sizeof (data_buf) / sizeof (*data_buf)];
+    while (((uintptr_t)p & 0x1f) != 0
+	   && p < data_buf_end)
+      p++;
+    data = (v8sf_t *)p;
+  }
+
+  /* Check that data pointer is sufficiently aligned to use vmovaps.  */
+  assert (((uintptr_t)data & 0x1f) == 0);
+
+  /* Check that memcpy does not write out of bounds.  */
+  assert ((void *)data + sizeof (data_orig)
+	  <= (void *)data_buf + sizeof (data_buf));
+
+  /* Initialize data contents.  */
+  memcpy (data, data_orig, sizeof (data_orig));
+
   asm ("vmovaps 0(%0), %%ymm0\n\t"
        "vmovaps 32(%0), %%ymm1\n\t"
        "vmovaps 64(%0), %%ymm2\n\t"

base-commit: edc77c591add0a9c7740a9ed9f7e40358bf65dbf
-- 
2.26.2

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

[Andrew Burgess]

* 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.

Thanks,
Andrew



>    {
>      { {  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 +50,33 @@ v8sf_t data[] =
>  #endif
>    };
>  
> +float data_buf[sizeof (data_orig) * 2 / sizeof (float)];
>  
>  int
>  main (int argc, char **argv)
>  {
> +  v8sf_t *data;
> +
> +  /* Initialize data pointer.  */
> +  {
> +    float *p = data_buf;
> +    float *data_buf_end = &data_buf[sizeof (data_buf) / sizeof (*data_buf)];
> +    while (((uintptr_t)p & 0x1f) != 0
> +	   && p < data_buf_end)
> +      p++;
> +    data = (v8sf_t *)p;
> +  }
> +
> +  /* Check that data pointer is sufficiently aligned to use vmovaps.  */
> +  assert (((uintptr_t)data & 0x1f) == 0);
> +
> +  /* Check that memcpy does not write out of bounds.  */
> +  assert ((void *)data + sizeof (data_orig)
> +	  <= (void *)data_buf + sizeof (data_buf));
> +
> +  /* Initialize data contents.  */
> +  memcpy (data, data_orig, sizeof (data_orig));
> +
>    asm ("vmovaps 0(%0), %%ymm0\n\t"
>         "vmovaps 32(%0), %%ymm1\n\t"
>         "vmovaps 64(%0), %%ymm2\n\t"
> 
> base-commit: edc77c591add0a9c7740a9ed9f7e40358bf65dbf
> -- 
> 2.26.2
> 

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

[Tom de Vries]

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
$
...

Btw, I now realize that gdb.arch/i386-sse.exp has the same issue, I'll
try to factor out a solution that can used in both test-cases.

Thanks,
- Tom

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

[Andrew Burgess]

* 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.

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.

Thanks,
Andrew



> ...
> 
> Btw, I now realize that gdb.arch/i386-sse.exp has the same issue, I'll
> try to factor out a solution that can used in both test-cases.
> 
> Thanks,
> - Tom

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

[Tom de Vries]

[-- 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;
+}

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

[Pedro Alves]

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[] =

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

[Tom de Vries]

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.

Thanks,
- Tom

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

[Pedro Alves]

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?

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

[Tom de Vries]

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.

Thanks,
- Tom

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

[Andrew Burgess]

* Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 14:35:20 +0100]:

> 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.

I'm still not convinced.  The test doesn't appear to be about the
alignment, but about accessing the feature specific registers, so I
don't see how making a mistake with the alignment would be different
to any other bug - eventually it gets spotted and fixed.

Thanks,
Andrew

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

[Tom de Vries]

On 11/5/21 2:52 PM, Andrew Burgess wrote:
> * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 14:35:20 +0100]:
> 
>> 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.
> 
> I'm still not convinced.  The test doesn't appear to be about the
> alignment, 

Correct.

> but about accessing the feature specific registers, so I
> don't see how making a mistake with the alignment would be different
> to any other bug - eventually it gets spotted and fixed.

Right, and my perception is that using precise alignment gives me
confidence that I fixed it properly.

Thanks,
- Tom

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

[Pedro Alves]

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.  As soon as you write some explicit alignment, I don't think
that your proposal helps that much.  The new allocator won't help _finding_ the places
that miss alignment directives.  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.

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

[Tom de Vries]

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

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

[Pedro Alves]

On 2021-11-05 11:54, Andrew Burgess via Gdb-patches wrote:
> * Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 10:43:38 +0100]:

>>> 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.
> 
> 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.  

I agree, and I'm not aware of any serious production compiler that doesn't
support some way of forcing alignment.

Pedro Alves

> But I'm not blocking your proposed solution if you feel
> strongly about it.