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(-)
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
*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 <keescook@chromium.org> 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, Apr 16, 2024 at 05:29:02PM -0700, Linus Torvalds wrote:
> On Tue, 16 Apr 2024 at 14:15, Kees Cook <keescook@chromium.org> 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
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 <keescook@chromium.org> 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
* Matthew Wilcox <willy@infradead.org> wrote:
> On Tue, Apr 16, 2024 at 05:29:02PM -0700, Linus Torvalds wrote:
> > On Tue, 16 Apr 2024 at 14:15, Kees Cook <keescook@chromium.org> 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 Tue, 16 Apr 2024 at 17:29, Linus Torvalds
<torvalds@linux-foundation.org> 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 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
© 2016 - 2026 Red Hat, Inc.