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

Message ID 20210605001427.3597687-2-pedro@palves.net
State New
Headers show
Series
  • Fix alignment issue flagged by Clang in nat/amd64-linux-siginfo.c
Related show

Commit Message

Pedro Alves June 5, 2021, 12:14 a.m.
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(-)

-- 
2.26.2

Comments

John Baldwin June 5, 2021, 9:20 p.m. | #1
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
Pedro Alves June 7, 2021, 10:20 p.m. | #2
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
John Baldwin June 7, 2021, 10:42 p.m. | #3
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

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