Re: svn commit: r295768 - head/usr.sbin/iostat

[Dimitry Andric]

On 22 Feb 2016, at 10:56, David Chisnall  wrote:
> 
> On 19 Feb 2016, at 23:23, Dimitry Andric  wrote:
>> 
>> This warning is only produced when you use -Wall -W, and then initialize
>> structs partially, i.e. you initialize some fields but not others.  I
>> think this is a quite reasonable warning for a high warning level.
> 
> The warning is annoying in many ways.  You ought to be able to zero 
> initialise any struct with {0}, but clang objects if you do this and requires 
> every field to be filled in.  This warning really shouldn’t be enabled with 
> -Wall, because it has too hight a false positive rate.

It isn't, it is in -W (a.k.a -Wextra).  But gcc also warns in this case.  E.g. 
if I use this example:

struct foo {
int i;
int j;
} bar[] = {
{ 42 },
{ 43 }
};

I get the following warnings from gcc -Wextra:

$ gcc -Wextra -c initializers.c
initializers.c:5:2: warning: missing initializer for field 'j' of 'struct foo' 
[-Wmissing-field-initializers]
  { 42 },
  ^
initializers.c:3:6: note: 'j' declared here
  int j;
  ^
initializers.c:6:2: warning: missing initializer for field 'j' of 'struct foo' 
[-Wmissing-field-initializers]
  { 43 }
  ^
initializers.c:3:6: note: 'j' declared here
  int j;
  ^

Note that the warnings disappear if C99 initializers are used.

-Dimitry



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r295768 - head/usr.sbin/iostat

[Gleb Smirnoff]

On Fri, Feb 19, 2016 at 04:50:40PM -0800, Conrad Meyer wrote:
C> > It is not quite as simple as this would make it sound.  The elements or
C> > members of an aggregate (e.g.) structure type are initialized as if it were
C> > an object of static storage duration (i.e., to zero) if the initializer 
list
C> > does not contain enough initializers for all members of the aggregate type,
C> > per item 21 of section 6.7.8 of n1256.pdf.  However, such initialization
C> > does not necessarily need to zero any padding bytes that are present, which
C> > may take unspecified values.  Personally, I think this particular clang
C> > warning can be too aggressive, especially for complex structs, but on the
C> > other hand given the indeterminateness of padding, bzero/memset are often a
C> > better choice anyway.
C> 
C> By definition, padding byte contents are unused.  There is no reason
C> their values matter one way or another, so why do we care about the
C> distinction between bzero and member zero initialization?

Well, one can run bcmp() or a hashing function over entire structure,
in that case contents of padding matter.

But I still don't want to see this warning in -Wall, since it kills
very useful C99 initializers.

The code, that runs bcmp() or hashes over structs, should take care, and
we shouldn't pessimize the entire build for its sake.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[David Chisnall]

On 22 Feb 2016, at 10:15, Kubilay Kocak  wrote:
> 
> For the lay persons among us (I'm genuinely interested), what are the
> the downsides to requiring initialization of all fields?

Explicit initialisation, or initialisation in general?

Being able to initialise the entire structure with code that will always 
initialise everything with zero makes the code less fragile because you can add 
a field to the structure and not have to find all of the places where it’s 
initialised.  The clang warning makes it easy to find the places you need to 
change, but now you have code churn for no good reason.

Implicit initialisation is often useful for generating more efficient code.  If 
you’re intialising with a bunch of common fields, the compiler will end up 
creating a static variable and doing a memcpy (which the back end may 
optimise), which is very cheap.  A load of code in libc ends up like this.

> And in addition, the upsides, if any, of 'deferred' field initialization?

The same as the upsides for deferred variable initialization.  Modern compilers 
are good at tracking intraprocedural data flow.  If you don’t initialise a 
field, then the compiler can typically tell that you’ve read from a field but 
not written to it.  It’s generally good defensive programming for zero to have 
a well-defined meaning (C codifies this for pointers, Go codifies it for all 
structure types), but if you haven’t done this then the zero may be a valid but 
incorrect value and no amount of static analysis can tell you that the 
programmer did something that is valid but wrong.  Some languages with richer 
type systems explicitly encode the invalidity of the variable in its type and 
transform it to the valid version once it is initialised (some also provide 
maybe types as a programmer-level constructs.  You can implement them fairly 
trivially in C++, see for example LLVM’s ErrorOr<> template).

Sane coding style for C (not style(9)) typically includes the principle of 
minimum scope, where variables must be declared as close to possible their 
initialisation.  This helps minimise these problems, because you either declare 
the variable where it is initialised, or (if initialisation is conditional) 
right next to it where you can clearly see that it has been initialised on all 
code paths.  Unfortunately, as you can see from the PVS results, style(9) could 
have been carefully designed to maximise the introduction of accidental bugs.

David

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Kubilay Kocak]

On 22/02/2016 8:56 PM, David Chisnall wrote:
> On 19 Feb 2016, at 23:23, Dimitry Andric  wrote:
>> 
>> This warning is only produced when you use -Wall -W, and then
>> initialize structs partially, i.e. you initialize some fields but
>> not others.  I think this is a quite reasonable warning for a high
>> warning level.
> 
> The warning is annoying in many ways.  You ought to be able to zero
> initialise any struct with {0}, but clang objects if you do this and
> requires every field to be filled in.  This warning really shouldn’t
> be enabled with -Wall, because it has too hight a false positive
> rate.

For the lay persons among us (I'm genuinely interested), what are the
the downsides to requiring initialization of all fields?

And in addition, the upsides, if any, of 'deferred' field initialization?

Is there a proper term for the above?

> With regard to Bruce’s comment about padding, this is a known issue
> in C11.  There is an open DR about it and it’s scheduled for
> discussion at the WG14 meeting in London in April.
> 
> David
> 

Koobs
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[David Chisnall]

On 19 Feb 2016, at 23:23, Dimitry Andric  wrote:
> 
> This warning is only produced when you use -Wall -W, and then initialize
> structs partially, i.e. you initialize some fields but not others.  I
> think this is a quite reasonable warning for a high warning level.

The warning is annoying in many ways.  You ought to be able to zero initialise 
any struct with {0}, but clang objects if you do this and requires every field 
to be filled in.  This warning really shouldn’t be enabled with -Wall, because 
it has too hight a false positive rate.

With regard to Bruce’s comment about padding, this is a known issue in C11.  
There is an open DR about it and it’s scheduled for discussion at the WG14 
meeting in London in April.

David

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Alan Somers]

On Sun, Feb 21, 2016 at 3:00 PM, Andrew Turner  wrote:
> On Thu, 18 Feb 2016 20:08:01 + (UTC)
> Alan Somers  wrote:
>
>> Author: asomers
>> Date: Thu Feb 18 20:08:01 2016
>> New Revision: 295768
>> URL: https://svnweb.freebsd.org/changeset/base/295768
>>
>> Log:
>>   Fix compiler warnings in iostat
>>
>>   Raise WARNS from 1 to 6 (the default)
>
> This breaks building with gcc:
>

I'll fix it in the morning.  Thanks for reporting it.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Andrew Turner]

On Thu, 18 Feb 2016 20:08:01 + (UTC)
Alan Somers  wrote:

> Author: asomers
> Date: Thu Feb 18 20:08:01 2016
> New Revision: 295768
> URL: https://svnweb.freebsd.org/changeset/base/295768
> 
> Log:
>   Fix compiler warnings in iostat
>   
>   Raise WARNS from 1 to 6 (the default)

This breaks building with gcc:

===> usr.sbin/ifmcstat (all)
===> usr.sbin/iostat (all)
cc1: warnings being treated as errors
/scratch/tmp/andrew/head-git/usr.sbin/iostat/iostat.c: In function 'devstats':
/scratch/tmp/andrew/head-git/usr.sbin/iostat/iostat.c:800: warning: declaration 
of 'devname' shadows a global declaration
/scratch/tmp/andrew/obj/powerpc.powerpc/scratch/tmp/andrew/head-git/tmp/usr/include/stdlib.h:282:
 warning: shadowed declaration is here
/scratch/tmp/andrew/head-git/usr.sbin/iostat/iostat.c: In function 'cpustats':
/scratch/tmp/andrew/head-git/usr.sbin/iostat/iostat.c:982: warning: declaration 
of 'time' shadows a global declaration
/scratch/tmp/andrew/obj/powerpc.powerpc/scratch/tmp/andrew/head-git/tmp/usr/include/time.h:154:
 warning: shadowed declaration is here
*** Error code 1

Stop.
make[4]: stopped in /scratch/tmp/andrew/head-git/usr.sbin/iostat
*** Error code 1

Stop.
make[3]: stopped in /scratch/tmp/andrew/head-git/usr.sbin
*** Error code 1

Stop.
make[2]: stopped in /scratch/tmp/andrew/head-git
*** Error code 1

Stop.
make[1]: stopped in /scratch/tmp/andrew/head-git
*** Error code 1

Stop.
make: stopped in /scratch/tmp/andrew/head-git

-Wshadow is added to the warning flags with WARNS >= 4.

Andrew
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Bruce Evans]

On Sat, 20 Feb 2016, Dimitry Andric wrote:


On 19 Feb 2016, at 16:49, Alan Somers  wrote:


On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov  wrote:

...

-struct nlist namelist[] = {
+static struct nlist namelist[] = {
#define X_TTY_NIN  0
-   { "_tty_nin" },
+   { .n_name = "_tty_nin",
+ .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
[...]


You unlikely need this excessive explicit zeroization.
In this case it is implicitly prezeroed.

...

Yeah, it was being implicitly zeroized before.  But Clang complained
about the structures being only partially initialized.  Since the
whole point of my commit was to increase the WARNS level, I explicitly
zeroed the zero fields to silence Clang.


You got this warning, most likely:

usr.sbin/iostat/iostat.c:122:15: error: missing field 'n_type' initializer 
[-Werror,-Wmissing-field-initializers]
   { "_tty_nin" },
^

This warning is only produced when you use -Wall -W, and then initialize
structs partially, i.e. you initialize some fields but not others.  I
think this is a quite reasonable warning for a high warning level.


No, this more than defeats the reason for existence of C99 initializers.
With C90 initializors, you sometimes had to write initializers for all
the fields between the 2 that you care about, and get the order and number
of al the fields right.  At least you didn't have to write initializers
for the trailing fields.  With compiler warnings about missing initializers,
you also have to write initializers for trailing fields.  I forget if
C99 initializers allows mixing named fields with unnamed fields, but style
rules shouldn't allow it.


On the other hand, if this kind of construct is used throughout the
tree, and it is not seen as a big problem, we can simply silence this
particular warning using -Wno-missing-field -initializers.  There is
already quite a list of warnings which are suppressed by default, even
at WARNS=6, namely:

-Wno-empty-body
-Wno-format-y2k
-Wno-pointer-sign
-Wno-string-plus-int
-Wno-unused-const-variable
-Wno-unused-parameter


We started using it for lists of functions for vfs, where some of the
entries should be optional but all had to be supplied, in the correct
order, for C90 initializers.  These lists were reduced from excessively
dynamic lists, and they ended up with not many optional entries, so
they don't use the feature of omitting optional entries much.

Bruce

Bruce
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Bruce Evans]

On Fri, 19 Feb 2016, Benjamin Kaduk wrote:


On Fri, Feb 19, 2016 at 5:06 PM, Gleb Smirnoff  wrote:


On Fri, Feb 19, 2016 at 08:49:43AM -0700, Alan Somers wrote:
A> On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov 
wrote:
A> Yeah, it was being implicitly zeroized before.  But Clang complained
A> about the structures being only partially initialized.  Since the
A> whole point of my commit was to increase the WARNS level, I explicitly
A> zeroed the zero fields to silence Clang.

Isn't zero filling part of the standard? I don't see why lack of
explicit zeroing is a warning? Looks a false warning to me.


It is not quite as simple as this would make it sound.  The elements or
members of an aggregate (e.g.) structure type are initialized as if it were
an object of static storage duration (i.e., to zero) if the initializer
list does not contain enough initializers for all members of the aggregate
type, per item 21 of section 6.7.8 of n1256.pdf.  However, such
initialization does not necessarily need to zero any padding bytes that are
present, which may take unspecified values.


Perhaps, but then there is even less reason to expect that initializing
all the struct members initialzes the padding between them.


Personally, I think this
particular clang warning can be too aggressive, especially for complex
structs, but on the other hand given the indeterminateness of padding,
bzero/memset are often a better choice anyway.


It is just a bug in clang.

Using auto structs with initialzers, auto structs initialized by memset/
bzero followed by initializing a few fields, are almost equally good
pessimizations.  Some compilers generate similar code for them (like the
code shown by pluknet -- the initializer is optimized to an inlined
memset followed by initializing 1 field.  But doing this this on every
function call is a good pessimization if the struct never changes or
rarely changes or only changes slightly.  Some compilers generate the
worse code of keeping a static copy and generating an inline memcpy to
the auto variable on every function call.

6.7.8p9 says that unnamed members have indeterminate values even after
initialization, except where explicitly stated otherwise.

I couldn't find anywhere "explicitly stating otherwise" that padding
is initialized.  It takes an explicit statements in p21 to get the
unnamed members in an initializer initialized, but padding isn't even.
p21 says that any unnamed struct member is initialized as if it had
static storage duration with no initializer, but I couldn't find
anything requiring initialization for padding in that case.  Apparently,
only quality of implementation prevents it being initialized with
passwords from the previous program :-(.

Bruce
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Conrad Meyer]

On Fri, Feb 19, 2016 at 3:14 PM, Benjamin Kaduk  wrote:
> On Fri, Feb 19, 2016 at 5:06 PM, Gleb Smirnoff  wrote:
>> Isn't zero filling part of the standard? I don't see why lack of
>> explicit zeroing is a warning? Looks a false warning to me.
>
>
> It is not quite as simple as this would make it sound.  The elements or
> members of an aggregate (e.g.) structure type are initialized as if it were
> an object of static storage duration (i.e., to zero) if the initializer list
> does not contain enough initializers for all members of the aggregate type,
> per item 21 of section 6.7.8 of n1256.pdf.  However, such initialization
> does not necessarily need to zero any padding bytes that are present, which
> may take unspecified values.  Personally, I think this particular clang
> warning can be too aggressive, especially for complex structs, but on the
> other hand given the indeterminateness of padding, bzero/memset are often a
> better choice anyway.


By definition, padding byte contents are unused.  There is no reason
their values matter one way or another, so why do we care about the
distinction between bzero and member zero initialization?

Best,
Conrad
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Conrad Meyer]

On Fri, Feb 19, 2016 at 3:06 PM, Gleb Smirnoff  wrote:
> Isn't zero filling part of the standard? I don't see why lack of
> explicit zeroing is a warning? Looks a false warning to me.

Yep.  This is Clang being too whiney, IMO.  I think a good workaround
is to add -Wno-missing-field-initializers to warn flags (but otherwise
keep high WARNS).

Best,
Conrad
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Gleb Smirnoff]

On Sat, Feb 20, 2016 at 12:23:40AM +0100, Dimitry Andric wrote:
D> You got this warning, most likely:
D> 
D> usr.sbin/iostat/iostat.c:122:15: error: missing field 'n_type' initializer 
[-Werror,-Wmissing-field-initializers]
D> { "_tty_nin" },
D>  ^
D> 
D> This warning is only produced when you use -Wall -W, and then initialize
D> structs partially, i.e. you initialize some fields but not others.  I
D> think this is a quite reasonable warning for a high warning level.
D> 
D> On the other hand, if this kind of construct is used throughout the
D> tree, and it is not seen as a big problem, we can simply silence this
D> particular warning using -Wno-missing-field -initializers.  There is
D> already quite a list of warnings which are suppressed by default, even
D> at WARNS=6, namely:
D> 
D> -Wno-empty-body
D> -Wno-format-y2k
D> -Wno-pointer-sign
D> -Wno-string-plus-int
D> -Wno-unused-const-variable
D> -Wno-unused-parameter

I think we should add -Wno-missing-field-initializers there. We use this
feature in the kernel intentionally, since it allows to write code that
is less dependant on API changes.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Dimitry Andric]

On 19 Feb 2016, at 16:49, Alan Somers  wrote:
> 
> On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov  wrote:
...
>>> -struct nlist namelist[] = {
>>> +static struct nlist namelist[] = {
>>> #define X_TTY_NIN  0
>>> -   { "_tty_nin" },
>>> +   { .n_name = "_tty_nin",
>>> + .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
>>> [...]
>> 
>> You unlikely need this excessive explicit zeroization.
>> In this case it is implicitly prezeroed.
...
> Yeah, it was being implicitly zeroized before.  But Clang complained
> about the structures being only partially initialized.  Since the
> whole point of my commit was to increase the WARNS level, I explicitly
> zeroed the zero fields to silence Clang.

You got this warning, most likely:

usr.sbin/iostat/iostat.c:122:15: error: missing field 'n_type' initializer 
[-Werror,-Wmissing-field-initializers]
{ "_tty_nin" },
 ^

This warning is only produced when you use -Wall -W, and then initialize
structs partially, i.e. you initialize some fields but not others.  I
think this is a quite reasonable warning for a high warning level.

On the other hand, if this kind of construct is used throughout the
tree, and it is not seen as a big problem, we can simply silence this
particular warning using -Wno-missing-field -initializers.  There is
already quite a list of warnings which are suppressed by default, even
at WARNS=6, namely:

-Wno-empty-body
-Wno-format-y2k
-Wno-pointer-sign
-Wno-string-plus-int
-Wno-unused-const-variable
-Wno-unused-parameter

-Dimitry



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r295768 - head/usr.sbin/iostat

[Benjamin Kaduk]

On Fri, Feb 19, 2016 at 5:06 PM, Gleb Smirnoff  wrote:

> On Fri, Feb 19, 2016 at 08:49:43AM -0700, Alan Somers wrote:
> A> On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov 
> wrote:
> A> Yeah, it was being implicitly zeroized before.  But Clang complained
> A> about the structures being only partially initialized.  Since the
> A> whole point of my commit was to increase the WARNS level, I explicitly
> A> zeroed the zero fields to silence Clang.
>
> Isn't zero filling part of the standard? I don't see why lack of
> explicit zeroing is a warning? Looks a false warning to me.
>

It is not quite as simple as this would make it sound.  The elements or
members of an aggregate (e.g.) structure type are initialized as if it were
an object of static storage duration (i.e., to zero) if the initializer
list does not contain enough initializers for all members of the aggregate
type, per item 21 of section 6.7.8 of n1256.pdf.  However, such
initialization does not necessarily need to zero any padding bytes that are
present, which may take unspecified values.  Personally, I think this
particular clang warning can be too aggressive, especially for complex
structs, but on the other hand given the indeterminateness of padding,
bzero/memset are often a better choice anyway.

-Ben
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Gleb Smirnoff]

On Fri, Feb 19, 2016 at 08:49:43AM -0700, Alan Somers wrote:
A> On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov  wrote:
A> > On 18 February 2016 at 23:08, Alan Somers  wrote:
A> >> Author: asomers
A> >> Date: Thu Feb 18 20:08:01 2016
A> >> New Revision: 295768
A> >> URL: https://svnweb.freebsd.org/changeset/base/295768
A> >>
A> >> Log:
A> >>   Fix compiler warnings in iostat
A> >
A> >> Modified: head/usr.sbin/iostat/iostat.c
A> >> 
==
A> >> --- head/usr.sbin/iostat/iostat.c   Thu Feb 18 19:37:39 2016
(r295767)
A> >> +++ head/usr.sbin/iostat/iostat.c   Thu Feb 18 20:08:01 2016
(r295768)
A> >> @@ -117,30 +117,34 @@
A> >>  #include 
A> >>  #include 
A> >>
A> >> -struct nlist namelist[] = {
A> >> +static struct nlist namelist[] = {
A> >>  #define X_TTY_NIN  0
A> >> -   { "_tty_nin" },
A> >> +   { .n_name = "_tty_nin",
A> >> + .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
A> >>  [...]
A> >
A> > You unlikely need this excessive explicit zeroization.
A> > In this case it is implicitly prezeroed.
A> >
A> > Consider these two cases:
A> >
A> > : #include 
A> > :
A> > : int main(void) {
A> > :   struct nlist namelist[2] = {{ .n_type = 0x42 }};
A> > :   return sizeof(namelist);
A> > : }
A> >
A> > (__TEXT,__text) section
A> > _main:
A> > pushq%rbp
A> > 0001movq%rsp, %rbp
A> > 0004leaq-0x30(%rbp), %rdx
A> > 0008movl$0x0, %eax
A> > 000dmovl$0x6, %ecx
A> > 0012movq%rdx, %rdi
A> > 0015rep
A> > 0016stosq
A> > 0018movb$0x42, -0x28(%rbp)
A> > 001cmovl$0x30, %eax
A> > 0021popq%rbp
A> > 0022retq
A> >
A> > rep stosq does zero 48 bytes, that is namelist[].
A> >
A> > Or, if it is static.
A> >
A> > : #include 
A> > :
A> > : int main(void) {
A> > :   static struct nlist namelist[2] = {{ .n_type = 0x42 }};
A> > :   return sizeof(namelist);
A> > : }
A> >
A> > (__DATA,__data) section
A> > 002000 00 00 00 00 00 00 00 42 00 00 00 00 00 00 00
A> > 003000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
A> > 004000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
A> 
A> Yeah, it was being implicitly zeroized before.  But Clang complained
A> about the structures being only partially initialized.  Since the
A> whole point of my commit was to increase the WARNS level, I explicitly
A> zeroed the zero fields to silence Clang.

Isn't zero filling part of the standard? I don't see why lack of
explicit zeroing is a warning? Looks a false warning to me.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Alan Somers]

On Fri, Feb 19, 2016 at 5:24 AM, Sergey Kandaurov  wrote:
> On 18 February 2016 at 23:08, Alan Somers  wrote:
>> Author: asomers
>> Date: Thu Feb 18 20:08:01 2016
>> New Revision: 295768
>> URL: https://svnweb.freebsd.org/changeset/base/295768
>>
>> Log:
>>   Fix compiler warnings in iostat
>
>> Modified: head/usr.sbin/iostat/iostat.c
>> ==
>> --- head/usr.sbin/iostat/iostat.c   Thu Feb 18 19:37:39 2016
>> (r295767)
>> +++ head/usr.sbin/iostat/iostat.c   Thu Feb 18 20:08:01 2016
>> (r295768)
>> @@ -117,30 +117,34 @@
>>  #include 
>>  #include 
>>
>> -struct nlist namelist[] = {
>> +static struct nlist namelist[] = {
>>  #define X_TTY_NIN  0
>> -   { "_tty_nin" },
>> +   { .n_name = "_tty_nin",
>> + .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
>>  [...]
>
> You unlikely need this excessive explicit zeroization.
> In this case it is implicitly prezeroed.
>
> Consider these two cases:
>
> : #include 
> :
> : int main(void) {
> :   struct nlist namelist[2] = {{ .n_type = 0x42 }};
> :   return sizeof(namelist);
> : }
>
> (__TEXT,__text) section
> _main:
> pushq%rbp
> 0001movq%rsp, %rbp
> 0004leaq-0x30(%rbp), %rdx
> 0008movl$0x0, %eax
> 000dmovl$0x6, %ecx
> 0012movq%rdx, %rdi
> 0015rep
> 0016stosq
> 0018movb$0x42, -0x28(%rbp)
> 001cmovl$0x30, %eax
> 0021popq%rbp
> 0022retq
>
> rep stosq does zero 48 bytes, that is namelist[].
>
> Or, if it is static.
>
> : #include 
> :
> : int main(void) {
> :   static struct nlist namelist[2] = {{ .n_type = 0x42 }};
> :   return sizeof(namelist);
> : }
>
> (__DATA,__data) section
> 002000 00 00 00 00 00 00 00 42 00 00 00 00 00 00 00
> 003000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 004000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Yeah, it was being implicitly zeroized before.  But Clang complained
about the structures being only partially initialized.  Since the
whole point of my commit was to increase the WARNS level, I explicitly
zeroed the zero fields to silence Clang.

-Alan
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r295768 - head/usr.sbin/iostat

[Sergey Kandaurov]

On 18 February 2016 at 23:08, Alan Somers  wrote:
> Author: asomers
> Date: Thu Feb 18 20:08:01 2016
> New Revision: 295768
> URL: https://svnweb.freebsd.org/changeset/base/295768
>
> Log:
>   Fix compiler warnings in iostat

> Modified: head/usr.sbin/iostat/iostat.c
> ==
> --- head/usr.sbin/iostat/iostat.c   Thu Feb 18 19:37:39 2016
> (r295767)
> +++ head/usr.sbin/iostat/iostat.c   Thu Feb 18 20:08:01 2016
> (r295768)
> @@ -117,30 +117,34 @@
>  #include 
>  #include 
>
> -struct nlist namelist[] = {
> +static struct nlist namelist[] = {
>  #define X_TTY_NIN  0
> -   { "_tty_nin" },
> +   { .n_name = "_tty_nin",
> + .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
>  [...]

You unlikely need this excessive explicit zeroization.
In this case it is implicitly prezeroed.

Consider these two cases:

: #include 
:
: int main(void) {
:   struct nlist namelist[2] = {{ .n_type = 0x42 }};
:   return sizeof(namelist);
: }

(__TEXT,__text) section
_main:
pushq%rbp
0001movq%rsp, %rbp
0004leaq-0x30(%rbp), %rdx
0008movl$0x0, %eax
000dmovl$0x6, %ecx
0012movq%rdx, %rdi
0015rep
0016stosq
0018movb$0x42, -0x28(%rbp)
001cmovl$0x30, %eax
0021popq%rbp
0022retq

rep stosq does zero 48 bytes, that is namelist[].

Or, if it is static.

: #include 
:
: int main(void) {
:   static struct nlist namelist[2] = {{ .n_type = 0x42 }};
:   return sizeof(namelist);
: }

(__DATA,__data) section
002000 00 00 00 00 00 00 00 42 00 00 00 00 00 00 00
003000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
004000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

-- 
wbr,
pluknet
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r295768 - head/usr.sbin/iostat

[Alan Somers]

Author: asomers
Date: Thu Feb 18 20:08:01 2016
New Revision: 295768
URL: https://svnweb.freebsd.org/changeset/base/295768

Log:
  Fix compiler warnings in iostat
  
  Raise WARNS from 1 to 6 (the default)
  Fix warnings:
  * Use C99 designated initializers for structs, and initialize all fields
  * Mark global variables as static
  * Mark unused function arguments
  * Be careful about signed/unsigned comparisons
  
  Reviewed by:  eadler
  MFC after:4 weeks
  Sponsored by: Spectra Logic Corp
  Differential Revision:https://reviews.freebsd.org/D5328

Modified:
  head/usr.sbin/iostat/Makefile
  head/usr.sbin/iostat/iostat.c

Modified: head/usr.sbin/iostat/Makefile
==
--- head/usr.sbin/iostat/Makefile   Thu Feb 18 19:37:39 2016
(r295767)
+++ head/usr.sbin/iostat/Makefile   Thu Feb 18 20:08:01 2016
(r295768)
@@ -6,6 +6,4 @@ MAN=iostat.8
 
 LIBADD=devstat kvm m
 
-WARNS?=1
-
 .include 

Modified: head/usr.sbin/iostat/iostat.c
==
--- head/usr.sbin/iostat/iostat.c   Thu Feb 18 19:37:39 2016
(r295767)
+++ head/usr.sbin/iostat/iostat.c   Thu Feb 18 20:08:01 2016
(r295768)
@@ -117,30 +117,34 @@
 #include 
 #include 
 
-struct nlist namelist[] = {
+static struct nlist namelist[] = {
 #define X_TTY_NIN  0
-   { "_tty_nin" },
+   { .n_name = "_tty_nin",
+ .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
 #define X_TTY_NOUT 1
-   { "_tty_nout" },
+   { .n_name = "_tty_nout",
+ .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
 #define X_BOOTTIME 2
-   { "_boottime" },
+   { .n_name = "_boottime",
+ .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
 #define X_END  2
-   { NULL },
+   { .n_name = NULL,
+ .n_type = 0, .n_other = 0, .n_desc = 0, .n_value = 0 },
 };
 
 #defineIOSTAT_DEFAULT_ROWS 20  /* Traditional default `wrows' 
*/
 
-struct statinfo cur, last;
-int num_devices;
-struct device_selection *dev_select;
-int maxshowdevs;
-volatile sig_atomic_t headercount;
-volatile sig_atomic_t wresized;/* Tty resized, when non-zero. 
*/
-volatile sig_atomic_t alarm_rang;
-volatile sig_atomic_t return_requested;
-unsigned short wrows;  /* Current number of tty rows. */
-int dflag = 0, Iflag = 0, Cflag = 0, Tflag = 0, oflag = 0, Kflag = 0;
-int xflag = 0, zflag = 0;
+static struct statinfo cur, last;
+static int num_devices;
+static struct device_selection *dev_select;
+static int maxshowdevs;
+static volatile sig_atomic_t headercount;
+static volatile sig_atomic_t wresized; /* Tty resized, when non-zero. */
+static volatile sig_atomic_t alarm_rang;
+static volatile sig_atomic_t return_requested;
+static unsigned short wrows;   /* Current number of tty rows. */
+static int dflag = 0, Iflag = 0, Cflag = 0, Tflag = 0, oflag = 0, Kflag = 0;
+static int xflag = 0, zflag = 0;
 
 /* local function declarations */
 static void usage(void);
@@ -650,7 +654,7 @@ main(int argc, char **argv)
  * Force a header to be prepended to the next output.
  */
 void
-needhdr(int signo)
+needhdr(int signo __unused)
 {
 
headercount = 1;
@@ -662,7 +666,7 @@ needhdr(int signo)
  * prepended to the next output.
  */
 void
-needresize(int signo)
+needresize(int signo __unused)
 {
 
wresized = 1;
@@ -673,7 +677,7 @@ needresize(int signo)
  * Record the alarm so the main loop can break its sleep
  */
 void
-alarm_clock(int signo)
+alarm_clock(int signo __unused)
 {
alarm_rang = 1;
 }
@@ -682,7 +686,7 @@ alarm_clock(int signo)
  * Request that the main loop exit soon
  */
 void
-needreturn(int signo)
+needreturn(int signo __unused)
 {
return_requested = 1;
 }
@@ -998,8 +1002,7 @@ readvar(kvm_t *kd, const char *name, int
warnx("kvm_read(%s): %s", namelist[nlid].n_name,
kvm_geterr(kd));
return (1);
-   }
-   if (nbytes != len) {
+   } else if ((size_t)nbytes != len) {
warnx("kvm_read(%s): expected %zu bytes, got %zd bytes",
  namelist[nlid].n_name, len, nbytes);
return (1);
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"