[PATCH 0/2] kbuild: clang: Disable spurious warnings

[PATCH 0/2] kbuild: clang: Disable spurious warnings

[Matthias Kaehlcke]

Disable two clang warnings that generate a lot of noise.

Matthias Kaehlcke (2):
  kbuild: clang: Disable 'address-of-packed-member' warning
  kbuild: clang: Disable the 'duplicate-decl-specifier' warning

 Makefile | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.12.2.816.g2cccc81164-goog

[PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Matthias Kaehlcke]

clang generates plenty of these warnings in different parts of the code,
to an extent that the warnings are little more than noise. Disable the
'address-of-packed-member' warning.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 5039b9148d15..df5abf346354 100644
--- a/Makefile
+++ b/Makefile
@@ -703,6 +703,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
 KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
+KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
 # Quiet clang warning: comparison of unsigned expression < 0 is always false
 KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
 # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
-- 
2.12.2.816.g2cccc81164-goog

[PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

clang generates plenty of these warnings in different parts of the code.
They are mostly caused by container_of() and other macros which declare
a "const <type> *" variable for their internal use which triggers a
"duplicate 'const' specifier" warning if the <type> is already const
qualified.

Wording-mostly-from: Michael Davidson <md@google.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index df5abf346354..6cd6d428db43 100644
--- a/Makefile
+++ b/Makefile
@@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
 KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
+KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier)
 # Quiet clang warning: comparison of unsigned expression < 0 is always false
 KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
 # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
-- 
2.12.2.816.g2cccc81164-goog

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Masahiro Yamada]

Hi Matthias,



2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> clang generates plenty of these warnings in different parts of the code,
> to an extent that the warnings are little more than noise. Disable the
> 'address-of-packed-member' warning.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>


As far as I compiled arch/x86/configs/x86_64_defconfig,
all address-of-packed-member warnings came from the single point:

./arch/x86/include/asm/processor.h:534:30: warning: taking address of
packed member 'sp0' of class or structure 'x86_hw_tss' may result in
an unaligned pointer value [-Waddress-of-packed-member]
        return this_cpu_read_stable(cpu_tss.x86_tss.sp0);
                                    ^~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/percpu.h:391:59: note: expanded from macro
'this_cpu_read_stable'
#define this_cpu_read_stable(var)       percpu_stable_op("mov", var)
                                                                ^~~
./arch/x86/include/asm/percpu.h:228:16: note: expanded from macro
'percpu_stable_op'
                    : "p" (&(var)));                    \
                             ^~~



For this case, I was able to fix it with the following patch:


diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 9fa0360..de25d1c 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -211,26 +211,27 @@ do {
        \
 #define percpu_stable_op(op, var)                      \
 ({                                                     \
        typeof(var) pfo_ret__;                          \
+       void *__p = &(var);                             \
        switch (sizeof(var)) {                          \
        case 1:                                         \
                asm(op "b "__percpu_arg(P1)",%0"        \
                    : "=q" (pfo_ret__)                  \
-                   : "p" (&(var)));                    \
+                   : "p" (__p));                       \
                break;                                  \
        case 2:                                         \
                asm(op "w "__percpu_arg(P1)",%0"        \
                    : "=r" (pfo_ret__)                  \
-                   : "p" (&(var)));                    \
+                   : "p" (__p));                       \
                break;                                  \
        case 4:                                         \
                asm(op "l "__percpu_arg(P1)",%0"        \
                    : "=r" (pfo_ret__)                  \
-                   : "p" (&(var)));                    \
+                   : "p" (__p));                       \
                break;                                  \
        case 8:                                         \
                asm(op "q "__percpu_arg(P1)",%0"        \
                    : "=r" (pfo_ret__)                  \
-                   : "p" (&(var)));                    \
+                   : "p" (__p));                       \
                break;                                  \
        default: __bad_percpu_size();                   \
        }                                               \





I'd like to see as much effort as possible
before we decide to hide this kind of warning.

Is it OK with you ?


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Masahiro Yamada]

Hi Matthias,


2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> clang generates plenty of these warnings in different parts of the code.
> They are mostly caused by container_of() and other macros which declare
> a "const <type> *" variable for their internal use which triggers a
> "duplicate 'const' specifier" warning if the <type> is already const
> qualified.
>
> Wording-mostly-from: Michael Davidson <md@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>



I think container_of() can be more simple,
dropping the 'const'.

The following patch worked for me.


diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4c26dc3..d53672b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -846,11 +846,9 @@ static inline void ftrace_dump(enum
ftrace_dump_mode oops_dump_mode) { }
  * @ptr:       the pointer to the member.
  * @type:      the type of the container struct this is embedded in.
  * @member:    the name of the member within the struct.
- *
  */
 #define container_of(ptr, type, member) ({                     \
-       const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
-       (type *)( (char *)__mptr - offsetof(type,member) );})
+       (type *)((void *)(ptr) - offsetof(type, member));})

 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD



For also this one,
I'd like to try to fix the code rather than hiding warnings.

(We may end up with applying your patch after all,
but I'd like to think about it for now.)



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Matthias Kaehlcke]

Hi Masahiro,

El Sun, Apr 30, 2017 at 10:59:52PM +0900 Masahiro Yamada ha dit:

> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> > clang generates plenty of these warnings in different parts of the code,
> > to an extent that the warnings are little more than noise. Disable the
> > 'address-of-packed-member' warning.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> 
> As far as I compiled arch/x86/configs/x86_64_defconfig,
> all address-of-packed-member warnings came from the single point:
> 
> ./arch/x86/include/asm/processor.h:534:30: warning: taking address of
> packed member 'sp0' of class or structure 'x86_hw_tss' may result in
> an unaligned pointer value [-Waddress-of-packed-member]
>         return this_cpu_read_stable(cpu_tss.x86_tss.sp0);
>                                     ^~~~~~~~~~~~~~~~~~~
> ./arch/x86/include/asm/percpu.h:391:59: note: expanded from macro
> 'this_cpu_read_stable'
> #define this_cpu_read_stable(var)       percpu_stable_op("mov", var)
>                                                                 ^~~
> ./arch/x86/include/asm/percpu.h:228:16: note: expanded from macro
> 'percpu_stable_op'
>                     : "p" (&(var)));                    \
>                              ^~~
> 
> 
> 
> For this case, I was able to fix it with the following patch:
> 
> 
> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> index 9fa0360..de25d1c 100644
> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -211,26 +211,27 @@ do {
>         \
>  #define percpu_stable_op(op, var)                      \
>  ({                                                     \
>         typeof(var) pfo_ret__;                          \
> +       void *__p = &(var);                             \
>         switch (sizeof(var)) {                          \
>         case 1:                                         \
>                 asm(op "b "__percpu_arg(P1)",%0"        \
>                     : "=q" (pfo_ret__)                  \
> -                   : "p" (&(var)));                    \
> +                   : "p" (__p));                       \
>                 break;                                  \
>         case 2:                                         \
>                 asm(op "w "__percpu_arg(P1)",%0"        \
>                     : "=r" (pfo_ret__)                  \
> -                   : "p" (&(var)));                    \
> +                   : "p" (__p));                       \
>                 break;                                  \
>         case 4:                                         \
>                 asm(op "l "__percpu_arg(P1)",%0"        \
>                     : "=r" (pfo_ret__)                  \
> -                   : "p" (&(var)));                    \
> +                   : "p" (__p));                       \
>                 break;                                  \
>         case 8:                                         \
>                 asm(op "q "__percpu_arg(P1)",%0"        \
>                     : "=r" (pfo_ret__)                  \
> -                   : "p" (&(var)));                    \
> +                   : "p" (__p));                       \
>                 break;                                  \
>         default: __bad_percpu_size();                   \
>         }                                               \

Thanks for having a look!

It is odd though that you only see warnings from that origin, I
encounter plenty of others with x86_64_defconfig, mostly stemming
from uaccess macros:

kernel/power/user.c:439:35: warning: taking address of packed member
'dev' of class or structure 'compat_resume_swap_area' may result in an
unaligned pointer value [-Waddress-of-packed-member]
                err |= get_user(swap_area.dev, &u_swap_area->dev);
                                                ^~~~~~~~~~~~~~~~
./arch/x86/include/asm/uaccess.h:168:23: note: expanded from macro 'get_user'
        register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);            \
                             ^~~
./arch/x86/include/asm/uaccess.h:132:41: note: expanded from macro '__inttype'
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
                                        ^

I looked into fixing different cases, but didn't see a clear path
forward since we can't just cast the type away as in your patch above.

> I'd like to see as much effort as possible
> before we decide to hide this kind of warning.
> 
> Is it OK with you ?

Sounds good, though I will probably leave this one for someone else :)

Cheers

Matthias

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

Hi Masahiro,

El Sun, Apr 30, 2017 at 11:01:11PM +0900 Masahiro Yamada ha dit:

> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> > clang generates plenty of these warnings in different parts of the code.
> > They are mostly caused by container_of() and other macros which declare
> > a "const <type> *" variable for their internal use which triggers a
> > "duplicate 'const' specifier" warning if the <type> is already const
> > qualified.
> >
> > Wording-mostly-from: Michael Davidson <md@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> 
> 
> I think container_of() can be more simple,
> dropping the 'const'.
> 
> The following patch worked for me.
> 
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c26dc3..d53672b 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -846,11 +846,9 @@ static inline void ftrace_dump(enum
> ftrace_dump_mode oops_dump_mode) { }
>   * @ptr:       the pointer to the member.
>   * @type:      the type of the container struct this is embedded in.
>   * @member:    the name of the member within the struct.
> - *
>   */
>  #define container_of(ptr, type, member) ({                     \
> -       const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
> -       (type *)( (char *)__mptr - offsetof(type,member) );})
> +       (type *)((void *)(ptr) - offsetof(type, member));})
> 
>  /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD

Thanks, this eliminates indeed a huge amount of these warnings.

The other big source of warnings is MODULE_DEVICE_TABLE which declares
a const alias of a type that in most cases is already const. One possible
solution would be to remove the 'additional' const qualifier, which
might leave some tables non-const. Another option could be some
hackery to suppress the warning just for the macro. Not sure if any of
this would be acceptable.

> For also this one,
> I'd like to try to fix the code rather than hiding warnings.

I totally agree with the general approach. My clang kernel builds
started with plenty of warnings disabled. I fixed the code for most of
them until I was left with these two extremely noisy ones, for which I
didn't see a clear path.

Cheers

Matthias

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Masahiro Yamada]

Hi Matthias,


2017-05-02 10:23 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> Hi Masahiro,
>
> El Sun, Apr 30, 2017 at 10:59:52PM +0900 Masahiro Yamada ha dit:
>
>> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
>> > clang generates plenty of these warnings in different parts of the code,
>> > to an extent that the warnings are little more than noise. Disable the
>> > 'address-of-packed-member' warning.
>> >
>> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>
>>
>> As far as I compiled arch/x86/configs/x86_64_defconfig,
>> all address-of-packed-member warnings came from the single point:
>>
>> ./arch/x86/include/asm/processor.h:534:30: warning: taking address of
>> packed member 'sp0' of class or structure 'x86_hw_tss' may result in
>> an unaligned pointer value [-Waddress-of-packed-member]
>>         return this_cpu_read_stable(cpu_tss.x86_tss.sp0);
>>                                     ^~~~~~~~~~~~~~~~~~~
>> ./arch/x86/include/asm/percpu.h:391:59: note: expanded from macro
>> 'this_cpu_read_stable'
>> #define this_cpu_read_stable(var)       percpu_stable_op("mov", var)
>>                                                                 ^~~
>> ./arch/x86/include/asm/percpu.h:228:16: note: expanded from macro
>> 'percpu_stable_op'
>>                     : "p" (&(var)));                    \
>>                              ^~~
>>
>>
>>
>> For this case, I was able to fix it with the following patch:
>>
>>
>> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
>> index 9fa0360..de25d1c 100644
>> --- a/arch/x86/include/asm/percpu.h
>> +++ b/arch/x86/include/asm/percpu.h
>> @@ -211,26 +211,27 @@ do {
>>         \
>>  #define percpu_stable_op(op, var)                      \
>>  ({                                                     \
>>         typeof(var) pfo_ret__;                          \
>> +       void *__p = &(var);                             \
>>         switch (sizeof(var)) {                          \
>>         case 1:                                         \
>>                 asm(op "b "__percpu_arg(P1)",%0"        \
>>                     : "=q" (pfo_ret__)                  \
>> -                   : "p" (&(var)));                    \
>> +                   : "p" (__p));                       \
>>                 break;                                  \
>>         case 2:                                         \
>>                 asm(op "w "__percpu_arg(P1)",%0"        \
>>                     : "=r" (pfo_ret__)                  \
>> -                   : "p" (&(var)));                    \
>> +                   : "p" (__p));                       \
>>                 break;                                  \
>>         case 4:                                         \
>>                 asm(op "l "__percpu_arg(P1)",%0"        \
>>                     : "=r" (pfo_ret__)                  \
>> -                   : "p" (&(var)));                    \
>> +                   : "p" (__p));                       \
>>                 break;                                  \
>>         case 8:                                         \
>>                 asm(op "q "__percpu_arg(P1)",%0"        \
>>                     : "=r" (pfo_ret__)                  \
>> -                   : "p" (&(var)));                    \
>> +                   : "p" (__p));                       \
>>                 break;                                  \
>>         default: __bad_percpu_size();                   \
>>         }                                               \
>
> Thanks for having a look!
>
> It is odd though that you only see warnings from that origin, I
> encounter plenty of others with x86_64_defconfig, mostly stemming
> from uaccess macros:
>
> kernel/power/user.c:439:35: warning: taking address of packed member
> 'dev' of class or structure 'compat_resume_swap_area' may result in an
> unaligned pointer value [-Waddress-of-packed-member]
>                 err |= get_user(swap_area.dev, &u_swap_area->dev);
>                                                 ^~~~~~~~~~~~~~~~
> ./arch/x86/include/asm/uaccess.h:168:23: note: expanded from macro 'get_user'
>         register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);            \
>                              ^~~
> ./arch/x86/include/asm/uaccess.h:132:41: note: expanded from macro '__inttype'
> __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>                                         ^
>
> I looked into fixing different cases, but didn't see a clear path
> forward since we can't just cast the type away as in your patch above.


Curious.
I tested clang 3.0 thru 4.0, but I could not reproduce this.

This part just calculates sizeof(*(ptr)).
I think it is a false positive warning bug if clang reports this.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Masahiro Yamada]

Hi Matthias,


2017-05-05 4:50 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> Hi Masahiro,
>
> El Sun, Apr 30, 2017 at 11:01:11PM +0900 Masahiro Yamada ha dit:
>
>> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
>> > clang generates plenty of these warnings in different parts of the code.
>> > They are mostly caused by container_of() and other macros which declare
>> > a "const <type> *" variable for their internal use which triggers a
>> > "duplicate 'const' specifier" warning if the <type> is already const
>> > qualified.
>> >
>> > Wording-mostly-from: Michael Davidson <md@google.com>
>> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>
>>
>>
>> I think container_of() can be more simple,
>> dropping the 'const'.
>>
>> The following patch worked for me.
>>
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 4c26dc3..d53672b 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -846,11 +846,9 @@ static inline void ftrace_dump(enum
>> ftrace_dump_mode oops_dump_mode) { }
>>   * @ptr:       the pointer to the member.
>>   * @type:      the type of the container struct this is embedded in.
>>   * @member:    the name of the member within the struct.
>> - *
>>   */
>>  #define container_of(ptr, type, member) ({                     \
>> -       const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
>> -       (type *)( (char *)__mptr - offsetof(type,member) );})
>> +       (type *)((void *)(ptr) - offsetof(type, member));})
>>
>>  /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
>>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>
> Thanks, this eliminates indeed a huge amount of these warnings.


I noticed this solution will drop precious type checking
from container_of().  So, my workaround is not nice.



> The other big source of warnings is MODULE_DEVICE_TABLE which declares
> a const alias of a type that in most cases is already const. One possible
> solution would be to remove the 'additional' const qualifier, which
> might leave some tables non-const. Another option could be some
> hackery to suppress the warning just for the macro. Not sure if any of
> this would be acceptable.

Me neither.


>> For also this one,
>> I'd like to try to fix the code rather than hiding warnings.
>
> I totally agree with the general approach. My clang kernel builds
> started with plenty of warnings disabled. I fixed the code for most of
> them until I was left with these two extremely noisy ones, for which I
> didn't see a clear path.


If we do not come up with a good solution,
I will pick up this patch.





-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Matthias Kaehlcke]

Hi Masahiro,

El Sun, May 07, 2017 at 01:52:25AM +0900 Masahiro Yamada ha dit:

> 2017-05-02 10:23 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> > Hi Masahiro,
> >
> > El Sun, Apr 30, 2017 at 10:59:52PM +0900 Masahiro Yamada ha dit:
> >
> >> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> >> > clang generates plenty of these warnings in different parts of the code,
> >> > to an extent that the warnings are little more than noise. Disable the
> >> > 'address-of-packed-member' warning.
> >> >
> >> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>
> >>
> >> As far as I compiled arch/x86/configs/x86_64_defconfig,
> >> all address-of-packed-member warnings came from the single point:
> >>
> >> ./arch/x86/include/asm/processor.h:534:30: warning: taking address of
> >> packed member 'sp0' of class or structure 'x86_hw_tss' may result in
> >> an unaligned pointer value [-Waddress-of-packed-member]
> >>         return this_cpu_read_stable(cpu_tss.x86_tss.sp0);
> >>                                     ^~~~~~~~~~~~~~~~~~~
> >> ./arch/x86/include/asm/percpu.h:391:59: note: expanded from macro
> >> 'this_cpu_read_stable'
> >> #define this_cpu_read_stable(var)       percpu_stable_op("mov", var)
> >>                                                                 ^~~
> >> ./arch/x86/include/asm/percpu.h:228:16: note: expanded from macro
> >> 'percpu_stable_op'
> >>                     : "p" (&(var)));                    \
> >>                              ^~~
> >>
> >>
> >>
> >> For this case, I was able to fix it with the following patch:
> >>
> >>
> >> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> >> index 9fa0360..de25d1c 100644
> >> --- a/arch/x86/include/asm/percpu.h
> >> +++ b/arch/x86/include/asm/percpu.h
> >> @@ -211,26 +211,27 @@ do {
> >>         \
> >>  #define percpu_stable_op(op, var)                      \
> >>  ({                                                     \
> >>         typeof(var) pfo_ret__;                          \
> >> +       void *__p = &(var);                             \
> >>         switch (sizeof(var)) {                          \
> >>         case 1:                                         \
> >>                 asm(op "b "__percpu_arg(P1)",%0"        \
> >>                     : "=q" (pfo_ret__)                  \
> >> -                   : "p" (&(var)));                    \
> >> +                   : "p" (__p));                       \
> >>                 break;                                  \
> >>         case 2:                                         \
> >>                 asm(op "w "__percpu_arg(P1)",%0"        \
> >>                     : "=r" (pfo_ret__)                  \
> >> -                   : "p" (&(var)));                    \
> >> +                   : "p" (__p));                       \
> >>                 break;                                  \
> >>         case 4:                                         \
> >>                 asm(op "l "__percpu_arg(P1)",%0"        \
> >>                     : "=r" (pfo_ret__)                  \
> >> -                   : "p" (&(var)));                    \
> >> +                   : "p" (__p));                       \
> >>                 break;                                  \
> >>         case 8:                                         \
> >>                 asm(op "q "__percpu_arg(P1)",%0"        \
> >>                     : "=r" (pfo_ret__)                  \
> >> -                   : "p" (&(var)));                    \
> >> +                   : "p" (__p));                       \
> >>                 break;                                  \
> >>         default: __bad_percpu_size();                   \
> >>         }                                               \
> >
> > Thanks for having a look!
> >
> > It is odd though that you only see warnings from that origin, I
> > encounter plenty of others with x86_64_defconfig, mostly stemming
> > from uaccess macros:
> >
> > kernel/power/user.c:439:35: warning: taking address of packed member
> > 'dev' of class or structure 'compat_resume_swap_area' may result in an
> > unaligned pointer value [-Waddress-of-packed-member]
> >                 err |= get_user(swap_area.dev, &u_swap_area->dev);
> >                                                 ^~~~~~~~~~~~~~~~
> > ./arch/x86/include/asm/uaccess.h:168:23: note: expanded from macro 'get_user'
> >         register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);            \
> >                              ^~~
> > ./arch/x86/include/asm/uaccess.h:132:41: note: expanded from macro '__inttype'
> > __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> >                                         ^
> >
> > I looked into fixing different cases, but didn't see a clear path
> > forward since we can't just cast the type away as in your patch above.
> 
> 
> Curious.
> I tested clang 3.0 thru 4.0, but I could not reproduce this.
> 
> This part just calculates sizeof(*(ptr)).
> I think it is a false positive warning bug if clang reports this.

The instance above is indeed somewhat doubtful, in any case there are
plenty of others, most of them from fs/compat.c using __get/put_user_xyz():

fs/compat.c:366:33: warning: taking address of packed member 'l_whence' of class or structure 'compat_flock64' may result in an unaligned pointer value [-Waddress-of-packed-member]
            __get_user(kfl->l_whence, &ufl->l_whence) ||
                                       ^~~~~~~~~~~~~
arch/x86/include/asm/uaccess.h:505:27: note: expanded from macro '__get_user'
        __get_user_nocheck((x), (ptr), sizeof(*(ptr)))
                                 ^~~
arch/x86/include/asm/uaccess.h:436:29: note: expanded from macro '__get_user_nocheck'
        __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
                                   ^~~
arch/x86/include/asm/uaccess.h:361:21: note: expanded from macro '__get_user_size'
                __get_user_asm(x, ptr, retval, "w", "w", "=r", errret); \
                                  ^~~
arch/x86/include/asm/uaccess.h:385:19: note: expanded from macro '__get_user_asm'
                     : "m" (__m(addr)), "i" (errret), "0" (err))
                                ^~~~
arch/x86/include/asm/uaccess.h:444:51: note: expanded from macro '__m'
#define __m(x) (*(struct __large_struct __user *)(x))
                                                  ^

The clang version I use is fairly recent since it includes some
fixes needed to build a working kernel (mostly for ARM64).

clang --version
Chromium OS 5.0_pre300080-r1 clang version 5.0.0

Cheers

Matthias

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Masahiro Yamada]

Hi Matthias,

Sorry for my late reply.

2017-05-09 8:18 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> Hi Masahiro,
>
> El Sun, May 07, 2017 at 01:52:25AM +0900 Masahiro Yamada ha dit:
>
>> 2017-05-02 10:23 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
>> > Hi Masahiro,
>> >
>> > El Sun, Apr 30, 2017 at 10:59:52PM +0900 Masahiro Yamada ha dit:
>> >
>> >> 2017-04-22 6:39 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
>> >> > clang generates plenty of these warnings in different parts of the code,
>> >> > to an extent that the warnings are little more than noise. Disable the
>> >> > 'address-of-packed-member' warning.
>> >> >
>> >> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> >>
>> >>
>> >> As far as I compiled arch/x86/configs/x86_64_defconfig,
>> >> all address-of-packed-member warnings came from the single point:
>> >>
>> >> ./arch/x86/include/asm/processor.h:534:30: warning: taking address of
>> >> packed member 'sp0' of class or structure 'x86_hw_tss' may result in
>> >> an unaligned pointer value [-Waddress-of-packed-member]
>> >>         return this_cpu_read_stable(cpu_tss.x86_tss.sp0);
>> >>                                     ^~~~~~~~~~~~~~~~~~~
>> >> ./arch/x86/include/asm/percpu.h:391:59: note: expanded from macro
>> >> 'this_cpu_read_stable'
>> >> #define this_cpu_read_stable(var)       percpu_stable_op("mov", var)
>> >>                                                                 ^~~
>> >> ./arch/x86/include/asm/percpu.h:228:16: note: expanded from macro
>> >> 'percpu_stable_op'
>> >>                     : "p" (&(var)));                    \
>> >>                              ^~~
>> >>
>> >>
>> >>
>> >> For this case, I was able to fix it with the following patch:
>> >>
>> >>
>> >> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
>> >> index 9fa0360..de25d1c 100644
>> >> --- a/arch/x86/include/asm/percpu.h
>> >> +++ b/arch/x86/include/asm/percpu.h
>> >> @@ -211,26 +211,27 @@ do {
>> >>         \
>> >>  #define percpu_stable_op(op, var)                      \
>> >>  ({                                                     \
>> >>         typeof(var) pfo_ret__;                          \
>> >> +       void *__p = &(var);                             \
>> >>         switch (sizeof(var)) {                          \
>> >>         case 1:                                         \
>> >>                 asm(op "b "__percpu_arg(P1)",%0"        \
>> >>                     : "=q" (pfo_ret__)                  \
>> >> -                   : "p" (&(var)));                    \
>> >> +                   : "p" (__p));                       \
>> >>                 break;                                  \
>> >>         case 2:                                         \
>> >>                 asm(op "w "__percpu_arg(P1)",%0"        \
>> >>                     : "=r" (pfo_ret__)                  \
>> >> -                   : "p" (&(var)));                    \
>> >> +                   : "p" (__p));                       \
>> >>                 break;                                  \
>> >>         case 4:                                         \
>> >>                 asm(op "l "__percpu_arg(P1)",%0"        \
>> >>                     : "=r" (pfo_ret__)                  \
>> >> -                   : "p" (&(var)));                    \
>> >> +                   : "p" (__p));                       \
>> >>                 break;                                  \
>> >>         case 8:                                         \
>> >>                 asm(op "q "__percpu_arg(P1)",%0"        \
>> >>                     : "=r" (pfo_ret__)                  \
>> >> -                   : "p" (&(var)));                    \
>> >> +                   : "p" (__p));                       \
>> >>                 break;                                  \
>> >>         default: __bad_percpu_size();                   \
>> >>         }                                               \
>> >
>> > Thanks for having a look!
>> >
>> > It is odd though that you only see warnings from that origin, I
>> > encounter plenty of others with x86_64_defconfig, mostly stemming
>> > from uaccess macros:
>> >
>> > kernel/power/user.c:439:35: warning: taking address of packed member
>> > 'dev' of class or structure 'compat_resume_swap_area' may result in an
>> > unaligned pointer value [-Waddress-of-packed-member]
>> >                 err |= get_user(swap_area.dev, &u_swap_area->dev);
>> >                                                 ^~~~~~~~~~~~~~~~
>> > ./arch/x86/include/asm/uaccess.h:168:23: note: expanded from macro 'get_user'
>> >         register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);            \
>> >                              ^~~
>> > ./arch/x86/include/asm/uaccess.h:132:41: note: expanded from macro '__inttype'
>> > __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>> >                                         ^
>> >
>> > I looked into fixing different cases, but didn't see a clear path
>> > forward since we can't just cast the type away as in your patch above.
>>
>>
>> Curious.
>> I tested clang 3.0 thru 4.0, but I could not reproduce this.
>>
>> This part just calculates sizeof(*(ptr)).
>> I think it is a false positive warning bug if clang reports this.
>
> The instance above is indeed somewhat doubtful, in any case there are
> plenty of others, most of them from fs/compat.c using __get/put_user_xyz():
>
> fs/compat.c:366:33: warning: taking address of packed member 'l_whence' of class or structure 'compat_flock64' may result in an unaligned pointer value [-Waddress-of-packed-member]
>             __get_user(kfl->l_whence, &ufl->l_whence) ||
>                                        ^~~~~~~~~~~~~
> arch/x86/include/asm/uaccess.h:505:27: note: expanded from macro '__get_user'
>         __get_user_nocheck((x), (ptr), sizeof(*(ptr)))
>                                  ^~~
> arch/x86/include/asm/uaccess.h:436:29: note: expanded from macro '__get_user_nocheck'
>         __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
>                                    ^~~
> arch/x86/include/asm/uaccess.h:361:21: note: expanded from macro '__get_user_size'
>                 __get_user_asm(x, ptr, retval, "w", "w", "=r", errret); \
>                                   ^~~
> arch/x86/include/asm/uaccess.h:385:19: note: expanded from macro '__get_user_asm'
>                      : "m" (__m(addr)), "i" (errret), "0" (err))
>                                 ^~~~
> arch/x86/include/asm/uaccess.h:444:51: note: expanded from macro '__m'
> #define __m(x) (*(struct __large_struct __user *)(x))
>                                                   ^
>
> The clang version I use is fairly recent since it includes some
> fixes needed to build a working kernel (mostly for ARM64).
>
> clang --version
> Chromium OS 5.0_pre300080-r1 clang version 5.0.0
>
> Cheers
>
> Matthias


OK.  I understood it is difficult to eliminate these warnings.
I consider this series for v4.13 (unless somebody comes up with a
better solution).


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Doug Anderson]

Hi,

On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> clang generates plenty of these warnings in different parts of the code,
> to an extent that the warnings are little more than noise. Disable the
> 'address-of-packed-member' warning.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index 5039b9148d15..df5abf346354 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -703,6 +703,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
>  KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the

Though I'm no expert, after reading the discussion on this thread and
looking at the examples provided by Matthias, this seems sane to me.
Thus, FWIW:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Doug Anderson]

Hi

On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> clang generates plenty of these warnings in different parts of the code.
> They are mostly caused by container_of() and other macros which declare
> a "const <type> *" variable for their internal use which triggers a
> "duplicate 'const' specifier" warning if the <type> is already const
> qualified.
>
> Wording-mostly-from: Michael Davidson <md@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index df5abf346354..6cd6d428db43 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>  KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier)
>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the

It seems like gcc 7 may have the same warning.  Specifically I saw a
patch fly by from Arnd, which you can find in Mark Brown's tree now:

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c


+Arnd since he may be trying to solve the same issues?

-Doug

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Arnd Bergmann]

On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi
>
> On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> clang generates plenty of these warnings in different parts of the code.
>> They are mostly caused by container_of() and other macros which declare
>> a "const <type> *" variable for their internal use which triggers a
>> "duplicate 'const' specifier" warning if the <type> is already const
>> qualified.
>>
>> Wording-mostly-from: Michael Davidson <md@google.com>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  Makefile | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Makefile b/Makefile
>> index df5abf346354..6cd6d428db43 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier)
>>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
>>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
>
> It seems like gcc 7 may have the same warning.  Specifically I saw a
> patch fly by from Arnd, which you can find in Mark Brown's tree now:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c
>
>
> +Arnd since he may be trying to solve the same issues?

gcc-7 has a warning option with the same name, and I think I have
fixed all the occurrences we got in mainline (some patches my still
be in flight). However, it seems that only clang warns about
'const typeof(type)' with 'type' being already const.

I have not looked at clang warnings in a while, how many of these do
we get overall (aside from container_of)? We might be able
to turn off this particular warning by sprinkling in
'_Pragma("clang diagnostic push") _Pragma("clang diagnostic
ignored \"-Wduplicate-decl-specifier\"")' inside of the macro
(not sure if clang allows it there, gcc-4.4 and earlier I think did
not).

It might also be useful to open a bug against clang so they can
change it in future releases, as the gcc behavior seems more
sensible in this instance.

       Arnd

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:

> On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
> > Hi
> >
> > On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> clang generates plenty of these warnings in different parts of the code.
> >> They are mostly caused by container_of() and other macros which declare
> >> a "const <type> *" variable for their internal use which triggers a
> >> "duplicate 'const' specifier" warning if the <type> is already const
> >> qualified.
> >>
> >> Wording-mostly-from: Michael Davidson <md@google.com>
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>  Makefile | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index df5abf346354..6cd6d428db43 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> >>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> >>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> >>  KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> >> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier)
> >>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
> >>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> >>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
> >
> > It seems like gcc 7 may have the same warning.  Specifically I saw a
> > patch fly by from Arnd, which you can find in Mark Brown's tree now:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c
> >
> >
> > +Arnd since he may be trying to solve the same issues?
> 
> gcc-7 has a warning option with the same name, and I think I have
> fixed all the occurrences we got in mainline (some patches my still
> be in flight). However, it seems that only clang warns about
> 'const typeof(type)' with 'type' being already const.
> 
> I have not looked at clang warnings in a while, how many of these do
> we get overall (aside from container_of)? We might be able
> to turn off this particular warning by sprinkling in
> '_Pragma("clang diagnostic push") _Pragma("clang diagnostic
> ignored \"-Wduplicate-decl-specifier\"")' inside of the macro
> (not sure if clang allows it there, gcc-4.4 and earlier I think did
> not).
> 
> It might also be useful to open a bug against clang so they can
> change it in future releases, as the gcc behavior seems more
> sensible in this instance.

I asked our toolchain folks to follow up with the clang devs. They
asked me for a simple test case, to my suprise clang didn't raise a
warning when building this:

static const int x;
static const typeof(x) y;

It turns out that the warning is only raised when -std=gnu89 (and
potentially others) is set, which is the case of the
kernel. Definitely looks like this should be fixed in clang.

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit:

> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:
> 
> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
> > > Hi
> > >
> > > On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > >> clang generates plenty of these warnings in different parts of the code.
> > >> They are mostly caused by container_of() and other macros which declare
> > >> a "const <type> *" variable for their internal use which triggers a
> > >> "duplicate 'const' specifier" warning if the <type> is already const
> > >> qualified.
> > >>
> > >> Wording-mostly-from: Michael Davidson <md@google.com>
> > >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > >> ---
> > >>  Makefile | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/Makefile b/Makefile
> > >> index df5abf346354..6cd6d428db43 100644
> > >> --- a/Makefile
> > >> +++ b/Makefile
> > >> @@ -704,6 +704,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> > >>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> > >>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> > >>  KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> > >> +KBUILD_CFLAGS += $(call cc-disable-warning, duplicate-decl-specifier)
> > >>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
> > >>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> > >>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
> > >
> > > It seems like gcc 7 may have the same warning.  Specifically I saw a
> > > patch fly by from Arnd, which you can find in Mark Brown's tree now:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/rt5614&id=03ba791df98d15d07ea74075122af71e35c7611c
> > >
> > >
> > > +Arnd since he may be trying to solve the same issues?
> > 
> > gcc-7 has a warning option with the same name, and I think I have
> > fixed all the occurrences we got in mainline (some patches my still
> > be in flight). However, it seems that only clang warns about
> > 'const typeof(type)' with 'type' being already const.
> > 
> > I have not looked at clang warnings in a while, how many of these do
> > we get overall (aside from container_of)? We might be able
> > to turn off this particular warning by sprinkling in
> > '_Pragma("clang diagnostic push") _Pragma("clang diagnostic
> > ignored \"-Wduplicate-decl-specifier\"")' inside of the macro
> > (not sure if clang allows it there, gcc-4.4 and earlier I think did
> > not).
> > 
> > It might also be useful to open a bug against clang so they can
> > change it in future releases, as the gcc behavior seems more
> > sensible in this instance.
> 
> I asked our toolchain folks to follow up with the clang devs. They
> asked me for a simple test case, to my suprise clang didn't raise a
> warning when building this:
> 
> static const int x;
> static const typeof(x) y;
> 
> It turns out that the warning is only raised when -std=gnu89 (and
> potentially others) is set, which is the case of the
> kernel. Definitely looks like this should be fixed in clang.

It seems the duplicate-decl-specifier warning targets specifically C89:

"The same type qualifier shall not appear more than once in the same
specifier list or qualifier list, either directly or via one or more
typedefs."

C89 (6.5.3)

gcc also raises a warning when '-pedantic' is specified and
-std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99.

This bug might help to shed more light on this:
https://bugs.llvm.org/show_bug.cgi?id=32985

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Arnd Bergmann]

On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit:
>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:
>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
> It seems the duplicate-decl-specifier warning targets specifically C89:
>
> "The same type qualifier shall not appear more than once in the same
> specifier list or qualifier list, either directly or via one or more
> typedefs."
>
> C89 (6.5.3)
>
> gcc also raises a warning when '-pedantic' is specified and
> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99.
>
> This bug might help to shed more light on this:
> https://bugs.llvm.org/show_bug.cgi?id=32985

I also notice that neither compiler differentiates between a)

typedef const int cint;
const cint i;

and b)
const int i;
const typeof(a) j;

I would have expected a warning for a) but not b), but both 'clang --std=gnu89'
and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn
for newer standards.

       Arnd

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Masahiro Yamada]

Hi.


2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit:
>>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:
>>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
>> It seems the duplicate-decl-specifier warning targets specifically C89:
>>
>> "The same type qualifier shall not appear more than once in the same
>> specifier list or qualifier list, either directly or via one or more
>> typedefs."
>>
>> C89 (6.5.3)
>>
>> gcc also raises a warning when '-pedantic' is specified and
>> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99.
>>
>> This bug might help to shed more light on this:
>> https://bugs.llvm.org/show_bug.cgi?id=32985
>
> I also notice that neither compiler differentiates between a)
>
> typedef const int cint;
> const cint i;
>
> and b)
> const int i;
> const typeof(a) j;
>
> I would have expected a warning for a) but not b), but both 'clang --std=gnu89'
> and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn
> for newer standards.
>
>        Arnd




I think we agreed to apply 1/2.

How about 2/2?

I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier,
but we did not come up with an idea to solve the problem for
already shipped clang versions.
(BTW, we have not defined the minimal supported version of clang yet.)


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Arnd Bergmann]

On Wed, Jun 21, 2017 at 11:11 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
>>> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit:
>>>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:
>>>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
>>> It seems the duplicate-decl-specifier warning targets specifically C89:
>>>
>>> "The same type qualifier shall not appear more than once in the same
>>> specifier list or qualifier list, either directly or via one or more
>>> typedefs."
>>>
>>> C89 (6.5.3)
>>>
>>> gcc also raises a warning when '-pedantic' is specified and
>>> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99.
>>>
>>> This bug might help to shed more light on this:
>>> https://bugs.llvm.org/show_bug.cgi?id=32985
>>
>> I also notice that neither compiler differentiates between a)
>>
>> typedef const int cint;
>> const cint i;
>>
>> and b)
>> const int i;
>> const typeof(a) j;
>>
>> I would have expected a warning for a) but not b), but both 'clang --std=gnu89'
>> and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn
>> for newer standards.
>>
>>        Arnd
>
>
>
>
> I think we agreed to apply 1/2.
>
> How about 2/2?
>
> I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier,
> but we did not come up with an idea to solve the problem for
> already shipped clang versions.
> (BTW, we have not defined the minimal supported version of clang yet.)

I see that container_of() has been modified in linux-next and no longer adds
the 'const' keyword, do we actually still need the patch?

        Arnd

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit:

> On Wed, Jun 21, 2017 at 11:11 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > 2017-05-24 17:21 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> >> On Wed, May 24, 2017 at 2:04 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >>> El Wed, May 17, 2017 at 11:45:29AM -0700 Matthias Kaehlcke ha dit:
> >>>> El Wed, May 17, 2017 at 09:35:57AM +0200 Arnd Bergmann ha dit:
> >>>> > On Tue, May 16, 2017 at 11:41 PM, Doug Anderson <dianders@chromium.org> wrote:
> >>> It seems the duplicate-decl-specifier warning targets specifically C89:
> >>>
> >>> "The same type qualifier shall not appear more than once in the same
> >>> specifier list or qualifier list, either directly or via one or more
> >>> typedefs."
> >>>
> >>> C89 (6.5.3)
> >>>
> >>> gcc also raises a warning when '-pedantic' is specified and
> >>> -std=gnu89/c89 (or unspecified), but not with -std=gnu99/c99.
> >>>
> >>> This bug might help to shed more light on this:
> >>> https://bugs.llvm.org/show_bug.cgi?id=32985
> >>
> >> I also notice that neither compiler differentiates between a)
> >>
> >> typedef const int cint;
> >> const cint i;
> >>
> >> and b)
> >> const int i;
> >> const typeof(a) j;
> >>
> >> I would have expected a warning for a) but not b), but both 'clang --std=gnu89'
> >> and 'gcc --pedantic --std=gnu89' warn about both of b as well, and don't warn
> >> for newer standards.
> >>
> >>        Arnd
> >
> >
> >
> >
> > I think we agreed to apply 1/2.
> >
> > How about 2/2?
> >
> > I think we mostly discussed preferable behavior of -Wduplicate-decl-specifier,
> > but we did not come up with an idea to solve the problem for
> > already shipped clang versions.
> > (BTW, we have not defined the minimal supported version of clang yet.)

I think it will have to be a future version. There is still an issue
affecting at least llist_for_each_entry_safe(), where clang optimizes
away a check for check for a NULL pointer. For gcc this optimization
is switched off with -fno-delete-null-pointer-check, clang currently
does not have this flag or an equivalent.

For now a workaround like this is needed for newer kernels:

https://android-git.linaro.org/kernel/hikey-clang.git/commit/?h=android-hikey-linaro-4.9-clang&id=4f3c3c1e7b153e333603be74d786d79bb872e8ff

For arm64 at least one other clang fix is missing, to make
-mgeneral-regs-only consistent with gcc
(https://bugs.llvm.org/show_bug.cgi?id=30792)

> I see that container_of() has been modified in linux-next and no longer adds
> the 'const' keyword, do we actually still need the patch?

There is still (at least) the case of const arrays passed to
MODULE_DEVICE_TABLE.

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Arnd Bergmann]

On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit:
>> I see that container_of() has been modified in linux-next and no longer adds
>> the 'const' keyword, do we actually still need the patch?
>
> There is still (at least) the case of const arrays passed to
> MODULE_DEVICE_TABLE.

Does the 'const' have any effect there? As it's just an alias, it
should at least
not impact the placement of the symbol in the object file, right? Maybe we can
just remove that 'const' too. Do you see any other instances?

       Arnd

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

El Wed, Jun 21, 2017 at 07:59:42PM +0200 Arnd Bergmann ha dit:

> On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit:
> >> I see that container_of() has been modified in linux-next and no longer adds
> >> the 'const' keyword, do we actually still need the patch?
> >
> > There is still (at least) the case of const arrays passed to
> > MODULE_DEVICE_TABLE.
> 
> Does the 'const' have any effect there? As it's just an alias, it
> should at least
> not impact the placement of the symbol in the object file, right?

I agree, it shouldn't make a difference.

> Maybe we can just remove that 'const' too.

Seems worth a try. Do you want to send a patch for the removal?

> Do you see any other instances?

For both x86 and arm64 defconfig the instances are all from
container_of() or MODULE_DEVICE_TABLE.

Re: [PATCH 1/2] kbuild: clang: Disable 'address-of-packed-member' warning

[Masahiro Yamada]

2017-05-17 6:32 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Fri, Apr 21, 2017 at 2:39 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> clang generates plenty of these warnings in different parts of the code,
>> to an extent that the warnings are little more than noise. Disable the
>> 'address-of-packed-member' warning.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  Makefile | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 5039b9148d15..df5abf346354 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -703,6 +703,7 @@ KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>>  # Quiet clang warning: comparison of unsigned expression < 0 is always false
>>  KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>>  # CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
>
> Though I'm no expert, after reading the discussion on this thread and
> looking at the examples provided by Matthias, this seems sane to me.
> Thus, FWIW:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>


1/2 applied to linux-kbuild/kbuild.  Thanks!


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] kbuild: clang: Disable the 'duplicate-decl-specifier' warning

[Matthias Kaehlcke]

El Wed, Jun 21, 2017 at 07:59:42PM +0200 Arnd Bergmann ha dit:

> On Wed, Jun 21, 2017 at 6:58 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > El Wed, Jun 21, 2017 at 12:11:55PM +0200 Arnd Bergmann ha dit:
> >> I see that container_of() has been modified in linux-next and no longer adds
> >> the 'const' keyword, do we actually still need the patch?
> >
> > There is still (at least) the case of const arrays passed to
> > MODULE_DEVICE_TABLE.
> 
> Does the 'const' have any effect there? As it's just an alias, it
> should at least
> not impact the placement of the symbol in the object file, right? Maybe we can
> just remove that 'const' too. Do you see any other instances?

For the record: https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git/commit/?h=modules-next&id=0bf8bf50eddc7511b52461bae798cbfaa0157a34