I thought I'd choose one of the more core parts of the kernel to
demonstrate the value of -Wshadow. It found two places where there are
shadowed variables that are at least confusing. For all I know they're
buggy and my resolution of these warnings is wrong.
The first 12 patches just untangle the unclean uses of __ret in wait.h
& friends. Then 4 patches to fix problems in headers that are noticed
by kernel/sched. Two patches fix the two places in kernel/sched/
with shadowed variables and the final patch adds -Wshadow=local to
the Makefile.
I'm quite certain this patch series isn't going in as-is. But maybe
it'll inspire some patches that can go in.
Matthew Wilcox (Oracle) (19):
wait: Parameterize the return variable to ___wait_event()
swait: Parameterize the return variable to __swait_event_timeout()
swait: Parameterize the return variable to
__swait_event_interruptible_timeout()
swait: Parameterize the return variable to
__swait_event_idle_timeout()
wait: Parameterize the return variable to __wait_event_timeout()
wait: Parameterize the return variable to
__wait_event_freezable_timeout()
wait: Parameterize the return variable to
__wait_event_interruptible_timeout()
wait: Parameterize the return variable to __wait_event_idle_timeout()
wait: Parameterize the return variable to
__wait_event_idle_exclusive_timeout()
wait: Parameterize the return variable to
__wait_event_killable_timeout()
wait: Parameterize the return variable to
__wait_event_lock_irq_timeout()
wait_bit: Parameterize the return variable to
__wait_var_event_timeout()
Add UNIQUE_ID
wait: Add a unique identifier to ___wait_event()
x86: Use a unique identifier in __WARN_FLAGS()
x86: Pass a unique identifier to __xchg_op()
sched/rt: Rename a shadowed variable
sched/topology: Rename the cpu parameter
sched: Enable -Wshadow=local
arch/x86/include/asm/bug.h | 4 +-
arch/x86/include/asm/cmpxchg.h | 6 +-
include/linux/compiler.h | 1 +
include/linux/swait.h | 24 ++--
include/linux/wait.h | 223 ++++++++++++++++-----------------
include/linux/wait_bit.h | 9 +-
kernel/sched/Makefile | 1 +
kernel/sched/rt.c | 4 +-
kernel/sched/topology.c | 6 +-
9 files changed, 138 insertions(+), 140 deletions(-)
--
2.34.1
Macros should not refer to variables which aren't in their arguments.
Pass the name from both its callers.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/wait.h | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 524e0d1690a4..757285a04c88 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1109,7 +1109,7 @@ do { \
__ret; \
})
-#define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state) \
+#define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state, __ret) \
___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
state, 0, timeout, \
spin_unlock_irq(&lock); \
@@ -1140,25 +1140,25 @@ do { \
* was interrupted by a signal, and the remaining jiffies otherwise
* if the condition evaluated to true before the timeout elapsed.
*/
-#define wait_event_interruptible_lock_irq_timeout(wq_head, condition, lock, \
- timeout) \
-({ \
- long __ret = timeout; \
- if (!___wait_cond_timeout(condition, __ret)) \
- __ret = __wait_event_lock_irq_timeout( \
- wq_head, condition, lock, timeout, \
- TASK_INTERRUPTIBLE); \
- __ret; \
+#define wait_event_interruptible_lock_irq_timeout(wq_head, condition, lock, \
+ timeout) \
+({ \
+ long __ret = timeout; \
+ if (!___wait_cond_timeout(condition, __ret)) \
+ __ret = __wait_event_lock_irq_timeout( \
+ wq_head, condition, lock, timeout, \
+ TASK_INTERRUPTIBLE, __ret); \
+ __ret; \
})
-#define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout) \
-({ \
- long __ret = timeout; \
- if (!___wait_cond_timeout(condition, __ret)) \
- __ret = __wait_event_lock_irq_timeout( \
- wq_head, condition, lock, timeout, \
- TASK_UNINTERRUPTIBLE); \
- __ret; \
+#define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout) \
+({ \
+ long __ret = timeout; \
+ if (!___wait_cond_timeout(condition, __ret)) \
+ __ret = __wait_event_lock_irq_timeout( \
+ wq_head, condition, lock, timeout, \
+ TASK_UNINTERRUPTIBLE, __ret); \
+ __ret; \
})
/*
--
2.34.1
On Wed, Mar 02, 2022 at 04:34:32AM +0000, Matthew Wilcox (Oracle) wrote:
> I thought I'd choose one of the more core parts of the kernel to
> demonstrate the value of -Wshadow. It found two places where there are
> shadowed variables that are at least confusing. For all I know they're
> buggy and my resolution of these warnings is wrong.
>
> The first 12 patches just untangle the unclean uses of __ret in wait.h
> & friends. Then 4 patches to fix problems in headers that are noticed
> by kernel/sched. Two patches fix the two places in kernel/sched/
> with shadowed variables and the final patch adds -Wshadow=local to
> the Makefile.
You are my hero. I was pulling my hair out trying to figure out how
to deal with this a few months ago, and the use of UNIQUE_ID was the
key. Yay!
> I'm quite certain this patch series isn't going in as-is. But maybe
> it'll inspire some patches that can go in.
I think it's pretty darn close. One thing that can be done to test the
results for the first 12 patches is to do a binary comparison -- these
changes _should_ have no impact on the final machine code. (It'll
totally change the debug sections, etc, but the machine code should be
the same.)
--
Kees Cook
On Wed, Mar 02, 2022 at 06:43:57PM +0000, Matthew Wilcox wrote:
> ie "__ret = freezable_schedule_timeout(__ret)" is supposed to refer to
> the inner __ret, not the outer __ret. Which was the opposite of what
> I thought was supposed to happen.
>
> We can fix this, of course. Something like ...
>
> #define ___wait_event_freezable_timeout(wq_head, condition, timeout, ret) \
> ___wait_event(wq_head, ___wait_cond_timeout(condition, ret), \
> TASK_INTERRUPTIBLE, 0, timeout, \
> ret = freezable_schedule_timeout(ret), ret)
>
> #define __wait_event_freezable_timeout(wq_head, condition, timeout) \
> ___wait_event_freezable_timeout(wq_head, condition, timeout, UNIQUE_ID)
>
> ... and now all the 'ret' refer to the thing that they look like they're
> referring to.
Right; so the trick is to make sure all ___wait_event() users will have
a ret and then the inner ret can go away. The interruptible/timeout
variants all already have a ret variable, but the unconditional things
like wait_event() do not (which is where all the trouble started).
By simply adding a ret variable, even to the variants without return
value, the inner variable can go away and the shadowing goes away.
Macros should not refer to variables which aren't in their arguments.
Pass the name from its caller.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/wait.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 5f2c43373061..a4a17f3b6871 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -503,7 +503,7 @@ do { \
__ret; \
})
-#define __wait_event_interruptible_timeout(wq_head, condition, timeout) \
+#define __wait_event_interruptible_timeout(wq_head, condition, timeout, __ret) \
___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_INTERRUPTIBLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -534,7 +534,7 @@ do { \
might_sleep(); \
if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_interruptible_timeout(wq_head, \
- condition, timeout); \
+ condition, timeout, __ret); \
__ret; \
})
--
2.34.1
Macros should not refer to variables which aren't in their arguments.
Pass the name from its callers.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/swait.h | 12 ++++++------
include/linux/wait.h | 32 ++++++++++++++++----------------
include/linux/wait_bit.h | 4 ++--
3 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/include/linux/swait.h b/include/linux/swait.h
index 6a8c22b8c2a5..5e8e9b13be2d 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -191,14 +191,14 @@ do { \
} while (0)
#define __swait_event_timeout(wq, condition, timeout) \
- ___swait_event(wq, ___wait_cond_timeout(condition), \
+ ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \
TASK_UNINTERRUPTIBLE, timeout, \
__ret = schedule_timeout(__ret))
#define swait_event_timeout_exclusive(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __swait_event_timeout(wq, condition, timeout); \
__ret; \
})
@@ -216,14 +216,14 @@ do { \
})
#define __swait_event_interruptible_timeout(wq, condition, timeout) \
- ___swait_event(wq, ___wait_cond_timeout(condition), \
+ ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \
TASK_INTERRUPTIBLE, timeout, \
__ret = schedule_timeout(__ret))
#define swait_event_interruptible_timeout_exclusive(wq, condition, timeout)\
({ \
long __ret = timeout; \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __swait_event_interruptible_timeout(wq, \
condition, timeout); \
__ret; \
@@ -252,7 +252,7 @@ do { \
} while (0)
#define __swait_event_idle_timeout(wq, condition, timeout) \
- ___swait_event(wq, ___wait_cond_timeout(condition), \
+ ___swait_event(wq, ___wait_cond_timeout(condition, __ret), \
TASK_IDLE, timeout, \
__ret = schedule_timeout(__ret))
@@ -278,7 +278,7 @@ do { \
#define swait_event_idle_timeout_exclusive(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __swait_event_idle_timeout(wq, \
condition, timeout); \
__ret; \
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 851e07da2583..890cce3c0f2e 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -271,7 +271,7 @@ static inline void wake_up_pollfree(struct wait_queue_head *wq_head)
__wake_up_pollfree(wq_head);
}
-#define ___wait_cond_timeout(condition) \
+#define ___wait_cond_timeout(condition, __ret) \
({ \
bool __cond = (condition); \
if (__cond && !__ret) \
@@ -386,7 +386,7 @@ do { \
})
#define __wait_event_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_UNINTERRUPTIBLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -413,13 +413,13 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_timeout(wq_head, condition, timeout); \
__ret; \
})
#define __wait_event_freezable_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_INTERRUPTIBLE, 0, timeout, \
__ret = freezable_schedule_timeout(__ret))
@@ -431,7 +431,7 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_freezable_timeout(wq_head, condition, timeout); \
__ret; \
})
@@ -503,7 +503,7 @@ do { \
})
#define __wait_event_interruptible_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_INTERRUPTIBLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -531,7 +531,7 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_interruptible_timeout(wq_head, \
condition, timeout); \
__ret; \
@@ -698,7 +698,7 @@ do { \
} while (0)
#define __wait_event_idle_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_IDLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -725,13 +725,13 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_idle_timeout(wq_head, condition, timeout); \
__ret; \
})
#define __wait_event_idle_exclusive_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_IDLE, 1, timeout, \
__ret = schedule_timeout(__ret))
@@ -762,7 +762,7 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_idle_exclusive_timeout(wq_head, condition, timeout);\
__ret; \
})
@@ -932,7 +932,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
})
#define __wait_event_killable_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
TASK_KILLABLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -962,7 +962,7 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_killable_timeout(wq_head, \
condition, timeout); \
__ret; \
@@ -1107,7 +1107,7 @@ do { \
})
#define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
state, 0, timeout, \
spin_unlock_irq(&lock); \
__ret = schedule_timeout(__ret); \
@@ -1141,7 +1141,7 @@ do { \
timeout) \
({ \
long __ret = timeout; \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_lock_irq_timeout( \
wq_head, condition, lock, timeout, \
TASK_INTERRUPTIBLE); \
@@ -1151,7 +1151,7 @@ do { \
#define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout) \
({ \
long __ret = timeout; \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_event_lock_irq_timeout( \
wq_head, condition, lock, timeout, \
TASK_UNINTERRUPTIBLE); \
diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h
index 7dec36aecbd9..227e6a20a978 100644
--- a/include/linux/wait_bit.h
+++ b/include/linux/wait_bit.h
@@ -292,7 +292,7 @@ do { \
})
#define __wait_var_event_timeout(var, condition, timeout) \
- ___wait_var_event(var, ___wait_cond_timeout(condition), \
+ ___wait_var_event(var, ___wait_cond_timeout(condition, __ret), \
TASK_UNINTERRUPTIBLE, 0, timeout, \
__ret = schedule_timeout(__ret))
@@ -300,7 +300,7 @@ do { \
({ \
long __ret = timeout; \
might_sleep(); \
- if (!___wait_cond_timeout(condition)) \
+ if (!___wait_cond_timeout(condition, __ret)) \
__ret = __wait_var_event_timeout(var, condition, timeout); \
__ret; \
})
--
2.34.1
Macros should not refer to variables which aren't in their arguments.
Pass the name from its caller.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
include/linux/wait.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 9089c8bde04b..524e0d1690a4 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -934,9 +934,9 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
__ret; \
})
-#define __wait_event_killable_timeout(wq_head, condition, timeout) \
- ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
- TASK_KILLABLE, 0, timeout, \
+#define __wait_event_killable_timeout(wq_head, condition, timeout, __ret) \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
+ TASK_KILLABLE, 0, timeout, \
__ret = schedule_timeout(__ret))
/**
@@ -961,14 +961,14 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
*
* Only kill signals interrupt this process.
*/
-#define wait_event_killable_timeout(wq_head, condition, timeout) \
-({ \
- long __ret = timeout; \
- might_sleep(); \
- if (!___wait_cond_timeout(condition, __ret)) \
- __ret = __wait_event_killable_timeout(wq_head, \
- condition, timeout); \
- __ret; \
+#define wait_event_killable_timeout(wq_head, condition, timeout) \
+({ \
+ long __ret = timeout; \
+ might_sleep(); \
+ if (!___wait_cond_timeout(condition, __ret)) \
+ __ret = __wait_event_killable_timeout(wq_head, \
+ condition, timeout, __ret); \
+ __ret; \
})
--
2.34.1
On Wed, Mar 02, 2022 at 10:32:23AM -0800, Kees Cook wrote:
> On Wed, Mar 02, 2022 at 04:34:32AM +0000, Matthew Wilcox (Oracle) wrote:
> > I thought I'd choose one of the more core parts of the kernel to
> > demonstrate the value of -Wshadow. It found two places where there are
> > shadowed variables that are at least confusing. For all I know they're
> > buggy and my resolution of these warnings is wrong.
> >
> > The first 12 patches just untangle the unclean uses of __ret in wait.h
> > & friends. Then 4 patches to fix problems in headers that are noticed
> > by kernel/sched. Two patches fix the two places in kernel/sched/
> > with shadowed variables and the final patch adds -Wshadow=local to
> > the Makefile.
>
> You are my hero. I was pulling my hair out trying to figure out how
> to deal with this a few months ago, and the use of UNIQUE_ID was the
> key. Yay!
>
> > I'm quite certain this patch series isn't going in as-is. But maybe
> > it'll inspire some patches that can go in.
>
> I think it's pretty darn close. One thing that can be done to test the
> results for the first 12 patches is to do a binary comparison -- these
> changes _should_ have no impact on the final machine code. (It'll
> totally change the debug sections, etc, but the machine code should be
> the same.)
Peter pointed out that I got confused about which __ret was being
referred to:
<peterz> +#define __wait_event_freezable_timeout(wq_head, condition, timeout, __ret) \
<peterz> + ___wait_event(wq_head, ___wait_cond_timeout(condition, __ret), \
<peterz> + TASK_INTERRUPTIBLE, 0, timeout, \
<peterz> + __ret = freezable_schedule_timeout(__ret), UNIQUE_ID)
<peterz> so now that internal variable is UNIQUE_ID, whatever that is
<peterz> but the condition argument was supposed to look at that
<peterz> but you explicitly pulled that out
ie "__ret = freezable_schedule_timeout(__ret)" is supposed to refer to
the inner __ret, not the outer __ret. Which was the opposite of what
I thought was supposed to happen.
We can fix this, of course. Something like ...
#define ___wait_event_freezable_timeout(wq_head, condition, timeout, ret) \
___wait_event(wq_head, ___wait_cond_timeout(condition, ret), \
TASK_INTERRUPTIBLE, 0, timeout, \
ret = freezable_schedule_timeout(ret), ret)
#define __wait_event_freezable_timeout(wq_head, condition, timeout) \
___wait_event_freezable_timeout(wq_head, condition, timeout, UNIQUE_ID)
... and now all the 'ret' refer to the thing that they look like they're
referring to.
*thread necromancy*
On Wed, Mar 02, 2022 at 04:34:32AM +0000, Matthew Wilcox (Oracle) wrote:
> I thought I'd choose one of the more core parts of the kernel to
> demonstrate the value of -Wshadow. It found two places where there are
> shadowed variables that are at least confusing. For all I know they're
> buggy and my resolution of these warnings is wrong.
>
> The first 12 patches just untangle the unclean uses of __ret in wait.h
> & friends. Then 4 patches to fix problems in headers that are noticed
> by kernel/sched. Two patches fix the two places in kernel/sched/
> with shadowed variables and the final patch adds -Wshadow=local to
> the Makefile.
>
> I'm quite certain this patch series isn't going in as-is. But maybe
> it'll inspire some patches that can go in.
I was looking at -Wshadow=local again, and remembered this series. It
sounded like things were close, but a tweak was needed. What would be
next to get this working?
Thanks!
-Kees
--
Kees Cook
On Tue, 16 Apr 2024 at 14:15, Kees Cook <[email protected]> wrote:
>
> I was looking at -Wshadow=local again, and remembered this series. It
> sounded like things were close, but a tweak was needed. What would be
> next to get this working?
So what is the solution to
#define MAX(a,b) ({ \
typeof(a) __a = (a); \
typeof(b) __b = (b); \
__a > __b ? __a : __b; \
})
int test(int a, int b, int c)
{
return MAX(a, MAX(b,c));
}
where -Wshadow=all causes insane warnings that are bogus garbage?
Honestly, Willy's patch-series is a hack to avoid this kind of very
natural nested macro pattern.
But it's a horrible hack, and it does it by making the code actively worse.
Here's the deal: if we can't handle somethng like the above without
warning, -Wshadow isn't getting enabled.
Because we don't write worse code because of bad warnings.
IOW, what is the sane way to just say "this variable can shadow the
use site, and it's fine"?
Without that kind of out, I don't think -Wshadow=local is workable.
Linus
On Tue, 16 Apr 2024 at 17:29, Linus Torvalds
<[email protected]> wrote:
>
> So what is the solution to
>
> #define MAX(a,b) ({ \
Side note: do not focus on the macro name. I'm not interested in "the
solution is MAX3()" kinds of answers.
And the macro doesn't have to be physically nested like that.
The macro could be a list traversal thing. Appended is an example
list traversal macro that is type-safe and simple to use, and would
absolutely improve on our current "list_for_each_entry()" in many
ways.
Imagine now traversing a list within an entry that happens while
traversing an outer one. Which is not at all an odd thing, IOW, you'd
have
traverse(bus_list, bus) {
traverse(&bus->devices, device) {
.. do something with the device ..
}
}
this kind of macro use that has internal variables that will
inevitably shadow each other when used in some kind of nested model is
pretty fundamental.
So no. The answer is *NOT* some kind of "don't do that then".
Linus
PS. The list trraversal thing below worked at some point. It's an old
snippet of mine, it might not work any more. It depends on the kernel
'list_head' definitions, it's not a standalone example.
---
#define list_traversal_head(type, name, member) union { \
struct list_head name; \
struct type *name##_traversal_type; \
struct type##_##name##_##member##_traversal_struct
*name##_traversal_info; \
}
#define list_traversal_node(name) union { \
struct list_head name; \
int name##_traversal_node; \
}
#define DEFINE_TRAVERSAL(from, name, to, member) \
struct to##_##name##_##member##_traversal_struct { \
char dummy[offsetof(struct to, member##_traversal_node)]; \
struct list_head node; \
}
#define __traverse_type(head, ext) typeof(head##ext)
#define traverse_type(head, ext) __traverse_type(head, ext)
#define traverse_offset(head) \
offsetof(traverse_type(*head,_traversal_info), node)
#define traverse_is_head(head, raw) \
((void *)(raw) == (void *)(head))
/*
* Very annoying. We want 'node' to be of the right type, and __raw to be
* the underlying "struct list_head". But while we can declare multiple
* variables in a for-loop in C99, we can't declare multiple _types_.
*
* So __raw has the wrong type, making everything pointlessly uglier.
*/
#define traverse(head, node) \
for (typeof(*head##_traversal_type) __raw = (void
*)(head)->next, node; \
node = (void *)__raw + traverse_offset(*head),
!traverse_is_head(head, __raw); \
__raw = (void *) ((struct list_head *)__raw)->next)
struct first_struct {
int offset[6];
list_traversal_head(second_struct, head, entry);
};
struct second_struct {
int hash;
int offset[17];
list_traversal_node(entry);
};
DEFINE_TRAVERSAL(first_struct, head, second_struct, entry);
struct second_struct *find(struct first_struct *p)
{
traverse(&p->head, node) {
if (node->hash == 1234)
return node;
}
return NULL;
}
On Tue, Apr 16, 2024 at 05:29:02PM -0700, Linus Torvalds wrote:
> On Tue, 16 Apr 2024 at 14:15, Kees Cook <[email protected]> wrote:
> >
> > I was looking at -Wshadow=local again, and remembered this series. It
> > sounded like things were close, but a tweak was needed. What would be
> > next to get this working?
>
> So what is the solution to
>
> #define MAX(a,b) ({ \
> typeof(a) __a = (a); \
> typeof(b) __b = (b); \
> __a > __b ? __a : __b; \
> })
#define __MAX(a, __a, b, __b) ({ \
typeof(a) __a = (a); \
typeof(b) __b = (b); \
__a > __b ? __a : __b; \
})
#define MAX(a, b) __MAX(a, UNIQUE_ID(a), b, UNIQUE_ID(b))
At least, I think that was the plan. This was two years ago and I've
mostly forgotten.
> int test(int a, int b, int c)
> {
> return MAX(a, MAX(b,c));
> }
>
> where -Wshadow=all causes insane warnings that are bogus garbage?
>
> Honestly, Willy's patch-series is a hack to avoid this kind of very
> natural nested macro pattern.
>
> But it's a horrible hack, and it does it by making the code actively worse.
>
> Here's the deal: if we can't handle somethng like the above without
> warning, -Wshadow isn't getting enabled.
>
> Because we don't write worse code because of bad warnings.
>
> IOW, what is the sane way to just say "this variable can shadow the
> use site, and it's fine"?
>
> Without that kind of out, I don't think -Wshadow=local is workable.
>
> Linus
* Matthew Wilcox <[email protected]> wrote:
> On Tue, Apr 16, 2024 at 05:29:02PM -0700, Linus Torvalds wrote:
> > On Tue, 16 Apr 2024 at 14:15, Kees Cook <[email protected]> wrote:
> > >
> > > I was looking at -Wshadow=local again, and remembered this series. It
> > > sounded like things were close, but a tweak was needed. What would be
> > > next to get this working?
> >
> > So what is the solution to
> >
> > #define MAX(a,b) ({ \
> > typeof(a) __a = (a); \
> > typeof(b) __b = (b); \
> > __a > __b ? __a : __b; \
> > })
>
> #define __MAX(a, __a, b, __b) ({ \
> typeof(a) __a = (a); \
> typeof(b) __b = (b); \
> __a > __b ? __a : __b; \
> })
>
> #define MAX(a, b) __MAX(a, UNIQUE_ID(a), b, UNIQUE_ID(b))
>
> At least, I think that was the plan. This was two years ago and I've
> mostly forgotten.
I think as long as we can keep any additional complexity inside macros it
would be acceptable, at least from the scheduler's POV. A UNIQUE_ID() layer
of indirection for names doesn't sound look a too high price.
I had good reasults with -Wshadow in user-space projects: once the false
positives got ironed out, the vast percentage of new warnings was for
genuinely problematic new code. But they rarely used block-nested macros
like the kernel does.
Thanks,
Ingo
On Wed, Apr 17, 2024 at 01:52:28AM +0100, Matthew Wilcox wrote:
> On Tue, Apr 16, 2024 at 05:29:02PM -0700, Linus Torvalds wrote:
> > On Tue, 16 Apr 2024 at 14:15, Kees Cook <[email protected]> wrote:
> > >
> > > I was looking at -Wshadow=local again, and remembered this series. It
> > > sounded like things were close, but a tweak was needed. What would be
> > > next to get this working?
> >
> > So what is the solution to
> >
> > #define MAX(a,b) ({ \
> > typeof(a) __a = (a); \
> > typeof(b) __b = (b); \
> > __a > __b ? __a : __b; \
> > })
>
> #define __MAX(a, __a, b, __b) ({ \
> typeof(a) __a = (a); \
> typeof(b) __b = (b); \
> __a > __b ? __a : __b; \
> })
>
> #define MAX(a, b) __MAX(a, UNIQUE_ID(a), b, UNIQUE_ID(b))
Yup, this is what we've had for a long time now. See
include/linux/minmax.h
> At least, I think that was the plan. This was two years ago and I've
> mostly forgotten.
>
> > int test(int a, int b, int c)
> > {
> > return MAX(a, MAX(b,c));
> > }
> >
> > where -Wshadow=all causes insane warnings that are bogus garbage?
> >
> > Honestly, Willy's patch-series is a hack to avoid this kind of very
> > natural nested macro pattern.
> >
> > But it's a horrible hack, and it does it by making the code actively worse.
> >
> > Here's the deal: if we can't handle somethng like the above without
> > warning, -Wshadow isn't getting enabled.
> >
> > Because we don't write worse code because of bad warnings.
> >
> > IOW, what is the sane way to just say "this variable can shadow the
> > use site, and it's fine"?
> >
> > Without that kind of out, I don't think -Wshadow=local is workable.
This isn't a hill I want to die on, but it's just another case where
we've fought bugs more than once that would have stood out immediately
if we had -Wshadow=local enabled, but there is basically only 1 user. In
my bug-fighting calculus, it makes sense to deal with fixing the 1 user
so we can gain the coverage everywhere else.
But there are much worse bug sources, so if Willy's series isn't
workable, I'll drop this again for now. :)
-Kees
--
Kees Cook