Re: CVS commit: src/sys/kern
On 02.08.2020 17:50, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 17:35:06 +0200
>> From: Kamil Rytarowski
>>
>> On 02.08.2020 16:44, Taylor R Campbell wrote:
Date: Sun, 2 Aug 2020 16:04:15 +0200
From: Kamil Rytarowski
On 02.08.2020 15:57, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.
This is not true:
>>>
>>> Which part of what I said are you claiming is not true, and what are
>>> you illustrating with the example program below?
>>
>> Calling it a compiler bug.
>>
>> Clang behaves the same way.
>>
>> $ clang -Wvla test.c
>> test.c:6:37: warning: variable length array used [-Wvla]
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>>^
>> 1 warning generated.
>>
>> Creating VLA is not needed for using it as an intermediate. In practice
>> in most/all cases it is optimized and actual VLA is not allocated.
>
> This is not a matter of optimization in practice. It's absolutely not
> an `intermediate' at all -- there is no VLA created, period, any more
> than sizeof(malloc(1)) causes any actual call to malloc or sizeof(*p)
> causes any dereference of a pointer.
>
> This makes -Wvla less useful as a tool, because apparently it flags
> code that unquestionably does not have any bad effects of VLAs. This
> happens because -Wvla is dumb -- it just looks for the syntax, not for
> the semantics of creating VLAs. That's why I call it a bug -- it
> raises a false positive that makes it less useful.
>
Compilers warns about VLA using ("variable length array used"), not
creating. There is no distinct compiler warning to discriminate VLA
creating from usage.
Anyway, code just using VLA is not that frequent, avoiding it is rather
simple and VLA can be avoided altogether (at least for non-external).
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
> Date: Sun, 2 Aug 2020 17:35:06 +0200
> From: Kamil Rytarowski
>
> On 02.08.2020 16:44, Taylor R Campbell wrote:
> >> Date: Sun, 2 Aug 2020 16:04:15 +0200
> >> From: Kamil Rytarowski
> >>
> >> On 02.08.2020 15:57, Taylor R Campbell wrote:
> >>> But it sounds like the original motivation is that it triggered
> >>> -Wvla...which frankly strikes me as a compiler bug since there's
> >>> obviously no actual VLA created in sizeof; as far as I can tell
> >>> there's no semantic difference between sizeof(device_t[n]) and
> >>> sizeof(device_t) * n.
> >>
> >> This is not true:
> >
> > Which part of what I said are you claiming is not true, and what are
> > you illustrating with the example program below?
>
> Calling it a compiler bug.
>
> Clang behaves the same way.
>
> $ clang -Wvla test.c
> test.c:6:37: warning: variable length array used [-Wvla]
> printf("sizeof = %zu\n", sizeof(int[argc]));
>^
> 1 warning generated.
>
> Creating VLA is not needed for using it as an intermediate. In practice
> in most/all cases it is optimized and actual VLA is not allocated.
This is not a matter of optimization in practice. It's absolutely not
an `intermediate' at all -- there is no VLA created, period, any more
than sizeof(malloc(1)) causes any actual call to malloc or sizeof(*p)
causes any dereference of a pointer.
This makes -Wvla less useful as a tool, because apparently it flags
code that unquestionably does not have any bad effects of VLAs. This
happens because -Wvla is dumb -- it just looks for the syntax, not for
the semantics of creating VLAs. That's why I call it a bug -- it
raises a false positive that makes it less useful.
Re: CVS commit: src/sys/kern
On 02.08.2020 16:44, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 16:04:15 +0200
>> From: Kamil Rytarowski
>>
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>
>> This is not true:
>
> Which part of what I said are you claiming is not true, and what are
> you illustrating with the example program below?
>
Calling it a compiler bug.
Clang behaves the same way.
$ clang -Wvla test.c
test.c:6:37: warning: variable length array used [-Wvla]
printf("sizeof = %zu\n", sizeof(int[argc]));
^
1 warning generated.
Creating VLA is not needed for using it as an intermediate. In practice
in most/all cases it is optimized and actual VLA is not allocated.
> It seems to illustrate that sizeof(int[argc]) does exactly what one
> would expect it to do -- return the size of an argc-length array of
> ints, just like sizeof(int) * argc does.
>
>
The result is the same and I find the change as beneficial.
>
>> #include
>>
>> int
>> main(int argc, char **argv)
>> {
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>> return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
Le dim. 2 août 2020 à 15:57, Taylor R Campbell a écrit : > Why does it improve readability? Certainly using cringe language features does not help readability. > What else does -Wvla choke on in src/sys? Some drm drivers, and uvm, particularly uvm_bio.c. I'd like to work towards enabling -Wvla in kernel (i.e. remove all VLAs, same as Linux kernel did in 2018), but I have some other stuff I want to finish before I pick a new project. Jaromir
Re: CVS commit: src/sys/kern
> Date: Sun, 2 Aug 2020 16:04:15 +0200
> From: Kamil Rytarowski
>
> On 02.08.2020 15:57, Taylor R Campbell wrote:
> > But it sounds like the original motivation is that it triggered
> > -Wvla...which frankly strikes me as a compiler bug since there's
> > obviously no actual VLA created in sizeof; as far as I can tell
> > there's no semantic difference between sizeof(device_t[n]) and
> > sizeof(device_t) * n.
>
> This is not true:
Which part of what I said are you claiming is not true, and what are
you illustrating with the example program below?
It seems to illustrate that sizeof(int[argc]) does exactly what one
would expect it to do -- return the size of an argc-length array of
ints, just like sizeof(int) * argc does.
> #include
>
> int
> main(int argc, char **argv)
> {
> printf("sizeof = %zu\n", sizeof(int[argc]));
> return 0;
> }
>
> $ ./a.out
>
> sizeof = 4
> $ ./a.out 12 3
> sizeof = 12
> $ ./a.out 12 3 45 6
> sizeof = 20
Re: CVS commit: src/sys/kern
On Sun, Aug 02, 2020 at 01:57:11PM +, Taylor R Campbell wrote: > But it sounds like the original motivation is that it triggered > -Wvla...which frankly strikes me as a compiler bug since there's > obviously no actual VLA created in sizeof; as far as I can tell > there's no semantic difference between sizeof(device_t[n]) and > sizeof(device_t) * n. Yes, it seems strange to trigger a VLA warning in code that it is required to be compile-time evaluated. Joerg
Re: CVS commit: src/sys/kern
On 02.08.2020 16:25, Paul Goyette wrote:
> On Sun, 2 Aug 2020, Kamil Rytarowski wrote:
>
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>>
>>
>> This is not true:
>>
>> #include
>>
>> int
>> main(int argc, char **argv)
>> {
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>> return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20
>
> Modifying your example slightly, I print both variations:
>
> #include
>
> int
> main(int argc, char **argv)
> {
> printf("sizeof = %zu\t%zu\n", sizeof(int[argc]), sizeof(int) * argc);
> return 0;
> }
> speedy:paul {653} ./a.out
> sizeof = 4 4
> speedy:paul {654} ./a.out 12 3
> sizeof = 12 12
> speedy:paul {655} ./a.out 12 3 45 6
> sizeof = 20 20
>
>
> Looks the same to me!
>
The result is the same, but one uses VLA (at least formally), other not.
>
> ++--+---+
> | Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
> | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
> | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
> ++--+---+
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Sun, 2 Aug 2020, Kamil Rytarowski wrote:
On 02.08.2020 15:57, Taylor R Campbell wrote:
But it sounds like the original motivation is that it triggered
-Wvla...which frankly strikes me as a compiler bug since there's
obviously no actual VLA created in sizeof; as far as I can tell
there's no semantic difference between sizeof(device_t[n]) and
sizeof(device_t) * n.
This is not true:
#include
int
main(int argc, char **argv)
{
printf("sizeof = %zu\n", sizeof(int[argc]));
return 0;
}
$ ./a.out
sizeof = 4
$ ./a.out 12 3
sizeof = 12
$ ./a.out 12 3 45 6
sizeof = 20
Modifying your example slightly, I print both variations:
#include
int
main(int argc, char **argv)
{
printf("sizeof = %zu\t%zu\n", sizeof(int[argc]), sizeof(int) * argc);
return 0;
}
speedy:paul {653} ./a.out
sizeof = 4 4
speedy:paul {654} ./a.out 12 3
sizeof = 12 12
speedy:paul {655} ./a.out 12 3 45 6
sizeof = 20 20
Looks the same to me!
++--+---+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
++--+---+
Re: CVS commit: src/sys/kern
On 02.08.2020 15:57, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.
>
This is not true:
#include
int
main(int argc, char **argv)
{
printf("sizeof = %zu\n", sizeof(int[argc]));
return 0;
}
$ ./a.out
sizeof = 4
$ ./a.out 12 3
sizeof = 12
$ ./a.out 12 3 45 6
sizeof = 20
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
> Date: Sun, 2 Aug 2020 10:47:21 +0200 > From: Jarom�r Dole ek > > Readability first and foremost in this case. > > I was exploring if I can disable VLAs for the kernel altogether, this > can't be done for now. Nevertheless, this change looked like it would > be useful to make anyway. Why does it improve readability? Surely if we want the size of an array of device_t of length n it's more to the point to say sizeof(device_t[n]) directly than to talk about multiplying sizeof(device_t) by n. If that were the only question I think the change makes readability worse, personally! But it sounds like the original motivation is that it triggered -Wvla...which frankly strikes me as a compiler bug since there's obviously no actual VLA created in sizeof; as far as I can tell there's no semantic difference between sizeof(device_t[n]) and sizeof(device_t) * n. What else does -Wvla choke on in src/sys?
Re: CVS commit: src/usr.bin/make
On 02.08.2020 13:06, Simon Burge wrote: > "Roland Illig" wrote: > >> Module Name: src >> Committed By:rillig >> Date:Sun Aug 2 09:43:22 UTC 2020 >> >> Modified Files: >> >> src/usr.bin/make: var.c >> >> Log Message: >> >> make(1): use shorter local variable names >> >> The c in cp was redundant since the context makes it obvious that this >> is a character pointer. In a tight loop where lots of characters are >> compared, every letter counts. > > I don't understand the intent of this commit. Are you saying the length > of a C variable name has some sort of impact on code execution speed? No, I know that it has absolutely no influence on execution speed. My concern was about the ease of reading by humans. In var.c, there are several functions that do low-level parsing by reading the input byte by byte and comparing the current character. These functions are ParseModifierPart, ApplyModifier_Defined, ApplyModifier_Match, ModifyWord_SubstRegex. In these functions, there are often 3 to 4 comparisons in a single line. The comparisons all have the same left-hand side, so that expression becomes uninteresting to the reader and should therefore be as short as possible, to leave room for the more interesting details, which are the right-hand sides of the comparisons. Therefore I renamed the cp to p, since the c did not add any valuable information to the code. It's pretty obvious that in a comparison like *p == '$', the p must be a pointer to a character. Only in these very tight loops, and only because these have only a single interesting pointer, do I prefer this short variable name. For comparison, in ApplyModifier_Match, in the second big loop, I named the variables src and dst, to keep them apart. Another reason for renaming cp and cp2 is that the make(1) source code uses these names in a lot of places, and these names do not add any valuable information to the reader, while a more expressive variable name could do that very well. Therefore I'm generally against this variable name, especially when these variables are used as general-purpose storage that serves 3 or 4 completely distinct purposes during its lifetime, happily mixing between storing const char * and char * and maybe a void * in between, followed by UNCONST to free the cp at the end. How should a casual reader know at the end what exactly is freed here? It's obviously cp, but what does this mean? In summary: By the variable name p, I mean a moving pointer in a parsing function, therefore it's usually the only moving pointer in that function. I'm using that variable name consistently for this single purpose. This is unlike the original make(1) code, which uses cp and cp2 for everything. I notice I'm getting too much into rant mode right now, therefore I'd rather stop here. :) Roland
Re: CVS commit: src/usr.bin/make
"Roland Illig" wrote: > Module Name: src > Committed By: rillig > Date: Sun Aug 2 09:43:22 UTC 2020 > > Modified Files: > > src/usr.bin/make: var.c > > Log Message: > > make(1): use shorter local variable names > > The c in cp was redundant since the context makes it obvious that this > is a character pointer. In a tight loop where lots of characters are > compared, every letter counts. I don't understand the intent of this commit. Are you saying the length of a C variable name has some sort of impact on code execution speed? Cheers, Simon.
Re: CVS commit: src/usr.bin/make
On Sun, 2 Aug 2020, Roland Illig wrote: Module Name:src Committed By: rillig Date: Sun Aug 2 09:43:22 UTC 2020 Modified Files: src/usr.bin/make: var.c Log Message: make(1): use shorter local variable names The c in cp was redundant since the context makes it obvious that this is a character pointer. In a tight loop where lots of characters are compared, every letter counts. I don't follow the rationale here. Can you expand on this?
Re: CVS commit: src/sys/kern
Readability first and foremost in this case. I was exploring if I can disable VLAs for the kernel altogether, this can't be done for now. Nevertheless, this change looked like it would be useful to make anyway. Le dim. 2 août 2020 à 01:15, Taylor R Campbell a écrit : > > > Module Name:src > > Committed By: jdolecek > > Date: Sat Aug 1 11:18:26 UTC 2020 > > > > Modified Files: > > src/sys/kern: subr_autoconf.c > > > > Log Message: > > avoid VLA for the sizeof() calculations > > Why?
Re: CVS commit: src/share/misc
On Sat, Aug 1, 2020, 6:26 PM Luke Mewburn wrote:
> On 20-08-01 23:07, Taylor R Campbell wrote:
> | Index: share/misc/style
> | ===
> | RCS file: /cvsroot/src/share/misc/style,v
> | retrieving revision 1.56
> | diff -p -p -u -r1.56 style
> | --- share/misc/style1 Aug 2020 02:45:35 - 1.56
> | +++ share/misc/style1 Aug 2020 22:54:53 -
> | @@ -241,9 +241,8 @@ main(int argc, char *argv[])
> | errno = 0;
> | num = strtol(optarg, , 10);
> | if (num <= 0 || *ep != '\0' || (errno == ERANGE &&
> | - (num == LONG_MAX || num == LONG_MIN)) ) {
> | + (num == LONG_MAX || num == LONG_MIN)) )
> | errx(1, "illegal number -- %s", optarg);
> | - }
> | break;
>
> IMO, that example is a case in where if the style is "minimal braces"
> that's still a good case to retain it. The if condition is across
> multiple lines, and the brakes make it clearer (to me) where the statement
> is.
>
> This all comes down to a matter of style, where some people
> prefer "purple" and some prefer "orange", yet the arguments are
> often claimed to be (by all concerned) as "technical reasons".
>
I thought the only two color choices were green and purple given the Bab 5
heritage of the project...
Warner
Anyway, I haven't got the motivation to bikeshed like this in NetBSD
> anymore.
>
> Luke.
>
