Xen devel - Oct 2005 - [PATCH] Cleanup use of strlen() to check for empty string

I came across checks for strlen(s)==0 a few places in drivers/xen.
Here is a patch to fix that.

Jacob
--
Save time and bandwidth with EDelta: http://www.diku.dk/~jacobg/edelta/


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 4 Oct 2005, at 23:50, Jacob Gorm Hansen wrote:
> I came across checks for strlen(s)==0 a few places in drivers/xen.
> Here is a patch to fix that.
Isn''t your patch exactly equivalent? None of that code is time critical
enough that a call to strlen matters.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/5/05, Keir Fraser <Keir.Fraser@cl.cam.ac.uk>
wrote:
>
> On 4 Oct 2005, at 23:50, Jacob Gorm Hansen wrote:
>
> > I came across checks for strlen(s)==0 a few places in drivers/xen.
> > Here is a patch to fix that.
>
> Isn''t your patch exactly equivalent? None of that code is time
critical
> enough that a call to strlen matters.
It''s just gross (looping over the entire length of a string just to
see if the first char is empty) that''s all. Apply if you will.

Jacob

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com 
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of 
> Jacob Gorm Hansen
> Sent: 05 October 2005 11:45
> To: Keir Fraser; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Cleanup use of strlen() to 
> check for empty string
> 
> On 10/5/05, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> >
> > On 4 Oct 2005, at 23:50, Jacob Gorm Hansen wrote:
> >
> > > I came across checks for strlen(s)==0 a few places in
drivers/xen.
> > > Here is a patch to fix that.
> >
> > Isn''t your patch exactly equivalent? None of that code is
time
> > critical enough that a call to strlen matters.
> 
> It''s just gross (looping over the entire length of a string 
> just to see if the first char is empty) that''s all. Apply if you
will.
I can see the argument both ways. A statement containing (strlen(s) =0) is very
obvious what the meaning is. The compiler may even optimise
it into the form you''ve done (if it''s clever enough).

On the other hand, if the compiler doesn''t understand that (strlen(s)
=0) is the same as (*s == 0), the code you''ve supplied would generate
shorter code (and run faster), which considering the number of calls to
strlen in the patch would give a decent benefit in code-size, and since
the code isn''t speed critical (according to Keir), size is of more
benefit than speed.  

I created a function:

int foo(char *s)
{
	if (strlen(s)) return 1; else return 0;
}

Compiled with gcc 4.0.0 as:
gcc -O3 -s x.c (or gcc -O2 ...)
Gives the following code:

foo:
	xorl  %eax, %eax
	cmpb  $(0), (%rdi)
	sete  %al
	ret

So, assuming the compiler is still equally clever on more complex
varations of this code, it should end up with exactly the same code as
your patch. 

So although you''ve tried to optimise the code, it actually ends up
being
nothing gained, since the compiler already can figure out what you
ACTUALLY wanted... 

Of course, other versions of gcc may not be as clever... :-(

--
Mats
> 
> Jacob
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/5/05, Petersson, Mats <mats.petersson@amd.com> wrote:
> I created a function:
>
> int foo(char *s)
> {
>         if (strlen(s)) return 1; else return 0;
> }
>
> Compiled with gcc 4.0.0 as:
> gcc -O3 -s x.c (or gcc -O2 ...)
> Gives the following code:
>
> foo:
>         xorl  %eax, %eax
>         cmpb  $(0), (%rdi)
>         sete  %al
>         ret
Cool, I am surprised gcc4 is able to fix this. gcc 3.3.4 which I am
using is not, even with -O3. How does gcc4 behave without -O3?

foo:
push   %ebp
mov    %esp,%ebp
mov    0x8(%ebp),%edx
pop    %ebp
cmpb   $0x0,(%edx)
setne  %dl
movzbl %dl,%eax
ret

I guess I only reacted to this because I have seen worse examples
resulting from the "strlen is constant-time" assumption (people
iterating over a string in a while(strlen(s)) loop), and because I was
bored with trying to get block devices running with xenbus. :-)

Jacob

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Oops sorry I take that back, gcc3 just produces different output...

Never mind then. Must get coffee.

Jacob

On 10/5/05, Jacob Gorm Hansen <jacobg@diku.dk>
wrote:
> On 10/5/05, Petersson, Mats <mats.petersson@amd.com> wrote:
>
> > I created a function:
> >
> > int foo(char *s)
> > {
> >         if (strlen(s)) return 1; else return 0;
> > }
> >
> > Compiled with gcc 4.0.0 as:
> > gcc -O3 -s x.c (or gcc -O2 ...)
> > Gives the following code:
> >
> > foo:
> >         xorl  %eax, %eax
> >         cmpb  $(0), (%rdi)
> >         sete  %al
> >         ret
>
> Cool, I am surprised gcc4 is able to fix this. gcc 3.3.4 which I am
> using is not, even with -O3. How does gcc4 behave without -O3?
>
> foo:
> push   %ebp
> mov    %esp,%ebp
> mov    0x8(%ebp),%edx
> pop    %ebp
> cmpb   $0x0,(%edx)
> setne  %dl
> movzbl %dl,%eax
> ret
>
> I guess I only reacted to this because I have seen worse examples
> resulting from the "strlen is constant-time" assumption (people
> iterating over a string in a while(strlen(s)) loop), and because I was
> bored with trying to get block devices running with xenbus. :-)
>
> Jacob
>

--
Save time and bandwidth with EDelta: http://www.diku.dk/~jacobg/edelta/

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel