[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH 1/3] fix literal error warning
From:       walter harms <wharms () bfs ! de>
Date:       2012-07-31 17:33:33
Message-ID: 501816ED.4020901 () bfs ! de
[Download RAW message or body]



Am 31.07.2012 17:12, schrieb Laurent Bercot:
>>> -		printf(usage_array[i].aname);
>>> +		printf("%s", usage_array[i].aname);
>> This can be replaced with fputs().
> 
>  gcc does this automatically, see below.
> 
> 
>>>  static void xputenv(char *str)
>>>  {
>>>  	if (putenv(str))
>>> -		bb_error_msg_and_die(bb_msg_memory_exhausted);
>>> +		bb_error_msg_and_die("%s", bb_msg_memory_exhausted); 
>> And this, and many other similar warnings, are just silly.
> 
>  Worse: these changes are *harmful*.
> 
>  It's ugly, and we really should not rely on that, and Manuel cannot be
> blamed for not knowing it, but gcc performs a lot of magic around printf().
> In this case, the bit of magic that interests us is that
>   printf(CONSTANT)
> gets automagically replaced with
>   fputs(CONSTANT, stdout)
> 
> (You can test it by dynamically linking a minimal test program using
> printf to write a constant string, and objdumping the resulting
> binary. You will see a reference to fputs and no reference to printf.)
> 
>  So, since bb_msg_memory_exhausted is constant, the printf part of
>    bb_error_msg_and_die(bb_msg_memory_exhausted);
> is correctly optimized into
>    fputs(bb_msg_memory_exhausted, stdout);


bb_simple_error_msg_and_die() should be fixed that way. It is not nice to rely
on magic done with a specific compiler.

re,
 wh

> whereas
>    bb_error_msg_and_die("%s", bb_msg_memory_exhausted);
> uses the full printf() call - which behaves correctly, but is
> suboptimal.
> 
> 
>> Yes, gcc can't verify the format string, but it is declared
>> as constant for a reason.  These changes (all around bb_msg_*)
>> are just adding code without fixing anything.
> 
>  They fix the spurious warnings, but prevent fputs() optimizations
> from being performed.
> 
> 
>>  In my opinion,
>> gcc should be fixed instead, to at least treat const char[]
>> fmt string as non-dangerous.
> 
>  Exactly. Those gcc warnings are silly, because gcc will optimize away
> the printf() call anyway.
> 
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]