Message ID | alpine.DEB.2.20.1803021755370.29959@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Series | Fix s390 -Os iconv build | expand |
On 03/02/2018 06:56 PM, Joseph Myers wrote: > Building glibc for s390 with -Os (32-bit only, with GCC 7) fails with: > > In file included from ../sysdeps/s390/multiarch/8bit-generic.c:370:0, > from ebcdic-at-de.c:28: > ../iconv/loop.c: In function '__to_generic_vx': > ../iconv/loop.c:264:22: error: 'ch' may be used uninitialized in this function [-Werror=maybe-uninitialized] > if (((Character) >> 7) == (0xe0000 >> 7)) \ > ^~ > In file included from ebcdic-at-de.c:28:0: > ../sysdeps/s390/multiarch/8bit-generic.c:340:15: note: 'ch' was declared here > uint32_t ch; \ > ^ > ../iconv/loop.c:325:7: note: in expansion of macro 'BODY' > BODY > ^~~~ > > It's fairly easy to see, looking at the (long) expansion of the BODY > macro, that this is a false positive and the relevant variable 'ch' is > always initialized before use, in one of two possible places. As > such, disabling the warning for -Os with the DIAG_* macros is the > natural approach to fix this build failure. However, because of the > location at which the warning is reported, the disabling needs to go > in iconv/loop.c, around the definition of UNICODE_TAG_HANDLER (not > inside the definition), as that macro definition is where the > uninitialized use is reported, whereas the code that needs to be > reasoned about to see that the warning is a false positive is in the > definition of BODY elsewhere. > > Thus, the patch adds such disabling in iconv/loop.c, with a comment > pointing to the s390-specific code and a comment in the s390-specific > code pointing to the generic file to alert people to the possible need > to update one place when changing the other. It would be possible if > desired to use #ifdef __s390__ around the disabling, though in general > we try to avoid that sort of thing in generic files. (Or some > extremely specialized macros for "disable -Wmaybe-uninitialized in > this particular place" could be specified, defined to 0 in a lot of > different files that include iconv/loop.c and to 1 in that particular > s390 file.) > > Tested that this fixed -Os compilation for s390-linux-gnu with > build-many-glibcs.py. > Build locally on s390 / s390x with -Os / -O3. This patch suppresses the Werror in all these mentioned cases. From s390 perspective, this patch is okay. Thanks. Stefan
diff --git a/iconv/loop.c b/iconv/loop.c index 25609f1..d571b59 100644 --- a/iconv/loop.c +++ b/iconv/loop.c @@ -254,6 +254,16 @@ } +/* With GCC 7 when compiling with -Os for 32-bit s390 the compiler + warns that the variable 'ch', in the definition of BODY in + sysdeps/s390/multiarch/8bit-generic.c, may be used uninitialized in + the call to UNICODE_TAG_HANDLER in that macro. This variable is + actually always initialized before use, in the prior loop if INDEX + is nonzero and in the following 'if' if INDEX is zero. That code + has a comment referencing this diagnostic disabling; updates in one + place may require updates in the other. */ +DIAG_PUSH_NEEDS_COMMENT; +DIAG_IGNORE_Os_NEEDS_COMMENT (7, "-Wmaybe-uninitialized"); /* Handling of Unicode 3.1 TAG characters. Unicode recommends "If language codes are not relevant to the particular processing operation, then they should be ignored." This macro is usually @@ -267,6 +277,7 @@ continue; \ } \ } +DIAG_POP_NEEDS_COMMENT; /* The function returns the status, as defined in gconv.h. */ diff --git a/sysdeps/s390/multiarch/8bit-generic.c b/sysdeps/s390/multiarch/8bit-generic.c index 8d44cd8..d608bea 100644 --- a/sysdeps/s390/multiarch/8bit-generic.c +++ b/sysdeps/s390/multiarch/8bit-generic.c @@ -358,6 +358,10 @@ } \ } \ \ + /* iconv/loop.c disables -Wmaybe-uninitialized for a false \ + positive warning in this code with -Os and has a \ + comment referencing this code accordingly. Updates in \ + one place may require updates in the other. */ \ UNICODE_TAG_HANDLER (ch, 4); \ \ /* This is an illegal character. */ \