[PATCH] waitqueue: fix clang -Wuninitialized warnings

[PATCH] waitqueue: fix clang -Wuninitialized warnings

[Arnd Bergmann]

When CONFIG_LOCKDEP is set, every use of DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
produces an annoying warning from clang, which is particularly annoying
for allmodconfig builds:

fs/namei.c:1646:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
        DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
                                        ^~
include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
        struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
                               ~~~~                                  ^~~~
include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
        ({ init_waitqueue_head(&name); name; })
                                       ^~~~

After playing with it for a while, I have found a way to rephrase the
macro in a way that should work well with both gcc and clang and not
produce this warning. The open-coded __WAIT_QUEUE_HEAD_INIT_ONSTACK
is a little more verbose than the original version by Peter Zijlstra,
but avoids the gcc-ism that suppresses warnings when assigning a
variable to itself.

Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/wait.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index ddb959641709..84e39643b780 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -68,8 +68,15 @@ extern void __init_waitqueue_head(struct wait_queue_head *wq_head, const char *n
 	} while (0)
 
 #ifdef CONFIG_LOCKDEP
-# define __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) \
-	({ init_waitqueue_head(&name); name; })
+# define __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) {					\
+	.lock		= __SPIN_LOCK_UNLOCKED(name.lock),			\
+	.head		= ({							\
+		static struct lock_class_key __key;				\
+		lockdep_set_class_and_name(&(name).lock, &__key, # name);	\
+		(struct list_head){ &(name).head, &(name).head };		\
+	}),									\
+}
+
 # define DECLARE_WAIT_QUEUE_HEAD_ONSTACK(name) \
 	struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
 #else
-- 
2.20.0

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Nathan Chancellor]

On Wed, Jul 03, 2019 at 10:10:55AM +0200, Arnd Bergmann wrote:
> When CONFIG_LOCKDEP is set, every use of DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> produces an annoying warning from clang, which is particularly annoying
> for allmodconfig builds:
> 
> fs/namei.c:1646:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
>         DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
>                                         ^~
> include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
>         struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
>                                ~~~~                                  ^~~~
> include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
>         ({ init_waitqueue_head(&name); name; })
>                                        ^~~~
> 
> After playing with it for a while, I have found a way to rephrase the
> macro in a way that should work well with both gcc and clang and not
> produce this warning. The open-coded __WAIT_QUEUE_HEAD_INIT_ONSTACK
> is a little more verbose than the original version by Peter Zijlstra,
> but avoids the gcc-ism that suppresses warnings when assigning a
> variable to itself.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thank you for sending the fix for these warnings, they are the last
major ones that I can see across various defconfig and allyesconfig
testing. This resolves all of them.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Arnd Bergmann]

On Wed, Jul 3, 2019 at 7:58 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Wed, Jul 03, 2019 at 10:10:55AM +0200, Arnd Bergmann wrote:
> > When CONFIG_LOCKDEP is set, every use of DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> > produces an annoying warning from clang, which is particularly annoying
> > for allmodconfig builds:
> >
> > fs/namei.c:1646:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
> >         DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> >                                         ^~
> > include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
> >         struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
> >                                ~~~~                                  ^~~~
> > include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
> >         ({ init_waitqueue_head(&name); name; })
> >                                        ^~~~
> >
> > After playing with it for a while, I have found a way to rephrase the
> > macro in a way that should work well with both gcc and clang and not
> > produce this warning. The open-coded __WAIT_QUEUE_HEAD_INIT_ONSTACK
> > is a little more verbose than the original version by Peter Zijlstra,
> > but avoids the gcc-ism that suppresses warnings when assigning a
> > variable to itself.
> >
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>

Who would be the right person to pick this patch up for mainline?

I guess it may have to wait until the end of the merge window now,
but I'd still like this to be part of 4.3 and possibly backported to
the stable kernels as we build those with clang as well.

       Arnd

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Andrew Morton]

On Wed,  3 Jul 2019 10:10:55 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> When CONFIG_LOCKDEP is set, every use of DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> produces an annoying warning from clang, which is particularly annoying
> for allmodconfig builds:
> 
> fs/namei.c:1646:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
>         DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
>                                         ^~
> include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
>         struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
>                                ~~~~                                  ^~~~
> include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
>         ({ init_waitqueue_head(&name); name; })
>                                        ^~~~

<scratches head>

Surely clang is being extraordinarily dumb here?

DECLARE_WAIT_QUEUE_HEAD_ONSTACK() is effectively doing

	struct wait_queue_head name = ({ __init_waitqueue_head(&name) ; name; })

which is perfectly legitimate!  clang has no business assuming that
__init_waitqueue_head() will do any reads from the pointer which it was
passed, nor can clang assume that __init_waitqueue_head() leaves any of
*name uninitialized.

Does it also warn if code does this?

	struct wait_queue_head name;
	__init_waitqueue_head(&name);
	name = name;

which is equivalent, isn't it?


The proposed solution is, effectively, to open-code
__init_waitqueue_head() at each DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
callsite.  That's pretty unpleasant and calls for an explanatory
comment at the __WAIT_QUEUE_HEAD_INIT_ONSTACK() definition site as well
as a cautionary comment at the __init_waitqueue_head() definition so we
can keep the two versions in sync as code evolves.


Hopefully clang will soon be hit with the cluebat (yes?) and this
change becomes obsolete in the quite short term.  Surely 6-12 months
from now nobody will be using the uncluebatted version of clang on
contemporary kernel sources so we get to remove this nastiness again. 
Which makes me wonder whether we should merge it at all.

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Peter Zijlstra]

On Tue, Jul 09, 2019 at 09:27:17PM +0200, Arnd Bergmann wrote:
> On Wed, Jul 3, 2019 at 7:58 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Wed, Jul 03, 2019 at 10:10:55AM +0200, Arnd Bergmann wrote:
> > > When CONFIG_LOCKDEP is set, every use of DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> > > produces an annoying warning from clang, which is particularly annoying
> > > for allmodconfig builds:
> > >
> > > fs/namei.c:1646:34: error: variable 'wq' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
> > >         DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> > >                                         ^~
> > > include/linux/wait.h:74:63: note: expanded from macro 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
> > >         struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
> > >                                ~~~~                                  ^~~~
> > > include/linux/wait.h:72:33: note: expanded from macro '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
> > >         ({ init_waitqueue_head(&name); name; })
> > >                                        ^~~~
> > >
> > > After playing with it for a while, I have found a way to rephrase the
> > > macro in a way that should work well with both gcc and clang and not
> > > produce this warning. The open-coded __WAIT_QUEUE_HEAD_INIT_ONSTACK
> > > is a little more verbose than the original version by Peter Zijlstra,
> > > but avoids the gcc-ism that suppresses warnings when assigning a
> > > variable to itself.
> > >
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> Who would be the right person to pick this patch up for mainline?

That would be me; but like Andrew, I'm not a fan of this patch.

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 2:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  3 Jul 2019 10:10:55 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> <scratches head>
>
> Surely clang is being extraordinarily dumb here?
>
> DECLARE_WAIT_QUEUE_HEAD_ONSTACK() is effectively doing
>
>         struct wait_queue_head name = ({ __init_waitqueue_head(&name) ; name; })
>
> which is perfectly legitimate!  clang has no business assuming that
> __init_waitqueue_head() will do any reads from the pointer which it was
> passed, nor can clang assume that __init_waitqueue_head() leaves any of
> *name uninitialized.
>
> Does it also warn if code does this?
>
>         struct wait_queue_head name;
>         __init_waitqueue_head(&name);
>         name = name;
>
> which is equivalent, isn't it?

No, it does not warn for this.

I've tried a few more variants here: https://godbolt.org/z/ykSX0r

What I think is going on here is a result of clang and gcc fundamentally
treating -Wuninitialized warnings differently. gcc tries to make the warnings
as helpful as possible, but given the NP-complete nature of this problem
it won't always get it right, and it traditionally allowed this syntax as a
workaround.

int f(void)
{
    int i = i; // tell gcc not to warn
    return i;
}

clang apparently implements the warnings in a way that is as
completely predictable (and won't warn in cases that it
doesn't completely understand), but decided as a result that the
gcc 'int i = i' syntax is bogus and it always warns about a variable
used in its own declaration that is later referenced, without looking
at whether the declaration does initialize it or not.

> The proposed solution is, effectively, to open-code
> __init_waitqueue_head() at each DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> callsite.  That's pretty unpleasant and calls for an explanatory
> comment at the __WAIT_QUEUE_HEAD_INIT_ONSTACK() definition site as well
> as a cautionary comment at the __init_waitqueue_head() definition so we
> can keep the two versions in sync as code evolves.

Yes, makes sense.

> Hopefully clang will soon be hit with the cluebat (yes?) and this
> change becomes obsolete in the quite short term.  Surely 6-12 months
> from now nobody will be using the uncluebatted version of clang on
> contemporary kernel sources so we get to remove this nastiness again.
> Which makes me wonder whether we should merge it at all.

Would it make you feel better to keep the current code but have an alternative
version guarded with e.g. "#if defined(__clang__ && (__clang_major__ <= 9)"?

While it is probably a good idea to fix clang here, this is one of the last
issues that causes a significant difference between gcc and clang in build
testing with kernelci:
https://kernelci.org/build/next/branch/master/kernel/next-20190709/
I'm trying to get all the warnings fixed there so we can spot build-time
regressions more easily.

      Arnd

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Nathan Chancellor]

On Fri, Jul 12, 2019 at 09:45:06AM +0200, Arnd Bergmann wrote:
> On Fri, Jul 12, 2019 at 2:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed,  3 Jul 2019 10:10:55 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > <scratches head>
> >
> > Surely clang is being extraordinarily dumb here?
> >
> > DECLARE_WAIT_QUEUE_HEAD_ONSTACK() is effectively doing
> >
> >         struct wait_queue_head name = ({ __init_waitqueue_head(&name) ; name; })
> >
> > which is perfectly legitimate!  clang has no business assuming that
> > __init_waitqueue_head() will do any reads from the pointer which it was
> > passed, nor can clang assume that __init_waitqueue_head() leaves any of
> > *name uninitialized.
> >
> > Does it also warn if code does this?
> >
> >         struct wait_queue_head name;
> >         __init_waitqueue_head(&name);
> >         name = name;
> >
> > which is equivalent, isn't it?
> 
> No, it does not warn for this.
> 
> I've tried a few more variants here: https://godbolt.org/z/ykSX0r
> 
> What I think is going on here is a result of clang and gcc fundamentally
> treating -Wuninitialized warnings differently. gcc tries to make the warnings
> as helpful as possible, but given the NP-complete nature of this problem
> it won't always get it right, and it traditionally allowed this syntax as a
> workaround.
> 
> int f(void)
> {
>     int i = i; // tell gcc not to warn
>     return i;
> }
> 
> clang apparently implements the warnings in a way that is as
> completely predictable (and won't warn in cases that it
> doesn't completely understand), but decided as a result that the
> gcc 'int i = i' syntax is bogus and it always warns about a variable
> used in its own declaration that is later referenced, without looking
> at whether the declaration does initialize it or not.
> 
> > The proposed solution is, effectively, to open-code
> > __init_waitqueue_head() at each DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> > callsite.  That's pretty unpleasant and calls for an explanatory
> > comment at the __WAIT_QUEUE_HEAD_INIT_ONSTACK() definition site as well
> > as a cautionary comment at the __init_waitqueue_head() definition so we
> > can keep the two versions in sync as code evolves.
> 
> Yes, makes sense.
> 
> > Hopefully clang will soon be hit with the cluebat (yes?) and this
> > change becomes obsolete in the quite short term.  Surely 6-12 months
> > from now nobody will be using the uncluebatted version of clang on
> > contemporary kernel sources so we get to remove this nastiness again.
> > Which makes me wonder whether we should merge it at all.
> 
> Would it make you feel better to keep the current code but have an alternative
> version guarded with e.g. "#if defined(__clang__ && (__clang_major__ <= 9)"?
> 
> While it is probably a good idea to fix clang here, this is one of the last
> issues that causes a significant difference between gcc and clang in build
> testing with kernelci:
> https://kernelci.org/build/next/branch/master/kernel/next-20190709/
> I'm trying to get all the warnings fixed there so we can spot build-time
> regressions more easily.
> 
>       Arnd

I'm just spitballing here since I am about to go to sleep but could we
do something like you did for bee20031772a ("disable -Wattribute-alias
warning for SYSCALL_DEFINEx()") and disable the warning in
DECLARE_WAIT_QUEUE_HEAD_ONSTACK only since we know it is not going to
be a problem? That way, if/when Clang is fixed, we can just have the
warning be disabled for older versions?

Cheers,
Nathan

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 9:54 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Fri, Jul 12, 2019 at 09:45:06AM +0200, Arnd Bergmann wrote:
> > On Fri, Jul 12, 2019 at 2:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > On Wed,  3 Jul 2019 10:10:55 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > > <scratches head>
> > >
> > > Surely clang is being extraordinarily dumb here?
> > >
> > > DECLARE_WAIT_QUEUE_HEAD_ONSTACK() is effectively doing
> > >
> > >         struct wait_queue_head name = ({ __init_waitqueue_head(&name) ; name; })
> > >
> > > which is perfectly legitimate!  clang has no business assuming that
> > > __init_waitqueue_head() will do any reads from the pointer which it was
> > > passed, nor can clang assume that __init_waitqueue_head() leaves any of
> > > *name uninitialized.
> > >
> > > Does it also warn if code does this?
> > >
> > >         struct wait_queue_head name;
> > >         __init_waitqueue_head(&name);
> > >         name = name;
> > >
> > > which is equivalent, isn't it?
> >
> > No, it does not warn for this.
> >
> > I've tried a few more variants here: https://godbolt.org/z/ykSX0r
> >
> > What I think is going on here is a result of clang and gcc fundamentally
> > treating -Wuninitialized warnings differently. gcc tries to make the warnings
> > as helpful as possible, but given the NP-complete nature of this problem
> > it won't always get it right, and it traditionally allowed this syntax as a
> > workaround.
> >
> > int f(void)
> > {
> >     int i = i; // tell gcc not to warn
> >     return i;
> > }
> >
> > clang apparently implements the warnings in a way that is as
> > completely predictable (and won't warn in cases that it
> > doesn't completely understand), but decided as a result that the
> > gcc 'int i = i' syntax is bogus and it always warns about a variable
> > used in its own declaration that is later referenced, without looking
> > at whether the declaration does initialize it or not.
> >
> > > The proposed solution is, effectively, to open-code
> > > __init_waitqueue_head() at each DECLARE_WAIT_QUEUE_HEAD_ONSTACK()
> > > callsite.  That's pretty unpleasant and calls for an explanatory
> > > comment at the __WAIT_QUEUE_HEAD_INIT_ONSTACK() definition site as well
> > > as a cautionary comment at the __init_waitqueue_head() definition so we
> > > can keep the two versions in sync as code evolves.
> >
> > Yes, makes sense.
> >
> > > Hopefully clang will soon be hit with the cluebat (yes?) and this
> > > change becomes obsolete in the quite short term.  Surely 6-12 months
> > > from now nobody will be using the uncluebatted version of clang on
> > > contemporary kernel sources so we get to remove this nastiness again.
> > > Which makes me wonder whether we should merge it at all.
> >
> > Would it make you feel better to keep the current code but have an alternative
> > version guarded with e.g. "#if defined(__clang__ && (__clang_major__ <= 9)"?
> >
> > While it is probably a good idea to fix clang here, this is one of the last
> > issues that causes a significant difference between gcc and clang in build
> > testing with kernelci:
> > https://kernelci.org/build/next/branch/master/kernel/next-20190709/
> > I'm trying to get all the warnings fixed there so we can spot build-time
> > regressions more easily.
> >
> >       Arnd
>
> I'm just spitballing here since I am about to go to sleep but could we
> do something like you did for bee20031772a ("disable -Wattribute-alias
> warning for SYSCALL_DEFINEx()") and disable the warning in
> DECLARE_WAIT_QUEUE_HEAD_ONSTACK only since we know it is not going to
> be a problem? That way, if/when Clang is fixed, we can just have the
> warning be disabled for older versions?

I managed to get that to work, but there are two problems:

- the __diag_ignore() infrastructure was never added for clang, so
  I ended up copying a lot from gcc. There is probably a nicer way
  to do this, but that would require a larger rework
- adding __diag_pop() between two variable declarations is seen as
  a statement that causes a warning with both gcc and clang,
  so I had to turn that warning off as well for all compilers, and at that
  point it gets rather ugly in the macro.

       Arnd

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 333a6695a918..0d30c0489ad7 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -42,3 +42,31 @@
  * compilers, like ICC.
  */
 #define barrier() __asm__ __volatile__("" : : : "memory")
+
+/*
+ * Turn individual warnings and errors on and off locally, depending
+ * on version.
+ */
+#define __diag_clang(version, severity, s) \
+       __diag_clang_ ## version(__diag_clang_ ## severity s)
+
+/* Severity used in pragma directives */
+#define __diag_clang_ignore    ignored
+#define __diag_clang_warn      warning
+#define __diag_clang_error     error
+
+#define __diag_str1(s)         #s
+#define __diag_str(s)          __diag_str1(s)
+#define __diag(s)              _Pragma(__diag_str(clang diagnostic s))
+
+#if __clang_major__ >= 8
+#define __diag_clang_8(s)              __diag(s)
+#else
+#define __diag_clang_8(s)
+#endif
+
+#if __clang_major__ >= 9
+#define __diag_clang_9(s)              __diag(s)
+#else
+#define __diag_clang_9(s)
+#endif
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e8579412ad21..c5f8d9ae0530 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -165,8 +165,16 @@
 #define __diag_str(s)          __diag_str1(s)
 #define __diag(s)              _Pragma(__diag_str(GCC diagnostic s))

+#if GCC_VERSION >= 40006
+#define __diag_GCC_4_6(s)      __diag(s)
+#else
+#define __diag_GCC_4_6(s)
+#endif
+
 #if GCC_VERSION >= 80000
 #define __diag_GCC_8(s)                __diag(s)
 #else
 #define __diag_GCC_8(s)
 #endif
+
+#define __diag_clang(s...)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index ddb959641709..0e33fe589f49 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -71,7 +71,12 @@ extern void __init_waitqueue_head(struct
wait_queue_head *wq_head, const char *n
 # define __WAIT_QUEUE_HEAD_INIT_ONSTACK(name) \
        ({ init_waitqueue_head(&name); name; })
 # define DECLARE_WAIT_QUEUE_HEAD_ONSTACK(name) \
-       struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
+       __diag_push();                  \
+       __diag_ignore(clang, 8, "-Wuninitialized",
"https://godbolt.org/z/ykSX0r");     \
+       __diag_ignore(clang, 8, "-Wdeclaration-after-statement", "for
__diag_pop");     \
+       __diag_ignore(GCC, 4_6, "-Wdeclaration-after-statement", "for
__diag_pop");     \
+       struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name); \
+       __diag_pop()
 #else
 # define DECLARE_WAIT_QUEUE_HEAD_ONSTACK(name) DECLARE_WAIT_QUEUE_HEAD(name)
 #endif

Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings

[Nick Desaulniers]

On Fri, Jul 12, 2019 at 12:45 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Jul 12, 2019 at 2:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed,  3 Jul 2019 10:10:55 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>
> > <scratches head>
> >
> > Surely clang is being extraordinarily dumb here?
> >
> > DECLARE_WAIT_QUEUE_HEAD_ONSTACK() is effectively doing
> >
> >         struct wait_queue_head name = ({ __init_waitqueue_head(&name) ; name; })
> >
> > which is perfectly legitimate!  clang has no business assuming that
> > __init_waitqueue_head() will do any reads from the pointer which it was
> > passed, nor can clang assume that __init_waitqueue_head() leaves any of
> > *name uninitialized.
> >
> > Does it also warn if code does this?
> >
> >         struct wait_queue_head name;
> >         __init_waitqueue_head(&name);
> >         name = name;
> >
> > which is equivalent, isn't it?
>
> No, it does not warn for this.

So I think this is just a bug in Clang, where it's getting tripped up
due to the GNU C statement expression.  See the example I put in this
bug report: https://bugs.llvm.org/show_bug.cgi?id=42604

Clang is warning for this pattern of struct assignment, but not for
non-aggregate (integral) assignment.

(I think that pattern is pretty cool; it makes it more straightforward
to provide macro's that properly construct aggregates in C; in
particular I feel like I've been looking for something like this to
simply the use of __attribute__((__cleanup__)) to do RAII in C and
make smart pointers, fd's, etc.).

Let's fix Clang, drop the kernel workaround, and thanks for the discussion.
-- 
Thanks,
~Nick Desaulniers