Messages in this thread | | | Date | Wed, 2 Mar 2022 18:43:57 +0000 | From | Matthew Wilcox <> | Subject | Re: [PATCH 00/19] Enable -Wshadow=local for kernel/sched |
| |
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.
| |