New: Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
https://bugzilla.kernel.org/show_bug.cgi?id=207959
Bug ID: 207959
Summary: Don't warn about the universal zero initializer for a
structure with the 'designated_init' attribute.
Product: Tools
Version: unspecified
Kernel Version: Sparse 0.6.1 (Debian: 0.6.1-2+b1)
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: enhancement
Priority: P1
Component: Sparse
Assignee: [email protected]
Reporter: [email protected]
Regression: No
Created attachment 289383
--> https://bugzilla.kernel.org/attachment.cgi?id=289383&action=edit
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.
Quoted 6 lines
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.
Quoted 6 lines
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.
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
https://bugzilla.kernel.org/show_bug.cgi?id=207959
Luc Van Oostenryck ([email protected]) changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]
--- Comment #1 from Luc Van Oostenryck ([email protected]) --- 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.
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
Quoted 23 lines · On 28/05/2020 17:27, [email protected] wrote:
https://bugzilla.kernel.org/show_bug.cgi?id=207959
Bug ID: 207959
Summary: Don't warn about the universal zero initializer for a
structure with the 'designated_init' attribute.
Product: Tools
Version: unspecified
Kernel Version: Sparse 0.6.1 (Debian: 0.6.1-2+b1)
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: enhancement
Priority: P1
Component: Sparse
Assignee: [email protected]
Reporter: [email protected]
Regression: NoCreated attachment 289383
--> https://bugzilla.kernel.org/attachment.cgi?id=289383&action=edit
A test program illustrating the issue
Quoted 17 lines
I reported this bug to GCC here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379They 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.
Quoted 10 lines
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?
No this is not standard C:
$ cat -n junk1.c
1 #include <stdio.h>
2
3 int main (int argc, char *argv[])
4 {
5 struct { char *f; int i; } fred = {};
6 printf("fred.f %p, fred.i %d\n", fred.f, fred.i);
7 return 0;
8 }
$ gcc -pedantic -o junk1 junk1.c
junk1.c: In function ‘main’:
junk1.c:5:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
struct { char *f; int i; } fred = {};
^
$ ./junk1
fred.f (nil), fred.i 0
$ ... and you get similar results with clang:
$ clang -pedantic -o junk1 junk1.c
junk1.c:5:36: warning: use of GNU empty initializer extension
[-Wgnu-empty-initializer]
struct { char *f; int i; } fred = {};
^
junk1.c:6:35: warning: format specifies type 'void *' but the argument has type
'char *' [-Wformat-pedantic]
printf("fred.f %p, fred.i %d\n", fred.f, fred.i);
~~ ^~~~~~
%s
2 warnings generated.
$ ATB,
Ramsay Jones
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #2 from Asher Gordon ([email protected]) --- (In reply to Luc Van Oostenryck from comment #1)
Quoted 2 lines
In fact, sparse already support this via the option '-Wno-universal-initializer'.
Perhaps '-Wno-universal-initializer' should be the default?
Quoted 3 lines
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.
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #3 from Luc Van Oostenryck ([email protected]) --- (In reply to Asher Gordon from comment #2)
Quoted 1 line
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 }'.
Quoted 7 lines
Quoted 3 lines
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'.
Quoted 2 lines
Andrew Pinski's suggestion[3] is also an option,
but that seems ugly to me.
Yes, it's far from ideal.
Quoted 4 lines
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?
Quoted 2 lines
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 :(
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
Re: [Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
Quoted 5 lines · On Thu, May 28, 2020 at 2:24 PM <[email protected]> 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
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #4 from Linus Torvalds ([email protected]) ---
Quoted 5 lines · On Thu, May 28, 2020 at 2:24 PM <[email protected]> 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
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
Linus Torvalds ([email protected]) changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected] | |rg
--- Comment #5 from Linus Torvalds ([email protected]) --- 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.
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #6 from Asher Gordon ([email protected]) --- (In reply to Luc Van Oostenryck from comment #3)
Quoted 2 lines
Can't you just add this option in your
SPARSE_FLAGSor 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)
Quoted 1 line
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).
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #7 from Luc Van Oostenryck ([email protected]) --- (In reply to Linus Torvalds from comment #5)
Quoted 6 lines
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.
Quoted 2 lines
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
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
--- Comment #8 from Linus Torvalds ([email protected]) --- 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.
Signature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.
[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.
In reply to [email protected]https://bugzilla.kernel.org/show_bug.cgi?id=207959
Luc Van Oostenryck ([email protected]) changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |CODE_FIXSignature · 2 lines
You are receiving this mail because: You are watching the assignee of the bug.