Re: [libvirt] [PATCH] maint: update to latest gnulib

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(a)redhat.com&gt; > > > > > --- > > > > > > > > > > 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.