diff mbox series

[v2,4/5] powerpc: Fix duplicate const clang warning in user access code

Message ID 20180914040649.1794-5-joel@jms.id.au (mailing list archive)
State Accepted
Commit e00d93ac9a189673028ac125a74b9bc8ae73eebc
Headers show
Series powerpc: Clang build fixes | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied
snowpatch_ozlabs/checkpatch fail Test checkpatch on branch next

Commit Message

Joel Stanley Sept. 14, 2018, 4:06 a.m. UTC 
From: Anton Blanchard <anton@samba.org>

This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
...").

We see a large number of duplicate const errors in the user access
code when building with llvm/clang:

  include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
      [-Wduplicate-decl-specifier]
        ret = __get_user(c, uaddr);

The problem is we are doing const __typeof__(*(ptr)), which will hit the
warning if ptr is marked const.

Removing const does not seem to have any effect on GCC code generation.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
If we don't want to apply this, other options are suppressing the
warning, or wait for a fix to land in clang
(https://github.com/ClangBuiltLinux/linux/issues/52).
---
 arch/powerpc/include/asm/uaccess.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Nick Desaulniers Sept. 14, 2018, 5:57 p.m. UTC | #1 
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
>
> From: Anton Blanchard <anton@samba.org>
>
> This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
> d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
> ...").
>
> We see a large number of duplicate const errors in the user access
> code when building with llvm/clang:
>
>   include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
>       [-Wduplicate-decl-specifier]
>         ret = __get_user(c, uaddr);
>
> The problem is we are doing const __typeof__(*(ptr)), which will hit the
> warning if ptr is marked const.
>
> Removing const does not seem to have any effect on GCC code generation.

I wouldn't expect it to for a local variable with such localized
usage.  I myself am quite liberal in applying `const` to everything,
so I will try to fix this in Clang as well, but this should silence
the warning for users of older versions of Clang and results in no
functional change.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> If we don't want to apply this, other options are suppressing the
> warning, or wait for a fix to land in clang
> (https://github.com/ClangBuiltLinux/linux/issues/52).
> ---
>  arch/powerpc/include/asm/uaccess.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index bac225bb7f64..15bea9a0f260 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -260,7 +260,7 @@ do {                                                                \
>  ({                                                             \
>         long __gu_err;                                          \
>         __long_type(*(ptr)) __gu_val;                           \
> -       const __typeof__(*(ptr)) __user *__gu_addr = (ptr);     \
> +       __typeof__(*(ptr)) __user *__gu_addr = (ptr);   \
>         __chk_user_ptr(ptr);                                    \
>         if (!is_kernel_addr((unsigned long)__gu_addr))          \
>                 might_fault();                                  \
> @@ -274,7 +274,7 @@ do {                                                                \
>  ({                                                                     \
>         long __gu_err = -EFAULT;                                        \
>         __long_type(*(ptr)) __gu_val = 0;                               \
> -       const __typeof__(*(ptr)) __user *__gu_addr = (ptr);             \
> +       __typeof__(*(ptr)) __user *__gu_addr = (ptr);           \
>         might_fault();                                                  \
>         if (access_ok(VERIFY_READ, __gu_addr, (size))) {                \
>                 barrier_nospec();                                       \
> @@ -288,7 +288,7 @@ do {                                                                \
>  ({                                                             \
>         long __gu_err;                                          \
>         __long_type(*(ptr)) __gu_val;                           \
> -       const __typeof__(*(ptr)) __user *__gu_addr = (ptr);     \
> +       __typeof__(*(ptr)) __user *__gu_addr = (ptr);   \
>         __chk_user_ptr(ptr);                                    \
>         barrier_nospec();                                       \
>         __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> --
> 2.17.1
>
Joel Stanley Sept. 19, 2018, 7:45 a.m. UTC | #2 
On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
> >
> > From: Anton Blanchard <anton@samba.org>
> >
> > This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
> > d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
> > ...").
> >
> > We see a large number of duplicate const errors in the user access
> > code when building with llvm/clang:
> >
> >   include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
> >       [-Wduplicate-decl-specifier]
> >         ret = __get_user(c, uaddr);
> >
> > The problem is we are doing const __typeof__(*(ptr)), which will hit the
> > warning if ptr is marked const.
> >
> > Removing const does not seem to have any effect on GCC code generation.
>
> I wouldn't expect it to for a local variable with such localized
> usage.  I myself am quite liberal in applying `const` to everything,
> so I will try to fix this in Clang as well, but this should silence
> the warning for users of older versions of Clang and results in no
> functional change.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Nick has written a clang patch that suppresses the warning in the same
way as GCC. Assuming it gets merged, as we depend on clang-8 we could
chose to not merge the kernel patch.

https://reviews.llvm.org/D52248

Cheers,

Joel
Michael Ellerman Sept. 20, 2018, 4:21 a.m. UTC | #3 
On Fri, 2018-09-14 at 04:06:48 UTC, Joel Stanley wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
> d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
> ...").
> 
> We see a large number of duplicate const errors in the user access
> code when building with llvm/clang:
> 
>   include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
>       [-Wduplicate-decl-specifier]
>         ret = __get_user(c, uaddr);
> 
> The problem is we are doing const __typeof__(*(ptr)), which will hit the
> warning if ptr is marked const.
> 
> Removing const does not seem to have any effect on GCC code generation.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e00d93ac9a189673028ac125a74b9b

cheers
Christophe Leroy Sept. 20, 2018, 4:54 a.m. UTC | #4 
Le 19/09/2018 à 09:45, Joel Stanley a écrit :
> On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
>>>
>>> From: Anton Blanchard <anton@samba.org>
>>>
>>> This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
>>> d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
>>> ...").
>>>
>>> We see a large number of duplicate const errors in the user access
>>> code when building with llvm/clang:
>>>
>>>    include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
>>>        [-Wduplicate-decl-specifier]
>>>          ret = __get_user(c, uaddr);
>>>
>>> The problem is we are doing const __typeof__(*(ptr)), which will hit the
>>> warning if ptr is marked const.
>>>
>>> Removing const does not seem to have any effect on GCC code generation.
>>
>> I wouldn't expect it to for a local variable with such localized
>> usage.  I myself am quite liberal in applying `const` to everything,
>> so I will try to fix this in Clang as well, but this should silence
>> the warning for users of older versions of Clang and results in no
>> functional change.
>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> Nick has written a clang patch that suppresses the warning in the same
> way as GCC. Assuming it gets merged, as we depend on clang-8 we could
> chose to not merge the kernel patch.
> 
> https://reviews.llvm.org/D52248
> 

Seems like Michael has merged this patch anyway.

Christophe
Michael Ellerman Sept. 20, 2018, 12:35 p.m. UTC | #5 
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 19/09/2018 à 09:45, Joel Stanley a écrit :
>> On Sat, 15 Sep 2018 at 03:27, Nick Desaulniers <ndesaulniers@google.com> wrote:
>>>
>>> On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
>>>>
>>>> From: Anton Blanchard <anton@samba.org>
>>>>
>>>> This re-applies b91c1e3e7a6f which was reverted in f2ca80905929
>>>> d466f6c5cac1 f84ed59a612d (powerpc/sparse: Constify the address pointer
>>>> ...").
>>>>
>>>> We see a large number of duplicate const errors in the user access
>>>> code when building with llvm/clang:
>>>>
>>>>    include/linux/pagemap.h:576:8: warning: duplicate 'const' declaration specifier
>>>>        [-Wduplicate-decl-specifier]
>>>>          ret = __get_user(c, uaddr);
>>>>
>>>> The problem is we are doing const __typeof__(*(ptr)), which will hit the
>>>> warning if ptr is marked const.
>>>>
>>>> Removing const does not seem to have any effect on GCC code generation.
>>>
>>> I wouldn't expect it to for a local variable with such localized
>>> usage.  I myself am quite liberal in applying `const` to everything,
>>> so I will try to fix this in Clang as well, but this should silence
>>> the warning for users of older versions of Clang and results in no
>>> functional change.
>>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>> 
>> Nick has written a clang patch that suppresses the warning in the same
>> way as GCC. Assuming it gets merged, as we depend on clang-8 we could
>> chose to not merge the kernel patch.
>> 
>> https://reviews.llvm.org/D52248
>
> Seems like Michael has merged this patch anyway.

I'll happily revert it though if someone tells me to.

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index bac225bb7f64..15bea9a0f260 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -260,7 +260,7 @@  do {								\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
-	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
+	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__chk_user_ptr(ptr);					\
 	if (!is_kernel_addr((unsigned long)__gu_addr))		\
 		might_fault();					\
@@ -274,7 +274,7 @@  do {								\
 ({									\
 	long __gu_err = -EFAULT;					\
 	__long_type(*(ptr)) __gu_val = 0;				\
-	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
+	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
 	might_fault();							\
 	if (access_ok(VERIFY_READ, __gu_addr, (size))) {		\
 		barrier_nospec();					\
@@ -288,7 +288,7 @@  do {								\
 ({								\
 	long __gu_err;						\
 	__long_type(*(ptr)) __gu_val;				\
-	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
+	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__chk_user_ptr(ptr);					\
 	barrier_nospec();					\
 	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\