Linus Torvalds
2022-Mar-01 00:45 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox <willy at infradead.org> wrote:> > #define ___PASTE(a, b) a##b > #define __PASTE(a, b) ___PASTE(a, b) > #define _min(a, b, u) ({ \Yeah, except that's ugly beyond belief, plus it's literally not what we do in the kernel. Really. The "-Wshadow doesn't work on the kernel" is not some new issue, because you have to do completely insane things to the source code to enable it. Just compare your uglier-than-sin version to my straightforward one. One does the usual and obvious "use a private variable to avoid the classic multi-use of a macro argument". And the other one is an abomination. Linus
Linus Torvalds
2022-Mar-01 00:57 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds <torvalds at linux-foundation.org> wrote:> > Yeah, except that's ugly beyond belief, plus it's literally not what > we do in the kernel.(Of course, I probably shouldn't have used 'min()' as an example, because that is actually one of the few places where we do exactly that, using our __UNIQUE_ID() macros. Exactly because people _have_ tried to do -Wshadow when doing W=2). Linus
Kees Cook
2022-Mar-01 18:14 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote:> Really. The "-Wshadow doesn't work on the kernel" is not some new > issue, because you have to do completely insane things to the source > code to enable it.The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :) Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives. I tried to capture some of the rationale and research here: https://github.com/KSPP/linux/issues/152 -- Kees Cook
Linus Torvalds
2022-Mar-01 18:47 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 1, 2022 at 10:14 AM Kees Cook <keescook at chromium.org> wrote:> > The first big glitch with -Wshadow was with shadowed global variables. > GCC 4.8 fixed that, but it still yells about shadowed functions. What > _almost_ works is -Wshadow=local.Heh. Yeah, I just have long memories of "-Wshadow was a disaster". You looked into the details.> Another way to try to catch misused shadow variables is > -Wunused-but-set-varible, but it, too, has tons of false positives.That on the face of it should be an easy warning to get technically right for a compiler. So I assume the "false positives" are simply because we end up having various variables that really don't end up being used - and "intentionally" so). Or rather, they might only be used under some config option - perhaps the use is even syntactically there and parsed, but the compiler notices that it's turned off under some if (IS_ENABLED(..)) option? Because yeah, we have a lot of those. I think that's a common theme with a lot of compiler warnings: on the face of it they sound "obviously sane" and nobody should ever write code like that. A conditional that is always true? Sounds idiotic, and sounds like a reasonable thing for a compiler to warn about, since why would you have a conditional in the first place for that? But then you realize that maybe the conditional is a build config option, and "always true" suddenly makes sense. Or it's a test for something that is always true on _that_architecture_ but not in some general sense (ie testing "sizeof()"). Or it's a purely syntactic conditional, like "do { } while (0)". It's why I'm often so down on a lot of the odd warnings that are hiding under W=1 and friends. They all may make sense in the trivial case ("That is insane") but then in the end they happen for sane code. And yeah, -Wshadow has had tons of history with macro nesting, and just being badly done in the first place (eg "strlen" can be a perfectly fine local variable). That said, maybe people could ask the gcc and clan people for a way to _mark_ the places where we expect to validly see shadowing. For example, that "local variable in a macro expression statement" thing is absolutely horrendous to fix with preprocessor tricks to try to make for unique identifiers. But I think it would be much more syntactically reasonable to add (for example) a "shadow" attribute to such a variable exactly to tell the compiler "yeah, yeah, I know this identifier could shadow an outer one" and turn it off that way. Linus
Matthew Wilcox
2022-Mar-01 19:01 UTC
[Nouveau] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 10:14:07AM -0800, Kees Cook wrote:> On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote: > > Really. The "-Wshadow doesn't work on the kernel" is not some new > > issue, because you have to do completely insane things to the source > > code to enable it. > > The first big glitch with -Wshadow was with shadowed global variables. > GCC 4.8 fixed that, but it still yells about shadowed functions. What > _almost_ works is -Wshadow=local. At first glace, all the warnings > look solvable, but then one will eventually discover __wait_event() > and associated macros that mix when and how deeply it intentionally > shadows variables. :)Well, that's just disgusting. Macros fundamentally shouldn't be referring to things that aren't in their arguments. The first step to cleaning this up is ... I'll take a look at the rest of cleaning this up soon.>From 28ffe35d56223d4242b915832299e5acc926737e Mon Sep 17 00:00:00 2001From: "Matthew Wilcox (Oracle)" <willy at infradead.org> Date: Tue, 1 Mar 2022 13:47:07 -0500 Subject: [PATCH] wait: Parameterize the return variable to ___wait_event() Macros should not refer to variables which aren't in their arguments. Pass the name from its callers. Signed-off-by: Matthew Wilcox (Oracle) <willy at infradead.org> --- 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