Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On Fri, Oct 6, 2017 at 11:04 AM Roy Marples wrote: > On 05/10/2017 23:14, Kevin Lyda wrote: > > That's not actually correct in practice. > And top posting is? > Yes. It is. We lost that battle, sorry. If it worries you, t-prot is a thing. Interesting that you assume I use bash and gcc. > This is more portable. > for f in foo bar; do cc -S $f.c; done > I use zsh actually, and you figured out what I meant. Neither version will work in csh style shells. And an odd complaint about a shortcut here, since later you claim more text makes it harder to read. Also I specified gcc specifically because I had only tested with gcc - though from past experience I know modern compilers like clang, icc and others also do such optimisation. Also you removed the -O2 that I had in there. > Now, if I do apply -O2 then clang (my system compiler) does indeed > optimise it away. > What is worring is that even with -O0 or no -O gcc *always* optimises it > away. What if I want to call the function to be called regardless? > Ugh, and my tests with gcc -O0 were actually with clang because Apple links gcc to clang. Checking gcc -O0 on Linux shows that it does optimize even at -O2. Right, so to get foo.c to actually call strlen with gcc: gcc -fno-builtin-strlen -S foo.c or more generically: gcc -fno-builtin -S foo.c > Spoken like a man who has never dealt with compiler issues! > A rather funny comment since I'm currently dealing with multiple compiler issues. if (strlen(foo) != 0) > if the function to calculate the length of the string is not zero > > if (*foo != '\0') > if the first character of the string is not NUL > > I'm having a hard time beliving that the former is more readable AND > just as performant. > It has more text for the brain to digest and a man page to read per > platform. > Where "more text" is four characters and replaces symbols for words that are easy for the eye to skim. Maybe I've lead a sheltered career for the past three decades, but I don't really recall a lot of APL shops in my travels. Or are you going to assume that gcc is the only compiler ever used anywhere? > Nope. Never do. Though I do like it a great deal more than VMS's C compiler. I do wish Apple wouldn't point gcc at clang though. Regardless, the dnsmasq Makefile sets -O2 in its CFLAGS so gcc or clang or, as I said previously, any modern C compiler, it will optimise out the strlen call and the strlen call is easier to read. Kevin ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On 05/10/2017 23:14, Kevin Lyda wrote:
> That's not actually correct in practice.
And top posting is?
> If you'd like to see that I'm
> correct take the following two programs:
>
> foo.c:
> #include
> #include
>
> int main(int argc, char *argv[]) {
> if (strlen(argv[0]) == 0) {
> printf("Command empty");
> } else {
> printf("Command not empty");
> }
> }
> bar.c:
> #include
> #include
>
> int main(int argc, char *argv[]) {
> if (*argv[0] == '\0') {
> printf("Command empty");
> } else {
> printf("Command not empty");
> }
> }
>
> Next compile them to assembly like so:
>
> for f in {foo,bar}.c; do gcc -O2 -S $f; done
Interesting that you assume I use bash and gcc.
This is more portable.
for f in foo bar; do cc -S $f.c; done
> And then compare them:
>
> diff {foo,bar}.s
>
> They should be the same (possibly different by a ".file" line). And if
> you inspect the resulting code, there won't be a call to strlen or to
> any function at all (well, except printf).
netbsd$ diff foo.s bar.s
2c2
< .file "foo.c"
---
> .file "bar.c"
22,24c22,24
< movq(%rsi), %rdi
< callq strlen
< cmpq$0, %rax
---
> movq(%rsi), %rsi
> movsbl (%rsi), %edi
> cmpl$0, %edi
Now, if I do apply -O2 then clang (my system compiler) does indeed
optimise it away.
What is worring is that even with -O0 or no -O gcc *always* optimises it
away. What if I want to call the function to be called regardless?
> The reason is that gcc, like most C compilers since the 90s, optimises a
> number of common functions in the standard C library. Which means that
> developers can stick with writing code that will be well-optimised *and*
> highly readable.
Spoken like a man who has never dealt with compiler issues!
if (strlen(foo) != 0)
if the function to calculate the length of the string is not zero
if (*foo != '\0')
if the first character of the string is not NUL
I'm having a hard time beliving that the former is more readable AND
just as performant.
It has more text for the brain to digest and a man page to read per
platform.
Or are you going to assume that gcc is the only compiler ever used anywhere?
Roy
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
That's not actually correct in practice. If you'd like to see that I'm
correct take the following two programs:
foo.c:
#include
#include
int main(int argc, char *argv[]) {
if (strlen(argv[0]) == 0) {
printf("Command empty");
} else {
printf("Command not empty");
}
}
bar.c:
#include
#include
int main(int argc, char *argv[]) {
if (*argv[0] == '\0') {
printf("Command empty");
} else {
printf("Command not empty");
}
}
Next compile them to assembly like so:
for f in {foo,bar}.c; do gcc -O2 -S $f; done
And then compare them:
diff {foo,bar}.s
They should be the same (possibly different by a ".file" line). And if you
inspect the resulting code, there won't be a call to strlen or to any
function at all (well, except printf).
The reason is that gcc, like most C compilers since the 90s, optimises a
number of common functions in the standard C library. Which means that
developers can stick with writing code that will be well-optimised *and*
highly readable.
Kevin
On Thu, Oct 5, 2017 at 8:31 PM Roy Marples wrote:
> On 05/10/2017 03:23, Rosen Penev wrote:
> > @@ -1239,7 +1238,7 @@ static struct serverfd *allocate_sfd(union
> mysockaddr *addr, char *intname)
> > #endif
> > }
> >
> > - if (intname && strlen(intname) != 0)
> > + if (!strlen(intname))
> > ifindex = if_nametoindex(intname); /* index == 0 when not binding
> to an interface */
> >
> > /* may have a suitable one already */
> >
>
> I have no comment on the functionality of the patch (it if intname needs
> to be NULL checked or not), but this is not a good use of strlen.
>
> This could be re-written as
>
> if (*intname != '\0')
>
> Which saves a function call because the actual length of the string is
> not used.
>
> Roy
>
> ___
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On 05/10/2017 03:23, Rosen Penev wrote: @@ -1239,7 +1238,7 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname) #endif } - if (intname && strlen(intname) != 0) + if (!strlen(intname)) ifindex = if_nametoindex(intname); /* index == 0 when not binding to an interface */ /* may have a suitable one already */ I have no comment on the functionality of the patch (it if intname needs to be NULL checked or not), but this is not a good use of strlen. This could be re-written as if (*intname != '\0') Which saves a function call because the actual length of the string is not used. Roy ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On Wed, Oct 04, 2017 at 10:20:24PM -0700, ros...@gmail.com wrote: > > Compile time. Looks fine to me. To add to my commit message, > strlen(NULL) causes a segmentation fault, meaning intname cannot be > NULL. > > Actually I wonder how many more warnings I can find like this... Probably a lot, but you should stop. Forgive me, but this is code you don't appear to understand and have not even tested. Some of it introduces nasty side effects and some of it even outright inverts logic. Blindly pasting static analysis output is not how programs are maintained, but it does introduce a lot of work for those of us who care sufficiently to review the code. This code introduces bugs and does not even have the grace to improve functionality in the process. khm ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On 05/10/17 06:20, ros...@gmail.com wrote: On Wed, 2017-10-04 at 20:43 -0700, Kurt H Maier wrote: On Wed, Oct 04, 2017 at 07:23:22PM -0700, Rosen Penev wrote: - if (intname && strlen(intname) != 0) + if (!strlen(intname)) ifindex = if_nametoindex(intname); /* index == 0 when not binding to an interface */ How much testing have you done of these patches? Compile time. Looks fine to me. To add to my commit message, strlen(NULL) causes a segmentation fault, meaning intname cannot be NULL. Which is precisely why the 'intname' guard is there. C evaluation order is left to right, if 'intname' is NULL then evaluation stops there strlen is never called with a NULL intname. Kevin ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On Wed, 2017-10-04 at 20:43 -0700, Kurt H Maier wrote: > On Wed, Oct 04, 2017 at 07:23:22PM -0700, Rosen Penev wrote: > > > > - if (intname && strlen(intname) != 0) > > + if (!strlen(intname)) > > ifindex = if_nametoindex(intname); /* index == 0 when not > > binding to an interface */ > > How much testing have you done of these patches? > Compile time. Looks fine to me. To add to my commit message, strlen(NULL) causes a segmentation fault, meaning intname cannot be NULL. Actually I wonder how many more warnings I can find like this... > khm > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Remove NULL check for intname.
On Wed, Oct 04, 2017 at 07:23:22PM -0700, Rosen Penev wrote: > > - if (intname && strlen(intname) != 0) > + if (!strlen(intname)) > ifindex = if_nametoindex(intname); /* index == 0 when not binding to an > interface */ How much testing have you done of these patches? khm ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
