[PATCH v2 0/2] CLANG_VERSION and __diag macros

[PATCH v2 0/2] CLANG_VERSION and __diag macros

[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

[PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros

[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

[PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nathan Chancellor]

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
> 

Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros

[Nathan Chancellor]

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
> 

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[kbuild test robot]

[-- 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 --]

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[kbuild test robot]

[-- 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 --]

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nick Desaulniers]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nick Desaulniers]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Kees Cook]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nathan Chancellor]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nick Desaulniers]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nick Desaulniers]

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

Re: [PATCH v2 2/2] kernel.h: Disable -Wreturn-stack-address for _THIS_IP_

[Nick Desaulniers]

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

Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros

[Nick Desaulniers]

+ 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

Re: [PATCH v2 1/2] compiler-clang.h: Add CLANG_VERSION and __diag macros

[Miguel Ojeda]

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