[PATCH 0/7] Silence even more W=2 warnings

[PATCH 0/7] Silence even more W=2 warnings

[Jeff Kirsher]

The following patches silence over 100,000 warnings in a W=2
kernel build. This series does most of it by using the compilers
diagnostic controls. The first patch in the series adds macros to
invoke the pragmas for those controls. Macros are provided for GCC
and clang. Although they are highly compatible in this area, macros
are provided for compiler-specific controls, and there is one
example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).

Some missing-field-initializers warnings were resolved using
the diagnostic control macros simply because so many lines
would have had to have been changed. At this stage Mark thought 
about avoiding possible merge issues. If the maintainer would 
rather resolve them by using designated initialization, just 
say so.

The combined effect of this patch series and his other patches
that did not use these diagnostic control macros was to reduce 
the number of W=2 warnings from 127,164 to 1,345!

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Mark Rustad (7):
  compiler: Add diagnostic control macros
  x86: Silence initializer-overrides warnings
  atomic: Silence nested-externs warnings
  bitops: Silence nested-externs warnings
  signal: Silence nested-externs warnings
  mm: Silence nested-externs warnings
  sched: Silence nested-externs warnings

 arch/x86/ia32/syscall_ia32.c   |  2 ++
 include/linux/atomic.h         |  2 ++
 include/linux/bitops.h         |  2 ++
 include/linux/compiler-clang.h | 26 ++++++++++++++++++++++++++
 include/linux/compiler-gcc4.h  | 31 +++++++++++++++++++++++++++++++
 include/linux/compiler.h       | 20 ++++++++++++++++++++
 include/linux/mm.h             |  2 ++
 include/linux/sched.h          |  2 ++
 include/linux/signal.h         |  6 ++++++
 9 files changed, 93 insertions(+)

-- 
1.9.3

[PATCH 1/7] compiler: Add diagnostic control macros

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Add macros to control diagnostic messages where needed. These
are used to silence warning messages that are expected, normal
and do not indicate any sort of problem. Reducing the stream
of messages in this way helps possible problems to stand out.

The macros provided are:
	DIAG_PUSH() - to save diagnostic settings
	DIAG_POP() - to restore diagnostic settings
	DIAG_IGNORE(option) - to ignore a particular warning
	DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
	DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only

CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/compiler-clang.h | 26 ++++++++++++++++++++++++++
 include/linux/compiler-gcc4.h  | 31 +++++++++++++++++++++++++++++++
 include/linux/compiler.h       | 20 ++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index d1e49d5..039b112 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -10,3 +10,29 @@
 #undef uninitialized_var
 #define uninitialized_var(x) x = *(&(x))
 #endif
+
+/*
+ * Provide macros to manipulate diagnostic messages when possible.
+ * DIAG_PUSH pushes the diagnostic settings
+ * DIAG_POP pops the diagnostic settings
+ * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
+ *
+ * Example:
+ *	DIAG_PUSH DIAG_IGNORE(aggregate-return)
+ *	struct timespec ns_to_timespec(const s64 nsec)
+ *	{
+ *		...
+ *	}
+ *	DIAG_POP
+ *
+ * Will prevent the warning on compilation of the function. Other
+ * steps are necessary to do the same thing for the call sites.
+ */
+#define DIAG_STR(x) #x
+#define DIAG_CAT(x, y) DIAG_STR(x##y)
+#define DIAG_DO_PRAGMA(x) _Pragma(#x)
+#define DIAG_PRAGMA(x) DIAG_DO_PRAGMA(clang diagnostic x)
+#define DIAG_IGNORE(x) DIAG_PRAGMA(ignored DIAG_CAT(-W, x))
+#define DIAG_PUSH DIAG_PRAGMA(push)
+#define DIAG_POP DIAG_PRAGMA(pop)
+#define DIAG_CLANG_IGNORE(x) DIAG_IGNORE(x)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 2507fd2..78387f6 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -86,3 +86,34 @@
 #define __HAVE_BUILTIN_BSWAP16__
 #endif
 #endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP */
+
+/*
+ * Provide macros to manipulate diagnostic messages when possible.
+ * DIAG_PUSH pushes the diagnostic settings
+ * DIAG_POP pops the diagnostic settings
+ * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
+ *
+ * Example:
+ *	DIAG_PUSH DIAG_IGNORE(aggregate-return)
+ *	struct timespec ns_to_timespec(const s64 nsec)
+ *	{
+ *		...
+ *	}
+ *	DIAG_POP
+ *
+ * Will prevent the warning on compilation of the function. Other
+ * steps are necessary to do the same thing for the call sites.
+ */
+#if GCC_VERSION >= 40600
+#define DIAG_STR(x) #x
+#define DIAG_CAT(x, y) DIAG_STR(x##y)
+#define DIAG_DO_PRAGMA(x) _Pragma(#x)
+#define DIAG_PRAGMA(x) DIAG_DO_PRAGMA(GCC diagnostic x)
+#define DIAG_IGNORE(x) DIAG_PRAGMA(ignored DIAG_CAT(-W, x))
+/* GCC 4.8 - 4.8.2 has issues with pop, so do not define push or pop */
+#if GCC_VERSION < 40800 || GCC_VERSION >= 40803
+#define DIAG_PUSH DIAG_PRAGMA(push)
+#define DIAG_POP DIAG_PRAGMA(pop)
+#endif /* GCC_VERSION < 40800 || GCC_VERSION >= 40803 */
+#define DIAG_GCC_IGNORE(x) DIAG_IGNORE(x)
+#endif /* GCC_VERSION >= 40600 */
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1..fde7b21 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -94,6 +94,26 @@ struct ftrace_branch_data {
 };
 
 /*
+ * Provide null definitions for diagnostic manipulation macros not provided
+ * for a particular compiler.
+ */
+#ifndef DIAG_PUSH
+#define DIAG_PUSH
+#endif
+#ifndef DIAG_POP
+#define DIAG_POP
+#endif
+#ifndef DIAG_IGNORE
+#define DIAG_IGNORE(x)
+#endif
+#ifndef DIAG_CLANG_IGNORE
+#define DIAG_CLANG_IGNORE(x)
+#endif
+#ifndef DIAG_GCC_IGNORE
+#define DIAG_GCC_IGNORE(x)
+#endif
+
+/*
  * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
  * to disable branch tracing on a per file basis.
  */
-- 
1.9.3

[PATCH 2/7] x86: Silence initializer-overrides warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Since the syscall table intentionally uses override initialization,
simply silence those warnings.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: <x86@kernel.org>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 arch/x86/ia32/syscall_ia32.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/ia32/syscall_ia32.c b/arch/x86/ia32/syscall_ia32.c
index 4754ba0..7bc553b 100644
--- a/arch/x86/ia32/syscall_ia32.c
+++ b/arch/x86/ia32/syscall_ia32.c
@@ -15,6 +15,7 @@ typedef void (*sys_call_ptr_t)(void);
 
 extern void compat_ni_syscall(void);
 
+DIAG_PUSH DIAG_CLANG_IGNORE(initializer-overrides)
 const sys_call_ptr_t ia32_sys_call_table[__NR_ia32_syscall_max+1] = {
 	/*
 	 * Smells like a compiler bug -- it doesn't work
@@ -23,3 +24,4 @@ const sys_call_ptr_t ia32_sys_call_table[__NR_ia32_syscall_max+1] = {
 	[0 ... __NR_ia32_syscall_max] = &compat_ni_syscall,
 #include <asm/syscalls_32.h>
 };
+DIAG_POP
-- 
1.9.3

[PATCH 3/7] atomic: Silence nested-externs warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Silence the nested-externs warnings for these, as they are
truly wanted.

CC: Peter Zijlstra <peterz@infradead.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/atomic.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index fef3a80..81c7ad4 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -7,6 +7,7 @@
  * Provide __deprecated wrappers for the new interface, avoid flag day changes.
  * We need the ugly external functions to break header recursion hell.
  */
+DIAG_PUSH DIAG_IGNORE(nested-externs)
 #ifndef smp_mb__before_atomic_inc
 static inline void __deprecated smp_mb__before_atomic_inc(void)
 {
@@ -38,6 +39,7 @@ static inline void __deprecated smp_mb__after_atomic_dec(void)
 	__smp_mb__after_atomic();
 }
 #endif
+DIAG_POP
 
 /**
  * atomic_add_unless - add unless the number is already a given value
-- 
1.9.3

Re: [PATCH 3/7] atomic: Silence nested-externs warnings

[Peter Zijlstra]

On Fri, Sep 19, 2014 at 08:29:36AM -0700, Jeff Kirsher wrote:
> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Silence the nested-externs warnings for these, as they are
> truly wanted.
> 

You're patching an old tree.

---
commit 2e39465abc4b7856a0ea6fcf4f6b4668bb5db877
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Mon Aug 4 12:07:15 2014 +0200

    locking: Remove deprecated smp_mb__() barriers
    
    Its been a while and there are no in-tree users left, so remove the
    deprecated barriers.
    
    Signed-off-by: Peter Zijlstra <peterz@infradead.org>
    Cc: Chen, Gong <gong.chen@linux.intel.com>
    Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
    Cc: Joe Perches <joe@perches.com>
    Cc: John Sullivan <jsrhbz@kanargh.force9.co.uk>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
    Cc: Theodore Ts'o <tytso@mit.edu>
    Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index fef3a80..5b08a85 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -3,42 +3,6 @@
 #define _LINUX_ATOMIC_H
 #include <asm/atomic.h>
 
-/*
- * Provide __deprecated wrappers for the new interface, avoid flag day changes.
- * We need the ugly external functions to break header recursion hell.
- */
-#ifndef smp_mb__before_atomic_inc
-static inline void __deprecated smp_mb__before_atomic_inc(void)
-{
-	extern void __smp_mb__before_atomic(void);
-	__smp_mb__before_atomic();
-}
-#endif
-
-#ifndef smp_mb__after_atomic_inc
-static inline void __deprecated smp_mb__after_atomic_inc(void)
-{
-	extern void __smp_mb__after_atomic(void);
-	__smp_mb__after_atomic();
-}
-#endif
-
-#ifndef smp_mb__before_atomic_dec
-static inline void __deprecated smp_mb__before_atomic_dec(void)
-{
-	extern void __smp_mb__before_atomic(void);
-	__smp_mb__before_atomic();
-}
-#endif
-
-#ifndef smp_mb__after_atomic_dec
-static inline void __deprecated smp_mb__after_atomic_dec(void)
-{
-	extern void __smp_mb__after_atomic(void);
-	__smp_mb__after_atomic();
-}
-#endif
-
 /**
  * atomic_add_unless - add unless the number is already a given value
  * @v: pointer of type atomic_t
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index cbc5833..be5fd38 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -32,26 +32,6 @@ extern unsigned long __sw_hweight64(__u64 w);
  */
 #include <asm/bitops.h>
 
-/*
- * Provide __deprecated wrappers for the new interface, avoid flag day changes.
- * We need the ugly external functions to break header recursion hell.
- */
-#ifndef smp_mb__before_clear_bit
-static inline void __deprecated smp_mb__before_clear_bit(void)
-{
-	extern void __smp_mb__before_atomic(void);
-	__smp_mb__before_atomic();
-}
-#endif
-
-#ifndef smp_mb__after_clear_bit
-static inline void __deprecated smp_mb__after_clear_bit(void)
-{
-	extern void __smp_mb__after_atomic(void);
-	__smp_mb__after_atomic();
-}
-#endif
-
 #define for_each_set_bit(bit, addr, size) \
 	for ((bit) = find_first_bit((addr), (size));		\
 	     (bit) < (size);					\
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 1211575..76c518c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -90,22 +90,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/sched.h>
 
-#ifdef smp_mb__before_atomic
-void __smp_mb__before_atomic(void)
-{
-	smp_mb__before_atomic();
-}
-EXPORT_SYMBOL(__smp_mb__before_atomic);
-#endif
-
-#ifdef smp_mb__after_atomic
-void __smp_mb__after_atomic(void)
-{
-	smp_mb__after_atomic();
-}
-EXPORT_SYMBOL(__smp_mb__after_atomic);
-#endif
-
 void start_bandwidth_timer(struct hrtimer *period_timer, ktime_t period)
 {
 	unsigned long delta;

Re: [PATCH 3/7] atomic: Silence nested-externs warnings

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 4407 bytes --]

On Fri, 2014-09-19 at 22:43 +0200, Peter Zijlstra wrote:
> On Fri, Sep 19, 2014 at 08:29:36AM -0700, Jeff Kirsher wrote:
> > From: Mark Rustad <mark.d.rustad@intel.com>
> > 
> > Silence the nested-externs warnings for these, as they are
> > truly wanted.
> > 
> 
> You're patching an old tree.

Sorry, I used Linus's latest tree (as of last night) because of number
of the patches in the series went to various trees, so I used Linus's
tree.

> 
> ---
> commit 2e39465abc4b7856a0ea6fcf4f6b4668bb5db877
> Author: Peter Zijlstra <peterz@infradead.org>
> Date:   Mon Aug 4 12:07:15 2014 +0200
> 
>     locking: Remove deprecated smp_mb__() barriers
>     
>     Its been a while and there are no in-tree users left, so remove the
>     deprecated barriers.
>     
>     Signed-off-by: Peter Zijlstra <peterz@infradead.org>
>     Cc: Chen, Gong <gong.chen@linux.intel.com>
>     Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>     Cc: Joe Perches <joe@perches.com>
>     Cc: John Sullivan <jsrhbz@kanargh.force9.co.uk>
>     Cc: Linus Torvalds <torvalds@linux-foundation.org>
>     Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>     Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>     Cc: Theodore Ts'o <tytso@mit.edu>
>     Signed-off-by: Ingo Molnar <mingo@kernel.org>
> 
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index fef3a80..5b08a85 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -3,42 +3,6 @@
>  #define _LINUX_ATOMIC_H
>  #include <asm/atomic.h>
>  
> -/*
> - * Provide __deprecated wrappers for the new interface, avoid flag day changes.
> - * We need the ugly external functions to break header recursion hell.
> - */
> -#ifndef smp_mb__before_atomic_inc
> -static inline void __deprecated smp_mb__before_atomic_inc(void)
> -{
> -	extern void __smp_mb__before_atomic(void);
> -	__smp_mb__before_atomic();
> -}
> -#endif
> -
> -#ifndef smp_mb__after_atomic_inc
> -static inline void __deprecated smp_mb__after_atomic_inc(void)
> -{
> -	extern void __smp_mb__after_atomic(void);
> -	__smp_mb__after_atomic();
> -}
> -#endif
> -
> -#ifndef smp_mb__before_atomic_dec
> -static inline void __deprecated smp_mb__before_atomic_dec(void)
> -{
> -	extern void __smp_mb__before_atomic(void);
> -	__smp_mb__before_atomic();
> -}
> -#endif
> -
> -#ifndef smp_mb__after_atomic_dec
> -static inline void __deprecated smp_mb__after_atomic_dec(void)
> -{
> -	extern void __smp_mb__after_atomic(void);
> -	__smp_mb__after_atomic();
> -}
> -#endif
> -
>  /**
>   * atomic_add_unless - add unless the number is already a given value
>   * @v: pointer of type atomic_t
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index cbc5833..be5fd38 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -32,26 +32,6 @@ extern unsigned long __sw_hweight64(__u64 w);
>   */
>  #include <asm/bitops.h>
>  
> -/*
> - * Provide __deprecated wrappers for the new interface, avoid flag day changes.
> - * We need the ugly external functions to break header recursion hell.
> - */
> -#ifndef smp_mb__before_clear_bit
> -static inline void __deprecated smp_mb__before_clear_bit(void)
> -{
> -	extern void __smp_mb__before_atomic(void);
> -	__smp_mb__before_atomic();
> -}
> -#endif
> -
> -#ifndef smp_mb__after_clear_bit
> -static inline void __deprecated smp_mb__after_clear_bit(void)
> -{
> -	extern void __smp_mb__after_atomic(void);
> -	__smp_mb__after_atomic();
> -}
> -#endif
> -
>  #define for_each_set_bit(bit, addr, size) \
>  	for ((bit) = find_first_bit((addr), (size));		\
>  	     (bit) < (size);					\
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1211575..76c518c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -90,22 +90,6 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/sched.h>
>  
> -#ifdef smp_mb__before_atomic
> -void __smp_mb__before_atomic(void)
> -{
> -	smp_mb__before_atomic();
> -}
> -EXPORT_SYMBOL(__smp_mb__before_atomic);
> -#endif
> -
> -#ifdef smp_mb__after_atomic
> -void __smp_mb__after_atomic(void)
> -{
> -	smp_mb__after_atomic();
> -}
> -EXPORT_SYMBOL(__smp_mb__after_atomic);
> -#endif
> -
>  void start_bandwidth_timer(struct hrtimer *period_timer, ktime_t period)
>  {
>  	unsigned long delta;



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/7] bitops: Silence nested-externs warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Use the diagnostic control macros to ignore nested-externs warnings
in these caes.

CC: Ingo Molnar <mingo@kernel.org>
CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Theodore Ts'o" <tytso@mit.edu>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/bitops.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index cbc5833..0e072bd 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -36,6 +36,7 @@ extern unsigned long __sw_hweight64(__u64 w);
  * Provide __deprecated wrappers for the new interface, avoid flag day changes.
  * We need the ugly external functions to break header recursion hell.
  */
+DIAG_PUSH DIAG_IGNORE(nested-externs)
 #ifndef smp_mb__before_clear_bit
 static inline void __deprecated smp_mb__before_clear_bit(void)
 {
@@ -51,6 +52,7 @@ static inline void __deprecated smp_mb__after_clear_bit(void)
 	__smp_mb__after_atomic();
 }
 #endif
+DIAG_POP
 
 #define for_each_set_bit(bit, addr, size) \
 	for ((bit) = find_first_bit((addr), (size));		\
-- 
1.9.3

[PATCH 5/7] signal: Silence nested-externs warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Silence nested-externs warnings for these, as these nested
externs are truly wanted.

CC: Oleg Nesterov <oleg@redhat.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Richard Weinberger <richard@nod.at>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/signal.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 750196f..e68ae6b 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -67,7 +67,9 @@ static inline int sigismember(sigset_t *set, int _sig)
 
 static inline int sigisemptyset(sigset_t *set)
 {
+	DIAG_PUSH DIAG_IGNORE(nested-externs)
 	extern void _NSIG_WORDS_is_unsupported_size(void);
+	DIAG_POP
 	switch (_NSIG_WORDS) {
 	case 4:
 		return (set->sig[3] | set->sig[2] |
@@ -90,7 +92,9 @@ static inline int sigisemptyset(sigset_t *set)
 #define _SIG_SET_BINOP(name, op)					\
 static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
 {									\
+	DIAG_PUSH DIAG_IGNORE(nested-externs)				\
 	extern void _NSIG_WORDS_is_unsupported_size(void);		\
+	DIAG_POP							\
 	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
 									\
 	switch (_NSIG_WORDS) {						\
@@ -128,7 +132,9 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
 {									\
+	DIAG_PUSH DIAG_IGNORE(nested-externs)				\
 	extern void _NSIG_WORDS_is_unsupported_size(void);		\
+	DIAG_POP							\
 									\
 	switch (_NSIG_WORDS) {						\
 	    case 4: set->sig[3] = op(set->sig[3]);			\
-- 
1.9.3

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Richard Weinberger]

Am 19.09.2014 17:29, schrieb Jeff Kirsher:
> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Silence nested-externs warnings for these, as these nested
> externs are truly wanted.
> 
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Richard Weinberger <richard@nod.at>
> CC: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  include/linux/signal.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 750196f..e68ae6b 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -67,7 +67,9 @@ static inline int sigismember(sigset_t *set, int _sig)
>  
>  static inline int sigisemptyset(sigset_t *set)
>  {
> +	DIAG_PUSH DIAG_IGNORE(nested-externs)

Do we really want to clutter the source with such tags?
Does this even build? i.e. how does gcc know to ignore that?

rw@azrael:~/linux (ubi-wlcrash $)> git grep DIAG_PUSH | wc -l
0

Thanks,
//richard

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Fri, 2014-09-19 at 17:35 +0200, Richard Weinberger wrote:
> Am 19.09.2014 17:29, schrieb Jeff Kirsher:
> > From: Mark Rustad <mark.d.rustad@intel.com>
> > 
> > Silence nested-externs warnings for these, as these nested
> > externs are truly wanted.
> > 
> > CC: Oleg Nesterov <oleg@redhat.com>
> > CC: Andrew Morton <akpm@linux-foundation.org>
> > CC: Geert Uytterhoeven <geert@linux-m68k.org>
> > CC: Richard Weinberger <richard@nod.at>
> > CC: Brian Norris <computersforpeace@gmail.com>
> > Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> >  include/linux/signal.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/linux/signal.h b/include/linux/signal.h
> > index 750196f..e68ae6b 100644
> > --- a/include/linux/signal.h
> > +++ b/include/linux/signal.h
> > @@ -67,7 +67,9 @@ static inline int sigismember(sigset_t *set, int _sig)
> >  
> >  static inline int sigisemptyset(sigset_t *set)
> >  {
> > +	DIAG_PUSH DIAG_IGNORE(nested-externs)
> 
> Do we really want to clutter the source with such tags?
> Does this even build? i.e. how does gcc know to ignore that?
> 
> rw@azrael:~/linux (ubi-wlcrash $)> git grep DIAG_PUSH | wc -l
> 0
> 
> Thanks,
> //richard

See patch 1 of the series.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

Am 19.09.2014 17:37, schrieb Jeff Kirsher:
> On Fri, 2014-09-19 at 17:35 +0200, Richard Weinberger wrote:
>> Am 19.09.2014 17:29, schrieb Jeff Kirsher:
>>> From: Mark Rustad <mark.d.rustad@intel.com>
>>>
>>> Silence nested-externs warnings for these, as these nested
>>> externs are truly wanted.
>>>
>>> CC: Oleg Nesterov <oleg@redhat.com>
>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>> CC: Geert Uytterhoeven <geert@linux-m68k.org>
>>> CC: Richard Weinberger <richard@nod.at>
>>> CC: Brian Norris <computersforpeace@gmail.com>
>>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>>  include/linux/signal.h | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>>> index 750196f..e68ae6b 100644
>>> --- a/include/linux/signal.h
>>> +++ b/include/linux/signal.h
>>> @@ -67,7 +67,9 @@ static inline int sigismember(sigset_t *set, int _sig)
>>>  
>>>  static inline int sigisemptyset(sigset_t *set)
>>>  {
>>> +	DIAG_PUSH DIAG_IGNORE(nested-externs)
>>
>> Do we really want to clutter the source with such tags?
>> Does this even build? i.e. how does gcc know to ignore that?
>>
>> rw@azrael:~/linux (ubi-wlcrash $)> git grep DIAG_PUSH | wc -l
>> 0
>>
>> Thanks,
>> //richard
> 
> See patch 1 of the series.

I was not CC'ed...

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Oleg Nesterov]

On 09/19, Richard Weinberger wrote:
>
> Am 19.09.2014 17:37, schrieb Jeff Kirsher:
> >
> > See patch 1 of the series.
>
> I was not CC'ed...

Me too, and thus I don't understand this patch.

But I have to admit it looks a bit ugly to me anyway.
Can't we simply kill _NSIG_WORDS_is_unsupported_size ?

Oleg.
---

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 750196f..679c9b4 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
 
 static inline int sigisemptyset(sigset_t *set)
 {
-	extern void _NSIG_WORDS_is_unsupported_size(void);
 	switch (_NSIG_WORDS) {
 	case 4:
 		return (set->sig[3] | set->sig[2] |
@@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
 	case 1:
 		return set->sig[0] == 0;
 	default:
-		_NSIG_WORDS_is_unsupported_size();
+		BUILD_BUG();
 		return 0;
 	}
 }
@@ -90,7 +89,6 @@ static inline int sigisemptyset(sigset_t *set)
 #define _SIG_SET_BINOP(name, op)					\
 static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
 {									\
-	extern void _NSIG_WORDS_is_unsupported_size(void);		\
 	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
 									\
 	switch (_NSIG_WORDS) {						\
@@ -107,7 +105,7 @@ static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
 		r->sig[0] = op(a0, b0);					\
 		break;							\
 	    default:							\
-		_NSIG_WORDS_is_unsupported_size();			\
+	    	BUILD_BUG();						\
 	}								\
 }
 
@@ -128,8 +126,6 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
 {									\
-	extern void _NSIG_WORDS_is_unsupported_size(void);		\
-									\
 	switch (_NSIG_WORDS) {						\
 	    case 4: set->sig[3] = op(set->sig[3]);			\
 		    set->sig[2] = op(set->sig[2]);			\
@@ -137,7 +133,7 @@ static inline void name(sigset_t *set)					\
 	    case 1: set->sig[0] = op(set->sig[0]);			\
 		    break;						\
 	    default:							\
-		_NSIG_WORDS_is_unsupported_size();			\
+		BUILD_BUG();						\
 	}								\
 }
 

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Josh Triplett]

On Fri, Sep 19, 2014 at 07:20:30PM +0200, Oleg Nesterov wrote:
> On 09/19, Richard Weinberger wrote:
> >
> > Am 19.09.2014 17:37, schrieb Jeff Kirsher:
> > >
> > > See patch 1 of the series.
> >
> > I was not CC'ed...
> 
> Me too, and thus I don't understand this patch.
> 
> But I have to admit it looks a bit ugly to me anyway.
> Can't we simply kill _NSIG_WORDS_is_unsupported_size ?

This looks quite preferable.  Can you post that with a commit message
and signoff?  Also, the indentation on the second of the three BUILD_BUG
calls has some spaces in it, which it shouldn't.  With those fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 750196f..679c9b4 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
>  
>  static inline int sigisemptyset(sigset_t *set)
>  {
> -	extern void _NSIG_WORDS_is_unsupported_size(void);
>  	switch (_NSIG_WORDS) {
>  	case 4:
>  		return (set->sig[3] | set->sig[2] |
> @@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
>  	case 1:
>  		return set->sig[0] == 0;
>  	default:
> -		_NSIG_WORDS_is_unsupported_size();
> +		BUILD_BUG();
>  		return 0;
>  	}
>  }
> @@ -90,7 +89,6 @@ static inline int sigisemptyset(sigset_t *set)
>  #define _SIG_SET_BINOP(name, op)					\
>  static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>  {									\
> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
>  	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
>  									\
>  	switch (_NSIG_WORDS) {						\
> @@ -107,7 +105,7 @@ static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>  		r->sig[0] = op(a0, b0);					\
>  		break;							\
>  	    default:							\
> -		_NSIG_WORDS_is_unsupported_size();			\
> +	    	BUILD_BUG();						\
>  	}								\
>  }
>  
> @@ -128,8 +126,6 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
>  #define _SIG_SET_OP(name, op)						\
>  static inline void name(sigset_t *set)					\
>  {									\
> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
> -									\
>  	switch (_NSIG_WORDS) {						\
>  	    case 4: set->sig[3] = op(set->sig[3]);			\
>  		    set->sig[2] = op(set->sig[2]);			\
> @@ -137,7 +133,7 @@ static inline void name(sigset_t *set)					\
>  	    case 1: set->sig[0] = op(set->sig[0]);			\
>  		    break;						\
>  	    default:							\
> -		_NSIG_WORDS_is_unsupported_size();			\
> +		BUILD_BUG();						\
>  	}								\
>  }
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]

On Sep 19, 2014, at 2:21 PM, Josh Triplett <josh@joshtriplett.org> wrote:

> On Fri, Sep 19, 2014 at 07:20:30PM +0200, Oleg Nesterov wrote:
>> On 09/19, Richard Weinberger wrote:
>>> 
>>> Am 19.09.2014 17:37, schrieb Jeff Kirsher:
>>>> 
>>>> See patch 1 of the series.
>>> 
>>> I was not CC'ed...
>> 
>> Me too, and thus I don't understand this patch.
>> 
>> But I have to admit it looks a bit ugly to me anyway.
>> Can't we simply kill _NSIG_WORDS_is_unsupported_size ?
> 
> This looks quite preferable.  Can you post that with a commit message
> and signoff?  Also, the indentation on the second of the three BUILD_BUG
> calls has some spaces in it, which it shouldn't.  With those fixed:
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

I haven't tried this patch myself yet, but assuming that it works, it is a far better way to go.

>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>> index 750196f..679c9b4 100644
>> --- a/include/linux/signal.h
>> +++ b/include/linux/signal.h
>> @@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
>> 
>> static inline int sigisemptyset(sigset_t *set)
>> {
>> -	extern void _NSIG_WORDS_is_unsupported_size(void);
>> 	switch (_NSIG_WORDS) {
>> 	case 4:
>> 		return (set->sig[3] | set->sig[2] |
>> @@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
>> 	case 1:
>> 		return set->sig[0] == 0;
>> 	default:
>> -		_NSIG_WORDS_is_unsupported_size();
>> +		BUILD_BUG();
>> 		return 0;
>> 	}
>> }
>> @@ -90,7 +89,6 @@ static inline int sigisemptyset(sigset_t *set)
>> #define _SIG_SET_BINOP(name, op)					\
>> static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>> {									\
>> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
>> 	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
>> 									\
>> 	switch (_NSIG_WORDS) {						\
>> @@ -107,7 +105,7 @@ static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>> 		r->sig[0] = op(a0, b0);					\
>> 		break;							\
>> 	    default:							\
>> -		_NSIG_WORDS_is_unsupported_size();			\
>> +	    	BUILD_BUG();						\
>> 	}								\
>> }
>> 
>> @@ -128,8 +126,6 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
>> #define _SIG_SET_OP(name, op)						\
>> static inline void name(sigset_t *set)					\
>> {									\
>> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
>> -									\
>> 	switch (_NSIG_WORDS) {						\
>> 	    case 4: set->sig[3] = op(set->sig[3]);			\
>> 		    set->sig[2] = op(set->sig[2]);			\
>> @@ -137,7 +133,7 @@ static inline void name(sigset_t *set)					\
>> 	    case 1: set->sig[0] = op(set->sig[0]);			\
>> 		    break;						\
>> 	    default:							\
>> -		_NSIG_WORDS_is_unsupported_size();			\
>> +		BUILD_BUG();						\
>> 	}								\
>> }

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[PATCH 0/1] signal: use BUILD_BUG() instead of _NSIG_WORDS_is_unsupported_size()

[Oleg Nesterov]

On 09/19, Josh Triplett wrote:
>
> On Fri, Sep 19, 2014 at 07:20:30PM +0200, Oleg Nesterov wrote:
>
> > But I have to admit it looks a bit ugly to me anyway.
> > Can't we simply kill _NSIG_WORDS_is_unsupported_size ?
>
> This looks quite preferable.  Can you post that with a commit message
> and signoff?

OK, please see 1/1.

> Also, the indentation on the second of the three BUILD_BUG
> calls has some spaces in it, which it shouldn't.

Yes, thanks, and it makes sense to also fix the indentation.

> With those fixed:
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Thanks. Could you ack it again?

I didn't preserve your ack because it seems that BUILD_BUG() and/or
BUILD_BUG_ON_MSG() deserve some cleanups... For example,

	if (0)
		BUILD_BUG();

can't be compiled if __compiletime_error_fallback() falls back to a
negative-size array (see the changelog).

But I think that the code above must be correct by definition, see
the comment and the original changelog (1399ff86f2a2 "kernel.h: add
BUILD_BUG() macro"). So if this patch breaks the compilation with
some compiler/version we should update include/linux/compiler-xxx.h
or fix BUILD_BUG().

Oleg.

 include/linux/signal.h |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

[PATCH 1/1] signal: use BUILD_BUG() instead of _NSIG_WORDS_is_unsupported_size()

[Oleg Nesterov]

Kill _NSIG_WORDS_is_unsupported_size(), use BUILD_BUG() instead.
This simplifies the code, avoids the nested-externs warnings, and
this way we do not defer the problem to linker.

Also, fix the indentation in _SIG_SET_BINOP() and _SIG_SET_OP().

Note: this patch assumes that the code like "if (0) BUILD_BUG();"
is valid. If not (say __compiletime_error() is not defined and thus
__compiletime_error_fallback() uses a negative array) we should fix
BUILD_BUG() and/or BUILD_BUG_ON_MSG(). This code should be fine by
definition, this is the documented purpose of BUILD_BUG().

Reported-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/signal.h |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 750196f..14acfd5 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
 
 static inline int sigisemptyset(sigset_t *set)
 {
-	extern void _NSIG_WORDS_is_unsupported_size(void);
 	switch (_NSIG_WORDS) {
 	case 4:
 		return (set->sig[3] | set->sig[2] |
@@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
 	case 1:
 		return set->sig[0] == 0;
 	default:
-		_NSIG_WORDS_is_unsupported_size();
+		BUILD_BUG();
 		return 0;
 	}
 }
@@ -90,24 +89,23 @@ static inline int sigisemptyset(sigset_t *set)
 #define _SIG_SET_BINOP(name, op)					\
 static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
 {									\
-	extern void _NSIG_WORDS_is_unsupported_size(void);		\
 	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
 									\
 	switch (_NSIG_WORDS) {						\
-	    case 4:							\
+	case 4:								\
 		a3 = a->sig[3]; a2 = a->sig[2];				\
 		b3 = b->sig[3]; b2 = b->sig[2];				\
 		r->sig[3] = op(a3, b3);					\
 		r->sig[2] = op(a2, b2);					\
-	    case 2:							\
+	case 2:								\
 		a1 = a->sig[1]; b1 = b->sig[1];				\
 		r->sig[1] = op(a1, b1);					\
-	    case 1:							\
+	case 1:								\
 		a0 = a->sig[0]; b0 = b->sig[0];				\
 		r->sig[0] = op(a0, b0);					\
 		break;							\
-	    default:							\
-		_NSIG_WORDS_is_unsupported_size();			\
+	default:							\
+		BUILD_BUG();						\
 	}								\
 }
 
@@ -128,16 +126,14 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
 #define _SIG_SET_OP(name, op)						\
 static inline void name(sigset_t *set)					\
 {									\
-	extern void _NSIG_WORDS_is_unsupported_size(void);		\
-									\
 	switch (_NSIG_WORDS) {						\
-	    case 4: set->sig[3] = op(set->sig[3]);			\
-		    set->sig[2] = op(set->sig[2]);			\
-	    case 2: set->sig[1] = op(set->sig[1]);			\
-	    case 1: set->sig[0] = op(set->sig[0]);			\
+	case 4:	set->sig[3] = op(set->sig[3]);				\
+		set->sig[2] = op(set->sig[2]);				\
+	case 2:	set->sig[1] = op(set->sig[1]);				\
+	case 1:	set->sig[0] = op(set->sig[0]);				\
 		    break;						\
-	    default:							\
-		_NSIG_WORDS_is_unsupported_size();			\
+	default:							\
+		BUILD_BUG();						\
 	}								\
 }
 
-- 
1.5.5.1

Re: [PATCH 1/1] signal: use BUILD_BUG() instead of _NSIG_WORDS_is_unsupported_size()

[Josh Triplett]

On Sun, Sep 21, 2014 at 06:43:06PM +0200, Oleg Nesterov wrote:
> Kill _NSIG_WORDS_is_unsupported_size(), use BUILD_BUG() instead.
> This simplifies the code, avoids the nested-externs warnings, and
> this way we do not defer the problem to linker.
> 
> Also, fix the indentation in _SIG_SET_BINOP() and _SIG_SET_OP().
> 
> Note: this patch assumes that the code like "if (0) BUILD_BUG();"
> is valid. If not (say __compiletime_error() is not defined and thus
> __compiletime_error_fallback() uses a negative array) we should fix
> BUILD_BUG() and/or BUILD_BUG_ON_MSG(). This code should be fine by
> definition, this is the documented purpose of BUILD_BUG().
> 
> Reported-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Thanks for posting this.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  include/linux/signal.h |   28 ++++++++++++----------------
>  1 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 750196f..14acfd5 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
>  
>  static inline int sigisemptyset(sigset_t *set)
>  {
> -	extern void _NSIG_WORDS_is_unsupported_size(void);
>  	switch (_NSIG_WORDS) {
>  	case 4:
>  		return (set->sig[3] | set->sig[2] |
> @@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
>  	case 1:
>  		return set->sig[0] == 0;
>  	default:
> -		_NSIG_WORDS_is_unsupported_size();
> +		BUILD_BUG();
>  		return 0;
>  	}
>  }
> @@ -90,24 +89,23 @@ static inline int sigisemptyset(sigset_t *set)
>  #define _SIG_SET_BINOP(name, op)					\
>  static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>  {									\
> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
>  	unsigned long a0, a1, a2, a3, b0, b1, b2, b3;			\
>  									\
>  	switch (_NSIG_WORDS) {						\
> -	    case 4:							\
> +	case 4:								\
>  		a3 = a->sig[3]; a2 = a->sig[2];				\
>  		b3 = b->sig[3]; b2 = b->sig[2];				\
>  		r->sig[3] = op(a3, b3);					\
>  		r->sig[2] = op(a2, b2);					\
> -	    case 2:							\
> +	case 2:								\
>  		a1 = a->sig[1]; b1 = b->sig[1];				\
>  		r->sig[1] = op(a1, b1);					\
> -	    case 1:							\
> +	case 1:								\
>  		a0 = a->sig[0]; b0 = b->sig[0];				\
>  		r->sig[0] = op(a0, b0);					\
>  		break;							\
> -	    default:							\
> -		_NSIG_WORDS_is_unsupported_size();			\
> +	default:							\
> +		BUILD_BUG();						\
>  	}								\
>  }
>  
> @@ -128,16 +126,14 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
>  #define _SIG_SET_OP(name, op)						\
>  static inline void name(sigset_t *set)					\
>  {									\
> -	extern void _NSIG_WORDS_is_unsupported_size(void);		\
> -									\
>  	switch (_NSIG_WORDS) {						\
> -	    case 4: set->sig[3] = op(set->sig[3]);			\
> -		    set->sig[2] = op(set->sig[2]);			\
> -	    case 2: set->sig[1] = op(set->sig[1]);			\
> -	    case 1: set->sig[0] = op(set->sig[0]);			\
> +	case 4:	set->sig[3] = op(set->sig[3]);				\
> +		set->sig[2] = op(set->sig[2]);				\
> +	case 2:	set->sig[1] = op(set->sig[1]);				\
> +	case 1:	set->sig[0] = op(set->sig[0]);				\
>  		    break;						\
> -	    default:							\
> -		_NSIG_WORDS_is_unsupported_size();			\
> +	default:							\
> +		BUILD_BUG();						\
>  	}								\
>  }
>  
> -- 
> 1.5.5.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] mm: Silence nested-externs warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Use diagnostic control macros to ignore nested-externs warnings
in this case.

CC: Andrew Morton <akpm@linux-foundation.org>
CC: <linux-mm@kvack.org>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/mm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 8981cc8..9bf2c7e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1623,7 +1623,9 @@ static inline void mark_page_reserved(struct page *page)
  */
 static inline unsigned long free_initmem_default(int poison)
 {
+	DIAG_PUSH DIAG_IGNORE(nested-externs)
 	extern char __init_begin[], __init_end[];
+	DIAG_POP
 
 	return free_reserved_area(&__init_begin, &__init_end,
 				  poison, "unused kernel");
-- 
1.9.3

[PATCH 7/7] sched: Silence nested-externs warnings

[Jeff Kirsher]

From: Mark Rustad <mark.d.rustad@intel.com>

Use diagnostic control macros to ignore nested-externs warnings
in this case.

CC: Ingo Molnar <mingo@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 include/linux/sched.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885..ed52c76 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -832,7 +832,9 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
 	return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
+	DIAG_PUSH DIAG_IGNORE(nested-externs)
 	extern int delayacct_on;
+	DIAG_POP
 	return delayacct_on;
 #else
 	return 0;
-- 
1.9.3

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Richard Weinberger]

On Fri, Sep 19, 2014 at 5:29 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> From: Mark Rustad <mark.d.rustad@intel.com>
>
> Use diagnostic control macros to ignore nested-externs warnings
> in this case.
>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  include/linux/sched.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5c2c885..ed52c76 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>  #ifdef CONFIG_SCHEDSTATS
>         return 1;
>  #elif defined(CONFIG_TASK_DELAY_ACCT)
> +       DIAG_PUSH DIAG_IGNORE(nested-externs)
>         extern int delayacct_on;
> +       DIAG_POP

This ridiculous, please try to move this extern into the appropriate header file
instead of surrounding it with these macros.

-- 
Thanks,
//richard

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On Sep 19, 2014, at 12:34 PM, Richard Weinberger <richard.weinberger@gmail.com> wrote:

> On Fri, Sep 19, 2014 at 5:29 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> Use diagnostic control macros to ignore nested-externs warnings
>> in this case.
>> 
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Brian Norris <computersforpeace@gmail.com>
>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> include/linux/sched.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 5c2c885..ed52c76 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>> #ifdef CONFIG_SCHEDSTATS
>>        return 1;
>> #elif defined(CONFIG_TASK_DELAY_ACCT)
>> +       DIAG_PUSH DIAG_IGNORE(nested-externs)
>>        extern int delayacct_on;
>> +       DIAG_POP
> 
> This ridiculous, please try to move this extern into the appropriate header file
> instead of surrounding it with these macros.

Excellent. I'll try adding an include of delayacct.h instead. My patch was based on the assumption that the existing code was wanted in that form for some reason, so the patch served a purpose in recognizing that it isn't. So the macros have served a purpose before even being in place. :-)

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Am 19.09.2014 22:34, schrieb Rustad, Mark D:
> Excellent. I'll try adding an include of delayacct.h instead. My patch was based on the assumption that the existing code was wanted in that form for some reason, so the patch served a purpose in recognizing that it isn't. So the macros have served a purpose before even being in place. :-)

I bet sched.h does not include delayacct.h for a good reason.
sched.h is included in a lot of files, maybe delayacct.h introduces nasty dependency issues.
You'll find out. ;-)

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Sep 19, 2014, at 1:41 PM, Richard Weinberger <richard@nod.at> wrote:

> Am 19.09.2014 22:34, schrieb Rustad, Mark D:
>> Excellent. I'll try adding an include of delayacct.h instead. My patch was based on the assumption that the existing code was wanted in that form for some reason, so the patch served a purpose in recognizing that it isn't. So the macros have served a purpose before even being in place. :-)
> 
> I bet sched.h does not include delayacct.h for a good reason.
> sched.h is included in a lot of files, maybe delayacct.h introduces nasty dependency issues.
> You'll find out. ;-)

Right you are, but when I'm done, there will only be one extern declaration for that global, which is better than more than one. I'll think a little before settling on the best resolution.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[PATCH] sched: Remove nested extern

[Mark D Rustad]

Avoid W=2 nested-externs warning by moving the nested extern to
a normal extern. This eliminates that warning which is generated
for every inclusion of sched.h in a kernel build when W=2 is used.
This also removes a point of maintenance if the definition of
delayacct_on were ever to change.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
 include/linux/delayacct.h |    1 -
 include/linux/sched.h     |    3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
index 6cee17c22313..51229790af00 100644
--- a/include/linux/delayacct.h
+++ b/include/linux/delayacct.h
@@ -30,7 +30,6 @@
 
 #ifdef CONFIG_TASK_DELAY_ACCT
 
-extern int delayacct_on;	/* Delay accounting turned on/off */
 extern struct kmem_cache *delayacct_cache;
 extern void delayacct_init(void);
 extern void __delayacct_tsk_init(struct task_struct *);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885ee52b..1f1dcfdcd92c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -825,6 +825,8 @@ struct task_delay_info {
 	u64 freepages_delay;	/* wait for memory reclaim */
 	u32 freepages_count;	/* total count of memory reclaim */
 };
+
+extern int delayacct_on;	/* Delay accounting turned on/off */
 #endif	/* CONFIG_TASK_DELAY_ACCT */
 
 static inline int sched_info_on(void)
@@ -832,7 +834,6 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
 	return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
-	extern int delayacct_on;
 	return delayacct_on;
 #else
 	return 0;

Re: [PATCH] sched: Remove nested extern

[Josh Triplett]

On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
> Avoid W=2 nested-externs warning by moving the nested extern to
> a normal extern. This eliminates that warning which is generated
> for every inclusion of sched.h in a kernel build when W=2 is used.
> This also removes a point of maintenance if the definition of
> delayacct_on were ever to change.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>

This seems sensible, and makes much more sense than wrapping the extern
in a directive to disable warnings.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  include/linux/delayacct.h |    1 -
>  include/linux/sched.h     |    3 ++-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
> index 6cee17c22313..51229790af00 100644
> --- a/include/linux/delayacct.h
> +++ b/include/linux/delayacct.h
> @@ -30,7 +30,6 @@
>  
>  #ifdef CONFIG_TASK_DELAY_ACCT
>  
> -extern int delayacct_on;	/* Delay accounting turned on/off */
>  extern struct kmem_cache *delayacct_cache;
>  extern void delayacct_init(void);
>  extern void __delayacct_tsk_init(struct task_struct *);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5c2c885ee52b..1f1dcfdcd92c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -825,6 +825,8 @@ struct task_delay_info {
>  	u64 freepages_delay;	/* wait for memory reclaim */
>  	u32 freepages_count;	/* total count of memory reclaim */
>  };
> +
> +extern int delayacct_on;	/* Delay accounting turned on/off */
>  #endif	/* CONFIG_TASK_DELAY_ACCT */
>  
>  static inline int sched_info_on(void)
> @@ -832,7 +834,6 @@ static inline int sched_info_on(void)
>  #ifdef CONFIG_SCHEDSTATS
>  	return 1;
>  #elif defined(CONFIG_TASK_DELAY_ACCT)
> -	extern int delayacct_on;
>  	return delayacct_on;
>  #else
>  	return 0;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched: Remove nested extern

[Peter Zijlstra]

On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
> Avoid W=2 nested-externs warning by moving the nested extern to
> a normal extern. This eliminates that warning which is generated
> for every inclusion of sched.h in a kernel build when W=2 is used.
> This also removes a point of maintenance if the definition of
> delayacct_on were ever to change.
> 

Yeah, so why is that warning worth using? Just disable the warn if
you're bothered by it.

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Sep 22, 2014, at 12:01 PM, Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
>> Avoid W=2 nested-externs warning by moving the nested extern to
>> a normal extern. This eliminates that warning which is generated
>> for every inclusion of sched.h in a kernel build when W=2 is used.
>> This also removes a point of maintenance if the definition of
>> delayacct_on were ever to change.
> 
> Yeah, so why is that warning worth using? Just disable the warn if
> you're bothered by it.

I assume that nested-externs is included in W=2 because many uses of
them, especially with function prototypes, creates a risk of inconsistent
declarations. The kernel has a fair number of them that seem to have
been added to disentangle include files. If they are judged to be
worthwhile, then it would be good to silence the warnings so that
attention can be given to other instances of the warnings. If those
nested externs are not worth the risk, well that is a different conversation.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] sched: Remove nested extern

[Peter Zijlstra]

On Mon, Sep 22, 2014 at 07:32:04PM +0000, Rustad, Mark D wrote:
> On Sep 22, 2014, at 12:01 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
> >> Avoid W=2 nested-externs warning by moving the nested extern to
> >> a normal extern. This eliminates that warning which is generated
> >> for every inclusion of sched.h in a kernel build when W=2 is used.
> >> This also removes a point of maintenance if the definition of
> >> delayacct_on were ever to change.
> > 
> > Yeah, so why is that warning worth using? Just disable the warn if
> > you're bothered by it.
> 
> I assume that nested-externs is included in W=2 because many uses of
> them, especially with function prototypes, creates a risk of inconsistent
> declarations. The kernel has a fair number of them that seem to have
> been added to disentangle include files. If they are judged to be
> worthwhile, then it would be good to silence the warnings so that
> attention can be given to other instances of the warnings. If those
> nested externs are not worth the risk, well that is a different conversation.

So would using something like LTO not be way better at actually finding
the problems, seeing how it can actually see the inconsistency in
declarations.

It seems to me that banning the use of nested extern is misguided as
they are a useful tool; they provide some means of keeping symbols from
being globally visible even though they actually are. Its a really poor
man's 'private', but its the best C provides.

Also, why do you care about W=2 nonsense anyhow? They're default
disabled for a reason.

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

On Sep 22, 2014, at 1:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 22, 2014 at 07:32:04PM +0000, Rustad, Mark D wrote:
>> I assume that nested-externs is included in W=2 because many uses of
>> them, especially with function prototypes, creates a risk of inconsistent
>> declarations. The kernel has a fair number of them that seem to have
>> been added to disentangle include files. If they are judged to be
>> worthwhile, then it would be good to silence the warnings so that
>> attention can be given to other instances of the warnings. If those
>> nested externs are not worth the risk, well that is a different conversation.
> 
> So would using something like LTO not be way better at actually finding
> the problems, seeing how it can actually see the inconsistency in
> declarations.
> 
> It seems to me that banning the use of nested extern is misguided as
> they are a useful tool; they provide some means of keeping symbols from
> being globally visible even though they actually are. Its a really poor
> man's 'private', but its the best C provides.

By using the macros the use of nested externs can be discouraged without
being banned. Presence of a DIAG_IGNORE macro should mean that the exception
has been noted and accepted. And the macro remains as an indicator of that.
Or to be taken as a warning label: readers choice perhaps.

> Also, why do you care about W=2 nonsense anyhow? They're default
> disabled for a reason.

Because I have found that enabling many warnings helps identify problems
in code and it has been my standard practice since about 1999 to do so.
The compiler warnings are really just another form of static analysis,
and I use it routinely on every compile. Here is how routinely: I have
W=1 in my environment, W=12 is just too painful. I would change that
default to W=12 if it wasn't insane to do so.

In my own code, I use way more warnings than even W=12 enables and in
that code I just resolve them. I had never resorted to using the attributes
in my own code, but they are an available tool.

I just finally thought it might be time to consider opening up a new
method to manage them for the kernel.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] sched: Remove nested extern

[Peter Zijlstra]

On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
> Because I have found that enabling many warnings helps identify problems
> in code and it has been my standard practice since about 1999 to do so.
> The compiler warnings are really just another form of static analysis,
> and I use it routinely on every compile. Here is how routinely: I have
> W=1 in my environment, W=12 is just too painful. I would change that
> default to W=12 if it wasn't insane to do so.

Many warnings are just plain insane and stupid. They're not helping
anybody. There's a very good reason many are disabled. I'm sure you can
find some entertaining discussions on the topic if you search the LKML
archives.

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On Sep 22, 2014, at 2:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
>> Because I have found that enabling many warnings helps identify problems
>> in code and it has been my standard practice since about 1999 to do so.
>> The compiler warnings are really just another form of static analysis,
>> and I use it routinely on every compile. Here is how routinely: I have
>> W=1 in my environment, W=12 is just too painful. I would change that
>> default to W=12 if it wasn't insane to do so.
> 
> Many warnings are just plain insane and stupid. They're not helping
> anybody. There's a very good reason many are disabled. I'm sure you can
> find some entertaining discussions on the topic if you search the LKML
> archives.

That is what I used to think. -Wshadow for example. What's the problem? It
is perfectly valid C. Well, I recently sent a patch that changed some
function parameters that used the name jiffies, which of course shadowed
the well-known global. If a macro were ever called in those functions
whose expansion ever tried to access jiffies, well it wouldn't do what
was expected. Not a bug now, but a trap for the future. I only found that
because I either resolved or silenced enough warnings to see something interesting.

Over the years I think I have resolved real bugs revealed by probably nearly
every one of the additional warning messages. They do have value, but that
value is typically lost if the norm is a flood of messages. Apple would not
have had their SSL problem if they had had -Wunreachable-code enabled and
noticed the message.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] sched: Remove nested extern

[Ingo Molnar]

* Rustad, Mark D <mark.d.rustad@intel.com> wrote:

> On Sep 22, 2014, at 2:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
> >> Because I have found that enabling many warnings helps identify problems
> >> in code and it has been my standard practice since about 1999 to do so.
> >> The compiler warnings are really just another form of static analysis,
> >> and I use it routinely on every compile. Here is how routinely: I have
> >> W=1 in my environment, W=12 is just too painful. I would change that
> >> default to W=12 if it wasn't insane to do so.
> > 
> > Many warnings are just plain insane and stupid. They're not 
> > helping anybody. There's a very good reason many are 
> > disabled. I'm sure you can find some entertaining discussions 
> > on the topic if you search the LKML archives.
> 
> That is what I used to think. -Wshadow for example. What's the 
> problem? [...]

Then please add it to the default build. There are some warnings 
that used to be crap but have been improved over the year - 
enable them one by one, with good case by case justification and 
analysis. Just going after all W=2 warnings is insane.

Thanks,

	Ingo

Re: [PATCH] sched: Remove nested extern

[Peter Zijlstra]

On Wed, Sep 24, 2014 at 09:41:44AM +0200, Ingo Molnar wrote:
> 
> * Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> 
> > On Sep 22, 2014, at 2:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
> > >> Because I have found that enabling many warnings helps identify problems
> > >> in code and it has been my standard practice since about 1999 to do so.
> > >> The compiler warnings are really just another form of static analysis,
> > >> and I use it routinely on every compile. Here is how routinely: I have
> > >> W=1 in my environment, W=12 is just too painful. I would change that
> > >> default to W=12 if it wasn't insane to do so.
> > > 
> > > Many warnings are just plain insane and stupid. They're not 
> > > helping anybody. There's a very good reason many are 
> > > disabled. I'm sure you can find some entertaining discussions 
> > > on the topic if you search the LKML archives.
> > 
> > That is what I used to think. -Wshadow for example. What's the 
> > problem? [...]
> 
> Then please add it to the default build. There are some warnings 
> that used to be crap but have been improved over the year - 
> enable them one by one, with good case by case justification and 
> analysis. Just going after all W=2 warnings is insane.

So I don't like the nested extern one because it either means not using
that language feature at all or pooping all over your code with that
DECL crap. That just a loose-loose proposition.

Re: [PATCH] sched: Remove nested extern

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Sep 24, 2014 at 09:41:44AM +0200, Ingo Molnar wrote:
> > 
> > * Rustad, Mark D <mark.d.rustad@intel.com> wrote:
> > 
> > > On Sep 22, 2014, at 2:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
> > > >> Because I have found that enabling many warnings helps identify problems
> > > >> in code and it has been my standard practice since about 1999 to do so.
> > > >> The compiler warnings are really just another form of static analysis,
> > > >> and I use it routinely on every compile. Here is how routinely: I have
> > > >> W=1 in my environment, W=12 is just too painful. I would change that
> > > >> default to W=12 if it wasn't insane to do so.
> > > > 
> > > > Many warnings are just plain insane and stupid. They're not 
> > > > helping anybody. There's a very good reason many are 
> > > > disabled. I'm sure you can find some entertaining discussions 
> > > > on the topic if you search the LKML archives.
> > > 
> > > That is what I used to think. -Wshadow for example. What's the 
> > > problem? [...]
> > 
> > Then please add it to the default build. There are some warnings 
> > that used to be crap but have been improved over the year - 
> > enable them one by one, with good case by case justification and 
> > analysis. Just going after all W=2 warnings is insane.

So by 'try to add it' I mean -Wshadow, which is a useful warning 
that I use in user-space projects as well: it does catch 
bogosities and isn't annoyingly inaccurate.

> So I don't like the nested extern one because it either means 
> not using that language feature at all or pooping all over your 
> code with that DECL crap. That just a loose-loose proposition.

Agreed.

Thanks,

	Ingo

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Peter Zijlstra]

On Fri, Sep 19, 2014 at 08:29:40AM -0700, Jeff Kirsher wrote:
> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Use diagnostic control macros to ignore nested-externs warnings
> in this case.
> 
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  include/linux/sched.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5c2c885..ed52c76 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>  #ifdef CONFIG_SCHEDSTATS
>  	return 1;
>  #elif defined(CONFIG_TASK_DELAY_ACCT)
> +	DIAG_PUSH DIAG_IGNORE(nested-externs)
>  	extern int delayacct_on;
> +	DIAG_POP
>  	return delayacct_on;

Who has this nested extern warn on in anycase? I've never seen it
generate a warning. Also WTF is DIAG_PUSH/POP, its not a GCC thing
afaict.

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On Sep 19, 2014, at 3:54 PM, Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Sep 19, 2014 at 08:29:40AM -0700, Jeff Kirsher wrote:
>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> Use diagnostic control macros to ignore nested-externs warnings
>> in this case.
>> 
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Brian Norris <computersforpeace@gmail.com>
>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> include/linux/sched.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 5c2c885..ed52c76 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>> #ifdef CONFIG_SCHEDSTATS
>> 	return 1;
>> #elif defined(CONFIG_TASK_DELAY_ACCT)
>> +	DIAG_PUSH DIAG_IGNORE(nested-externs)
>> 	extern int delayacct_on;
>> +	DIAG_POP
>> 	return delayacct_on;
> 
> Who has this nested extern warn on in anycase?

They appear in W=2 builds, so you do have to ask for them.

> I've never seen it
> generate a warning. Also WTF is DIAG_PUSH/POP, its not a GCC thing
> afaict.

The first patch in the series adds macros to use the gcc/clang pragmas to control these things. Both compilers have the capability. The macros generate nothing for compilers that lack it.

In any case, this particular one will be resolved instead of silenced.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Fri, Sep 19, 2014 at 08:29:33AM -0700, Jeff Kirsher wrote:
> The following patches silence over 100,000 warnings in a W=2
> kernel build. This series does most of it by using the compilers
> diagnostic controls. The first patch in the series adds macros to
> invoke the pragmas for those controls. Macros are provided for GCC
> and clang. Although they are highly compatible in this area, macros
> are provided for compiler-specific controls, and there is one
> example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).
> 
> Some missing-field-initializers warnings were resolved using
> the diagnostic control macros simply because so many lines
> would have had to have been changed. At this stage Mark thought 
> about avoiding possible merge issues. If the maintainer would 
> rather resolve them by using designated initialization, just 
> say so.
> 
> The combined effect of this patch series and his other patches
> that did not use these diagnostic control macros was to reduce 
> the number of W=2 warnings from 127,164 to 1,345!

Sorry but I don't see the point of actively adding macros to the code
just so that gcc is happy. There's a reason why a bunch of warnings are
disabled in the normal build and only enabled with the W= switch.

The W= things are supposed to be used when developing code and have the
compiler tell you about *possible* issues. That doesn't mean though that
we have to actively "fix" otherwise perfectly fine code.

Having the need to actively go in and add code so that gcc doesn't issue
obscure warnings is going too far, IMO.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Sep 22, 2014, at 8:33 AM, Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Sep 19, 2014 at 08:29:33AM -0700, Jeff Kirsher wrote:
>> The following patches silence over 100,000 warnings in a W=2
>> kernel build. This series does most of it by using the compilers
>> diagnostic controls. The first patch in the series adds macros to
>> invoke the pragmas for those controls. Macros are provided for GCC
>> and clang. Although they are highly compatible in this area, macros
>> are provided for compiler-specific controls, and there is one
>> example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).
>> 
>> Some missing-field-initializers warnings were resolved using
>> the diagnostic control macros simply because so many lines
>> would have had to have been changed. At this stage Mark thought 
>> about avoiding possible merge issues. If the maintainer would 
>> rather resolve them by using designated initialization, just 
>> say so.
>> 
>> The combined effect of this patch series and his other patches
>> that did not use these diagnostic control macros was to reduce 
>> the number of W=2 warnings from 127,164 to 1,345!
> 
> Sorry but I don't see the point of actively adding macros to the code
> just so that gcc is happy. There's a reason why a bunch of warnings are
> disabled in the normal build and only enabled with the W= switch.
> 
> The W= things are supposed to be used when developing code and have the
> compiler tell you about *possible* issues. That doesn't mean though that
> we have to actively "fix" otherwise perfectly fine code.

The problem is that the kernel include files throw so many warnings that it really discourages anyone from ever going through them, even for a single driver. The warnings are far more valuable and usable when known acceptable usages are silenced.

> Having the need to actively go in and add code so that gcc doesn't issue
> obscure warnings is going too far, IMO.

Well, the whole series of patches that I made definitely went too far - only the first 5 out of about 30 have been posted, but if we can make some progress on generating fewer warnings out of the include files, I think it would be helpful. Already the patches that use them have triggered some activity that has resulted in resolving warnings without use of the macros, and I see that as much better than simply using the macros.

The macros can serve a useful purpose, but they should not be widely used. When to use them is definitely a judgement call. If the macros are accepted, it may be worth adding a checkpatch.pl warning for adding a DIAG_*IGNORE macro.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 05:06:27PM +0000, Rustad, Mark D wrote:
> The problem is that the kernel include files throw so many warnings
> that it really discourages anyone from ever going through them, even
> for a single driver. The warnings are far more valuable and usable
> when known acceptable usages are silenced.

You can always do one file only, for example:

make W=2 arch/x86/kernel/msr.o > w.log 2>&1

> Well, the whole series of patches that I made definitely went too far
> - only the first 5 out of about 30 have been posted, but if we can
> make some progress on generating fewer warnings out of the include
> files, I think it would be helpful.

Helpful for what? Those are W=2 warnings which are disabled in the
default build.

> Already the patches that use them have triggered some activity that
> has resulted in resolving warnings without use of the macros, and I
> see that as much better than simply using the macros.
>
> The macros can serve a useful purpose, but they should not be widely
> used. When to use them is definitely a judgement call. If the macros
> are accepted, it may be worth adding a checkpatch.pl warning for
> adding a DIAG_*IGNORE macro.

Right, so add the macros and tell people *not* to use them. That won't
fly.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Sep 22, 2014, at 11:40 AM, Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Sep 22, 2014 at 05:06:27PM +0000, Rustad, Mark D wrote:
>> Well, the whole series of patches that I made definitely went too far
>> - only the first 5 out of about 30 have been posted, but if we can
>> make some progress on generating fewer warnings out of the include
>> files, I think it would be helpful.
> 
> Helpful for what? Those are W=2 warnings which are disabled in the
> default build.

It is helpful for using the warnings to look for problems or even just risks.

>> The macros can serve a useful purpose, but they should not be widely
>> used. When to use them is definitely a judgement call. If the macros
>> are accepted, it may be worth adding a checkpatch.pl warning for
>> adding a DIAG_*IGNORE macro.
> 
> Right, so add the macros and tell people *not* to use them. That won't
> fly.

Right now the number of warnings generated when using W=2 simply tells people to never use W=2. That severely limits the value of a useful tool. A checkpatch warning doesn't mean to never do that, just that it needs a critical look and justification. That is certainly true of every patch I made that uses those macros.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 06:59:23PM +0000, Rustad, Mark D wrote:
> It is helpful for using the warnings to look for problems or even just risks.

That's what W= builds are for.

> Right now the number of warnings generated when using W=2 simply tells
> people to never use W=2.

I showed you how to use W=2 and 3 for that matter - pipe the output into
a file and grep away.

> That severely limits the value of a useful tool. A checkpatch warning
> doesn't mean to never do that, just that it needs a critical look and
> justification. That is certainly true of every patch I made that uses
> those macros.

Sorry, if you need to shut up the compiler by adding code with the sole
purpose to not issue a warning for otherwise perfectly fine code, then
something's wrong with the whole endeavor in the first place.

There's a reason W= warnings are disabled in the default build.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On Mon, 2014-09-22 at 21:21 +0200, Borislav Petkov wrote:
> On Mon, Sep 22, 2014 at 06:59:23PM +0000, Rustad, Mark D wrote:
> > It is helpful for using the warnings to look for problems or even
> just risks.
> 
> That's what W= builds are for.
> 
> > Right now the number of warnings generated when using W=2 simply
> tells
> > people to never use W=2.
> 
> I showed you how to use W=2 and 3 for that matter - pipe the output
> into
> a file and grep away.
> 

Not sure you showed us, since that is how everyone has had to do to
actual find W= builds useful.  Just because that is how we HAVE to do it
now, does not make it the best way.  Here is a thought, we don't we fix
the potential issues, so that W= builds do not generate over 100,000
errors/warnings.

Mark did this approach because it would either spur the conversation
that this is a good idea OR let's fix the root problem.  Instead it
sounds like your response is "life sucks, get over it" and put your head
back in the sand to ignore the problem.

> > That severely limits the value of a useful tool. A checkpatch
> warning
> > doesn't mean to never do that, just that it needs a critical look
> and
> > justification. That is certainly true of every patch I made that
> uses
> > those macros.
> 
> Sorry, if you need to shut up the compiler by adding code with the
> sole
> purpose to not issue a warning for otherwise perfectly fine code, then
> something's wrong with the whole endeavor in the first place.
> 
> There's a reason W= warnings are disabled in the default build.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 12:44:17PM -0700, Jeff Kirsher wrote:
> Not sure you showed us, since that is how everyone has had to do to
> actual find W= builds useful.  Just because that is how we HAVE to do it
> now, does not make it the best way.  Here is a thought, we don't we fix
> the potential issues, so that W= builds do not generate over 100,000
> errors/warnings.
> 
> Mark did this approach because it would either spur the conversation
> that this is a good idea OR let's fix the root problem.  Instead it
> sounds like your response is "life sucks, get over it" and put your head
> back in the sand to ignore the problem.

Hey hey, relax a little - no need to get offensive all of a sudden.

Having to grep through a log file full of gcc warnings is a much better
thing to do IMNSVHO than adding code to the kernel just to shut up the
compiler. We had huge discussions even about something as silly as
uninitialized_var() which was supposed to shut up the compiler but ended
up actively causing bugs.

Now, you're arguing for adding obscure macros to shut up warnings which
are disabled in the first place because you don't want to grep through
log files.

If you can't see the absurdity of your proposal then maybe we should
agree to disagree and declare this back-and-forth for having run its
course. In any case, you get my NAK for it.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

On Mon, 2014-09-22 at 21:57 +0200, Borislav Petkov wrote:
> On Mon, Sep 22, 2014 at 12:44:17PM -0700, Jeff Kirsher wrote:
> > Not sure you showed us, since that is how everyone has had to do to
> > actual find W= builds useful.  Just because that is how we HAVE to
> do it
> > now, does not make it the best way.  Here is a thought, we don't we
> fix
> > the potential issues, so that W= builds do not generate over 100,000
> > errors/warnings.
> > 
> > Mark did this approach because it would either spur the conversation
> > that this is a good idea OR let's fix the root problem.  Instead it
> > sounds like your response is "life sucks, get over it" and put your
> head
> > back in the sand to ignore the problem.
> 
> Hey hey, relax a little - no need to get offensive all of a sudden.

Sorry I am very frustrated at your response.
> 
> Having to grep through a log file full of gcc warnings is a much
> better
> thing to do IMNSVHO than adding code to the kernel just to shut up the
> compiler. We had huge discussions even about something as silly as
> uninitialized_var() which was supposed to shut up the compiler but
> ended
> up actively causing bugs.

I am not saying that the proposed added MACRO is the best solution to
this issue.  Several other maintainers have actually responded in a
similar manner to the macros being added and came back that the better
solution would be to fix the code so that the warnings do not occur in
the first place.

So I guess I was hoping for more of the response, that "let's fix this
the code so that the warnings do not appear in the first place".

I agree with you completely that I do not like the idea of the MACROS
being added to silence these warnings.  I just disagree that not doing
anything to fix the warnings is far worse.

Why grep through 100,000 warnings, when we should be fixing the code to
prevent 100,000 warnings.  Not saying that the MACRO is the best
solution, it is just a solution, in hopes that it spurs discussions like
this on how to properly fix the warnings.  Not a discussion on how to
grep through the warnings and do nothing.

> 
> Now, you're arguing for adding obscure macros to shut up warnings
> which
> are disabled in the first place because you don't want to grep through
> log files.
> 
> If you can't see the absurdity of your proposal then maybe we should
> agree to disagree and declare this back-and-forth for having run its
> course. In any case, you get my NAK for it.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 01:09:33PM -0700, Jeff Kirsher wrote:
> Sorry I am very frustrated at your response.

You shouldn't be. Judging by your reply below it seems we do actually
agree... mostly :-)

> I am not saying that the proposed added MACRO is the best solution to
> this issue.  Several other maintainers have actually responded in a
> similar manner to the macros being added and came back that the better
> solution would be to fix the code so that the warnings do not occur in
> the first place.

Right, this would be optimal.

> So I guess I was hoping for more of the response, that "let's fix this
> the code so that the warnings do not appear in the first place".
> 
> I agree with you completely that I do not like the idea of the MACROS
> being added to silence these warnings.  I just disagree that not doing
> anything to fix the warnings is far worse.

Ok, good, so we're on the same page here.

> Why grep through 100,000 warnings, when we should be fixing the code to
> prevent 100,000 warnings.  Not saying that the MACRO is the best
> solution, it is just a solution, in hopes that it spurs discussions like
> this on how to properly fix the warnings.  Not a discussion on how to
> grep through the warnings and do nothing.

There's only one thing I don't understand: why is so bad to grep through
the warnings? I mean, sure, fixing them *without* jumping through hoops
to do so is the optimal thing. But what's wrong with grepping through
them?

Btw, out of curiosity, what is your use case for staring at those W=2
warnings?

In thinking about it, what we could also do is simply move the noisiest
ones to W=3 or so, or even add another W= level. It'll be interesting to
hear your use case though. AFAICT, this is the first time I hear of a
more, let's say, serious use case of W= since we added the W= things a
couple of years ago. :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 3310 bytes --]

On Mon, 2014-09-22 at 22:33 +0200, Borislav Petkov wrote:
> On Mon, Sep 22, 2014 at 01:09:33PM -0700, Jeff Kirsher wrote:
> > Sorry I am very frustrated at your response.
> 
> You shouldn't be. Judging by your reply below it seems we do actually
> agree... mostly :-)
> 
> > I am not saying that the proposed added MACRO is the best solution to
> > this issue.  Several other maintainers have actually responded in a
> > similar manner to the macros being added and came back that the better
> > solution would be to fix the code so that the warnings do not occur in
> > the first place.
> 
> Right, this would be optimal.
> 
> > So I guess I was hoping for more of the response, that "let's fix this
> > the code so that the warnings do not appear in the first place".
> > 
> > I agree with you completely that I do not like the idea of the MACROS
> > being added to silence these warnings.  I just disagree that not doing
> > anything to fix the warnings is far worse.
> 
> Ok, good, so we're on the same page here.
> 
> > Why grep through 100,000 warnings, when we should be fixing the code to
> > prevent 100,000 warnings.  Not saying that the MACRO is the best
> > solution, it is just a solution, in hopes that it spurs discussions like
> > this on how to properly fix the warnings.  Not a discussion on how to
> > grep through the warnings and do nothing.
> 
> There's only one thing I don't understand: why is so bad to grep through
> the warnings? I mean, sure, fixing them *without* jumping through hoops
> to do so is the optimal thing. But what's wrong with grepping through
> them?

Nothing is wrong with grepping for an error, especially when you know
the error your grepping for.  But then again, why grep when it can be
fixed to begin with?  The fact that there are over 100,000
warnings/errors to begin with is somewhat disconcerting.  It makes me
wonder whether it was due to coding laziness.

> 
> Btw, out of curiosity, what is your use case for staring at those W=2
> warnings?

To make a better (more solid) network driver?  Mark has found it useful
to do the W= builds.  For me personally, I do not bother because there
are over 100,000 warnings and it takes forever to get through a build
and then grep for our drivers to see if they are generating any
warnings.

> 
> In thinking about it, what we could also do is simply move the noisiest
> ones to W=3 or so, or even add another W= level. It'll be interesting to
> hear your use case though. AFAICT, this is the first time I hear of a
> more, let's say, serious use case of W= since we added the W= things a
> couple of years ago. :-)
> 

I could see this useful if there is no way to fix the issue that really
is not an issue and the compiler just does not know any better, but this
concerns me that we would get into a bad habit.  "Oh I really do not
want to fix this, so I will just make it so that people we not have to
see this warning/error"  Again, sounds lazy to me, of course I am just
speaking in a generalization. I am sure that some of the warnings will
fall into the category of, it needs to be silenced and not fixed because
the fix is far more troublesome.  I just cannot believe that most or all
the warnings would be that way.

> Thanks.
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 02:21:52PM -0700, Jeff Kirsher wrote:
> Nothing is wrong with grepping for an error, especially when you know
> the error your grepping for.  But then again, why grep when it can be
> fixed to begin with?

Oh sure, but at what cost?

But we're on the same page here - if it can be fixed cleanly, it should
be fixed. I simply don't think that adding code just to shut up the
compiler is a good idea.

> The fact that there are over 100,000 warnings/errors to begin with
> is somewhat disconcerting. It makes me wonder whether it was due to
> coding laziness.

Yeah, the reasons should be multiple and scattered over the years, I
think. Maybe the warnings were added to gcc later than the code in the
kernel, or simply some of them are just silly:

./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
 static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
                                                ^
In file included from ./arch/x86/include/asm/smp.h:12:0,
                 from include/linux/smp.h:59,
                 from include/linux/topology.h:33,
                 from include/linux/gfp.h:8,
                 from include/linux/kmod.h:22,
                 from include/linux/module.h:13,
                 from drivers/edac/amd64_edac.h:65,
                 from drivers/edac/amd64_edac.c:1:
./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here [-Wshadow]
 extern struct apic *apic;
                     ^

So gcc complains that an unsigned int shadows a struct apic pointer.
Yeah, that might be a problem in a big function (and it has been a
problem AFAIR) but here:

static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
{
        x86_io_apic_ops.modify(apic, reg, value);
}

So yeah, it is debatable. Should it be fixed? Sure, the fix is very easy.

But then fixing those could become a fighting windmills type of thing
as people don't see those warnings in normal builds. And new code will
get added which causes them again. And then the discussion would start
whether we should see those warnings...

I think you get the picture.

> To make a better (more solid) network driver?  Mark has found it useful
> to do the W= builds.  For me personally, I do not bother because there
> are over 100,000 warnings and it takes forever to get through a build
> and then grep for our drivers to see if they are generating any
> warnings.

Right.

> I could see this useful if there is no way to fix the issue that really
> is not an issue and the compiler just does not know any better, but this
> concerns me that we would get into a bad habit.  "Oh I really do not
> want to fix this, so I will just make it so that people we not have to
> see this warning/error"  Again, sounds lazy to me, of course I am just
> speaking in a generalization.

It doesn't have to be lazy - people simply don't see those warnings and
making them visible might turn out to be a net loss in the end. I think
it depends on the warning...

> I am sure that some of the warnings will fall into the category of,
> it needs to be silenced and not fixed because the fix is far more
> troublesome. I just cannot believe that most or all the warnings would
> be that way.

As I said above, I think it is a question of whether those should be
visible/enabled (which would make people fix them, more or less) or not.
And I think if someone comes up with a strong case for why a warning
should be enabled in the default build, then people wouldn't mind.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Josh Triplett]

On Tue, Sep 23, 2014 at 10:01:20AM +0200, Borislav Petkov wrote:
> ./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
> ./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
>  static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
>                                                 ^
> In file included from ./arch/x86/include/asm/smp.h:12:0,
>                  from include/linux/smp.h:59,
>                  from include/linux/topology.h:33,
>                  from include/linux/gfp.h:8,
>                  from include/linux/kmod.h:22,
>                  from include/linux/module.h:13,
>                  from drivers/edac/amd64_edac.h:65,
>                  from drivers/edac/amd64_edac.c:1:
> ./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here [-Wshadow]
>  extern struct apic *apic;
>                      ^
> 
> So gcc complains that an unsigned int shadows a struct apic pointer.

Here, I think the right fix involves picking a more descriptive name
than "apic" for the global varible.

- Josh Triplett

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Tue, Sep 23, 2014 at 07:49:36AM -0700, Josh Triplett wrote:
> Here, I think the right fix involves picking a more descriptive name
> than "apic" for the global varible.

Sure, this one is simple enough and could be fixed. Something for a
newbie to tackle I'd say.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Sep 23, 2014, at 7:49 AM, Josh Triplett <josh@joshtriplett.org> wrote:

> On Tue, Sep 23, 2014 at 10:01:20AM +0200, Borislav Petkov wrote:
>> ./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
>> ./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow]
>> static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned int value)
>>                                                ^
>> In file included from ./arch/x86/include/asm/smp.h:12:0,
>>                 from include/linux/smp.h:59,
>>                 from include/linux/topology.h:33,
>>                 from include/linux/gfp.h:8,
>>                 from include/linux/kmod.h:22,
>>                 from include/linux/module.h:13,
>>                 from drivers/edac/amd64_edac.h:65,
>>                 from drivers/edac/amd64_edac.c:1:
>> ./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here [-Wshadow]
>> extern struct apic *apic;
>>                     ^
>> 
>> So gcc complains that an unsigned int shadows a struct apic pointer.
> 
> Here, I think the right fix involves picking a more descriptive name
> than "apic" for the global varible.

I agree, but I don't know enough about the area to necessarily know what it should be called instead. I do have a patch that changes the local variables instead, but even as I made it, I didn't really think it was right. But it silenced a ton of warnings and let me see other things.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Geert Uytterhoeven]

On Mon, Sep 22, 2014 at 11:21 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
>> > Why grep through 100,000 warnings, when we should be fixing the code to
>> > prevent 100,000 warnings.  Not saying that the MACRO is the best
>> > solution, it is just a solution, in hopes that it spurs discussions like
>> > this on how to properly fix the warnings.  Not a discussion on how to
>> > grep through the warnings and do nothing.
>>
>> There's only one thing I don't understand: why is so bad to grep through
>> the warnings? I mean, sure, fixing them *without* jumping through hoops
>> to do so is the optimal thing. But what's wrong with grepping through
>> them?
>
> Nothing is wrong with grepping for an error, especially when you know
> the error your grepping for.  But then again, why grep when it can be
> fixed to begin with?  The fact that there are over 100,000
> warnings/errors to begin with is somewhat disconcerting.  It makes me
> wonder whether it was due to coding laziness.

Instead of grepping, you can feed the build log to linux-log-summary.
Or when changing a driver, feed the before and after build logs to
linux-log-diff. That way you won't miss the single new warning you've
just introduced.

https://github.com/geertu/linux-scripts

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Thu, Sep 25, 2014 at 09:45:29AM +0200, Geert Uytterhoeven wrote:
> Instead of grepping, you can feed the build log to linux-log-summary.
> Or when changing a driver, feed the before and after build logs to
> linux-log-diff. That way you won't miss the single new warning you've
> just introduced.
> 
> https://github.com/geertu/linux-scripts

Yep, nice stuff: linux-log-summary does nicely summarize it all like
this here:

...
include/linux/kernel.h:120:19: warning: cast from function call of type ‘void *’ to non-matching type ‘long unsigned int’ [-Wbad-function-cast]: 40 warnings in 1 logs
include/linux/kernel.h:67:27: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]: 2 warnings in 1 logs
include/linux/kernel.h:67:33: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]: 3 warnings in 1 logs
...

which I ran on a file built with W=123.

So I'd say everything needed is there to massage those build logs.

Thanks.

-- 
Regards/Gruss,
    Boris.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On Sep 25, 2014, at 12:45 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Instead of grepping, you can feed the build log to linux-log-summary.
> Or when changing a driver, feed the before and after build logs to
> linux-log-diff. That way you won't miss the single new warning you've
> just introduced.
> 
> https://github.com/geertu/linux-scripts

Thanks for making me aware of these tools. linux-log-summary does reduce the totally insane 125,000 warnings to the merely unreasonable 10,000 or so. I'm not being sarcastic, they *are* very nice tools that I will use. Thank you for pointing me to them.

I still observe that thousands of those unique warnings are emitted by the syscall table and its override initializations. I still think they should be silenced because they serve no purpose in that particular case. That is, the warning is complaining about what it is designed to complain about, but in this particular instance, the code is written as it needs to be, so why not acknowledge and capture that exception in the source?

A couple thousand more come from every use of compiletime_assert, for which I do not have a solution yet. Even my macros don't help with that one.

Most of the others come from null-entry table initializations, i.e. { 0 }, which give missing field initializer warnings. I'd like to define a macro something like:

#define ZERO_ENTRY DIAG_PUSH DIAG_IGNORE(missing-field-initializers) \
	{ 0 } DIAG_POP

Then simply using ZERO_ENTRY, a zero entry could be provided without a complaint from the compiler. But of course that can't be done if the DIAG_* macros aren't there. It also would keep the DIAG_* macros out of the .c files and just in the header where ZERO_ENTRY is defined.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[josh]

On Fri, Sep 26, 2014 at 07:37:19PM +0000, Rustad, Mark D wrote:
> Most of the others come from null-entry table initializations, i.e. {
> 0 }, which give missing field initializer warnings.

I'd suggest that such initializers should just be {}, not { 0 }, and we
should teach compilers to specifically *not* complain about empty
initializers even when otherwise complaining about missing fields.
Initializing a structure to 0 is completely sensible.

- Josh Triplett

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Sep 26, 2014, at 12:58 PM, <josh@joshtriplett.org> <josh@joshtriplett.org> wrote:

> On Fri, Sep 26, 2014 at 07:37:19PM +0000, Rustad, Mark D wrote:
>> Most of the others come from null-entry table initializations, i.e. {
>> 0 }, which give missing field initializer warnings.
> 
> I'd suggest that such initializers should just be {}, not { 0 }, and we
> should teach compilers to specifically *not* complain about empty
> initializers even when otherwise complaining about missing fields.
> Initializing a structure to 0 is completely sensible.

I agree completely! But of course that isn't how it is now. I guess I have spent too many years stuck on a single version of gcc that I tend not to think of changing the compiler readily enough. At least now I can upgrade the compiler freely.

Made me go check to be sure. Indeed even { } still throws the missing-initializers warning with gcc 4.8.3.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On Sep 22, 2014, at 1:33 PM, Borislav Petkov <bp@alien8.de> wrote:

> Btw, out of curiosity, what is your use case for staring at those W=2
> warnings?

I know no one cares about out-of-tree drivers, but I have a hack that
allows building out-of-tree drivers without getting warnings from the
kernel includes. We do an automated compile of every patch with W=12
and expect clean compiles.

It would be nice to compile drivers in-tree and have a similar expectation.
I guess a similar hack could be developed, but since we are contributing
upstream, I would rather uncover any potential issues that may exist, even
if they aren't in the driver. The hack would tend to cover up such issues.
This is definitely NOT about covering up things that could be problems!

> In thinking about it, what we could also do is simply move the noisiest
> ones to W=3 or so, or even add another W= level. It'll be interesting to
> hear your use case though. AFAICT, this is the first time I hear of a
> more, let's say, serious use case of W= since we added the W= things a
> couple of years ago. :-)

Well, I have W=1 in my environment, so I don't even have to ask for it, I
just get it. W=12 is just insane, or I would use that all the time. I think
it would be nice for new code, or at least new drivers, to compile clean with
W=12, but that isn't possible when the kernel includes throw so many warnings.

Nested-externs, for example, can catch people gratuitously providing a
function prototype that could become a hazard, but some use of that may
be justified. The macros provide a way to specifically allow certain
instances while generally discouraging it. Of course if you never use
W=2 you may never catch those gratuitous declarations.

> Thanks.

Hopefully the discussion is somewhat useful.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Mon, Sep 22, 2014 at 09:50:54PM +0000, Rustad, Mark D wrote:
> On Sep 22, 2014, at 1:33 PM, Borislav Petkov <bp@alien8.de> wrote:
> 
> > Btw, out of curiosity, what is your use case for staring at those W=2
> > warnings?
> 
> I know no one cares about out-of-tree drivers, but I have a hack that

Yah :-)

> allows building out-of-tree drivers without getting warnings from the
> kernel includes. We do an automated compile of every patch with W=12
> and expect clean compiles.
> 
> It would be nice to compile drivers in-tree and have a similar expectation.
> I guess a similar hack could be developed, but since we are contributing
> upstream, I would rather uncover any potential issues that may exist, even
> if they aren't in the driver. The hack would tend to cover up such issues.
> This is definitely NOT about covering up things that could be problems!

Yeah, as I said in the other mail to Jeff, I think there are a couple of
things to be pointed out:

* Fixing those is a good idea if the fixes are clean - I think we all
agree by now that adding code just to shut up gcc is not nice.

* Then, even if all those warnings were fixed one fine day, the people
who fix them would be fighting windmills because every new patch which
adds new places causing those warnings would simply go in because the
warnings are not visible in default builds.

So the question IMO turns into: are there some warnings which we should
promote to default builds so that they get taken care of eventually...

> Well, I have W=1 in my environment, so I don't even have to ask for it, I
> just get it.

I think this was the initial use case we had in mind for W= - use it
during development in order to have the compiler do extra checks to your
code. And it has caught a couple of issues, FWIW.

> W=12 is just insane, or I would use that all the time. I think it
> would be nice for new code, or at least new drivers, to compile clean
> with W=12, but that isn't possible when the kernel includes throw so
> many warnings.

Right, see above.

> Nested-externs, for example, can catch people gratuitously providing a
> function prototype that could become a hazard, but some use of that may
> be justified. The macros provide a way to specifically allow certain
> instances while generally discouraging it. Of course if you never use
> W=2 you may never catch those gratuitous declarations.

Sure, but the cost for fixing that is what bothers me. For that
particular case, it probably would even be cleaner to add a
nested-extern check to checkpatch instead of cluttering the code with
those macros.

> Hopefully the discussion is somewhat useful.

Well, it has become already, as you can see. :-D

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 3592 bytes --]

On Sep 23, 2014, at 1:22 AM, Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Sep 22, 2014 at 09:50:54PM +0000, Rustad, Mark D wrote:
> * Fixing those is a good idea if the fixes are clean - I think we all
> agree by now that adding code just to shut up gcc is not nice.

Yes, but I think there are a few cases where it could be helpful. When
there is something exceptional that will throw a warning. In one of the
patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
the warning that is thrown for every entry of the syscall table when
compiled with clang. The code is right and doing exactly what is wanted,
so note the exception and make it shut up.

> * Then, even if all those warnings were fixed one fine day, the people
> who fix them would be fighting windmills because every new patch which
> adds new places causing those warnings would simply go in because the
> warnings are not visible in default builds.
> 
> So the question IMO turns into: are there some warnings which we should
> promote to default builds so that they get taken care of eventually...

I would generally be in favor of that over time, but I recognize that with
the range of architectures and toolchains that need to be supported, that
may be difficult.

>> Well, I have W=1 in my environment, so I don't even have to ask for it, I
>> just get it.
> 
> I think this was the initial use case we had in mind for W= - use it
> during development in order to have the compiler do extra checks to your
> code. And it has caught a couple of issues, FWIW.

Yes, not everyone building the kernel is a developer. I certainly get that.
Right now W=2 is kind of useless for developers because so much noise is
generated. With the number of people working on Linux, it may be enough if
a handful are taking a look at W=2 messages, but right now it is a pretty
awful task to even try to look.

>> Nested-externs, for example, can catch people gratuitously providing a
>> function prototype that could become a hazard, but some use of that may
>> be justified. The macros provide a way to specifically allow certain
>> instances while generally discouraging it. Of course if you never use
>> W=2 you may never catch those gratuitous declarations.
> 
> Sure, but the cost for fixing that is what bothers me. For that
> particular case, it probably would even be cleaner to add a
> nested-extern check to checkpatch instead of cluttering the code with
> those macros.

Generally there should be relatively few exceptions that need to be tagged.
If there were 1000, that would be way too many. The full series of my
patches had 90 instances. Some of the few that have been sent have been
resolved in better ways, which we all agree is better. Suppose that 40
can't be reasonably resolved in direct ways. Is that really costly? I'm
trying to understand what your perception of the cost is.

Perhaps checkpatch would be a better gatekeeper for new code. OTOH, some of
those nested externs have already been eliminated, so at least for now the
warning is serving a purpose since it is scrubbing existing code.

>> Hopefully the discussion is somewhat useful.
> 
> Well, it has become already, as you can see. :-D

I take it as a given that the kernel sometimes has special needs and needs
to do special things. The macros make it possible to mark those special
usages. I prefer adding the macros in a few places to eliminating a warning
altogether unless the warning is always useless. I'm sure we agree that we
don't want to ever turn on -pedantic! :-)

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote:
> Yes, but I think there are a few cases where it could be helpful. When
> there is something exceptional that will throw a warning. In one of the
> patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
> the warning that is thrown for every entry of the syscall table when
> compiled with clang. The code is right and doing exactly what is wanted,
> so note the exception and make it shut up.

So we're doing some dancing around solely to shut up the compiler? Even
if the code is correct?!? Sorry, this is just ass backwards.

> I would generally be in favor of that over time, but I recognize that with
> the range of architectures and toolchains that need to be supported, that
> may be difficult.

Right.

> Generally there should be relatively few exceptions that need to be tagged.
> If there were 1000, that would be way too many. The full series of my
> patches had 90 instances. Some of the few that have been sent have been
> resolved in better ways, which we all agree is better. Suppose that 40
> can't be reasonably resolved in direct ways. Is that really costly? I'm
> trying to understand what your perception of the cost is.

If it were me, I'd say even one is too much. Because the very thing
of adding code just to shut up the compiler which generates otherwise
correct code is simply very very wrong in my book.

Bear in mind that even if initially you have a low number of macro
invocations, that number will grow because people will start using it in
other places too. And all of a sudden, the thing has spread like weed
and there's no removing it anymore. So we better not start in the first
place.

> Perhaps checkpatch would be a better gatekeeper for new code. OTOH,
> some of those nested externs have already been eliminated, so at
> least for now the warning is serving a purpose since it is scrubbing
> existing code.

Yep, eliminating would be optimal. If it is in checkpatch, it is much
easier to manage.

> I take it as a given that the kernel sometimes has special needs
> and needs to do special things. The macros make it possible to mark
> those special usages. I prefer adding the macros in a few places to
> eliminating a warning altogether unless the warning is always useless.

Please, no compiler-shutting-up code, see above.

> I'm sure we agree that we don't want to ever turn on -pedantic! :-)

Again, we should take compiler warnings with a grain of salt and judge
them only by the quality of the generated code. IMO.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Joe Perches]

On Tue, 2014-09-23 at 20:44 +0200, Borislav Petkov wrote:
> On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote:
> > Perhaps checkpatch would be a better gatekeeper for new code. OTOH,
> > some of those nested externs have already been eliminated, so at
> > least for now the warning is serving a purpose since it is scrubbing
> > existing code.
> 
> Yep, eliminating would be optimal. If it is in checkpatch, it is much
> easier to manage.

checkpatch is simply a regex tester, so it's only appropriate
if the false-positive false-negative rate is acceptable.

Coccinelle may be better at whatever test is being considered.

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote:
>> Yes, but I think there are a few cases where it could be helpful. When
>> there is something exceptional that will throw a warning. In one of the
>> patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
>> the warning that is thrown for every entry of the syscall table when
>> compiled with clang. The code is right and doing exactly what is wanted,
>> so note the exception and make it shut up.
> 
> So we're doing some dancing around solely to shut up the compiler? Even
> if the code is correct?!? Sorry, this is just ass backwards.

Well, please consider the specifics. The entire syscall table is initialized
with a constant pattern to be sure that every item is initialized. Then each
syscall is initialized into its proper place. The compiler is complaining that
entries are being initialized twice.

Most of the time, that is not done, and so it may catch a patch foulup or
something. In this particular case, it is normal and intended. There is
nothing wrong, so there is no reason to throw a warning for every single
entry in the table. Which is what happens with clang today.

So the code is correct, but in general the warning can reveal certain issues.
Just not in this particular usage. This happens to be a warning specific to
clang at the moment.

> If it were me, I'd say even one is too much. Because the very thing
> of adding code just to shut up the compiler which generates otherwise
> correct code is simply very very wrong in my book.
> 
> Bear in mind that even if initially you have a low number of macro
> invocations, that number will grow because people will start using it in
> other places too. And all of a sudden, the thing has spread like weed
> and there's no removing it anymore. So we better not start in the first
> place.

That is why it would be more than reasonable for checkpatch to warn on the
macro introductions. It is certainly a more significant thing than a
line > 80 characters.

> Again, we should take compiler warnings with a grain of salt and judge
> them only by the quality of the generated code. IMO.

I think more than the nature of the executable code matters. The namespace
issues revealed by -Wshadow can certainly create nasty surprises over time.
But we can't get that value from them when they are lost in an ocean of
warnings that are always there and are not the source of any trouble.

The problem is that when so many warnings are generated, particularly by
include files, even useful warnings will not be seen. Specifically
silencing ones that are deemed to be "ok" will help in seeing ones that
are not. The silencing has the greatest effect in the include files,
since there is such a multiplier effect.

Warnings that no one looks at, or bother to generate at all, have
absolutely no value. Even a silenced warning continues to wear its
shame attribute (macro). Hmm. Maybe instead of DIAG_* they should be
named SHAME_*.

Most of the time, it is new instances of warnings that are most likely to
reveal a problem. Hiding them in a flood of "normal" warnings prevents
them from ever being seen. And that is a shame.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 0/7] Silence even more W=2 warnings

[Borislav Petkov]

On Tue, Sep 23, 2014 at 08:43:17PM +0000, Rustad, Mark D wrote:
> Well, please consider the specifics. The entire syscall table is initialized
> with a constant pattern to be sure that every item is initialized. Then each
> syscall is initialized into its proper place. The compiler is complaining that
> entries are being initialized twice.
> 
> Most of the time, that is not done, and so it may catch a patch foulup or
> something. In this particular case, it is normal and intended. There is
> nothing wrong, so there is no reason to throw a warning for every single
> entry in the table. Which is what happens with clang today.
> 
> So the code is correct, but in general the warning can reveal certain issues.
> Just not in this particular usage. This happens to be a warning specific to
> clang at the moment.

Well, I read this as clang is wrong. It looks like the compiler is
unable to understand a perfectly valid usage so it throws a warning. If
we go and fix it in the kernel, we'll be wagging the dog, so to speak.
Which is a no-no obviously.

> That is why it would be more than reasonable for checkpatch to warn on the
> macro introductions. It is certainly a more significant thing than a
> line > 80 characters.

No sorry - I don't agree here. So now you're proposing of adding the
macros *and* checkpatch to warn about them. That's a really wrong thing
to do on so many levels.

...

> Most of the time, it is new instances of warnings that are most likely to
> reveal a problem. Hiding them in a flood of "normal" warnings prevents
> them from ever being seen. And that is a shame.

Sorry, I can only suggest grepping here and also using what Geert
suggested. There's simply no justification IMO to add code to the kernel
for solely silencing warnings.

-- 
Regards/Gruss,
    Boris.
--

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov <bp@alien8.de> wrote:

> Again, we should take compiler warnings with a grain of salt and judge
> them only by the quality of the generated code. IMO.

The more I thought about this, the more I think it argues for having some
diagnostic control macros. Tools such as the compiler can be very thorough -
that is their strength. The warnings that the compiler emits are things
that humans should consider. There are times when the human can judge
that nothing need be done about it, so why not use the macros to indicate
that that judgement has been made and get it out of the way so more
potentially interesting issues can be found? How many people need to
make that judgement over and over? That is clearly a waste of time and
is why these higher warning levels simply don't get looked at. So lets
capture that judgement in the code. That judgement would continue to be
open to reevaluation in any case.

The syscall issue is not arising from an include file, but I think
demonstrates the use nicely and I think is a fine example of a legitimate
use in a C file.

For me, it is about getting more value out of the evaluation that the compiler
can do. When the output of W=2 is unusable because it is so voluminous - filled
with things known to not cause problems - why not silence some of major
sources so that more interesting issues might be seen? By not doing so, W=2
has close to 0 value because no one looks at it.

I guess I also trust the maintainers to not accept lots of patches that add
uses of such macros. They have certainly demonstrated that up to this point.

:-)

-- 
Mark Rustad, Networking Division, Intel Corporation