Messages in this thread | | | Date | Fri, 12 Jul 2019 00:54:38 -0700 | From | Nathan Chancellor <> | Subject | Re: [PATCH] waitqueue: fix clang -Wuninitialized warnings |
| |
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
| |