Patchwork Fix s390 -Os iconv build

login
register
mail settings
Submitter Joseph Myers
Date March 2, 2018, 5:56 p.m.
Message ID <alpine.DEB.2.20.1803021755370.29959@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/26154/
State New
Headers show

Comments

Joseph Myers - March 2, 2018, 5:56 p.m.
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.

2018-03-02  Joseph Myers  <joseph@codesourcery.com>

	* iconv/loop.c (UNICODE_TAG_HANDLER): Disable
	-Wmaybe-uninitialized for -Os.
	* sysdeps/s390/multiarch/8bit-generic.c (BODY): Add comment about
	this disabling.
Stefan Liebler - March 5, 2018, 3:35 p.m.
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

Patch

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.  */			\