* [PATCH v2 0/2] CLANG_VERSION and __diag macros @ 2018-07-30 21:34 Nick Desaulniers 2018-07-30 21:34 ` [PATCH v2 1/2] compiler-clang.h: Add " Nick Desaulniers 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers 0 siblings, 2 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-30 21:34 UTC (permalink / raw) To: akpm, natechancellor Cc: arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin, Nick Desaulniers clang-7 has a new warning (-Wreturn-stack-address) for warning when a function returns the address of a local variable. This is in general a good warning, but the kernel has a few places where GNU statement expressions return the address of a label in order to get the current instruction pointer (see _THIS_IP_ and current_text_addr). In order to disable a warning at a single call site, the kernel already has __diag macros for inserting compiler and compiler-version specific _Pragma's. This series adds CLANG_VERSION macros necessary for proper __diag support, and whitelists the case in _THIS_IP_. current_text_addr will be consolidated in a follow up series. Nick Desaulniers (2): compiler-clang.h: Add CLANG_VERSION and __diag macros kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ include/linux/compiler-clang.h | 19 +++++++++++++++++++ include/linux/compiler_types.h | 4 ++++ include/linux/kernel.h | 10 +++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros 2018-07-30 21:34 [PATCH v2 0/2] CLANG_VERSION and __diag macros Nick Desaulniers @ 2018-07-30 21:34 ` Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor 2018-08-31 21:50 ` Nick Desaulniers 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers 1 sibling, 2 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-30 21:34 UTC (permalink / raw) To: akpm, natechancellor Cc: arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin, Nick Desaulniers These are needed for doing proper version checks, though feature detection via __has_attribute, __has_builtin, and __has_feature should be preferred, see: https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros Also adds __diag support, for generating compiler version specific _Pragma()'s. __diag support based on commit 8793bb7f4a9d ("kbuild: add macro for controlling warnings to linux/compiler.h") Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 Suggested-by: Nathan Chancellor <natechancellor@gmail.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- include/linux/compiler-clang.h | 19 +++++++++++++++++++ include/linux/compiler_types.h | 4 ++++ 2 files changed, 23 insertions(+) diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index 7087446c24c8..9442e07a361e 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -7,6 +7,10 @@ * for Clang compiler */ +#define CLANG_VERSION (__clang_major__ * 10000 \ + + __clang_minor__ * 100 \ + + __clang_patchlevel__) + #ifdef uninitialized_var #undef uninitialized_var #define uninitialized_var(x) x = *(&(x)) @@ -46,3 +50,18 @@ __has_builtin(__builtin_sub_overflow) #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 #endif + +#define __diag_str1(s) #s +#define __diag_str(s) __diag_str1(s) +#define __diag(s) _Pragma(__diag_str(clang diagnostic s)) +#define __diag_CLANG_ignore ignored +#define __diag_CLANG_warn warning +#define __diag_CLANG_error error +#define __diag_CLANG(version, severity, s) \ + __diag_CLANG_ ## version(__diag_CLANG_ ## severity s) + +#if CLANG_VERSION >= 70000 +#define __diag_CLANG_7(s) __diag(s) +#else +#define __diag_CLANG_7(s) +#endif diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index a8ba6b04152c..a04e6bd63476 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -279,6 +279,10 @@ struct ftrace_likely_data { #define __diag_GCC(version, severity, string) #endif +#ifndef __diag_CLANG +#define __diag_CLANG(version, severity, string) +#endif + #define __diag_push() __diag(push) #define __diag_pop() __diag(pop) -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros 2018-07-30 21:34 ` [PATCH v2 1/2] compiler-clang.h: Add " Nick Desaulniers @ 2018-07-30 23:25 ` Nathan Chancellor 2018-08-31 21:50 ` Nick Desaulniers 1 sibling, 0 replies; 16+ messages in thread From: Nathan Chancellor @ 2018-07-30 23:25 UTC (permalink / raw) To: Nick Desaulniers Cc: akpm, arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin On Mon, Jul 30, 2018 at 02:34:11PM -0700, Nick Desaulniers wrote: > These are needed for doing proper version checks, though feature > detection via __has_attribute, __has_builtin, and __has_feature should > be preferred, see: > https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros > > Also adds __diag support, for generating compiler version specific > _Pragma()'s. > > __diag support based on commit 8793bb7f4a9d ("kbuild: add macro for > controlling warnings to linux/compiler.h") > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > Suggested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Tested-and-reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > include/linux/compiler-clang.h | 19 +++++++++++++++++++ > include/linux/compiler_types.h | 4 ++++ > 2 files changed, 23 insertions(+) > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > index 7087446c24c8..9442e07a361e 100644 > --- a/include/linux/compiler-clang.h > +++ b/include/linux/compiler-clang.h > @@ -7,6 +7,10 @@ > * for Clang compiler > */ > > +#define CLANG_VERSION (__clang_major__ * 10000 \ > + + __clang_minor__ * 100 \ > + + __clang_patchlevel__) > + I know this was done to be consistent with GCC_VERSION but certain toolchains may set the patch level to the SVN revision (at least that's what it appears to be) like AOSP's clang-4053586, which would make CLANG_VERSION 350080. For this particular warning, it doesn't matter since Clang has supported it since 2011 and Clang 5.0 isn't supported in this tree anyways but maybe relying on just __clang_major__ below is more ideal. Other than that, patch functions just as it should. > #ifdef uninitialized_var > #undef uninitialized_var > #define uninitialized_var(x) x = *(&(x)) > @@ -46,3 +50,18 @@ > __has_builtin(__builtin_sub_overflow) > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > #endif > + > +#define __diag_str1(s) #s > +#define __diag_str(s) __diag_str1(s) > +#define __diag(s) _Pragma(__diag_str(clang diagnostic s)) > +#define __diag_CLANG_ignore ignored > +#define __diag_CLANG_warn warning > +#define __diag_CLANG_error error > +#define __diag_CLANG(version, severity, s) \ > + __diag_CLANG_ ## version(__diag_CLANG_ ## severity s) > + > +#if CLANG_VERSION >= 70000 > +#define __diag_CLANG_7(s) __diag(s) > +#else > +#define __diag_CLANG_7(s) > +#endif > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index a8ba6b04152c..a04e6bd63476 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -279,6 +279,10 @@ struct ftrace_likely_data { > #define __diag_GCC(version, severity, string) > #endif > > +#ifndef __diag_CLANG > +#define __diag_CLANG(version, severity, string) > +#endif > + > #define __diag_push() __diag(push) > #define __diag_pop() __diag(pop) > > -- > 2.18.0.345.g5c9ce644c3-goog > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros 2018-07-30 21:34 ` [PATCH v2 1/2] compiler-clang.h: Add " Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor @ 2018-08-31 21:50 ` Nick Desaulniers 2018-08-31 22:16 ` Miguel Ojeda 1 sibling, 1 reply; 16+ messages in thread From: Nick Desaulniers @ 2018-08-31 21:50 UTC (permalink / raw) To: Miguel Ojeda, joe Cc: Arnd Bergmann, paul.burton, christophe.leroy, Stafford Horne, Masahiro Yamada, Kees Cook, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, Rasmus Villemoes, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin, Nathan Chancellor, Andrew Morton + Miguel and Joe This is the old patch I had sent for detecting Clang version. If we wanted to set a minimal version, this plus the actual version should do it. Probably could drop the diag stuff, as we changed clang to not warn in this case, so my solution using _Pragma is obsolete. On Mon, Jul 30, 2018 at 2:34 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > These are needed for doing proper version checks, though feature > detection via __has_attribute, __has_builtin, and __has_feature should > be preferred, see: > https://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros > > Also adds __diag support, for generating compiler version specific > _Pragma()'s. > > __diag support based on commit 8793bb7f4a9d ("kbuild: add macro for > controlling warnings to linux/compiler.h") > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > Suggested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > include/linux/compiler-clang.h | 19 +++++++++++++++++++ > include/linux/compiler_types.h | 4 ++++ > 2 files changed, 23 insertions(+) > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > index 7087446c24c8..9442e07a361e 100644 > --- a/include/linux/compiler-clang.h > +++ b/include/linux/compiler-clang.h > @@ -7,6 +7,10 @@ > * for Clang compiler > */ > > +#define CLANG_VERSION (__clang_major__ * 10000 \ > + + __clang_minor__ * 100 \ > + + __clang_patchlevel__) > + > #ifdef uninitialized_var > #undef uninitialized_var > #define uninitialized_var(x) x = *(&(x)) > @@ -46,3 +50,18 @@ > __has_builtin(__builtin_sub_overflow) > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > #endif > + > +#define __diag_str1(s) #s > +#define __diag_str(s) __diag_str1(s) > +#define __diag(s) _Pragma(__diag_str(clang diagnostic s)) > +#define __diag_CLANG_ignore ignored > +#define __diag_CLANG_warn warning > +#define __diag_CLANG_error error > +#define __diag_CLANG(version, severity, s) \ > + __diag_CLANG_ ## version(__diag_CLANG_ ## severity s) > + > +#if CLANG_VERSION >= 70000 > +#define __diag_CLANG_7(s) __diag(s) > +#else > +#define __diag_CLANG_7(s) > +#endif > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index a8ba6b04152c..a04e6bd63476 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -279,6 +279,10 @@ struct ftrace_likely_data { > #define __diag_GCC(version, severity, string) > #endif > > +#ifndef __diag_CLANG > +#define __diag_CLANG(version, severity, string) > +#endif > + > #define __diag_push() __diag(push) > #define __diag_pop() __diag(pop) > > -- > 2.18.0.345.g5c9ce644c3-goog > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros 2018-08-31 21:50 ` Nick Desaulniers @ 2018-08-31 22:16 ` Miguel Ojeda 0 siblings, 0 replies; 16+ messages in thread From: Miguel Ojeda @ 2018-08-31 22:16 UTC (permalink / raw) To: Nick Desaulniers Cc: Joe Perches, Arnd Bergmann, Paul Burton, christophe.leroy, Stafford Horne, Masahiro Yamada, Kees Cook, Ingo Molnar, Greg KH, Thomas Gleixner, Randy Dunlap, Borislav Petkov, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, Rasmus Villemoes, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin, Nathan Chancellor, Andrew Morton Hi Nick, On Fri, Aug 31, 2018 at 11:50 PM, Nick Desaulniers <ndesaulniers@google.com> wrote: > + Miguel and Joe > > This is the old patch I had sent for detecting Clang version. If we > wanted to set a minimal version, this plus the actual version should > do it. Probably could drop the diag stuff, as we changed clang to not > warn in this case, so my solution using _Pragma is obsolete. Nitpick: do we want to use the CLANG_VERSION notation? In some cases I think it is easier to simply use directly the original values (like I did in compiler_attributes.h for gcc). Consider that we almost never need the patchlevel (and if we do, it is better to *see* we are explicitly testing for it), and given the versioning schemes that compilers nowadays use (i.e. only major versions), I would bet not even the minor (except maybe for GCC in compiler_attributes.h until we drop gcc < 5). Cheers, Miguel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-30 21:34 [PATCH v2 0/2] CLANG_VERSION and __diag macros Nick Desaulniers 2018-07-30 21:34 ` [PATCH v2 1/2] compiler-clang.h: Add " Nick Desaulniers @ 2018-07-30 21:34 ` Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-30 21:34 UTC (permalink / raw) To: akpm, natechancellor Cc: arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin, Nick Desaulniers Starting with Clang-7.0, _THIS_IP_ generates -Wreturn-stack-address warnings for almost every translation unit. In general, I'd prefer to leave this on (returning the address of a stack allocated variable is in general a bad idea) and disable it only at whitelisted call sites. We can't do something like: #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wreturn-stack-address" <code> #pragma clang diagnostic pop in a GNU Statement Expression or macro, hence we use _Pragma, which is its raison d'être: https://gcc.gnu.org/onlinedocs/cpp/Pragmas.html We also can't use compiler specific pragma's without triggering -Werror=unknown-pragmas in other compilers, so use __diag. Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 Suggested-by: Nathan Chancellor <natechancellor@gmail.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- include/linux/kernel.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 941dc0a5a877..909bb771c0ca 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -168,7 +168,15 @@ #define _RET_IP_ (unsigned long)__builtin_return_address(0) -#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; }) +#define _THIS_IP_ ( \ +{ \ + __label__ __here; \ + __diag_push() \ + __diag_ignore(CLANG, 7, "-Wreturn-stack-address", "") \ +__here: (unsigned long)&&__here; \ + __diag_pop() \ +} \ +) #ifdef CONFIG_LBDAF # include <asm/div64.h> -- 2.18.0.345.g5c9ce644c3-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers @ 2018-07-30 23:25 ` Nathan Chancellor 2018-07-31 10:27 ` kbuild test robot 2018-07-31 13:53 ` kbuild test robot 2 siblings, 0 replies; 16+ messages in thread From: Nathan Chancellor @ 2018-07-30 23:25 UTC (permalink / raw) To: Nick Desaulniers Cc: akpm, arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin On Mon, Jul 30, 2018 at 02:34:12PM -0700, Nick Desaulniers wrote: > Starting with Clang-7.0, _THIS_IP_ generates -Wreturn-stack-address > warnings for almost every translation unit. In general, I'd prefer to > leave this on (returning the address of a stack allocated variable is in > general a bad idea) and disable it only at whitelisted call sites. > > We can't do something like: > #pragma clang diagnostic push > #pragma clang diagnostic ignored "-Wreturn-stack-address" > <code> > #pragma clang diagnostic pop > > in a GNU Statement Expression or macro, hence we use _Pragma, which is > its raison d'être: https://gcc.gnu.org/onlinedocs/cpp/Pragmas.html > > We also can't use compiler specific pragma's without triggering > -Werror=unknown-pragmas in other compilers, so use __diag. > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > Suggested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Tested-and-reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > include/linux/kernel.h | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 941dc0a5a877..909bb771c0ca 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -168,7 +168,15 @@ > > > #define _RET_IP_ (unsigned long)__builtin_return_address(0) > -#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; }) > +#define _THIS_IP_ ( \ > +{ \ > + __label__ __here; \ > + __diag_push() \ > + __diag_ignore(CLANG, 7, "-Wreturn-stack-address", "") \ > +__here: (unsigned long)&&__here; \ > + __diag_pop() \ > +} \ > +) > > #ifdef CONFIG_LBDAF > # include <asm/div64.h> > -- > 2.18.0.345.g5c9ce644c3-goog > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor @ 2018-07-31 10:27 ` kbuild test robot 2018-07-31 16:48 ` Nick Desaulniers 2018-07-31 13:53 ` kbuild test robot 2 siblings, 1 reply; 16+ messages in thread From: kbuild test robot @ 2018-07-31 10:27 UTC (permalink / raw) To: Nick Desaulniers Cc: kbuild-all, akpm, natechancellor, arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin, Nick Desaulniers [-- Attachment #1: Type: text/plain, Size: 4282 bytes --] Hi Nick, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.18-rc7 next-20180727] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/compiler-clang-h-Add-CLANG_VERSION-and-__diag-macros/20180731-161932 config: x86_64-fedora-25 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] if (!(cmd->flags & CMD_ASYNC)) ^~ drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' lock_map_acquire_read(&trans->sync_cmd_lockdep_map); ^ ~ vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c 92fe8343 Emmanuel Grumbach 2015-12-01 116 92fe8343 Emmanuel Grumbach 2015-12-01 117 int iwl_trans_send_cmd(struct iwl_trans *trans, struct iwl_host_cmd *cmd) 92fe8343 Emmanuel Grumbach 2015-12-01 118 { 92fe8343 Emmanuel Grumbach 2015-12-01 119 int ret; 92fe8343 Emmanuel Grumbach 2015-12-01 120 92fe8343 Emmanuel Grumbach 2015-12-01 121 if (unlikely(!(cmd->flags & CMD_SEND_IN_RFKILL) && 326477e4 Johannes Berg 2017-04-25 122 test_bit(STATUS_RFKILL_OPMODE, &trans->status))) 92fe8343 Emmanuel Grumbach 2015-12-01 123 return -ERFKILL; 92fe8343 Emmanuel Grumbach 2015-12-01 124 92fe8343 Emmanuel Grumbach 2015-12-01 125 if (unlikely(test_bit(STATUS_FW_ERROR, &trans->status))) 92fe8343 Emmanuel Grumbach 2015-12-01 126 return -EIO; 92fe8343 Emmanuel Grumbach 2015-12-01 127 92fe8343 Emmanuel Grumbach 2015-12-01 128 if (unlikely(trans->state != IWL_TRANS_FW_ALIVE)) { 92fe8343 Emmanuel Grumbach 2015-12-01 129 IWL_ERR(trans, "%s bad state = %d\n", __func__, trans->state); 92fe8343 Emmanuel Grumbach 2015-12-01 130 return -EIO; 92fe8343 Emmanuel Grumbach 2015-12-01 131 } 92fe8343 Emmanuel Grumbach 2015-12-01 132 92fe8343 Emmanuel Grumbach 2015-12-01 133 if (WARN_ON((cmd->flags & CMD_WANT_ASYNC_CALLBACK) && 92fe8343 Emmanuel Grumbach 2015-12-01 134 !(cmd->flags & CMD_ASYNC))) 92fe8343 Emmanuel Grumbach 2015-12-01 135 return -EINVAL; 92fe8343 Emmanuel Grumbach 2015-12-01 136 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); 92fe8343 Emmanuel Grumbach 2015-12-01 139 5b88792c Sara Sharon 2016-08-15 140 if (trans->wide_cmd_header && !iwl_cmd_groupid(cmd->id)) 5b88792c Sara Sharon 2016-08-15 141 cmd->id = DEF_ID(cmd->id); 5b88792c Sara Sharon 2016-08-15 142 92fe8343 Emmanuel Grumbach 2015-12-01 143 ret = trans->ops->send_cmd(trans, cmd); 92fe8343 Emmanuel Grumbach 2015-12-01 144 92fe8343 Emmanuel Grumbach 2015-12-01 145 if (!(cmd->flags & CMD_ASYNC)) 92fe8343 Emmanuel Grumbach 2015-12-01 146 lock_map_release(&trans->sync_cmd_lockdep_map); 92fe8343 Emmanuel Grumbach 2015-12-01 147 0ec971fd Johannes Berg 2017-04-10 148 if (WARN_ON((cmd->flags & CMD_WANT_SKB) && !ret && !cmd->resp_pkt)) 0ec971fd Johannes Berg 2017-04-10 149 return -EIO; 0ec971fd Johannes Berg 2017-04-10 150 92fe8343 Emmanuel Grumbach 2015-12-01 151 return ret; 92fe8343 Emmanuel Grumbach 2015-12-01 152 } 92fe8343 Emmanuel Grumbach 2015-12-01 153 IWL_EXPORT_SYMBOL(iwl_trans_send_cmd); 39bdb17e Sharon Dvir 2015-10-15 154 :::::: The code at line 137 was first introduced by commit :::::: 92fe83430b899b786c837e5b716a328220d47ae5 iwlwifi: uninline iwl_trans_send_cmd :::::: TO: Emmanuel Grumbach <emmanuel.grumbach@intel.com> :::::: CC: Emmanuel Grumbach <emmanuel.grumbach@intel.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 48111 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 10:27 ` kbuild test robot @ 2018-07-31 16:48 ` Nick Desaulniers 2018-07-31 17:02 ` Kees Cook 2018-07-31 17:02 ` Nathan Chancellor 0 siblings, 2 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-31 16:48 UTC (permalink / raw) To: lkp Cc: kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Kees Cook, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin Does anyone understand this error? The code looks just fine to me, and the source file doesn't conflict with any of the macros I've added (certainly not in any way that could cause an indentation error as reported here). On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: > > Hi Nick, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on linus/master] > [also build test WARNING on v4.18-rc7 next-20180727] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/compiler-clang-h-Add-CLANG_VERSION-and-__diag-macros/20180731-161932 > config: x86_64-fedora-25 (attached as .config) > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All warnings (new ones prefixed by >>): > > drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] > if (!(cmd->flags & CMD_ASYNC)) > ^~ > drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' > lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > ^ ~ > > vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c > > 92fe8343 Emmanuel Grumbach 2015-12-01 116 > 92fe8343 Emmanuel Grumbach 2015-12-01 117 int iwl_trans_send_cmd(struct iwl_trans *trans, struct iwl_host_cmd *cmd) > 92fe8343 Emmanuel Grumbach 2015-12-01 118 { > 92fe8343 Emmanuel Grumbach 2015-12-01 119 int ret; > 92fe8343 Emmanuel Grumbach 2015-12-01 120 > 92fe8343 Emmanuel Grumbach 2015-12-01 121 if (unlikely(!(cmd->flags & CMD_SEND_IN_RFKILL) && > 326477e4 Johannes Berg 2017-04-25 122 test_bit(STATUS_RFKILL_OPMODE, &trans->status))) > 92fe8343 Emmanuel Grumbach 2015-12-01 123 return -ERFKILL; > 92fe8343 Emmanuel Grumbach 2015-12-01 124 > 92fe8343 Emmanuel Grumbach 2015-12-01 125 if (unlikely(test_bit(STATUS_FW_ERROR, &trans->status))) > 92fe8343 Emmanuel Grumbach 2015-12-01 126 return -EIO; > 92fe8343 Emmanuel Grumbach 2015-12-01 127 > 92fe8343 Emmanuel Grumbach 2015-12-01 128 if (unlikely(trans->state != IWL_TRANS_FW_ALIVE)) { > 92fe8343 Emmanuel Grumbach 2015-12-01 129 IWL_ERR(trans, "%s bad state = %d\n", __func__, trans->state); > 92fe8343 Emmanuel Grumbach 2015-12-01 130 return -EIO; > 92fe8343 Emmanuel Grumbach 2015-12-01 131 } > 92fe8343 Emmanuel Grumbach 2015-12-01 132 > 92fe8343 Emmanuel Grumbach 2015-12-01 133 if (WARN_ON((cmd->flags & CMD_WANT_ASYNC_CALLBACK) && > 92fe8343 Emmanuel Grumbach 2015-12-01 134 !(cmd->flags & CMD_ASYNC))) > 92fe8343 Emmanuel Grumbach 2015-12-01 135 return -EINVAL; > 92fe8343 Emmanuel Grumbach 2015-12-01 136 > 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) > 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > 92fe8343 Emmanuel Grumbach 2015-12-01 139 > 5b88792c Sara Sharon 2016-08-15 140 if (trans->wide_cmd_header && !iwl_cmd_groupid(cmd->id)) > 5b88792c Sara Sharon 2016-08-15 141 cmd->id = DEF_ID(cmd->id); > 5b88792c Sara Sharon 2016-08-15 142 > 92fe8343 Emmanuel Grumbach 2015-12-01 143 ret = trans->ops->send_cmd(trans, cmd); > 92fe8343 Emmanuel Grumbach 2015-12-01 144 > 92fe8343 Emmanuel Grumbach 2015-12-01 145 if (!(cmd->flags & CMD_ASYNC)) > 92fe8343 Emmanuel Grumbach 2015-12-01 146 lock_map_release(&trans->sync_cmd_lockdep_map); > 92fe8343 Emmanuel Grumbach 2015-12-01 147 > 0ec971fd Johannes Berg 2017-04-10 148 if (WARN_ON((cmd->flags & CMD_WANT_SKB) && !ret && !cmd->resp_pkt)) > 0ec971fd Johannes Berg 2017-04-10 149 return -EIO; > 0ec971fd Johannes Berg 2017-04-10 150 > 92fe8343 Emmanuel Grumbach 2015-12-01 151 return ret; > 92fe8343 Emmanuel Grumbach 2015-12-01 152 } > 92fe8343 Emmanuel Grumbach 2015-12-01 153 IWL_EXPORT_SYMBOL(iwl_trans_send_cmd); > 39bdb17e Sharon Dvir 2015-10-15 154 > > :::::: The code at line 137 was first introduced by commit > :::::: 92fe83430b899b786c837e5b716a328220d47ae5 iwlwifi: uninline iwl_trans_send_cmd > > :::::: TO: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > :::::: CC: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 16:48 ` Nick Desaulniers @ 2018-07-31 17:02 ` Kees Cook 2018-07-31 17:09 ` Nick Desaulniers 2018-07-31 18:58 ` Nick Desaulniers 2018-07-31 17:02 ` Nathan Chancellor 1 sibling, 2 replies; 16+ messages in thread From: Kees Cook @ 2018-07-31 17:02 UTC (permalink / raw) To: Nick Desaulniers Cc: kbuild test robot, kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, Christophe Leroy, Stafford Horne, Masahiro Yamada, Ingo Molnar, Greg KH, Thomas Gleixner, Randy Dunlap, Borislav Petkov, NeilBrown, LKML, Andrey Ryabinin, Woodhouse, David, sandipan, Rasmus Villemoes, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, # 3.4.x, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, Andrew Vagin On Tue, Jul 31, 2018 at 9:48 AM, Nick Desaulniers <ndesaulniers@google.com> wrote: > Does anyone understand this error? The code looks just fine to me, > and the source file doesn't conflict with any of the macros I've added > (certainly not in any way that could cause an indentation error as > reported here). It does _use_ the macro, but yeah, I'm stumped... > On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': >> >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] >> if (!(cmd->flags & CMD_ASYNC)) >> ^~ >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' >> lock_map_acquire_read(&trans->sync_cmd_lockdep_map); >> ^ ~ >> >> vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c >> >> 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) >> 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); #define lock_map_acquire_read(l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 1, n, i) The config doesn't have CONFIG_LOCKDEP, so it's not: extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, int trylock, int read, int check, struct lockdep_map *nest_lock, unsigned long ip); but rather: # define lock_acquire(l, s, t, r, c, n, i) do { } while (0) If you build with the same gcc and config, does it reproduce for you? -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 17:02 ` Kees Cook @ 2018-07-31 17:09 ` Nick Desaulniers 2018-07-31 18:58 ` Nick Desaulniers 1 sibling, 0 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-31 17:09 UTC (permalink / raw) To: Kees Cook Cc: lkp, kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin On Tue, Jul 31, 2018 at 10:02 AM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jul 31, 2018 at 9:48 AM, Nick Desaulniers > <ndesaulniers@google.com> wrote: > > Does anyone understand this error? The code looks just fine to me, > > and the source file doesn't conflict with any of the macros I've added > > (certainly not in any way that could cause an indentation error as > > reported here). > > It does _use_ the macro, but yeah, I'm stumped... > > > On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': > >> >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] > >> if (!(cmd->flags & CMD_ASYNC)) > >> ^~ > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' > >> lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > >> ^ ~ > >> > >> vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c > >> > >> 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) > >> 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > #define lock_map_acquire_read(l) > lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) > > #define lock_acquire_shared_recursive(l, s, t, n, i) > lock_acquire(l, s, t, 2, 1, n, i) > > The config doesn't have CONFIG_LOCKDEP, so it's not: > > extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, > int trylock, int read, int check, > struct lockdep_map *nest_lock, unsigned long ip); > > but rather: > > # define lock_acquire(l, s, t, r, c, n, i) do { } while (0) > > If you build with the same gcc and config, does it reproduce for you? Indeed, gcc 6, 7, or 8 + the provided config reproduce the issue. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 17:02 ` Kees Cook 2018-07-31 17:09 ` Nick Desaulniers @ 2018-07-31 18:58 ` Nick Desaulniers 2018-07-31 21:10 ` Nick Desaulniers 1 sibling, 1 reply; 16+ messages in thread From: Nick Desaulniers @ 2018-07-31 18:58 UTC (permalink / raw) To: Kees Cook Cc: lkp, kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin On Tue, Jul 31, 2018 at 10:02 AM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jul 31, 2018 at 9:48 AM, Nick Desaulniers > > On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': > >> >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] > >> if (!(cmd->flags & CMD_ASYNC)) > >> ^~ > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' > >> lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > >> ^ ~ > >> > >> vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c > >> > >> 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) > >> 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > #define lock_map_acquire_read(l) > lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) > > #define lock_acquire_shared_recursive(l, s, t, n, i) > lock_acquire(l, s, t, 2, 1, n, i) > > The config doesn't have CONFIG_LOCKDEP, so it's not: > > extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, > int trylock, int read, int check, > struct lockdep_map *nest_lock, unsigned long ip); > > but rather: > > # define lock_acquire(l, s, t, r, c, n, i) do { } while (0) This is tricky, if I preprocess that translation unit with the exact flags used during compilation, I get: ``` if (!(cmd->flags & CMD_ASYNC)) #pragma GCC diagnostic push #pragma GCC diagnostic pop do { } while (0); ``` Which is not enough to trigger -Wmisleading-indentation alone. It is curious that if we add braces to that if statement (as Nathan notes in a sibling post) or removing the pop (not shippable) seems to fix the warning. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 18:58 ` Nick Desaulniers @ 2018-07-31 21:10 ` Nick Desaulniers 0 siblings, 0 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-31 21:10 UTC (permalink / raw) To: Kees Cook Cc: lkp, kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin On Tue, Jul 31, 2018 at 11:58 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Jul 31, 2018 at 10:02 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Tue, Jul 31, 2018 at 9:48 AM, Nick Desaulniers > > > On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: > > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': > > >> >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] > > >> if (!(cmd->flags & CMD_ASYNC)) > > >> ^~ > > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' > > >> lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > >> ^ ~ > > >> > > >> vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c > > >> > > >> 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) > > >> 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > > > #define lock_map_acquire_read(l) > > lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) > > > > #define lock_acquire_shared_recursive(l, s, t, n, i) > > lock_acquire(l, s, t, 2, 1, n, i) > > > > The config doesn't have CONFIG_LOCKDEP, so it's not: > > > > extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, > > int trylock, int read, int check, > > struct lockdep_map *nest_lock, unsigned long ip); > > > > but rather: > > > > # define lock_acquire(l, s, t, r, c, n, i) do { } while (0) > > This is tricky, if I preprocess that translation unit with the exact > flags used during compilation, I get: > > ``` > if (!(cmd->flags & CMD_ASYNC)) > > #pragma GCC diagnostic push > > #pragma GCC diagnostic pop > do { } while (0); > ``` > > Which is not enough to trigger -Wmisleading-indentation alone. It is > curious that if we add braces to that if statement (as Nathan notes in > a sibling post) or removing the pop (not shippable) seems to fix the > warning. Something fishy is going on here: https://godbolt.org/g/b5dsqH It seems that gcc's warning is technically correct, but it seems to be a miscompile as puts() in my reduced test case is called unconditionally. I've filed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86765 In the meanwhile, I've reworked the patch to change _THIS_IP_ to a only contain a function call, to a new static inline function which does what the statement expression used to. This now triggers -Wreturn-local-addr warnings in gcc, which is a warning added in gcc-4.8, so I need to add another __diag_ignore, and case for gcc 4.8 to include/linux/compiler-gcc.h. At this point, I think I might as well consolidate current_text_addr() and _THIS_IP_. Stay tuned for v3. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 16:48 ` Nick Desaulniers 2018-07-31 17:02 ` Kees Cook @ 2018-07-31 17:02 ` Nathan Chancellor 1 sibling, 0 replies; 16+ messages in thread From: Nathan Chancellor @ 2018-07-31 17:02 UTC (permalink / raw) To: Nick Desaulniers Cc: lkp, kbuild-all, Andrew Morton, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Kees Cook, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin On Tue, Jul 31, 2018 at 09:48:57AM -0700, Nick Desaulniers wrote: > Does anyone understand this error? The code looks just fine to me, > and the source file doesn't conflict with any of the macros I've added > (certainly not in any way that could cause an indentation error as > reported here). Unfortunately, I've been trying to work through it this morning and I can't understand it either. lock_map_acquire_read's defintion does include _THIS_IP_ and adding curly braces to the if statement fixes it but that doesn't explain why it's happening. Maybe this is a GCC bug/issue? Cheers, Nathan > On Tue, Jul 31, 2018 at 3:27 AM kbuild test robot <lkp@intel.com> wrote: > > > > Hi Nick, > > > > Thank you for the patch! Perhaps something to improve: > > > > [auto build test WARNING on linus/master] > > [also build test WARNING on v4.18-rc7 next-20180727] > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > > > url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/compiler-clang-h-Add-CLANG_VERSION-and-__diag-macros/20180731-161932 > > config: x86_64-fedora-25 (attached as .config) > > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 > > reproduce: > > # save the attached .config to linux build tree > > make ARCH=x86_64 > > > > All warnings (new ones prefixed by >>): > > > > drivers/net//wireless/intel/iwlwifi/iwl-trans.c: In function 'iwl_trans_send_cmd': > > >> drivers/net//wireless/intel/iwlwifi/iwl-trans.c:137:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation] > > if (!(cmd->flags & CMD_ASYNC)) > > ^~ > > drivers/net//wireless/intel/iwlwifi/iwl-trans.c:138:1: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' > > lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > ^ ~ > > > > vim +/if +137 drivers/net//wireless/intel/iwlwifi/iwl-trans.c > > > > 92fe8343 Emmanuel Grumbach 2015-12-01 116 > > 92fe8343 Emmanuel Grumbach 2015-12-01 117 int iwl_trans_send_cmd(struct iwl_trans *trans, struct iwl_host_cmd *cmd) > > 92fe8343 Emmanuel Grumbach 2015-12-01 118 { > > 92fe8343 Emmanuel Grumbach 2015-12-01 119 int ret; > > 92fe8343 Emmanuel Grumbach 2015-12-01 120 > > 92fe8343 Emmanuel Grumbach 2015-12-01 121 if (unlikely(!(cmd->flags & CMD_SEND_IN_RFKILL) && > > 326477e4 Johannes Berg 2017-04-25 122 test_bit(STATUS_RFKILL_OPMODE, &trans->status))) > > 92fe8343 Emmanuel Grumbach 2015-12-01 123 return -ERFKILL; > > 92fe8343 Emmanuel Grumbach 2015-12-01 124 > > 92fe8343 Emmanuel Grumbach 2015-12-01 125 if (unlikely(test_bit(STATUS_FW_ERROR, &trans->status))) > > 92fe8343 Emmanuel Grumbach 2015-12-01 126 return -EIO; > > 92fe8343 Emmanuel Grumbach 2015-12-01 127 > > 92fe8343 Emmanuel Grumbach 2015-12-01 128 if (unlikely(trans->state != IWL_TRANS_FW_ALIVE)) { > > 92fe8343 Emmanuel Grumbach 2015-12-01 129 IWL_ERR(trans, "%s bad state = %d\n", __func__, trans->state); > > 92fe8343 Emmanuel Grumbach 2015-12-01 130 return -EIO; > > 92fe8343 Emmanuel Grumbach 2015-12-01 131 } > > 92fe8343 Emmanuel Grumbach 2015-12-01 132 > > 92fe8343 Emmanuel Grumbach 2015-12-01 133 if (WARN_ON((cmd->flags & CMD_WANT_ASYNC_CALLBACK) && > > 92fe8343 Emmanuel Grumbach 2015-12-01 134 !(cmd->flags & CMD_ASYNC))) > > 92fe8343 Emmanuel Grumbach 2015-12-01 135 return -EINVAL; > > 92fe8343 Emmanuel Grumbach 2015-12-01 136 > > 92fe8343 Emmanuel Grumbach 2015-12-01 @137 if (!(cmd->flags & CMD_ASYNC)) > > 92fe8343 Emmanuel Grumbach 2015-12-01 138 lock_map_acquire_read(&trans->sync_cmd_lockdep_map); > > 92fe8343 Emmanuel Grumbach 2015-12-01 139 > > 5b88792c Sara Sharon 2016-08-15 140 if (trans->wide_cmd_header && !iwl_cmd_groupid(cmd->id)) > > 5b88792c Sara Sharon 2016-08-15 141 cmd->id = DEF_ID(cmd->id); > > 5b88792c Sara Sharon 2016-08-15 142 > > 92fe8343 Emmanuel Grumbach 2015-12-01 143 ret = trans->ops->send_cmd(trans, cmd); > > 92fe8343 Emmanuel Grumbach 2015-12-01 144 > > 92fe8343 Emmanuel Grumbach 2015-12-01 145 if (!(cmd->flags & CMD_ASYNC)) > > 92fe8343 Emmanuel Grumbach 2015-12-01 146 lock_map_release(&trans->sync_cmd_lockdep_map); > > 92fe8343 Emmanuel Grumbach 2015-12-01 147 > > 0ec971fd Johannes Berg 2017-04-10 148 if (WARN_ON((cmd->flags & CMD_WANT_SKB) && !ret && !cmd->resp_pkt)) > > 0ec971fd Johannes Berg 2017-04-10 149 return -EIO; > > 0ec971fd Johannes Berg 2017-04-10 150 > > 92fe8343 Emmanuel Grumbach 2015-12-01 151 return ret; > > 92fe8343 Emmanuel Grumbach 2015-12-01 152 } > > 92fe8343 Emmanuel Grumbach 2015-12-01 153 IWL_EXPORT_SYMBOL(iwl_trans_send_cmd); > > 39bdb17e Sharon Dvir 2015-10-15 154 > > > > :::::: The code at line 137 was first introduced by commit > > :::::: 92fe83430b899b786c837e5b716a328220d47ae5 iwlwifi: uninline iwl_trans_send_cmd > > > > :::::: TO: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > :::::: CC: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > > > --- > > 0-DAY kernel test infrastructure Open Source Technology Center > > https://lists.01.org/pipermail/kbuild-all Intel Corporation > > > > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor 2018-07-31 10:27 ` kbuild test robot @ 2018-07-31 13:53 ` kbuild test robot 2018-07-31 16:55 ` Nick Desaulniers 2 siblings, 1 reply; 16+ messages in thread From: kbuild test robot @ 2018-07-31 13:53 UTC (permalink / raw) To: Nick Desaulniers Cc: kbuild-all, akpm, natechancellor, arnd, paul.burton, christophe.leroy, shorne, yamada.masahiro, keescook, mingo, gregkh, tglx, rdunlap, bp, neilb, linux-kernel, aryabinin, dwmw, sandipan, linux, paullawrence, andreyknvl, will.deacon, ghackmann, stable, ghackmann, mka, jpoimboe, wvw, avagin, Nick Desaulniers [-- Attachment #1: Type: text/plain, Size: 4587 bytes --] Hi Nick, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v4.18-rc7 next-20180727] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/compiler-clang-h-Add-CLANG_VERSION-and-__diag-macros/20180731-161932 config: x86_64-randconfig-s1-07312048 (attached as .config) compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/gpu//drm/i915/i915_gem.c: In function '__i915_gem_object_set_pages': >> drivers/gpu//drm/i915/i915_gem.c:2697:1: error: expected expression before '#pragma' GEM_BUG_ON(!HAS_PAGE_SIZES(i915, obj->mm.page_sizes.sg)); ^~~ vim +2697 drivers/gpu//drm/i915/i915_gem.c 03ac84f18 Chris Wilson 2016-10-28 2658 03ac84f18 Chris Wilson 2016-10-28 2659 void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, a5c081662 Matthew Auld 2017-10-06 2660 struct sg_table *pages, 84e8978e6 Matthew Auld 2017-10-09 2661 unsigned int sg_page_sizes) 03ac84f18 Chris Wilson 2016-10-28 2662 { a5c081662 Matthew Auld 2017-10-06 2663 struct drm_i915_private *i915 = to_i915(obj->base.dev); a5c081662 Matthew Auld 2017-10-06 2664 unsigned long supported = INTEL_INFO(i915)->page_sizes; a5c081662 Matthew Auld 2017-10-06 2665 int i; a5c081662 Matthew Auld 2017-10-06 2666 1233e2db1 Chris Wilson 2016-10-28 2667 lockdep_assert_held(&obj->mm.lock); 03ac84f18 Chris Wilson 2016-10-28 2668 03ac84f18 Chris Wilson 2016-10-28 2669 obj->mm.get_page.sg_pos = pages->sgl; 03ac84f18 Chris Wilson 2016-10-28 2670 obj->mm.get_page.sg_idx = 0; 03ac84f18 Chris Wilson 2016-10-28 2671 03ac84f18 Chris Wilson 2016-10-28 2672 obj->mm.pages = pages; 2c3a3f44d Chris Wilson 2016-11-04 2673 2c3a3f44d Chris Wilson 2016-11-04 2674 if (i915_gem_object_is_tiled(obj) && f2123818f Chris Wilson 2017-10-16 2675 i915->quirks & QUIRK_PIN_SWIZZLED_PAGES) { 2c3a3f44d Chris Wilson 2016-11-04 2676 GEM_BUG_ON(obj->mm.quirked); 2c3a3f44d Chris Wilson 2016-11-04 2677 __i915_gem_object_pin_pages(obj); 2c3a3f44d Chris Wilson 2016-11-04 2678 obj->mm.quirked = true; 2c3a3f44d Chris Wilson 2016-11-04 2679 } a5c081662 Matthew Auld 2017-10-06 2680 84e8978e6 Matthew Auld 2017-10-09 2681 GEM_BUG_ON(!sg_page_sizes); 84e8978e6 Matthew Auld 2017-10-09 2682 obj->mm.page_sizes.phys = sg_page_sizes; a5c081662 Matthew Auld 2017-10-06 2683 a5c081662 Matthew Auld 2017-10-06 2684 /* 84e8978e6 Matthew Auld 2017-10-09 2685 * Calculate the supported page-sizes which fit into the given 84e8978e6 Matthew Auld 2017-10-09 2686 * sg_page_sizes. This will give us the page-sizes which we may be able 84e8978e6 Matthew Auld 2017-10-09 2687 * to use opportunistically when later inserting into the GTT. For 84e8978e6 Matthew Auld 2017-10-09 2688 * example if phys=2G, then in theory we should be able to use 1G, 2M, 84e8978e6 Matthew Auld 2017-10-09 2689 * 64K or 4K pages, although in practice this will depend on a number of 84e8978e6 Matthew Auld 2017-10-09 2690 * other factors. a5c081662 Matthew Auld 2017-10-06 2691 */ a5c081662 Matthew Auld 2017-10-06 2692 obj->mm.page_sizes.sg = 0; a5c081662 Matthew Auld 2017-10-06 2693 for_each_set_bit(i, &supported, ilog2(I915_GTT_MAX_PAGE_SIZE) + 1) { a5c081662 Matthew Auld 2017-10-06 2694 if (obj->mm.page_sizes.phys & ~0u << i) a5c081662 Matthew Auld 2017-10-06 2695 obj->mm.page_sizes.sg |= BIT(i); a5c081662 Matthew Auld 2017-10-06 2696 } a5c081662 Matthew Auld 2017-10-06 @2697 GEM_BUG_ON(!HAS_PAGE_SIZES(i915, obj->mm.page_sizes.sg)); f2123818f Chris Wilson 2017-10-16 2698 f2123818f Chris Wilson 2017-10-16 2699 spin_lock(&i915->mm.obj_lock); f2123818f Chris Wilson 2017-10-16 2700 list_add(&obj->mm.link, &i915->mm.unbound_list); f2123818f Chris Wilson 2017-10-16 2701 spin_unlock(&i915->mm.obj_lock); 03ac84f18 Chris Wilson 2016-10-28 2702 } 03ac84f18 Chris Wilson 2016-10-28 2703 :::::: The code at line 2697 was first introduced by commit :::::: a5c08166265adc172a4cbde8ed26a1a96ce77fb7 drm/i915: introduce page_size members :::::: TO: Matthew Auld <matthew.auld@intel.com> :::::: CC: Chris Wilson <chris@chris-wilson.co.uk> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 29265 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ 2018-07-31 13:53 ` kbuild test robot @ 2018-07-31 16:55 ` Nick Desaulniers 0 siblings, 0 replies; 16+ messages in thread From: Nick Desaulniers @ 2018-07-31 16:55 UTC (permalink / raw) To: lkp Cc: kbuild-all, Andrew Morton, Nathan Chancellor, Arnd Bergmann, paul.burton, christophe.leroy, shorne, Masahiro Yamada, Kees Cook, Ingo Molnar, Greg KH, Thomas Gleixner, rdunlap, bp, neilb, LKML, Andrey Ryabinin, dwmw, sandipan, linux, Paul Lawrence, Andrey Konovalov, Will Deacon, ghackmann, stable, Greg Hackmann, Matthias Kaehlcke, Josh Poimboeuf, Wei Wang, avagin Another strange error from 0-day bot. The source file does not make use of my added macros, so not sure how they could generate such a warning. On Tue, Jul 31, 2018 at 6:54 AM kbuild test robot <lkp@intel.com> wrote: > > Hi Nick, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [also build test ERROR on v4.18-rc7 next-20180727] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Nick-Desaulniers/compiler-clang-h-Add-CLANG_VERSION-and-__diag-macros/20180731-161932 > config: x86_64-randconfig-s1-07312048 (attached as .config) > compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > > drivers/gpu//drm/i915/i915_gem.c: In function '__i915_gem_object_set_pages': > >> drivers/gpu//drm/i915/i915_gem.c:2697:1: error: expected expression before '#pragma' > GEM_BUG_ON(!HAS_PAGE_SIZES(i915, obj->mm.page_sizes.sg)); > ^~~ > > vim +2697 drivers/gpu//drm/i915/i915_gem.c > > 03ac84f18 Chris Wilson 2016-10-28 2658 > 03ac84f18 Chris Wilson 2016-10-28 2659 void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, > a5c081662 Matthew Auld 2017-10-06 2660 struct sg_table *pages, > 84e8978e6 Matthew Auld 2017-10-09 2661 unsigned int sg_page_sizes) > 03ac84f18 Chris Wilson 2016-10-28 2662 { > a5c081662 Matthew Auld 2017-10-06 2663 struct drm_i915_private *i915 = to_i915(obj->base.dev); > a5c081662 Matthew Auld 2017-10-06 2664 unsigned long supported = INTEL_INFO(i915)->page_sizes; > a5c081662 Matthew Auld 2017-10-06 2665 int i; > a5c081662 Matthew Auld 2017-10-06 2666 > 1233e2db1 Chris Wilson 2016-10-28 2667 lockdep_assert_held(&obj->mm.lock); > 03ac84f18 Chris Wilson 2016-10-28 2668 > 03ac84f18 Chris Wilson 2016-10-28 2669 obj->mm.get_page.sg_pos = pages->sgl; > 03ac84f18 Chris Wilson 2016-10-28 2670 obj->mm.get_page.sg_idx = 0; > 03ac84f18 Chris Wilson 2016-10-28 2671 > 03ac84f18 Chris Wilson 2016-10-28 2672 obj->mm.pages = pages; > 2c3a3f44d Chris Wilson 2016-11-04 2673 > 2c3a3f44d Chris Wilson 2016-11-04 2674 if (i915_gem_object_is_tiled(obj) && > f2123818f Chris Wilson 2017-10-16 2675 i915->quirks & QUIRK_PIN_SWIZZLED_PAGES) { > 2c3a3f44d Chris Wilson 2016-11-04 2676 GEM_BUG_ON(obj->mm.quirked); > 2c3a3f44d Chris Wilson 2016-11-04 2677 __i915_gem_object_pin_pages(obj); > 2c3a3f44d Chris Wilson 2016-11-04 2678 obj->mm.quirked = true; > 2c3a3f44d Chris Wilson 2016-11-04 2679 } > a5c081662 Matthew Auld 2017-10-06 2680 > 84e8978e6 Matthew Auld 2017-10-09 2681 GEM_BUG_ON(!sg_page_sizes); > 84e8978e6 Matthew Auld 2017-10-09 2682 obj->mm.page_sizes.phys = sg_page_sizes; > a5c081662 Matthew Auld 2017-10-06 2683 > a5c081662 Matthew Auld 2017-10-06 2684 /* > 84e8978e6 Matthew Auld 2017-10-09 2685 * Calculate the supported page-sizes which fit into the given > 84e8978e6 Matthew Auld 2017-10-09 2686 * sg_page_sizes. This will give us the page-sizes which we may be able > 84e8978e6 Matthew Auld 2017-10-09 2687 * to use opportunistically when later inserting into the GTT. For > 84e8978e6 Matthew Auld 2017-10-09 2688 * example if phys=2G, then in theory we should be able to use 1G, 2M, > 84e8978e6 Matthew Auld 2017-10-09 2689 * 64K or 4K pages, although in practice this will depend on a number of > 84e8978e6 Matthew Auld 2017-10-09 2690 * other factors. > a5c081662 Matthew Auld 2017-10-06 2691 */ > a5c081662 Matthew Auld 2017-10-06 2692 obj->mm.page_sizes.sg = 0; > a5c081662 Matthew Auld 2017-10-06 2693 for_each_set_bit(i, &supported, ilog2(I915_GTT_MAX_PAGE_SIZE) + 1) { > a5c081662 Matthew Auld 2017-10-06 2694 if (obj->mm.page_sizes.phys & ~0u << i) > a5c081662 Matthew Auld 2017-10-06 2695 obj->mm.page_sizes.sg |= BIT(i); > a5c081662 Matthew Auld 2017-10-06 2696 } > a5c081662 Matthew Auld 2017-10-06 @2697 GEM_BUG_ON(!HAS_PAGE_SIZES(i915, obj->mm.page_sizes.sg)); > f2123818f Chris Wilson 2017-10-16 2698 > f2123818f Chris Wilson 2017-10-16 2699 spin_lock(&i915->mm.obj_lock); > f2123818f Chris Wilson 2017-10-16 2700 list_add(&obj->mm.link, &i915->mm.unbound_list); > f2123818f Chris Wilson 2017-10-16 2701 spin_unlock(&i915->mm.obj_lock); > 03ac84f18 Chris Wilson 2016-10-28 2702 } > 03ac84f18 Chris Wilson 2016-10-28 2703 > > :::::: The code at line 2697 was first introduced by commit > :::::: a5c08166265adc172a4cbde8ed26a1a96ce77fb7 drm/i915: introduce page_size members > > :::::: TO: Matthew Auld <matthew.auld@intel.com> > :::::: CC: Chris Wilson <chris@chris-wilson.co.uk> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-08-31 22:16 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-30 21:34 [PATCH v2 0/2] CLANG_VERSION and __diag macros Nick Desaulniers 2018-07-30 21:34 ` [PATCH v2 1/2] compiler-clang.h: Add " Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor 2018-08-31 21:50 ` Nick Desaulniers 2018-08-31 22:16 ` Miguel Ojeda 2018-07-30 21:34 ` [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_ Nick Desaulniers 2018-07-30 23:25 ` Nathan Chancellor 2018-07-31 10:27 ` kbuild test robot 2018-07-31 16:48 ` Nick Desaulniers 2018-07-31 17:02 ` Kees Cook 2018-07-31 17:09 ` Nick Desaulniers 2018-07-31 18:58 ` Nick Desaulniers 2018-07-31 21:10 ` Nick Desaulniers 2018-07-31 17:02 ` Nathan Chancellor 2018-07-31 13:53 ` kbuild test robot 2018-07-31 16:55 ` Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).