modprobe update [Was: Re: Bug triage.]

Rob Landley rob at landley.net
Tue Dec 13 02:30:29 UTC 2005


On Monday 12 December 2005 16:31, Yann E. MORIN wrote:
> Hello all!
> Rob,
>
> On Monday 12 December 2005 203, Rob Landley wrote:
> >                p = strchr (buffer, ' ');
> >                if (p) {
> >                        *p = 0;
> > +                       for( p = buffer; ENABLE_FEATURE_2_6_MODULES &&
> > *p; p++ )
> >  {
> > +                               *p = ((*p)=='-')?'_':*p;
> > +                       }
> > I'm not 100% certain that can cleanly be optimized away, since p lives
> > outside your current scope and the for(;;) will assign to p before
> > terminating (and hopefully optimizing out the body of) the loop.
> > One reason I've been holding on the CONFIG_->ENABLE_ conversion is to get
> > more familiar with the optimization capabilities of gcc.  (What kind of
> > guards can we get away with doing without accidentally bloating the code?
> >  if(ENABLE) seems pretty safe.  I'd want to check this vs putting an
> > explicit if(ENABLE) around it.  Might be clearer from a code reading
> > point anyway...)
>
> You are right. I'm not familiar to C compiler optimisation either, but your
> reasoning sounds perfectly, well, sound, here. I was thinking, the loop
> condition is always false, so gcc (and others) will clean that up for us.

It's possible (in fact, likely) that assigning to a variable that is never 
read from again will be discarded.  But if this variable is re-used several 
times, I'm not sure.  (This falls under constant propogation and lifespan 
analysis.  I haven't looked at the surrounding code.  The nice thing about 
putting an if() around it is you don't _need_ to look at the surrounding 
code, which from a simplicity standpoint is a good thing.)

> On the other hand, we will always stumble on dumb (or 'broken') compilers
> that will not optimises those things out. If we want to be 100% sure no
> unused code sneaks into the binary, then #ifdef's are the only option, at
> the expense of readability.

I'm not worried about compilers that can't optimize out "if(0) { }" because 
turbo pascal could handle that back under dos and TCC can do it today.  Any 
compiler that can't do at least that much is going to bloat the busybox 
binary so much that this would just add insult to injury.

In fact, what I'm mostly worried about right now is how gcc handles it, since 
that's the compiler almost everyone is building this thing with right now.

I'm not necessarily against doing it the way you did if we know it works.  But 
it's a doubletake moment, where you have to stop and think about the code to 
understand what it's doing, and that says to me either "put comment here" or 
figure out how to simplify it so it's more obvious.

> Do you want a patch with if(ENABLE) ?

In 1.2 I'd like to convert everything over to ENABLE.  We need to re-measure 
the sizes of everything to make sure we don't accidentally bloat anything 
when we do this, though.

> argc_malloc is always declared.
> If CONFIG_FEATURE_CLEAN_UP is declared, then argc_malloc gets used
> (written, then read).
> If CONFIG_FEATURE_CLEAN_UP is not declared, then argc_malloc is only
> declared, and gcc optimises it out, at the expense of one compiler warning.
> Am I wrong?

I'd have to read the context.  (You're more familiar with this code than I am, 
I mostly wanted to confirm that it was intentional and that _you_ had thought 
this through.)

What compiler warning?

> > -#if defined(CONFIG_FEATURE_2_6_MODULES)
> > -               if ( argc ) {
> > +               if( argc ) {
> > Why was that guard unnecessary?
>
> Because we want to use the options from the command line, for both 2.6 and
> 2.4. That solves bug #272.

Ok.

> > I just checked it in.  Does it fix bugs 276 and 272?
>
> Good. And yes, both bugs are fixed with this patch.

Life is good.

> Regards,
> Yann E. MORIN.

Rob
-- 
Steve Ballmer: Innovation!  Inigo Montoya: You keep using that word.
I do not think it means what you think it means.



More information about the busybox mailing list