Messages in this thread | | | From | Arnd Bergmann <> | Date | Fri, 12 Jul 2019 09:45:06 +0200 | Subject | Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings |
| |
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
| |