[v3] Fix -Os related build and test failures.

Message ID db9cd7c1-1f32-8fc9-873d-f13bf563a301@redhat.com
State Superseded
Headers

Commit Message

Carlos O'Donell Oct. 29, 2016, 2:57 a.m. UTC
  On 10/28/2016 10:16 AM, Joseph Myers wrote:
> On Fri, 28 Oct 2016, Carlos O'Donell wrote:
> 
>> - Fix math/test-nan-overflow.c which uses malloc but doesn't include
>>   stdlib.h.
> 
> I don't know what's including <stdlib.h> but not for -Os, but this fix 
> should be separate (and committed as obvious).

Checked in.
 
>> - Define DIAG_IGNORE_Ox_NEEDS_COMMENT and DIAG_IGNORE_Os_NEEDS_COMMENT
>>   for use with diagnostics which should be ignored only when optimizing,
>>   or when optimizing for size. If we need *Ox* to be finer grained, then
>>   we'll have to setup something more complicated, but for now we don't
>>   need it.
> 
> I don't think we need the -Ox version.  glibc is always built with 
> optimization, and if we fix things (for better debugging) so that only a 
> few bits need to be built with optimization and the rest is correct 
> without, it's harmess for the pragmas to be in effect with -O0 even if not 
> needed in that case.

v3
- Remove DIAG_IGNORE_Ox_NEEDS_COMMENT.
- Mark for BZ #20729.
- Fix remaining instances of '5.3', and use '5' as the latest version
  the warning is seen in.

With Florian's fix for %ebp usage, and this fix for -Werror issues, the
-Os build for i486 is now working again, but still needs bug 19463 and
bug 15105 fixed before it's really working. However, I would consider
marking bug 20729 fixed once this goes in.

No regressions on x86.

OK?

2016-10-28  Carlos O'Donell  <carlos@redhat.com>

	[BZ #20729]
        * include/libc-internal.h (DIAG_IGNORE_Os_NEEDS_COMMENT):
        Define. 
        * iso-2022-cn-ext.c: Include libc-internal.h and ignore
        -Wmaybe-uninitialized for BODY macro only for -Os compiles.
        * locale/weight.h (findix): Ignore -Wmaybe-uninitialized error
        for seq2.back_us and seq1.back_us only for -Os compiles.
        * locale/weightwc.h (findix): Likewise.
        * nptl_db/thread_dbP.h: Ignore -Wmaybe-uninitialized error for
        DB_GET_FIELD_ADDRESS only for -Os compiles.
        * resolv/res_send (reopen): Ignore -Wmaybe-uninitialized error
        for slen only for -Os compiles.
        * string/strcoll_l.c (get_next_seq): Ignore
        -Wmaybe-uninitialized for seq2.save_idx and seq1.save_idx only
        for -Os compiles.

---
  

Comments

Carlos O'Donell Oct. 29, 2016, 3:26 a.m. UTC | #1
On 10/28/2016 10:57 PM, Carlos O'Donell wrote:
> OK?
> 
> 2016-10-28  Carlos O'Donell  <carlos@redhat.com>
> 
> 	[BZ #20729]
>         * include/libc-internal.h (DIAG_IGNORE_Os_NEEDS_COMMENT):
>         Define. 
>         * iso-2022-cn-ext.c: Include libc-internal.h and ignore
>         -Wmaybe-uninitialized for BODY macro only for -Os compiles.
>         * locale/weight.h (findix): Ignore -Wmaybe-uninitialized error
>         for seq2.back_us and seq1.back_us only for -Os compiles.
>         * locale/weightwc.h (findix): Likewise.
>         * nptl_db/thread_dbP.h: Ignore -Wmaybe-uninitialized error for
>         DB_GET_FIELD_ADDRESS only for -Os compiles.
>         * resolv/res_send (reopen): Ignore -Wmaybe-uninitialized error
>         for slen only for -Os compiles.
>         * string/strcoll_l.c (get_next_seq): Ignore
>         -Wmaybe-uninitialized for seq2.save_idx and seq1.save_idx only
>         for -Os compiles.
> 

> +/* Similar to DIAG_IGNORE_NEEDS_COMMENT the following macro ignores the
> +   diagnostic OPTION but only if optimizations for size are enabled.
> +   This is required because different warnings may be generated for
> +   different optimization levels.  For example a key piece of code may
> +   only generate a warning when compiled at -Os, but at -O2 you could
> +   still want the warning to be enabled to catch errors.  In this case
> +   you would use DIAG_IGNORE_Os_NEEDS_COMMENT to disable the warning
> +   only for -Os.  */
> +#if __OPTIMIZE_SIZE__
> +#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)	\
> +  _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> +#else
> +#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)
> +#endif

Typo. s/if/ifdef/g, since __OPTIMIZE_SIZE__ is not defined at -O2.

Fixed.
  
Joseph Myers Oct. 29, 2016, 5:35 p.m. UTC | #2
On Sat, 29 Oct 2016, Carlos O'Donell wrote:

> +#if __OPTIMIZE_SIZE__
> +#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)	\
> +  _Pragma (_DIAG_STR (GCC diagnostic ignored option))
> +#else
> +#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)
> +#endif

That should have "# define" preprocessor indentation inside #if.
  
Steve Ellcey Oct. 31, 2016, 6:38 p.m. UTC | #3
Carlos,

I am running into a build problem with your patch.  weight.h uses the
DIAG_* macros but does not include libc-internal.h, where those macros
are defined.  Apparently it assuming whatever file includes it will
include libc-internal.h.

That is not happening for me when I compile
string/strxfrm_l.c, posix/fnmatch.c, and posix/regex.c.  regex.c does
not include weight.h itself but includes regex_internal.h which
includes weight.h).  I think there are more files with this problem
too, I haven't finished my build yet.

I am building on an aarch64 machine with a prerelease version of GCC
7.0, I think the compiler I am using may be why other people are not
seeing this error.

I am not sure if weight.h should include libc-internal.h, since that is
what uses it, or if the .c and .h files that include weight.h should
also have include libc-internal.h.

Steve Ellcey
sellcey@caviumnetworks.com
  
Carlos O'Donell Oct. 31, 2016, 7:50 p.m. UTC | #4
On 10/31/2016 02:38 PM, Steve Ellcey wrote:
> Carlos,
> 
> I am running into a build problem with your patch.  weight.h uses the
> DIAG_* macros but does not include libc-internal.h, where those macros
> are defined.  Apparently it assuming whatever file includes it will
> include libc-internal.h.
> 
> That is not happening for me when I compile
> string/strxfrm_l.c, posix/fnmatch.c, and posix/regex.c.  regex.c does
> not include weight.h itself but includes regex_internal.h which
> includes weight.h).  I think there are more files with this problem
> too, I haven't finished my build yet.
> 
> I am building on an aarch64 machine with a prerelease version of GCC
> 7.0, I think the compiler I am using may be why other people are not
> seeing this error.
> 
> I am not sure if weight.h should include libc-internal.h, since that is
> what uses it, or if the .c and .h files that include weight.h should
> also have include libc-internal.h.

I have fix and I'm just testing it again on x86_64.

Thank you for your patience :-)

Cheers,
Carlos.
  
Steve Ellcey Oct. 31, 2016, 7:56 p.m. UTC | #5
On Mon, 2016-10-31 at 15:50 -0400, Carlos O'Donell wrote:

> I have fix and I'm just testing it again on x86_64.
> 
> Thank you for your patience :-)
> 
> Cheers,
> Carlos.

Sounds good.  FYI: I ran into this problem with nptl_db/thread_dbP.h,
in addition to the files I mentioned.  I think those are the only
instances of this problem.

Steve Ellcey
  

Patch

diff --git a/iconvdata/iso-2022-cn-ext.c b/iconvdata/iso-2022-cn-ext.c
index df5b5df..92970a0 100644
--- a/iconvdata/iso-2022-cn-ext.c
+++ b/iconvdata/iso-2022-cn-ext.c
@@ -27,6 +27,7 @@ 
 #include "cns11643.h"
 #include "cns11643l1.h"
 #include "cns11643l2.h"
+#include <libc-internal.h>
 
 #include <assert.h>
 
@@ -394,6 +395,16 @@  enum
 #define MIN_NEEDED_OUTPUT	TO_LOOP_MIN_NEEDED_TO
 #define MAX_NEEDED_OUTPUT	TO_LOOP_MAX_NEEDED_TO
 #define LOOPFCT			TO_LOOP
+/* With GCC 5.3 when compiling with -Os the compiler emits a warning
+   that buf[0] and buf[1] may be used uninitialized.  This can only
+   happen in the case where tmpbuf[3] is used, and in that case the
+   write to the tmpbuf[1] and tmpbuf[2] was assured because
+   ucs4_to_cns11643 would have filled in those entries.  The difficulty
+   is in getting the compiler to see this logic because tmpbuf[0] is
+   involved in determining the code page and is the indicator that
+   tmpbuf[2] is initialized.  */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 #define BODY \
   {									      \
     uint32_t ch;							      \
@@ -645,6 +656,7 @@  enum
     /* Now that we wrote the output increment the input pointer.  */	      \
     inptr += 4;								      \
   }
+DIAG_POP_NEEDS_COMMENT;
 #define EXTRA_LOOP_DECLS	, int *setp
 #define INIT_PARAMS		int set = (*setp >> 3) & CURRENT_MASK; \
 				int ann = (*setp >> 3) & ~CURRENT_MASK
diff --git a/include/libc-internal.h b/include/libc-internal.h
index 7a185bb..2d9096a 100644
--- a/include/libc-internal.h
+++ b/include/libc-internal.h
@@ -111,4 +111,19 @@  extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
 #define DIAG_IGNORE_NEEDS_COMMENT(version, option)	\
   _Pragma (_DIAG_STR (GCC diagnostic ignored option))
 
+/* Similar to DIAG_IGNORE_NEEDS_COMMENT the following macro ignores the
+   diagnostic OPTION but only if optimizations for size are enabled.
+   This is required because different warnings may be generated for
+   different optimization levels.  For example a key piece of code may
+   only generate a warning when compiled at -Os, but at -O2 you could
+   still want the warning to be enabled to catch errors.  In this case
+   you would use DIAG_IGNORE_Os_NEEDS_COMMENT to disable the warning
+   only for -Os.  */
+#if __OPTIMIZE_SIZE__
+#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)	\
+  _Pragma (_DIAG_STR (GCC diagnostic ignored option))
+#else
+#define DIAG_IGNORE_Os_NEEDS_COMMENT(version, option)
+#endif
+
 #endif /* _LIBC_INTERNAL  */
diff --git a/locale/weight.h b/locale/weight.h
index c99730c..1f61f01 100644
--- a/locale/weight.h
+++ b/locale/weight.h
@@ -61,9 +61,17 @@  findidx (const int32_t *table,
 	     already.  */
 	  size_t cnt;
 
+	  /* With GCC 5.3 when compiling with -Os the compiler warns
+	     that seq2.back_us, which becomes usrc, might be used
+	     uninitialized.  This can't be true because we pass a length
+	     of -1 for len at the same time which means that this loop
+	     never executes.  */
+	  DIAG_PUSH_NEEDS_COMMENT;
+	  DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 	  for (cnt = 0; cnt < nhere && cnt < len; ++cnt)
 	    if (cp[cnt] != usrc[cnt])
 	      break;
+	  DIAG_POP_NEEDS_COMMENT;
 
 	  if (cnt == nhere)
 	    {
diff --git a/locale/weightwc.h b/locale/weightwc.h
index ab26482..e42ce13 100644
--- a/locale/weightwc.h
+++ b/locale/weightwc.h
@@ -59,9 +59,17 @@  findidx (const int32_t *table,
 	     already.  */
 	  size_t cnt;
 
+	  /* With GCC 5.3 when compiling with -Os the compiler warns
+	     that seq2.back_us, which becomes usrc, might be used
+	     uninitialized.  This can't be true because we pass a length
+	     of -1 for len at the same time which means that this loop
+	     never executes.  */
+	  DIAG_PUSH_NEEDS_COMMENT;
+	  DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 	  for (cnt = 0; cnt < nhere && cnt < len; ++cnt)
 	    if (cp[cnt] != usrc[cnt])
 	      break;
+	  DIAG_POP_NEEDS_COMMENT;
 
 	  if (cnt == nhere)
 	    {
diff --git a/nptl_db/thread_dbP.h b/nptl_db/thread_dbP.h
index 39c3061..b53f1c1 100644
--- a/nptl_db/thread_dbP.h
+++ b/nptl_db/thread_dbP.h
@@ -160,10 +160,19 @@  extern ps_err_e td_mod_lookup (struct ps_prochandle *ps, const char *modname,
 		   SYM_##type##_FIELD_##field, \
 		   (psaddr_t) 0 + (idx), (ptr), &(var))
 
+/* With GCC 5.3 when compiling with -Os the compiler emits a warning
+   that slot may be used uninitialized.  This is never the case since
+   the dynamic loader initializes the slotinfo list and
+   dtv_slotinfo_list will point slot at the first entry.  Therefore
+   when DB_GET_FIELD_ADDRESS is called with a slot for ptr, the slot is
+   always initialized.  */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 #define DB_GET_FIELD_ADDRESS(var, ta, ptr, type, field, idx) \
   ((var) = (ptr), _td_locate_field ((ta), (ta)->ta_field_##type##_##field, \
 				    SYM_##type##_FIELD_##field, \
 				    (psaddr_t) 0 + (idx), &(var)))
+DIAG_POP_NEEDS_COMMENT;
 
 extern td_err_e _td_locate_field (td_thragent_t *ta,
 				  db_desc_t desc, int descriptor_name,
diff --git a/resolv/res_send.c b/resolv/res_send.c
index 6d46bb2..93efb7d 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -664,7 +664,7 @@  send_vc(res_state statp,
 	   a false-positive.
 	 */
 	DIAG_PUSH_NEEDS_COMMENT;
-	DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
+	DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 	int resplen;
 	DIAG_POP_NEEDS_COMMENT;
 	struct iovec iov[4];
@@ -930,7 +930,16 @@  reopen (res_state statp, int *terrno, int ns)
 		 * error message is received.  We can thus detect
 		 * the absence of a nameserver without timing out.
 		 */
+		/* With GCC 5.3 when compiling with -Os the compiler
+		   emits a warning that slen may be used uninitialized,
+		   but that is never true.  Both slen and
+		   EXT(statp).nssocks[ns] are initialized together or
+		   the function return -1 before control flow reaches
+		   the call to connect with slen.  */
+		DIAG_PUSH_NEEDS_COMMENT;
+		DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
 		if (connect(EXT(statp).nssocks[ns], nsap, slen) < 0) {
+		DIAG_POP_NEEDS_COMMENT;
 			Aerror(statp, stderr, "connect(dg)", errno, nsap);
 			__res_iclose(statp, false);
 			return (0);
diff --git a/string/strcoll_l.c b/string/strcoll_l.c
index 4d1e3ab..065392e 100644
--- a/string/strcoll_l.c
+++ b/string/strcoll_l.c
@@ -24,6 +24,7 @@ 
 #include <stdint.h>
 #include <string.h>
 #include <sys/param.h>
+#include <libc-internal.h>
 
 #ifndef STRING_TYPE
 # define STRING_TYPE char
@@ -170,7 +171,19 @@  get_next_seq (coll_seq *seq, int nrules, const unsigned char *rulesets,
 	    }
 	}
 
+      /* With GCC 5.3 when compiling with -Os the compiler complains
+	 that idx, taken from seq->idx (seq1 or seq2 from STRCOLL) may
+	 be used uninitialized.  In general this can't possibly be true
+	 since seq1.idx and seq2.idx are initialized to zero in the
+	 outer function.  Only one case where seq->idx is restored from
+	 seq->save_idx might result in an uninitialized idx value, but
+	 it is guarded by a sequence of checks against backw_stop which
+	 ensures that seq->save_idx was saved to first and contains a
+	 valid value.  */
+      DIAG_PUSH_NEEDS_COMMENT;
+      DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
       len = weights[idx++];
+      DIAG_POP_NEEDS_COMMENT;
       /* Skip over indices of previous levels.  */
       for (int i = 0; i < pass; i++)
 	{