[PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c

[PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c

[Pedro Alves]

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

[PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct

[Pedro Alves]

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

Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct

[John Baldwin]

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

Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct

[Pedro Alves]

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

Re: [PATCH 1/2] nat/amd64-linux-siginfo.c: Move align attribute from typedef to struct

[John Baldwin]

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

[PATCH 2/2] nat/amd64-linux-siginfo.c: Remove typedefs

[Pedro Alves]

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

Re: [PATCH 0/2] Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c

[Simon Marchi]

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