[libvirt] [PATCH] maint: update to latest gnulib
This fixes an incompatibility with glibc 2.25.90 Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- Pushed as a broken build fix to get CI back online .gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gnulib b/.gnulib index da830b5..ce4ee4c 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit da830b5146cb553ac2a4bcfe76caeb57bda24cc3 +Subproject commit ce4ee4cbb596a9d7de2786cf8c48cf62a4edede7 -- 2.9.3
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error: In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~ Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
.gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gnulib b/.gnulib index da830b5..ce4ee4c 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit da830b5146cb553ac2a4bcfe76caeb57bda24cc3 +Subproject commit ce4ee4cbb596a9d7de2786cf8c48cf62a4edede7 -- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error:
In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~
Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1 which gnulib turns on. There's a similar hit with mingw ../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ cc1: all warnings being treated as errors because WEXITSTATUS(x) expands to 'x' on Win32. We could use a pragma to turn off selectively, but I'm more inclined to just disable this new warning flag. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error:
In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~
Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1 which gnulib turns on. There's a similar hit with mingw
../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ cc1: all warnings being treated as errors
because WEXITSTATUS(x) expands to 'x' on Win32.
We could use a pragma to turn off selectively, but I'm more inclined to just disable this new warning flag.
Well, I'm not sure how that affects the line where we actually use it (with the atomic variables) or whether that line is not needed anymore (if that was a fix for older compilers or something similar). But I can send a patch for removing that warning. How about the other warning we get when we turn off the first one? I just found out. I think that could be turned off as well, either for some particular places or for the whole build: util/virtime.c: In function 'virTimeStringThenRaw': util/virtime.c:215:9: error: '%02d' directive output may be truncated writing between 2 and 11 bytes into a region of size between 5 and 21 [-Werror=format-truncation=] if (snprintf(buf, VIR_TIME_STRING_BUFLEN, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "%4d-%02d-%02d %02d:%02d:%02d.%03d+0000", ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_year, fields.tm_mon, fields.tm_mday, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_hour, fields.tm_min, fields.tm_sec, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) { ~~~~~~~~~~~~~~~~~~~~ util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for directive argument In file included from /usr/include/stdio.h:936:0, from ../gnulib/lib/stdio.h:43, from util/virtime.c:36: /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output between 29 and 89 bytes into a destination of size 29 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __bos (__s), __fmt, __va_arg_pack ()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jun 14, 2017 at 04:30:49PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error:
In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~
Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1 which gnulib turns on. There's a similar hit with mingw
../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ cc1: all warnings being treated as errors
because WEXITSTATUS(x) expands to 'x' on Win32.
We could use a pragma to turn off selectively, but I'm more inclined to just disable this new warning flag.
Well, I'm not sure how that affects the line where we actually use it (with the atomic variables) or whether that line is not needed anymore (if that was a fix for older compilers or something similar). But I can send a patch for removing that warning. How about the other warning we get when we turn off the first one? I just found out. I think that could be turned off as well, either for some particular places or for the whole build:
util/virtime.c: In function 'virTimeStringThenRaw': util/virtime.c:215:9: error: '%02d' directive output may be truncated writing between 2 and 11 bytes into a region of size between 5 and 21 [-Werror=format-truncation=] if (snprintf(buf, VIR_TIME_STRING_BUFLEN, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "%4d-%02d-%02d %02d:%02d:%02d.%03d+0000", ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_year, fields.tm_mon, fields.tm_mday, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_hour, fields.tm_min, fields.tm_sec, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) { ~~~~~~~~~~~~~~~~~~~~ util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for directive argument In file included from /usr/include/stdio.h:936:0, from ../gnulib/lib/stdio.h:43, from util/virtime.c:36: /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output between 29 and 89 bytes into a destination of size 29 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __bos (__s), __fmt, __va_arg_pack ()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think we might just want to switch to asprintf here instead of trying to optimize into a fixed stack allocated buffer. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jun 14, 2017 at 03:35:39PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 04:30:49PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error:
In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~
Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1 which gnulib turns on. There's a similar hit with mingw
../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ cc1: all warnings being treated as errors
because WEXITSTATUS(x) expands to 'x' on Win32.
We could use a pragma to turn off selectively, but I'm more inclined to just disable this new warning flag.
Well, I'm not sure how that affects the line where we actually use it (with the atomic variables) or whether that line is not needed anymore (if that was a fix for older compilers or something similar). But I can send a patch for removing that warning. How about the other warning we get when we turn off the first one? I just found out. I think that could be turned off as well, either for some particular places or for the whole build:
util/virtime.c: In function 'virTimeStringThenRaw': util/virtime.c:215:9: error: '%02d' directive output may be truncated writing between 2 and 11 bytes into a region of size between 5 and 21 [-Werror=format-truncation=] if (snprintf(buf, VIR_TIME_STRING_BUFLEN, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "%4d-%02d-%02d %02d:%02d:%02d.%03d+0000", ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_year, fields.tm_mon, fields.tm_mday, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_hour, fields.tm_min, fields.tm_sec, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) { ~~~~~~~~~~~~~~~~~~~~ util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for directive argument In file included from /usr/include/stdio.h:936:0, from ../gnulib/lib/stdio.h:43, from util/virtime.c:36: /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output between 29 and 89 bytes into a destination of size 29 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __bos (__s), __fmt, __va_arg_pack ()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think we might just want to switch to asprintf here instead of trying to optimize into a fixed stack allocated buffer.
I wanted to do that and started rewriting it, but I found out we use static buffers in lot of places and lot of them then use snprintf or similar. In some cases it doesn't even make much of a sense, e.g.: snprintf(buf, 3, "%d", var) even this can fail. I get the reason for the warning, but I don't really agree that it's something that is necessary to avoid, so I'm inclining to ignoring that warning as well.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 15, 2017 at 05:39:00PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 03:35:39PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 04:30:49PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 02:32:41PM +0100, Daniel P. Berrange wrote:
On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
This fixes an incompatibility with glibc 2.25.90
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
Pushed as a broken build fix to get CI back online
After this update the build fails for me with gcc-7.1.0 with the following error:
In file included from util/virobject.c:28:0: util/virobject.c: In function 'virClassNew': util/viratomic.h:176:46: error: this condition has identical branches [-Werror=duplicated-branches] (void)(0 ? *(atomic) ^ *(atomic) : 0); \ ^ util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc' klass->magic = virAtomicIntInc(&magicCounter); ^~~~~~~~~~~~~~~
Does that mean that gcc does optimize our prefetch trick away (considering I understood what that line is trying to do)? Or should we just turn the warning off for that header file?
Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1 which gnulib turns on. There's a similar hit with mingw
../../src/util/vircommand.c: In function 'virCommandWait': ../../src/util/vircommand.c:2562:51: error: this condition has identical branches [-Werror=duplicated-branches] *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); ^ cc1: all warnings being treated as errors
because WEXITSTATUS(x) expands to 'x' on Win32.
We could use a pragma to turn off selectively, but I'm more inclined to just disable this new warning flag.
Well, I'm not sure how that affects the line where we actually use it (with the atomic variables) or whether that line is not needed anymore (if that was a fix for older compilers or something similar). But I can send a patch for removing that warning. How about the other warning we get when we turn off the first one? I just found out. I think that could be turned off as well, either for some particular places or for the whole build:
util/virtime.c: In function 'virTimeStringThenRaw': util/virtime.c:215:9: error: '%02d' directive output may be truncated writing between 2 and 11 bytes into a region of size between 5 and 21 [-Werror=format-truncation=] if (snprintf(buf, VIR_TIME_STRING_BUFLEN, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "%4d-%02d-%02d %02d:%02d:%02d.%03d+0000", ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_year, fields.tm_mon, fields.tm_mday, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fields.tm_hour, fields.tm_min, fields.tm_sec, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (int) (when % 1000)) >= VIR_TIME_STRING_BUFLEN) { ~~~~~~~~~~~~~~~~~~~~ util/virtime.c:215:9: note: using the range [-2147483648, 2147483647] for directive argument In file included from /usr/include/stdio.h:936:0, from ../gnulib/lib/stdio.h:43, from util/virtime.c:36: /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output between 29 and 89 bytes into a destination of size 29 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __bos (__s), __fmt, __va_arg_pack ()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think we might just want to switch to asprintf here instead of trying to optimize into a fixed stack allocated buffer.
I wanted to do that and started rewriting it, but I found out we use static buffers in lot of places and lot of them then use snprintf or similar. In some cases it doesn't even make much of a sense, e.g.:
snprintf(buf, 3, "%d", var)
even this can fail. I get the reason for the warning, but I don't really agree that it's something that is necessary to avoid, so I'm inclining to ignoring that warning as well.
In that kind of scenario we should be using INT_BUFSIZE_BOUND(var) to declare the 'buf' array, rather than hardcoding a fixed size based on assumptions about likely max value of var. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrange -
Martin Kletzander