[libvirt] Non-zero constant warning on RHEL 6.1 with 0.9.7

[Eric Blake]

On 11/14/2011 05:54 PM, Stefan Berger wrote:
> On 11/11/2011 04:06 PM, Eric Blake wrote:
>> On 11/08/2011 11:46 PM, Justin Clift wrote:
>>> Hi guys,
>>>
>>> Just checking 0.9.7 on RHEL 6.1 x86_64.  Noticed this when compiling
>>> with make -j 3:
>>>
>>>      CC     libvirt_lxc-command.o
>>>    util/buf.c: In function 'virBufferEscape':
>>>    util/buf.c:469: warning: logical '&&' with non-zero constant will
>>> always evaluate as true [-Wlogical-op]
>>>
>>> Obviously not fatal, but it figured someone might want to keep things
>>> warning free. :)
>> This is a bug in gcc, although I'm not sure if anyone has raised a bug
>> report against the gcc folks yet:
>> https://www.redhat.com/archives/libvir-list/2011-October/msg00837.html
>>
>> I'm open to suggestions on how to work around it; perhaps we just need
>> to disable -Wlogical-op if glibc's headers are turning on the optimized
>> strchr (is that -D_FORTIFY_SOURCE that does it?) :(
> This should work, no?
> 
> --- a/src/util/buf.c
> +++ b/src/util/buf.c
> @@ -466,7 +466,8 @@ virBufferEscape(virBufferPtr buf, const char *toescape,
>      cur = str;
>      out = escaped;
>      while (*cur != 0) {
> -        if (strchr(toescape, *cur))
> +        char x[2] = { *cur, 0 };
> +        if (strstr(toescape, x))

The glibc headers are expanding the pre-patch code to:

if ((__extension__ (__builtin_constant_p (*cur) &&
!__builtin_constant_p (toescape) && (*cur) == '\0' ? (char *)
__rawmemchr (toescape, *cur) : __builtin_strchr (toescape, *cur))))

but warning about:
    util/buf.c:469: warning: logical '&&' with non-zero constant will
always evaluate as true [-Wlogical-op]

I see two constants in that expression:
  !__builtin_constant_p (toescape) is always 1
  (*cur) == '\0' is always 0 (since the containing while loop already
guaranteed *cur != 0)

There's definitely at least one gcc bug: that of an incorrect error
message.  After all, the expression 'expra && exprb' is constant in only
three cases: 'expra' is false, 'exprb' is false, or 'expra' is true and
'exprb' is true.  But the gcc message isn't clear on whether it is
warning about 'expra' or 'exprb', not to mention that it claims the
overall expression is true even though we just proved the overall
expression is always false by virtue of exprb being false.

Whether there is a secondary bug, about gcc being noisy with
-Wlogical-op inside __extension__, is also worth arguing.


At any rate, I agree that your proposed patch skirts the issue by using
strstr() instead of strchr(), so it would indeed do the trick (while
under the hood, strstr() on a single-byte needle tends to defer to
strchr() anyways).  It feels kind of gross, without at least a comment
explaining that we are working around a gcc bug in -Wlogical-op (and
even better, a URL of the gcc bug report); but I'm inclined to ACK it if
we can come up with an appropriate comment.

An even more aggressive patch would use strpbrk() to locate each
character that needs escaping and memcpy() to quickly copy chunks of
non-escaped text, rather than processing things one byte at a time, but
I'm not sure whether that would be a micro-optimization or whether this
function is called often enough to make it worth the extra complexity.

What does 'make V=1' show as the exact compiler line used when
triggering the error?  I still haven't been able to reproduce the bogus
compiler warning on a much smaller test case, but suspect I may be
missing a particular flag that makes it apparent.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111114/3c90f8ea/attachment-0001.sig>