* [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c
@ 2021-06-05 0:14 Pedro Alves
2021-06-05 0:14 ` [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct Pedro Alves
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Pedro Alves @ 2021-06-05 0:14 UTC (permalink / raw)
To: gdb-patches
Building GDB with current git Clang hits a number of problems. This
fixes one of them.
Pedro Alves (2):
nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
nat/amd64-linux-siginfo.c: Remove typedefs
gdb/nat/amd64-linux-siginfo.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
base-commit: 386de171cbffa86e804057030f3d64a404279f43
--
2.26.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
2021-06-05 0:14 [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Pedro Alves
@ 2021-06-05 0:14 ` Pedro Alves
2021-06-05 21:20 ` John Baldwin
2021-06-05 0:14 ` [PATCH 2/2] nat/amd64-linux-siginfo.c: Remove typedefs Pedro Alves
2021-06-06 14:41 ` [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Simon Marchi
2 siblings, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2021-06-05 0:14 UTC (permalink / raw)
To: gdb-patches
Compiling GDB with current git Clang (future 13) fails with (among
other problems), this issue:
$ make nat/amd64-linux-siginfo.o
CXX nat/amd64-linux-siginfo.o
src/gdb/nat/amd64-linux-siginfo.c:590:35: warning: passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'compat_x32_siginfo_from_siginfo' may result in an unaligned pointer access [-Walign-mismatch]
compat_x32_siginfo_from_siginfo ((struct compat_x32_siginfo *) inf,
^
1 warning generated.
The problem is that:
- The flagged code is casting to "struct compat_x32_siginfo" pointer
directly instead of to a pointer to the compat_x32_siginfo_t
typedef. The called function is declared with a
compat_x32_siginfo_t typedef pointer parameter.
- Only the typedef has the __aligned__ attribute.
Fix this by moving the attribute to the struct, so both struct and
typedef have the same alignment.
The next patch removes the typedefs.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <pedro@palves.net>
* nat/amd64-linux-siginfo.c (compat_x32_siginfo_t): Move
__attribute__ __aligned__ from the typedef to the struct.
---
gdb/nat/amd64-linux-siginfo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gdb/nat/amd64-linux-siginfo.c b/gdb/nat/amd64-linux-siginfo.c
index e2d2db6e112..9ff9361487a 100644
--- a/gdb/nat/amd64-linux-siginfo.c
+++ b/gdb/nat/amd64-linux-siginfo.c
@@ -206,7 +206,7 @@ typedef struct compat_siginfo
/* For x32, clock_t in _sigchld is 64bit aligned at 4 bytes. */
typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
-typedef struct compat_x32_siginfo
+typedef struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo
{
int si_signo;
int si_errno;
@@ -263,7 +263,7 @@ typedef struct compat_x32_siginfo
int _fd;
} _sigpoll;
} _sifields;
-} compat_x32_siginfo_t __attribute__ ((__aligned__ (8)));
+} compat_x32_siginfo_t;
/* To simplify usage of siginfo fields. */
--
2.26.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] nat/amd64-linux-siginfo.c: Remove typedefs
2021-06-05 0:14 [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Pedro Alves
2021-06-05 0:14 ` [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct Pedro Alves
@ 2021-06-05 0:14 ` Pedro Alves
2021-06-06 14:41 ` [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Simon Marchi
2 siblings, 0 replies; 7+ messages in thread
From: Pedro Alves @ 2021-06-05 0:14 UTC (permalink / raw)
To: gdb-patches
Since GDB is written in C++ now, we don't need struct/union typedefs
any more. Remove them from nat/amd64-linux-siginfo.c.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <pedro@palves.net>
* nat/amd64-linux-siginfo.c (union nat_sigval): Rename to ...
(nat_sigval_t): ... this and remove typedef of same name.
(struct nat_siginfo): Rename to ...
(nat_siginfo_t): ... this and remove typedef of same name.
(struct compat_sigval): Rename to ...
(compat_sigval_t): ... this and remove typedef of same name.
(struct compat_siginfo): Rename to ...
(compat_siginfo_t): ... this and remove typedef of same name.
(struct compat_x32_siginfo): Rename to ...
(compat_x32_siginfo_t): ... this and remove typedef of same name.
(amd64_linux_siginfo_fixup_common): Adjust.
---
gdb/nat/amd64-linux-siginfo.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/gdb/nat/amd64-linux-siginfo.c b/gdb/nat/amd64-linux-siginfo.c
index 9ff9361487a..342840e5085 100644
--- a/gdb/nat/amd64-linux-siginfo.c
+++ b/gdb/nat/amd64-linux-siginfo.c
@@ -45,13 +45,13 @@ typedef int nat_timer_t;
/* For native 64-bit, clock_t in _sigchld is 64-bit. */
typedef long nat_clock_t;
-typedef union nat_sigval
+union nat_sigval_t
{
nat_int_t sival_int;
nat_uptr_t sival_ptr;
-} nat_sigval_t;
+};
-typedef struct nat_siginfo
+struct nat_siginfo_t
{
int si_signo;
int si_errno;
@@ -112,7 +112,7 @@ typedef struct nat_siginfo
int _fd;
} _sigpoll;
} _sifields;
-} nat_siginfo_t;
+};
#endif /* __ILP32__ */
@@ -133,13 +133,13 @@ struct compat_timeval
int tv_usec;
};
-typedef union compat_sigval
+union compat_sigval_t
{
compat_int_t sival_int;
compat_uptr_t sival_ptr;
-} compat_sigval_t;
+};
-typedef struct compat_siginfo
+struct compat_siginfo_t
{
int si_signo;
int si_errno;
@@ -201,12 +201,12 @@ typedef struct compat_siginfo
int _fd;
} _sigpoll;
} _sifields;
-} compat_siginfo_t;
+};
/* For x32, clock_t in _sigchld is 64bit aligned at 4 bytes. */
typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
-typedef struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo
+struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo_t
{
int si_signo;
int si_errno;
@@ -263,7 +263,7 @@ typedef struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo
int _fd;
} _sigpoll;
} _sifields;
-} compat_x32_siginfo_t;
+};
/* To simplify usage of siginfo fields. */
@@ -578,20 +578,20 @@ amd64_linux_siginfo_fixup_common (siginfo_t *ptrace, gdb_byte *inf,
if (mode == FIXUP_32)
{
if (direction == 0)
- compat_siginfo_from_siginfo ((struct compat_siginfo *) inf, ptrace);
+ compat_siginfo_from_siginfo ((compat_siginfo_t *) inf, ptrace);
else
- siginfo_from_compat_siginfo (ptrace, (struct compat_siginfo *) inf);
+ siginfo_from_compat_siginfo (ptrace, (compat_siginfo_t *) inf);
return 1;
}
else if (mode == FIXUP_X32)
{
if (direction == 0)
- compat_x32_siginfo_from_siginfo ((struct compat_x32_siginfo *) inf,
+ compat_x32_siginfo_from_siginfo ((compat_x32_siginfo_t *) inf,
ptrace);
else
siginfo_from_compat_x32_siginfo (ptrace,
- (struct compat_x32_siginfo *) inf);
+ (compat_x32_siginfo_t *) inf);
return 1;
}
--
2.26.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
2021-06-05 0:14 ` [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct Pedro Alves
@ 2021-06-05 21:20 ` John Baldwin
2021-06-07 22:20 ` Pedro Alves
0 siblings, 1 reply; 7+ messages in thread
From: John Baldwin @ 2021-06-05 21:20 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 6/4/21 5:14 PM, Pedro Alves wrote:
> diff --git a/gdb/nat/amd64-linux-siginfo.c b/gdb/nat/amd64-linux-siginfo.c
> index e2d2db6e112..9ff9361487a 100644
> --- a/gdb/nat/amd64-linux-siginfo.c
> +++ b/gdb/nat/amd64-linux-siginfo.c
> @@ -206,7 +206,7 @@ typedef struct compat_siginfo
> /* For x32, clock_t in _sigchld is 64bit aligned at 4 bytes. */
> typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
>
> -typedef struct compat_x32_siginfo
> +typedef struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo
> {
> int si_signo;
> int si_errno;
> @@ -263,7 +263,7 @@ typedef struct compat_x32_siginfo
> int _fd;
> } _sigpoll;
> } _sifields;
> -} compat_x32_siginfo_t __attribute__ ((__aligned__ (8)));
> +} compat_x32_siginfo_t;
>
> /* To simplify usage of siginfo fields. */
Looks good to me. Note that alignas() is part of C++11, so you could
perhaps spell this a bit shorter as "alignas (8)" instead.
--
John Baldwin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c
2021-06-05 0:14 [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Pedro Alves
2021-06-05 0:14 ` [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct Pedro Alves
2021-06-05 0:14 ` [PATCH 2/2] nat/amd64-linux-siginfo.c: Remove typedefs Pedro Alves
@ 2021-06-06 14:41 ` Simon Marchi
2 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2021-06-06 14:41 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 2021-06-04 8:14 p.m., Pedro Alves wrote:
> Building GDB with current git Clang hits a number of problems. This
> fixes one of them.
>
> Pedro Alves (2):
> nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
> nat/amd64-linux-siginfo.c: Remove typedefs
>
> gdb/nat/amd64-linux-siginfo.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
>
> base-commit: 386de171cbffa86e804057030f3d64a404279f43
>
This LGTM, thanks!
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
2021-06-05 21:20 ` John Baldwin
@ 2021-06-07 22:20 ` Pedro Alves
2021-06-07 22:42 ` John Baldwin
0 siblings, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2021-06-07 22:20 UTC (permalink / raw)
To: John Baldwin, gdb-patches
Hi John,
On 2021-06-05 10:20 p.m., John Baldwin wrote:
> On 6/4/21 5:14 PM, Pedro Alves wrote:
>> diff --git a/gdb/nat/amd64-linux-siginfo.c b/gdb/nat/amd64-linux-siginfo.c
>> index e2d2db6e112..9ff9361487a 100644
>> --- a/gdb/nat/amd64-linux-siginfo.c
>> +++ b/gdb/nat/amd64-linux-siginfo.c
>> @@ -206,7 +206,7 @@ typedef struct compat_siginfo
>> /* For x32, clock_t in _sigchld is 64bit aligned at 4 bytes. */
>> typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
>> -typedef struct compat_x32_siginfo
>> +typedef struct __attribute__ ((__aligned__ (8))) compat_x32_siginfo
>> {
>> int si_signo;
>> int si_errno;
>> @@ -263,7 +263,7 @@ typedef struct compat_x32_siginfo
>> int _fd;
>> } _sigpoll;
>> } _sifields;
>> -} compat_x32_siginfo_t __attribute__ ((__aligned__ (8)));
>> +} compat_x32_siginfo_t;
>> /* To simplify usage of siginfo fields. */
>
> Looks good to me. Note that alignas() is part of C++11, so you could
> perhaps spell this a bit shorter as "alignas (8)" instead.
>
That sounded like a good idea at first, however, if we were to do that,
I think we should do it to all __aligned__ attributes in the file. But note
the one seen quoted above. Here again:
typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
We can't use alignas for that, because alignas doesn't work with typedefs.
GCC:
gdb/nat/amd64-linux-siginfo.c:207:14: error: attribute ignored [-Werror=attributes]
207 | typedef long alignas (4) compat_x32_clock_t;
| ^~~~~~~
Clang:
gdb/nat/amd64-linux-siginfo.c:207:14: error: 'alignas' attribute cannot be applied to types
typedef long alignas (4) compat_x32_clock_t;
^
Clang attempt 2:
gdb/nat/amd64-linux-siginfo.c:207:33: error: 'alignas' attribute only applies to variables, data members and tag types
typedef long compat_x32_clock_t alignas (4);
^
See answer 4 here:
https://stackoverflow.com/questions/15788947/where-can-i-use-alignas-in-c11
"These are the only places where the standard says an alignment-specifier (alignas(...)) may be applied.
Note that this does not include typedef declarations nor alias-declarations."
So for consistency I'm leaving the struct attribute as is too.
I'm going to merge the patches now.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct
2021-06-07 22:20 ` Pedro Alves
@ 2021-06-07 22:42 ` John Baldwin
0 siblings, 0 replies; 7+ messages in thread
From: John Baldwin @ 2021-06-07 22:42 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 6/7/21 3:20 PM, Pedro Alves wrote:
> See answer 4 here:
>
> https://stackoverflow.com/questions/15788947/where-can-i-use-alignas-in-c11
>
> "These are the only places where the standard says an alignment-specifier (alignas(...)) may be applied.
> Note that this does not include typedef declarations nor alias-declarations."
>
> So for consistency I'm leaving the struct attribute as is too.
>
> I'm going to merge the patches now.
Ah, I agree that it's good to be consistent.
--
John Baldwin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-07 22:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-05 0:14 [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Pedro Alves
2021-06-05 0:14 ` [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct Pedro Alves
2021-06-05 21:20 ` John Baldwin
2021-06-07 22:20 ` Pedro Alves
2021-06-07 22:42 ` John Baldwin
2021-06-05 0:14 ` [PATCH 2/2] nat/amd64-linux-siginfo.c: Remove typedefs Pedro Alves
2021-06-06 14:41 ` [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).