linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] seqlock: avoid -Wshadow warnings
@ 2020-10-26 16:50 Arnd Bergmann
  2020-10-26 16:58 ` Peter Zijlstra
  2020-12-03 10:35 ` [tip: locking/core] " tip-bot2 for Arnd Bergmann
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2020-10-26 16:50 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Ahmed S. Darwish
  Cc: Arnd Bergmann, Paul E. McKenney, Marco Elver, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

When building with W=2, there is a flood of warnings about the seqlock
macros shadowing local variables:

  19806 linux/seqlock.h:331:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]
     48 linux/seqlock.h:348:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]
      8 linux/seqlock.h:379:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]

Prefix the local variables to make the warning useful elsewhere again.

Fixes: 52ac39e5db51 ("seqlock: seqcount_t: Implement all read APIs as statement expressions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/seqlock.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index cbfc78b92b65..8d8552474c64 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -328,13 +328,13 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define __read_seqcount_begin(s)					\
 ({									\
-	unsigned seq;							\
+	unsigned __seq;							\
 									\
-	while ((seq = __seqcount_sequence(s)) & 1)			\
+	while ((__seq = __seqcount_sequence(s)) & 1)			\
 		cpu_relax();						\
 									\
 	kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX);			\
-	seq;								\
+	__seq;								\
 })
 
 /**
@@ -345,10 +345,10 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define raw_read_seqcount_begin(s)					\
 ({									\
-	unsigned seq = __read_seqcount_begin(s);			\
+	unsigned _seq = __read_seqcount_begin(s);			\
 									\
 	smp_rmb();							\
-	seq;								\
+	_seq;								\
 })
 
 /**
@@ -376,11 +376,11 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define raw_read_seqcount(s)						\
 ({									\
-	unsigned seq = __seqcount_sequence(s);				\
+	unsigned __seq = __seqcount_sequence(s);			\
 									\
 	smp_rmb();							\
 	kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX);			\
-	seq;								\
+	__seq;								\
 })
 
 /**
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] seqlock: avoid -Wshadow warnings
  2020-10-26 16:50 [PATCH] seqlock: avoid -Wshadow warnings Arnd Bergmann
@ 2020-10-26 16:58 ` Peter Zijlstra
  2020-10-27 23:34   ` Arnd Bergmann
  2020-12-03 10:35 ` [tip: locking/core] " tip-bot2 for Arnd Bergmann
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2020-10-26 16:58 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ingo Molnar, Will Deacon, Ahmed S. Darwish, Arnd Bergmann,
	Paul E. McKenney, Marco Elver, linux-kernel

On Mon, Oct 26, 2020 at 05:50:38PM +0100, Arnd Bergmann wrote:

> -	unsigned seq;							\
> +	unsigned __seq;							\

> -	unsigned seq = __read_seqcount_begin(s);			\
> +	unsigned _seq = __read_seqcount_begin(s);			\

> -	unsigned seq = __seqcount_sequence(s);				\
> +	unsigned __seq = __seqcount_sequence(s);			\

Can we have a consistent number of leading '_' ?

Also, I suppose you're going to find the explicit shadow in
___wait_event(), that one's not going to be trivial to fix.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] seqlock: avoid -Wshadow warnings
  2020-10-26 16:58 ` Peter Zijlstra
@ 2020-10-27 23:34   ` Arnd Bergmann
  2020-10-28  7:51     ` Rasmus Villemoes
  2020-10-28  8:51     ` Peter Zijlstra
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2020-10-27 23:34 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Will Deacon, Ahmed S. Darwish, Paul E. McKenney,
	Marco Elver, linux-kernel

On Mon, Oct 26, 2020 at 5:58 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Oct 26, 2020 at 05:50:38PM +0100, Arnd Bergmann wrote:
>
> > -     unsigned seq;                                                   \
> > +     unsigned __seq;                                                 \
>
> > -     unsigned seq = __read_seqcount_begin(s);                        \
> > +     unsigned _seq = __read_seqcount_begin(s);                       \
>
> > -     unsigned seq = __seqcount_sequence(s);                          \
> > +     unsigned __seq = __seqcount_sequence(s);                        \
>
> Can we have a consistent number of leading '_' ?

Not really ;-)

The warning comes from raw_read_seqcount_begin() calling
__read_seqcount_begin() and both using the same variable
name. I could rename one of them  and use double-underscores
for both, but I haven't come up with a good alternative name
that wouldn't make it less consistent in the process.

> Also, I suppose you're going to find the explicit shadow in
> ___wait_event(), that one's not going to be trivial to fix.

I have this patch in my tree at the moment but did not send that yet
because that caused a regression on powerpc:

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 57ccf26d3b96..5d00a6fb7154 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -265,7 +265,11 @@ extern void init_wait_entry(struct
wait_queue_entry *wq_entry, int flags);
 ({
         \
        __label__ __out;
         \
        struct wait_queue_entry __wq_entry;
         \
-       long __ret = ret;       /* explicit shadow */
         \
+       __diag_push()
         \
+       __diag_ignore(GCC, 8, "-Wshadow", "explicit shadow")
         \
+       __diag_ignore(CLANG, 9, "-Wshadow", "explicit shadow")
         \
+       long __ret = ret;
         \
+       __diag_pop();
         \

         \
        init_wait_entry(&__wq_entry, exclusive ? WQ_FLAG_EXCLUSIVE :
0);        \
        for (;;) {
         \


Still looking at alternative approaches.

      Arnd

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] seqlock: avoid -Wshadow warnings
  2020-10-27 23:34   ` Arnd Bergmann
@ 2020-10-28  7:51     ` Rasmus Villemoes
  2020-10-28  8:51     ` Peter Zijlstra
  1 sibling, 0 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2020-10-28  7:51 UTC (permalink / raw)
  To: Arnd Bergmann, Peter Zijlstra
  Cc: Ingo Molnar, Will Deacon, Ahmed S. Darwish, Paul E. McKenney,
	Marco Elver, linux-kernel

On 28/10/2020 00.34, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 5:58 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Mon, Oct 26, 2020 at 05:50:38PM +0100, Arnd Bergmann wrote:
>>
>>> -     unsigned seq;                                                   \
>>> +     unsigned __seq;                                                 \
>>
>>> -     unsigned seq = __read_seqcount_begin(s);                        \
>>> +     unsigned _seq = __read_seqcount_begin(s);                       \
>>
>>> -     unsigned seq = __seqcount_sequence(s);                          \
>>> +     unsigned __seq = __seqcount_sequence(s);                        \
>>
>> Can we have a consistent number of leading '_' ?
> 
> Not really ;-)
> 
> The warning comes from raw_read_seqcount_begin() calling
> __read_seqcount_begin() and both using the same variable
> name. I could rename one of them  and use double-underscores
> for both, but I haven't come up with a good alternative name
> that wouldn't make it less consistent in the process.

At least x86's put_user and get_user use _pu/_gu suffixes on their local
variables, so perhaps that could be made a weak default convention?

__seq_rsb
__seq_rrsb
__seq_rrs

Hm, or perhaps not. But it's still better than triplicating each macro
to do a UNIQUE_ID dance.

Rasmus

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] seqlock: avoid -Wshadow warnings
  2020-10-27 23:34   ` Arnd Bergmann
  2020-10-28  7:51     ` Rasmus Villemoes
@ 2020-10-28  8:51     ` Peter Zijlstra
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2020-10-28  8:51 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ingo Molnar, Will Deacon, Ahmed S. Darwish, Paul E. McKenney,
	Marco Elver, linux-kernel

On Wed, Oct 28, 2020 at 12:34:10AM +0100, Arnd Bergmann wrote:
> On Mon, Oct 26, 2020 at 5:58 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Oct 26, 2020 at 05:50:38PM +0100, Arnd Bergmann wrote:
> >
> > > -     unsigned seq;                                                   \
> > > +     unsigned __seq;                                                 \
> >
> > > -     unsigned seq = __read_seqcount_begin(s);                        \
> > > +     unsigned _seq = __read_seqcount_begin(s);                       \
> >
> > > -     unsigned seq = __seqcount_sequence(s);                          \
> > > +     unsigned __seq = __seqcount_sequence(s);                        \
> >
> > Can we have a consistent number of leading '_' ?
> 
> Not really ;-)

Duh.. ok, I'll take it as is.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tip: locking/core] seqlock: avoid -Wshadow warnings
  2020-10-26 16:50 [PATCH] seqlock: avoid -Wshadow warnings Arnd Bergmann
  2020-10-26 16:58 ` Peter Zijlstra
@ 2020-12-03 10:35 ` tip-bot2 for Arnd Bergmann
  1 sibling, 0 replies; 6+ messages in thread
From: tip-bot2 for Arnd Bergmann @ 2020-12-03 10:35 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Arnd Bergmann, Peter Zijlstra (Intel), x86, linux-kernel

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     a07c45312f06e288417049208c344ad76074627d
Gitweb:        https://git.kernel.org/tip/a07c45312f06e288417049208c344ad76074627d
Author:        Arnd Bergmann <arnd@arndb.de>
AuthorDate:    Mon, 26 Oct 2020 17:50:38 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 03 Dec 2020 11:20:50 +01:00

seqlock: avoid -Wshadow warnings

When building with W=2, there is a flood of warnings about the seqlock
macros shadowing local variables:

  19806 linux/seqlock.h:331:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]
     48 linux/seqlock.h:348:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]
      8 linux/seqlock.h:379:11: warning: declaration of 'seq' shadows a previous local [-Wshadow]

Prefix the local variables to make the warning useful elsewhere again.

Fixes: 52ac39e5db51 ("seqlock: seqcount_t: Implement all read APIs as statement expressions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20201026165044.3722931-1-arnd@kernel.org
---
 include/linux/seqlock.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index cbfc78b..8d85524 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -328,13 +328,13 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define __read_seqcount_begin(s)					\
 ({									\
-	unsigned seq;							\
+	unsigned __seq;							\
 									\
-	while ((seq = __seqcount_sequence(s)) & 1)			\
+	while ((__seq = __seqcount_sequence(s)) & 1)			\
 		cpu_relax();						\
 									\
 	kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX);			\
-	seq;								\
+	__seq;								\
 })
 
 /**
@@ -345,10 +345,10 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define raw_read_seqcount_begin(s)					\
 ({									\
-	unsigned seq = __read_seqcount_begin(s);			\
+	unsigned _seq = __read_seqcount_begin(s);			\
 									\
 	smp_rmb();							\
-	seq;								\
+	_seq;								\
 })
 
 /**
@@ -376,11 +376,11 @@ SEQCOUNT_LOCKNAME(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mu
  */
 #define raw_read_seqcount(s)						\
 ({									\
-	unsigned seq = __seqcount_sequence(s);				\
+	unsigned __seq = __seqcount_sequence(s);			\
 									\
 	smp_rmb();							\
 	kcsan_atomic_next(KCSAN_SEQLOCK_REGION_MAX);			\
-	seq;								\
+	__seq;								\
 })
 
 /**

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-03 10:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 16:50 [PATCH] seqlock: avoid -Wshadow warnings Arnd Bergmann
2020-10-26 16:58 ` Peter Zijlstra
2020-10-27 23:34   ` Arnd Bergmann
2020-10-28  7:51     ` Rasmus Villemoes
2020-10-28  8:51     ` Peter Zijlstra
2020-12-03 10:35 ` [tip: locking/core] " tip-bot2 for Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).