12:46 a.m.
Hi guys,
Just checking 0.9.7 on RHEL 6.1 x86_64. Noticed this when compiling
with make -j 3:
make[3]: Entering directory `/home/jc/git_repos/libvirt/src'
CC libvirt_util_la-bitmap.lo
CC libvirt_util_la-authhelper.lo
CC libvirt_util_la-bridge.lo
CC libvirt_util_la-buf.lo
CC libvirt_util_la-command.lo
util/buf.c: In function 'virBufferEscape':
util/buf.c:469: warning: logical '&&' with non-zero constant will always
evaluate as true [-Wlogical-op]
...
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. :)
Regards and best wishes,
Justin Clift
--
Aeolus Community Manager
http://www.aeolusproject.org
4:06 a.m.
On 09.11.2011 07:46, Justin Clift wrote:
Hi guys,
Just checking 0.9.7 on RHEL 6.1 x86_64. Noticed this when compiling
with make -j 3:
make[3]: Entering directory `/home/jc/git_repos/libvirt/src'
CC libvirt_util_la-bitmap.lo
CC libvirt_util_la-authhelper.lo
CC libvirt_util_la-bridge.lo
CC libvirt_util_la-buf.lo
CC libvirt_util_la-command.lo
util/buf.c: In function 'virBufferEscape':
util/buf.c:469: warning: logical '&&' with non-zero constant will
always evaluate as true [-Wlogical-op]
...
I've ran gcc -E on util/buf.c and interesting things are happening there:
the original code:
while (*cur != 0) {
if (strchr(toescape, *cur))
*out++ = '\\';
*out++ = *cur;
cur++;
}
[line 469 is: if (strchr(...))]
after preprocessing stage:
while (*cur != 0) {
if ((__extension__ (__builtin_constant_p (*cur) &&
!__builtin_constant_p (toescape) && (*cur) == '\0' ? (char *)
__rawmemchr (toescape, *cur) : __builtin_strchr (toescape, *cur))))
*out++ = '\\';
*out++ = *cur;
cur++;
}
So I think the problem lies within !__builtin_constant_p (toescape) but
i am not sure how to fix that.
Michal
3:06 p.m.
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?) :(
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:54 p.m.
On 12/11/2011, at 8:06 AM, 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?) :(
Hmmmm, we might as well report it, so it gets looked at.
Next question is where? Upstream gcc, or on the RG BZ as a Fedora15(+?)/RHEL6
specific gcc bug?
Regards and best wishes,
Justin Clift
--
Aeolus Community Manager
http://www.aeolusproject.org
6:49 p.m.
On 11/11/2011 04:54 PM, Justin Clift wrote:
On 12/11/2011, at 8:06 AM, 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?) :(
Hmmmm, we might as well report it, so it gets looked at.
Next question is where? Upstream gcc, or on the RG BZ as a Fedora15(+?)/RHEL6
specific gcc bug?
I am seeing it in this gcc version:
Using built-in specs.
Target: ppc64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info
--with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap
--enable-shared --enable-threads=posix --enable-checking=release
--with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions
--enable-gnu-unique-object
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada
--enable-java-awt=gtk --disable-dssi
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre
--enable-libgcj-multifile --enable-java-maintainer-mode
--with-ecj-jar=/usr/share/java/eclipse-ecj.jar
--disable-libjava-multilib --with-ppl --with-cloog --enable-secureplt
--with-long-double-128 --with-cpu-32=power4 --with-tune-32=power6
--with-cpu-64=power4 --with-tune-64=power6 --build=ppc64-redhat-linux
Thread model: posix
gcc version 4.4.5 20110214 (Red Hat 4.4.5-6) (GCC)
Stefan
6:54 p.m.
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))
*out++ = '\\';
*out++ = *cur;
cur++;
Stefan
9:36 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:34 a.m.
On 11/14/2011 10:36 PM, Eric Blake wrote:
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?) :(
>
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.
What about this here:
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -466,7 +466,11 @@ virBufferEscape(virBufferPtr buf, const char *toescape,
cur = str;
out = escaped;
while (*cur != 0) {
- if (strchr(toescape, *cur))
+ /* strchr work-around for gcc 4.3 & 4.4 bug with -Wlogical-op
+ * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
+ */
+ char needle[2] = { *cur, 0 };
+ if (strstr(toescape, needle))
*out++ = '\\';
*out++ = *cur;
cur++;
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.
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I..
-I../gnulib/lib -I../gnulib/lib -I../include -I../src/util -I../include
-DIN_LIBVIRT -I/usr/include/libxml2 -Wall -W -Wformat-y2k
-Wformat-security -Winit-self -Wmissing-include-dirs -Wunused
-Wunknown-pragmas -Wstrict-aliasing -Wshadow -Wpointer-arith
-Wbad-function-cast -Wcast-align -Wwrite-strings -Wlogical-op
-Waggregate-return -Wstrict-prototypes -Wold-style-definition
-Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn
-Wmissing-format-attribute -Wredundant-decls -Wnested-externs -Winline
-Winvalid-pch -Wvolatile-register-var -Wdisabled-optimization
-Wbuiltin-macro-redefined -Wmudflap -Wpacked-bitfield-compat -Wsync-nand
-Wattributes -Wcoverage-mismatch -Wmultichar
-Wno-missing-field-initializers -Wno-sign-compare
-Wframe-larger-than=4096 -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector-all
--param=ssp-buffer-size=4 -fexceptions -fasynchronous-unwind-tables
-fdiagnostics-show-option -funit-at-a-time -g -O2 -MT
libvirt_util_la-buf.lo -MD -MP -MF .deps/libvirt_util_la-buf.Tpo -c
util/buf.c -fPIC -DPIC -o .libs/libvirt_util_la-buf.o
util/buf.c: In function 'virBufferEscape':
util/buf.c:469: warning: logical '&&' with non-zero constant will always
evaluate as true [-Wlogical-op]
Stefan
5:40 a.m.
On 15/11/2011, at 10:34 PM, Stefan Berger wrote:
+ /* strchr work-around for gcc 4.3 & 4.4 bug with
-Wlogical-op
+ * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
+ */
Cool, that's good investigation Stefan. :)
+ Justin
--
Aeolus Community Manager
http://www.aeolusproject.org
5:41 a.m.
On Tue, Nov 15, 2011 at 06:34:27AM -0500, Stefan Berger wrote:
On 11/14/2011 10:36 PM, Eric Blake wrote:
>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?) :(
>>
>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.
>
What about this here:
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -466,7 +466,11 @@ virBufferEscape(virBufferPtr buf, const char *toescape,
cur = str;
out = escaped;
while (*cur != 0) {
- if (strchr(toescape, *cur))
+ /* strchr work-around for gcc 4.3 & 4.4 bug with -Wlogical-op
+ * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
+ */
+ char needle[2] = { *cur, 0 };
+ if (strstr(toescape, needle))
*out++ = '\\';
I'm puzzelled that we need this change here, but we have 175 other
uses of strchr without trouble
$ grep strchr src/*/*.c | wc -l
174
Is the difference just that we're deferencing a char * for the second arg
rather than using a constant value ?
Would it suffice to do
int c = *cur;
if (strchr(toescape, c))
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:53 a.m.
On 11/15/2011 06:41 AM, Daniel P. Berrange wrote:
On Tue, Nov 15, 2011 at 06:34:27AM -0500, Stefan Berger wrote:
Is the difference just that we're deferencing a char * for the second arg
rather than using a constant value ?
Would it suffice to do
int c = *cur;
if (strchr(toescape, c))
I still get the same error with this. It's the two
variables that cause
the trouble here.
Stefan
9:56 a.m.
On 11/15/2011 04:41 AM, Daniel P. Berrange wrote:
> while (*cur != 0) {
> - if (strchr(toescape, *cur))
> + /* strchr work-around for gcc 4.3 & 4.4 bug with -Wlogical-op
> + * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
> + */
> + char needle[2] = { *cur, 0 };
> + if (strstr(toescape, needle))
> *out++ = '\\';
I'm puzzelled that we need this change here, but we have 175 other
uses of strchr without trouble
$ grep strchr src/*/*.c | wc -l
174
Rather, look for instances of strchr that have a constant second argument:
$ git grep strchr".*'" src/*/*.c | wc -l
173
vs. those with a variable second argument:
$ git grep strchr"[^']*\$" src/*/*.c | wc -l
3
further, 2 of those 3 have a constant first argument. So this really
was the only instance of using strchr() with two variable arguments, and
that is the only use-case that can trigger the gcc bug.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:02 a.m.
On 11/15/2011 04:34 AM, Stefan Berger wrote:
What about this here:
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -466,7 +466,11 @@ virBufferEscape(virBufferPtr buf, const char
*toescape,
cur = str;
out = escaped;
while (*cur != 0) {
- if (strchr(toescape, *cur))
+ /* strchr work-around for gcc 4.3 & 4.4 bug with -Wlogical-op
+ * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
+ */
+ char needle[2] = { *cur, 0 };
+ if (strstr(toescape, needle))
*out++ = '\\';
*out++ = *cur;
cur++;
ACK. And I finally reproduced the issue with gcc 4.4.6, as well as
confirmed that gcc 4.6.2 is immune; it involves both -O1 (or greater)
and -Wlogical-op at the same time.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4756
days inactive
4762
days old
12 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Justin Clift -
Michal Privoznik -
Stefan Berger