[RFC][PATCH] Update -Wattribute-alias for gcc9

[RFC][PATCH] Update -Wattribute-alias for gcc9

[Laura Abbott]

Commit bee20031772a ("disable -Wattribute-alias warning for
SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
gcc9 changed the format of -Wattribute-alias to take a parameter.
This doesn't quite match with the existing disabling mechanism
so update for gcc9 to match with the default (-Wattribute-alias=1).

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
This is RFC because it feels ugly. I went ahead and did the obvious fixup
but it's worth discussing if we're going to end up with an explosion or
if there's a better way to handle this in one macro.
---
 include/linux/compat.h       | 2 ++
 include/linux/compiler-gcc.h | 7 ++++++-
 include/linux/syscalls.h     | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index 056be0d03722..d5d7700f26a6 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -75,6 +75,8 @@
 	__diag_push();								\
 	__diag_ignore(GCC, 8, "-Wattribute-alias",				\
 		      "Type aliasing is used to sanitize syscall arguments");\
+	__diag_ignore(GCC, 9, "-Wattribute-alias=1",				\
+		      "Type aliasing is used to sanitize syscall arguments");\
 	asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));	\
 	asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))	\
 		__attribute__((alias(__stringify(__se_compat_sys##name))));	\
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e8579412ad21..75079668344c 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -165,8 +165,13 @@
 #define __diag_str(s)		__diag_str1(s)
 #define __diag(s)		_Pragma(__diag_str(GCC diagnostic s))
 
-#if GCC_VERSION >= 80000
+#if GCC_VERSION >= 90000
+#define __diag_GCC_8(s)
+#define __diag_GCC_9(s)		__diag(s)
+#elif GCC_VERSION >= 80000
 #define __diag_GCC_8(s)		__diag(s)
+#define __diag_GCC_9(s)
 #else
 #define __diag_GCC_8(s)
+#define __diag_GCC_9(s)
 #endif
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 257cccba3062..a08059e1ccf4 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -236,6 +236,8 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
 	__diag_push();							\
 	__diag_ignore(GCC, 8, "-Wattribute-alias",			\
 		      "Type aliasing is used to sanitize syscall arguments");\
+	__diag_ignore(GCC, 9, "-Wattribute-alias=1",			\
+		      "Type aliasing is used to sanitize syscall arguments");\
 	asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))	\
 		__attribute__((alias(__stringify(__se_sys##name))));	\
 	ALLOW_ERROR_INJECTION(sys##name, ERRNO);			\
-- 
2.20.1

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Arnd Bergmann]

On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
>
> Commit bee20031772a ("disable -Wattribute-alias warning for
> SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
> gcc9 changed the format of -Wattribute-alias to take a parameter.
> This doesn't quite match with the existing disabling mechanism
> so update for gcc9 to match with the default (-Wattribute-alias=1).
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> This is RFC because it feels ugly. I went ahead and did the obvious fixup
> but it's worth discussing if we're going to end up with an explosion or
> if there's a better way to handle this in one macro.

Bernd Edlinger has sent a patch to gcc for this:
https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01120.html

and Miguel Ojeda said he wanted to send a patch for it to the
kernel as well, not sure if he wanted to take a different
approach there, so adding both to Cc here.

        Arnd




> ---
>  include/linux/compat.h       | 2 ++
>  include/linux/compiler-gcc.h | 7 ++++++-
>  include/linux/syscalls.h     | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 056be0d03722..d5d7700f26a6 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -75,6 +75,8 @@
>         __diag_push();                                                          \
>         __diag_ignore(GCC, 8, "-Wattribute-alias",                              \
>                       "Type aliasing is used to sanitize syscall arguments");\
> +       __diag_ignore(GCC, 9, "-Wattribute-alias=1",                            \
> +                     "Type aliasing is used to sanitize syscall arguments");\
>         asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));       \
>         asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))        \
>                 __attribute__((alias(__stringify(__se_compat_sys##name))));     \
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index e8579412ad21..75079668344c 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -165,8 +165,13 @@
>  #define __diag_str(s)          __diag_str1(s)
>  #define __diag(s)              _Pragma(__diag_str(GCC diagnostic s))
>
> -#if GCC_VERSION >= 80000
> +#if GCC_VERSION >= 90000
> +#define __diag_GCC_8(s)
> +#define __diag_GCC_9(s)                __diag(s)
> +#elif GCC_VERSION >= 80000
>  #define __diag_GCC_8(s)                __diag(s)
> +#define __diag_GCC_9(s)
>  #else
>  #define __diag_GCC_8(s)
> +#define __diag_GCC_9(s)
>  #endif
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 257cccba3062..a08059e1ccf4 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -236,6 +236,8 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
>         __diag_push();                                                  \
>         __diag_ignore(GCC, 8, "-Wattribute-alias",                      \
>                       "Type aliasing is used to sanitize syscall arguments");\
> +       __diag_ignore(GCC, 9, "-Wattribute-alias=1",                    \
> +                     "Type aliasing is used to sanitize syscall arguments");\
>         asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))       \
>                 __attribute__((alias(__stringify(__se_sys##name))));    \
>         ALLOW_ERROR_INJECTION(sys##name, ERRNO);                        \
> --
> 2.20.1
>

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Miguel Ojeda]

On Fri, Jan 25, 2019 at 11:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
> >
> > Commit bee20031772a ("disable -Wattribute-alias warning for
> > SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
> > gcc9 changed the format of -Wattribute-alias to take a parameter.
> > This doesn't quite match with the existing disabling mechanism
> > so update for gcc9 to match with the default (-Wattribute-alias=1).
> >
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > ---
> > This is RFC because it feels ugly. I went ahead and did the obvious fixup
> > but it's worth discussing if we're going to end up with an explosion or
> > if there's a better way to handle this in one macro.
>
> Bernd Edlinger has sent a patch to gcc for this:
> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01120.html
>
> and Miguel Ojeda said he wanted to send a patch for it to the
> kernel as well, not sure if he wanted to take a different
> approach there, so adding both to Cc here.

Thanks Arnd (I was working with Martin on the expanded
-Wmissing-attribute warnings, not on this, but thanks nevertheless :).

Martin/Bernd: from the GCC mailing list I am not sure if we should
expect the old behavior to be maintained or not.

Cheers,
Miguel

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Bernd Edlinger]

On 1/25/19 12:39 PM, Miguel Ojeda wrote:
> On Fri, Jan 25, 2019 at 11:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
>>>
>>> Commit bee20031772a ("disable -Wattribute-alias warning for
>>> SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
>>> gcc9 changed the format of -Wattribute-alias to take a parameter.
>>> This doesn't quite match with the existing disabling mechanism
>>> so update for gcc9 to match with the default (-Wattribute-alias=1).
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>> This is RFC because it feels ugly. I went ahead and did the obvious fixup
>>> but it's worth discussing if we're going to end up with an explosion or
>>> if there's a better way to handle this in one macro.
>>
>> Bernd Edlinger has sent a patch to gcc for this:
>> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01120.html
>>
>> and Miguel Ojeda said he wanted to send a patch for it to the
>> kernel as well, not sure if he wanted to take a different
>> approach there, so adding both to Cc here.
> 
> Thanks Arnd (I was working with Martin on the expanded
> -Wmissing-attribute warnings, not on this, but thanks nevertheless :).
> 
> Martin/Bernd: from the GCC mailing list I am not sure if we should
> expect the old behavior to be maintained or not.
> 

I believe it is not intentional to break the old syntax of the
pragma.  There will be new -Wattribute-alias=1 and -Wattribute-alias=2
and -Wattribute-alias is easy to retain as an alias for -Wattribute-alias=1.
That is what my patch will do.


Bernd.

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Bernd Edlinger]

On 1/25/19 1:24 PM, Bernd Edlinger wrote:
> On 1/25/19 12:39 PM, Miguel Ojeda wrote:
>> On Fri, Jan 25, 2019 at 11:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
>>>>
>>>> Commit bee20031772a ("disable -Wattribute-alias warning for
>>>> SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
>>>> gcc9 changed the format of -Wattribute-alias to take a parameter.
>>>> This doesn't quite match with the existing disabling mechanism
>>>> so update for gcc9 to match with the default (-Wattribute-alias=1).
>>>>
>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>> ---
>>>> This is RFC because it feels ugly. I went ahead and did the obvious fixup
>>>> but it's worth discussing if we're going to end up with an explosion or
>>>> if there's a better way to handle this in one macro.
>>>
>>> Bernd Edlinger has sent a patch to gcc for this:
>>> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01120.html
>>>
>>> and Miguel Ojeda said he wanted to send a patch for it to the
>>> kernel as well, not sure if he wanted to take a different
>>> approach there, so adding both to Cc here.
>>
>> Thanks Arnd (I was working with Martin on the expanded
>> -Wmissing-attribute warnings, not on this, but thanks nevertheless :).
>>
>> Martin/Bernd: from the GCC mailing list I am not sure if we should
>> expect the old behavior to be maintained or not.
>>
> 
> I believe it is not intentional to break the old syntax of the
> pragma.  There will be new -Wattribute-alias=1 and -Wattribute-alias=2
> and -Wattribute-alias is easy to retain as an alias for -Wattribute-alias=1.
> That is what my patch will do.
> 

Okay, I committed the -Wattribute-alias patch to gcc trunk, now
as https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268336 .
So there will be no need for a workaround on your side.

Also fixed a few false positive -Waddress-of-packed-member warnings with
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268118 and
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268337 .

However there remain a lot of warnings from -Waddress-of-packed-member,
that look more or less valid, has anybody an idea how to handle
these?


Bernd.

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Laura Abbott]

On 1/28/19 5:28 AM, Bernd Edlinger wrote:
> On 1/25/19 1:24 PM, Bernd Edlinger wrote:
>> On 1/25/19 12:39 PM, Miguel Ojeda wrote:
>>> On Fri, Jan 25, 2019 at 11:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>>>
>>>> On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
>>>>>
>>>>> Commit bee20031772a ("disable -Wattribute-alias warning for
>>>>> SYSCALL_DEFINEx()") disabled -Wattribute-alias with gcc8.
>>>>> gcc9 changed the format of -Wattribute-alias to take a parameter.
>>>>> This doesn't quite match with the existing disabling mechanism
>>>>> so update for gcc9 to match with the default (-Wattribute-alias=1).
>>>>>
>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>> ---
>>>>> This is RFC because it feels ugly. I went ahead and did the obvious fixup
>>>>> but it's worth discussing if we're going to end up with an explosion or
>>>>> if there's a better way to handle this in one macro.
>>>>
>>>> Bernd Edlinger has sent a patch to gcc for this:
>>>> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01120.html
>>>>
>>>> and Miguel Ojeda said he wanted to send a patch for it to the
>>>> kernel as well, not sure if he wanted to take a different
>>>> approach there, so adding both to Cc here.
>>>
>>> Thanks Arnd (I was working with Martin on the expanded
>>> -Wmissing-attribute warnings, not on this, but thanks nevertheless :).
>>>
>>> Martin/Bernd: from the GCC mailing list I am not sure if we should
>>> expect the old behavior to be maintained or not.
>>>
>>
>> I believe it is not intentional to break the old syntax of the
>> pragma.  There will be new -Wattribute-alias=1 and -Wattribute-alias=2
>> and -Wattribute-alias is easy to retain as an alias for -Wattribute-alias=1.
>> That is what my patch will do.
>>
> 
> Okay, I committed the -Wattribute-alias patch to gcc trunk, now
> as https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268336 .
> So there will be no need for a workaround on your side.
> 
> Also fixed a few false positive -Waddress-of-packed-member warnings with
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268118 and
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268337 .
> 
> However there remain a lot of warnings from -Waddress-of-packed-member,
> that look more or less valid, has anybody an idea how to handle
> these?
> 
> 
> Bernd.
> 

Thanks, I'll keep an eye out when this lands in Fedora gcc.
I think once everything lands it will be easier to work on
the remaining -Waddress-of-packed-member.

Laura

Re: [RFC][PATCH] Update -Wattribute-alias for gcc9

[Arnd Bergmann]

On Mon, Jan 28, 2019 at 2:28 PM Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> On 1/25/19 1:24 PM, Bernd Edlinger wrote:
> > On 1/25/19 12:39 PM, Miguel Ojeda wrote:
> >> On Fri, Jan 25, 2019 at 11:58 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >>>
> >>> On Fri, Jan 25, 2019 at 11:43 AM Laura Abbott <labbott@redhat.com> wrote:
> >
> > I believe it is not intentional to break the old syntax of the
> > pragma.  There will be new -Wattribute-alias=1 and -Wattribute-alias=2
> > and -Wattribute-alias is easy to retain as an alias for -Wattribute-alias=1.
> > That is what my patch will do.
> >
>
> Okay, I committed the -Wattribute-alias patch to gcc trunk, now
> as https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268336 .
> So there will be no need for a workaround on your side.
>
> Also fixed a few false positive -Waddress-of-packed-member warnings with
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268118 and
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=268337 .
>
> However there remain a lot of warnings from -Waddress-of-packed-member,
> that look more or less valid, has anybody an idea how to handle
> these?

The best idea I have is "one at a time", which unfortunately
does mean a lot of work. We can degrade the warning to the
W=1 level in the kernel by disabling it by default and re-enabling
it in scripts/Makefile.extrawarn, but we probably want to address
many of them anyway. Looking at the most common examples
(listed by number of instances in Laura's build log)

     64  warning: taking address of packed member of 'struct
scif_window' may result in an unaligned pointer value
[-Waddress-of-packed-member]

Structure should not be packed at all, it's kernel internal (I hope at least,
since it contains pointers to kernel structures), and packing it does lead to
unaligned accesses.

     71  warning: converting a packed 'struct desc_struct' pointer
(alignment 1) to a 'u32' {aka 'unsigned int'} pointer (alignment 4)
may result in an unaligned pointer value [-Waddress-of-packed-member]

'__packed' does not seem to be required here. This is a structure of bit
fields, which IIRC means the layout is architecture dependent, but
it is fully packed already.

     76  warning: converting a packed 'struct opa_smp' pointer
(alignment 1) to a 'struct ib_mad_hdr' pointer (alignment 8) may
result in an unaligned pointer value [-Waddress-of-packed-member]

We cast the pointers multiple times, from aligned to packed and back to aligned,
so I assume there is no bug, but it's unclear why the structure was ever marked
as packed.

     81  warning: taking address of packed member of 'struct
adf_accel_dev' may result in an unaligned pointer value
[-Waddress-of-packed-member]

Should clearly not be packed.

    122  warning: taking address of packed member of 'struct
fc_els_flogi' may result in an unaligned pointer value
[-Waddress-of-packed-member]

I'd suggest changing it to only mark the unaligned members as packed,
not the structure itself.

    182  warning: taking address of packed member of 'struct
rtl818x_csr' may result in an unaligned pointer value
[-Waddress-of-packed-member]

Cannot be packed, since this is an mmio structure, need to
carefully rework structure layout in case there are some packed
members we never access.

   138  warning: taking address of packed member of 'struct ib_reth'
may result in an unaligned pointer value [-Waddress-of-packed-member]
    414  warning: taking address of packed member of 'struct
ib_atomic_eth' may result in an unaligned pointer value
[-Waddress-of-packed-member]

I suppose ib_u64_get should take a packed argument, it already uses
get_unaligned_be64()
internally to avoid bugs.

       Arnd