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

Tom de Vries via Gdb-patches
2021-11-05T12:24:08Z

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