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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list