[Bug 207959] New: Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.

[Bug 207959] New: Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.

[bugzilla-daemon]

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: tools_sparse@kernel-bugs.kernel.org
          Reporter: AsDaGo@posteo.net
        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.

> 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.

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

Luc Van Oostenryck (luc.vanoostenryck@gmail.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |luc.vanoostenryck@gmail.com

--- Comment #1 from Luc Van Oostenryck (luc.vanoostenryck@gmail.com) ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

Re: [Bug 207959] New: Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.

[Ramsay Jones]

On 28/05/2020 17:27, bugzilla-daemon@bugzilla.kernel.org 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: tools_sparse@kernel-bugs.kernel.org
>           Reporter: AsDaGo@posteo.net
>         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.
> 
>> 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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #2 from Asher Gordon (AsDaGo@posteo.net) ---
(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.

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #3 from Luc Van Oostenryck (luc.vanoostenryck@gmail.com) ---
(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 :(

-- 
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.

[Linus Torvalds]

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

[Bug 207959] Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #4 from Linus Torvalds (torvalds@linux-foundation.org) ---
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

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

Linus Torvalds (torvalds@linux-foundation.org) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |torvalds@linux-foundation.o
                   |                            |rg

--- Comment #5 from Linus Torvalds (torvalds@linux-foundation.org) ---
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.

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #6 from Asher Gordon (AsDaGo@posteo.net) ---
(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).

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #7 from Luc Van Oostenryck (luc.vanoostenryck@gmail.com) ---
(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

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

--- Comment #8 from Linus Torvalds (torvalds@linux-foundation.org) ---
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.

-- 
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.

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=207959

Luc Van Oostenryck (luc.vanoostenryck@gmail.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |CODE_FIX

-- 
You are receiving this mail because:
You are watching the assignee of the bug.