Created attachment 289383 [details] A test program illustrating the issue I reported this bug to GCC here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379 They don't want to diverge GCC's behavior from Sparse's, but I think this would be a useful feature, and I think it should be implemented in Sparse as well. Below is my bug report to GCC. > When the 'designated_init' attribute is used on a structure type, GCC warns > when an instance of that structure is initialized with '{ 0 }'. I think GCC > should make an exception for this, since '{ 0 }' is often used to initialize > all fields of a structure to 0, and it does not depend on the internal > structure of the structure type. > > If '{ }' is used to initialize the structure, GCC does not warn. However, > although '{ }' seems to initialize the structure to zero in GCC, I'm not > sure if it's as portable as '{ 0 }' (and it's less readable, IMO). I think > '{ }' is part of the C++ standard; does anyone know if it's part of C too? > > See the attached test program (compile with 'gcc -o test test.c'). I have also included the same program I attached in the GCC bug report. Also, since this isn't a bug report for the kernel, I've used Sparse's version number for the "Kernel Version" field.
In fact, sparse already support this via the option '-Wno-universal-initializer'. It's really very recent and thus only in the mainline tree, not in a release (and it was introduced for another warning but the result is the same). My very personal point of view is that the correct syntax should be '{ }' because it conveys much better the idea of a default initializer. This single zero in '{ 0 }' is just confusing.
(In reply to Luc Van Oostenryck from comment #1) > In fact, sparse already support this via the option > '-Wno-universal-initializer'. Perhaps '-Wno-universal-initializer' should be the default? > My very personal point of view is that the correct syntax should be '{ }' > because it conveys much better the idea of a default initializer. This > single zero in '{ 0 }' is just confusing. I can see your point, but unfortunately, as Ramsay Jones says here[1] and Alexander Monakov here[2], this is not standard C. So '{ }' isn't an option if we want to be portable. Andrew Pinski's suggestion[3] is also an option, but that seems ugly to me. [1] https://marc.info/?l=linux-sparse&m=159069587406366&w=2 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379#c4 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379#c1 I'm writing a library, Mu[4], which has a structure for which the 'designated_init' attribute is appropriate (see the 'MU_OPT' structure here[5]). However, I don't want to force my users not to use '{ 0 }', which is why I think this feature would be useful. [4] https://nongnu.org/libmu/ [5] https://git.savannah.nongnu.org/cgit/libmu.git/tree/src/options.h#n85 Also, a minor note: In the test program I attached, the attribute needs to be specified after the closing brace to work with Sparse.
(In reply to Asher Gordon from comment #2) > Perhaps '-Wno-universal-initializer' should be the default? Well, that's really the point. The problem Sparse also gives the warnings corresponding to clang's -Wnonnull and my understanding is that these warnings are desired for the kernel even when coming from using '{ 0 }'. > > My very personal point of view is that the correct syntax should be '{ }' > > because it conveys much better the idea of a default initializer. This > > single zero in '{ 0 }' is just confusing. > > I can see your point, but unfortunately, as Ramsay Jones says here[1] and > Alexander Monakov here[2], this is not standard C. So '{ }' isn't an option > if we want to be portable. Yes, I know, it's a pity. It's why I said 'should be'. > Andrew Pinski's suggestion[3] is also an option, > but that seems ugly to me. Yes, it's far from ideal. > I'm writing a library, Mu[4], which has a structure for which the > 'designated_init' attribute is appropriate (see the 'MU_OPT' structure > here[5]). However, I don't want to force my users not to use '{ 0 }', which > is why I think this feature would be useful. Interesting. Yes, I understand. Git was in the same kind of situation, it's why I added '-Wno-universal-initializer'. Can't you just add this option in your SPARSE_FLAGS or something like that? > Also, a minor note: In the test program I attached, the attribute needs to > be specified after the closing brace to work with Sparse. Yes, it's a known problem. Sparse accept 'type attributes' (those situated just after the keyword 'struct', 'union' or 'enum') but ignore them. I've some unfinished patches for this ... since some time already :(
On Thu, May 28, 2020 at 2:24 PM <bugzilla-daemon@bugzilla.kernel.org> wrote: > > Well, that's really the point. > The problem Sparse also gives the warnings corresponding to clang's -Wnonnull > and my understanding is that these warnings are desired for the kernel even > when coming from using '{ 0 }'. In the kernel, the empty initializer is be the usual way to create a zero initializer. So yes, { 0 } may exist, but it generally should be used for initializing something that is known to be an integer. And if it's a pointer, it should warn, because '0' should never have been a valid pointer, traditional C or not. It's not like we use pedantic and portable standard C to begin with. So yeah, the sparse defaults may be a bit kernel-centric. Linus
That said, I'm not sure the kernel cares. If sparse makes '{ 0 }' be eqivalent to '{ }' and doesn't warn for it, it's not like it's a huge deal. The problem with using 0 instead of NULL (or vice versa, which is a crime, and which is why NULL should never have been defined to plain 0) comes when it is actually confusing. For something like a silly zero struct initializer it's not like it's the end of the world. I do find the whole "0 could be a pointer, and NULL could be used for an integer or float" to be very distasteful, and the C++ people in particular were in denial about their brokenness for much much too long. So I'd prefer the "0 for NULL" warning, even if this may not be the most important case for it.
(In reply to Luc Van Oostenryck from comment #3) > Can't you just add this option in your > SPARSE_FLAGS or something like that? Well, actually I'm not using Sparse for my project. I want to use the 'designated_init' attribute since it is supported by GCC. And I want to use the attribute mainly so that users of my library get the warning, and I can't control what flags the user uses (and GCC doesn't have a -Wno-universal-initializer flag anyway). (In reply to Linus Torvalds from comment #4) > So yeah, the sparse defaults may be a bit kernel-centric. That's fine, but perhaps GCC should add something like -Wno-universal-initializer and use it by default. I'll suggest that in the GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379).
(In reply to Linus Torvalds from comment #5) > That said, I'm not sure the kernel cares. If sparse makes '{ 0 }' be > equivalent to '{ }' and doesn't warn for it, it's not like it's a huge deal. > > The problem with using 0 instead of NULL (or vice versa, which is a crime, > and which is why NULL should never have been defined to plain 0) comes when > it is actually confusing. OK. I also detest this 'you can use 0 for pointers' but I think that '{ 0 }' should just be understood as the standard idiom for '{ }' and that the current situation where '{ 0 }' gives warnings while '{ }' doesn't s confusing and annoying. So, I'll change Sparse's default to -Wno-universal-initializer. > So I'd prefer the "0 for NULL" warning, even if this may not be the most > important case for it. Do you think it's worth to add -Wuniversal-initializer for the kernel so that these warnings are still present for '{ 0 }'? -- Luc
I'm happy to let the kernel not warn about {0} used for a pointer until I find some egregious horror-case, and then I know that I can add -Wuniversal-initializer to warn about it.