[PATCH] Fix -Os related -Werror failures.

[PATCH] Fix -Os related -Werror failures.

[Carlos O'Donell]

While investigating bug 20729 I decided to fix up the -Wmaybe-uninitialized
errors caused by building with -Os rather than build with -Wno-error.
The comments provide all the details.

No regressions on x86_64 and x86 (building with -O2). There are _tons_
of failures building with -Os, but nothing that is novel, lots of linkspace
failures because of lack of inlining, and lots of extra PLT references that
should not be in the dynamic loader, all things that would need fixing.

Is there value in checking in these fixes then to let others work out the
-Os failures?

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

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

diff --git a/iconvdata/iso-2022-cn-ext.c b/iconvdata/iso-2022-cn-ext.c
index df5b5df..6c9fc97 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_NEEDS_COMMENT (5.3, "-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/locale/weight.h b/locale/weight.h
index c99730c..4c35313 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_NEEDS_COMMENT (5.3, "-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..5dcfb2e 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_NEEDS_COMMENT (5.3, "-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..116a546 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_NEEDS_COMMENT (5.3, "-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..19a4c1f 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -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.3, "-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..8e689ba 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
       len = weights[idx++];
+      DIAG_POP_NEEDS_COMMENT;
       /* Skip over indices of previous levels.  */
       for (int i = 0; i < pass; i++)
 	{
---

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Andreas Schwab]

On Okt 28 2016, Carlos O'Donell <carlos@redhat.com> wrote:

> +DIAG_IGNORE_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");

That should use 5, we never distinguish different patch releases.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix -Os related -Werror failures.

[Florian Weimer]

On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");

This hides the warning for -O2 builds as well, so I don't think this is 
a good idea.

Those who want to build with -Os or other special compiler flags should 
just configure with --disable-werror.  We can't account for every 
optimization someone might want to disable in their build.

Florian

Re: [PATCH] Fix -Os related -Werror failures.

[Jeff Law]

On 10/28/2016 12:32 AM, Florian Weimer wrote:
> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>
> This hides the warning for -O2 builds as well, so I don't think this is
> a good idea.
>
> Those who want to build with -Os or other special compiler flags should
> just configure with --disable-werror.  We can't account for every
> optimization someone might want to disable in their build.
That'd be my recommendation.

What often happens in these cases is the compiler in its default mode of 
operation is able to statically eliminate a conditional branch on a 
particular path.  However, to do so the compiler has to duplicate code.

Not surprisingly, there's a cost/benefit tradeoff here and the 
heuristics are largely driven by the real or estimated profile data as 
well as the coarser "optimize for code space".  So changing flags 
changes the output of those heuristics and ultimately can result in 
leaving paths in the CFG that can not be executed -- and that often 
leads to false positive may-be-uninitialized warnings and such.

Long term I would like to find a good way to mark paths that are not 
executable, but are not profitable to eliminate, then utilize that 
information to prune various "may" warnings.  That would make those kind 
of warnings more stable across different optimization levels as well as 
more stable release-to-release.  But that's definitely in the "future 
work" area.

jeff

Re: [PATCH] Fix -Os related -Werror failures.

[Arnd Bergmann]

On Friday, October 28, 2016 12:44:32 AM CEST Jeff Law wrote:
> On 10/28/2016 12:32 AM, Florian Weimer wrote:
> > On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
> >> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
> >
> > This hides the warning for -O2 builds as well, so I don't think this is
> > a good idea.
> >
> > Those who want to build with -Os or other special compiler flags should
> > just configure with --disable-werror.  We can't account for every
> > optimization someone might want to disable in their build.
> That'd be my recommendation.
> 
> What often happens in these cases is the compiler in its default mode of 
> operation is able to statically eliminate a conditional branch on a 
> particular path.  However, to do so the compiler has to duplicate code.
> 
> Not surprisingly, there's a cost/benefit tradeoff here and the 
> heuristics are largely driven by the real or estimated profile data as 
> well as the coarser "optimize for code space".  So changing flags 
> changes the output of those heuristics and ultimately can result in 
> leaving paths in the CFG that can not be executed -- and that often 
> leads to false positive may-be-uninitialized warnings and such.
> 
> Long term I would like to find a good way to mark paths that are not 
> executable, but are not profitable to eliminate, then utilize that 
> information to prune various "may" warnings.  That would make those kind 
> of warnings more stable across different optimization levels as well as 
> more stable release-to-release.  But that's definitely in the "future 
> work" area.

I've spent a lot of time trying to eliminate -Wmaybe-uninitialized
warnings from the Linux kernel. Here are some data points that you
may find useful too:

- Building with -Os causes many false positives starting with gcc-4.9,
  and I have disabled the warning for this specific flag. I believe
  this is due to the lack of the "-fschedule-insns" optimization step
- Building with -O3 apparently also causes some false positives, but
  we don't normally do that in the kernel, and the only architecture
  port that does it also disables the warnings
- Two more gcc options that cause false positives are -fprofile-arcs
  and some of the -fsanitize=... options
- overall, gcc-4.9 improved much over gcc-4.8 in these warnings,
  but they have a different set of false-positives. As gcc-4.8 is
  getting old, I'm pushing a patch to also disable the warning
  for all 4.8 builds. Prior to v4.8, there was no option to disable
  maybe-uninitialized warnings.
- gcc-5 and gcc-6 appear to be slightly better than gcc-4.9 but also
  introduce a small number of additional false-positive warnings,
  apparently this happens mostly because they make different
  inlining decisions, not because something fundamentally changed.
  Generally speaking, if any of 4.9, 4.x or 5.x produce a warning
  in some configurations, it's likely that the other ones will
  do the same, depending on a combination target architecture and
  optimization flags that impact inlining.
- I found that most often when gcc is confused about whether a
  variable is uninitialized or not, the source code tends to be
  confusing to a human reader as well and rewriting it differently
  results in better readability and better object code while
  avoiding the warning. There are always other cases in which
  this is not possible though.

	Arnd

Re: [PATCH] Fix -Os related -Werror failures.

[Andrew Pinski]

On Fri, Oct 28, 2016 at 1:12 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, October 28, 2016 12:44:32 AM CEST Jeff Law wrote:
>> On 10/28/2016 12:32 AM, Florian Weimer wrote:
>> > On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>> >> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>> >
>> > This hides the warning for -O2 builds as well, so I don't think this is
>> > a good idea.
>> >
>> > Those who want to build with -Os or other special compiler flags should
>> > just configure with --disable-werror.  We can't account for every
>> > optimization someone might want to disable in their build.
>> That'd be my recommendation.
>>
>> What often happens in these cases is the compiler in its default mode of
>> operation is able to statically eliminate a conditional branch on a
>> particular path.  However, to do so the compiler has to duplicate code.
>>
>> Not surprisingly, there's a cost/benefit tradeoff here and the
>> heuristics are largely driven by the real or estimated profile data as
>> well as the coarser "optimize for code space".  So changing flags
>> changes the output of those heuristics and ultimately can result in
>> leaving paths in the CFG that can not be executed -- and that often
>> leads to false positive may-be-uninitialized warnings and such.
>>
>> Long term I would like to find a good way to mark paths that are not
>> executable, but are not profitable to eliminate, then utilize that
>> information to prune various "may" warnings.  That would make those kind
>> of warnings more stable across different optimization levels as well as
>> more stable release-to-release.  But that's definitely in the "future
>> work" area.
>
> I've spent a lot of time trying to eliminate -Wmaybe-uninitialized
> warnings from the Linux kernel. Here are some data points that you
> may find useful too:
>
> - Building with -Os causes many false positives starting with gcc-4.9,
>   and I have disabled the warning for this specific flag. I believe
>   this is due to the lack of the "-fschedule-insns" optimization step

No this is false.  It is usually due to jump threading is not as
aggressive at -O2 than -Os due to -Os not wanting to increase code
size.

Thanks,
Andrew

> - Building with -O3 apparently also causes some false positives, but
>   we don't normally do that in the kernel, and the only architecture
>   port that does it also disables the warnings
> - Two more gcc options that cause false positives are -fprofile-arcs
>   and some of the -fsanitize=... options
> - overall, gcc-4.9 improved much over gcc-4.8 in these warnings,
>   but they have a different set of false-positives. As gcc-4.8 is
>   getting old, I'm pushing a patch to also disable the warning
>   for all 4.8 builds. Prior to v4.8, there was no option to disable
>   maybe-uninitialized warnings.
> - gcc-5 and gcc-6 appear to be slightly better than gcc-4.9 but also
>   introduce a small number of additional false-positive warnings,
>   apparently this happens mostly because they make different
>   inlining decisions, not because something fundamentally changed.
>   Generally speaking, if any of 4.9, 4.x or 5.x produce a warning
>   in some configurations, it's likely that the other ones will
>   do the same, depending on a combination target architecture and
>   optimization flags that impact inlining.
> - I found that most often when gcc is confused about whether a
>   variable is uninitialized or not, the source code tends to be
>   confusing to a human reader as well and rewriting it differently
>   results in better readability and better object code while
>   avoiding the warning. There are always other cases in which
>   this is not possible though.
>
>         Arnd

Re: [PATCH] Fix -Os related -Werror failures.

[Carlos O'Donell]

On 10/28/2016 02:32 AM, Florian Weimer wrote:
> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
> 
> This hides the warning for -O2 builds as well, so I don't think this is a good idea.
> 
> Those who want to build with -Os or other special compiler flags
> should just configure with --disable-werror. We can't account for
> every optimization someone might want to disable in their build.

I agree that we can't account for _all_ optimizations someone might want
to disable in their build, but I think it is a reasonable goal to target
a few key _default_ optimization including -O3, -O2, and -Os.

In the case above we only limit the emitted warnings for the narrow
code involved in iso-2022-cn-ext conversions. I'd be more worried if it
required a widely used function with broadly disabled warnings.

I agree with Arnd that this code is _overly_ complex and could be
rewritten such that it's a little clearer and makes sense to the compiler
at -Os.

Should I try to cleanup the BODY code a bit to remove this particular
diagnostic disabling?

I know we've had several real uninitialized variable problems in the
conversion code recently, so I'm also interested in having the compiler
help us find more of these problems.

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Florian Weimer]

On 10/28/2016 02:08 PM, Carlos O'Donell wrote:
> On 10/28/2016 02:32 AM, Florian Weimer wrote:
>> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>>> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>>
>> This hides the warning for -O2 builds as well, so I don't think this is a good idea.
>>
>> Those who want to build with -Os or other special compiler flags
>> should just configure with --disable-werror. We can't account for
>> every optimization someone might want to disable in their build.
>
> I agree that we can't account for _all_ optimizations someone might want
> to disable in their build, but I think it is a reasonable goal to target
> a few key _default_ optimization including -O3, -O2, and -Os.
>
> In the case above we only limit the emitted warnings for the narrow
> code involved in iso-2022-cn-ext conversions. I'd be more worried if it
> required a widely used function with broadly disabled warnings.

We can probably find a compiler flag which needs this for a central 
function.

This might not look like a lot of work now, but it's an ongoing effort, 
for every target, GCC version, and flag variant.  It does not help 
default builds at all (in fact, it has some slight risk interfering with 
future development because we might miss some warning as a result).  I 
think it's just a distraction.

And with -Os in particular, the resulting libc is not really ABI-compliant.

Florian

Re: [PATCH] Fix -Os related -Werror failures.

[Joseph Myers]

On Fri, 28 Oct 2016, Florian Weimer wrote:

> Those who want to build with -Os or other special compiler flags should just
> configure with --disable-werror.  We can't account for every optimization
> someone might want to disable in their build.

I don't think --disable-werror should be encouraged.  We could add DIAG_* 
macro variants that do nothing except with -Os so don't disable the 
warnings in other cases, if there isn't a code cleanup to avoid the 
warnings.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix -Os related -Werror failures.

[Florian Weimer]

On 10/28/2016 02:49 PM, Joseph Myers wrote:
> On Fri, 28 Oct 2016, Florian Weimer wrote:
>
>> Those who want to build with -Os or other special compiler flags should just
>> configure with --disable-werror.  We can't account for every optimization
>> someone might want to disable in their build.
>
> I don't think --disable-werror should be encouraged.

-Wmaybe-uninitialized warnings can be issued very late from the 
optimizers, so this is very much a corner case in terms of usefulness 
for -Werror because it is practically guaranteed to have new false 
positives with unusual architectures, compiler versions, and 
optimization flags.

If the presence of this warning in particular leads people to use 
--disable-werror, maybe we should remove it from the default set of 
warnings which trigger errors.

Florian

Re: [PATCH] Fix -Os related -Werror failures.

[Joseph Myers]

On Fri, 28 Oct 2016, Carlos O'Donell wrote:

> I agree that we can't account for _all_ optimizations someone might want
> to disable in their build, but I think it is a reasonable goal to target
> a few key _default_ optimization including -O3, -O2, and -Os.

And -O1 (bug 19444).  And -Og for debuggability.

> I agree with Arnd that this code is _overly_ complex and could be
> rewritten such that it's a little clearer and makes sense to the compiler
> at -Os.

In general, making code clearer is a good approach.

For -O1 / -Og I'd also be willing to consider default initializers as a 
less verbose way than DIAG_* macros of making it obvious to the compiler 
that a variable is initialized (at -Os, that may not be such a good idea 
because presumably the user cares about saving a few instructions from an 
unnecessary initialization).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix -Os related -Werror failures.

[Carlos O'Donell]

On 10/28/2016 08:08 AM, Carlos O'Donell wrote:
> On 10/28/2016 02:32 AM, Florian Weimer wrote:
>> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>>> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>>
>> This hides the warning for -O2 builds as well, so I don't think this is a good idea.
>>
>> Those who want to build with -Os or other special compiler flags
>> should just configure with --disable-werror. We can't account for
>> every optimization someone might want to disable in their build.
> 
> I agree that we can't account for _all_ optimizations someone might want
> to disable in their build, but I think it is a reasonable goal to target
> a few key _default_ optimization including -O3, -O2, and -Os.
> 
> In the case above we only limit the emitted warnings for the narrow
> code involved in iso-2022-cn-ext conversions. I'd be more worried if it
> required a widely used function with broadly disabled warnings.
> 
> I agree with Arnd that this code is _overly_ complex and could be
> rewritten such that it's a little clearer and makes sense to the compiler
> at -Os.
> 
> Should I try to cleanup the BODY code a bit to remove this particular
> diagnostic disabling?
> 
> I know we've had several real uninitialized variable problems in the
> conversion code recently, so I'm also interested in having the compiler
> help us find more of these problems.

For example, initializing the tmpbuf in this fallback case is enough to
silence the compiler warning:

diff --git a/iconvdata/iso-2022-cn-ext.c b/iconvdata/iso-2022-cn-ext.c
index df5b5df..d0b32df 100644
--- a/iconvdata/iso-2022-cn-ext.c
+++ b/iconvdata/iso-2022-cn-ext.c
@@ -456,7 +456,7 @@ enum
              used = CNS11643_2_set;                                          \
            else                                                              \
              {                                                               \
-               unsigned char tmpbuf[3];                                      \
+               unsigned char tmpbuf[3] = { 0, 0, 0 };                        \
                                                                              \
                switch (0)                                                    \
                  {                                                           \
---

We already initialize buf similarly e.g. 
429         unsigned char buf[2] = { 0, 0 };                                      \

At -Os the compiler is unable to determine if tmpbuf can or can't be used
in one of the failure cases e.g. return __UNKNOWN_10646_CHAR;.

This particular case we are into the 3rd conversion attempt of an unknown
character, so it can't possibly be a performance case to zero tmpbuf and
simplify the analysis for all kinds of static analysis tooling.

Thoughts?

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Carlos O'Donell]

On 10/28/2016 08:55 AM, Florian Weimer wrote:
> On 10/28/2016 02:49 PM, Joseph Myers wrote:
>> On Fri, 28 Oct 2016, Florian Weimer wrote:
>> 
>>> Those who want to build with -Os or other special compiler flags
>>> should just configure with --disable-werror.  We can't account
>>> for every optimization someone might want to disable in their
>>> build.
>> 
>> I don't think --disable-werror should be encouraged.
> 
> -Wmaybe-uninitialized warnings can be issued very late from the
> optimizers, so this is very much a corner case in terms of usefulness
> for -Werror because it is practically guaranteed to have new false
> positives with unusual architectures, compiler versions, and
> optimization flags.
> 
> If the presence of this warning in particular leads people to use
> --disable-werror, maybe we should remove it from the default set of
> warnings which trigger errors.

Remove -Wmaybe-uninitialized?

That destroys some of the value of the warning and means we don't
interact with upstream gcc to make it better, either during initial
review or reviews when the gcc version gets new enough that we audit
the diagnostic.

I would rather follow Joseph's suggestion of adding optimization
specific DIAG_* macros.

e.g.
	DIAG_IGNORE_O3_NEEDS_COMMENT
	DIAG_IGNORE_O2_NEEDS_COMMENT
	DIAG_IGNORE_O1_NEEDS_COMMENT
	DIAG_IGNORE_Os_NEEDS_COMMENT
	DIAG_IGNORE_Og_NEEDS_COMMENT

Where the diagnostic is only ignored for the relevant optimization
mode.

This way the patch I just suggested would use the *_Os_* variants
and not interfere with -O2 builds. Since the kinds of warnings
generated are rather tightly coupled with the optimization passes
that are enabled, it makes sense to specialize them a bit.

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Jeff Law]

On 10/28/2016 02:17 AM, Andrew Pinski wrote:
> On Fri, Oct 28, 2016 at 1:12 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday, October 28, 2016 12:44:32 AM CEST Jeff Law wrote:
>>> On 10/28/2016 12:32 AM, Florian Weimer wrote:
>>>> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>>>>> +/* 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_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>>>>
>>>> This hides the warning for -O2 builds as well, so I don't think this is
>>>> a good idea.
>>>>
>>>> Those who want to build with -Os or other special compiler flags should
>>>> just configure with --disable-werror.  We can't account for every
>>>> optimization someone might want to disable in their build.
>>> That'd be my recommendation.
>>>
>>> What often happens in these cases is the compiler in its default mode of
>>> operation is able to statically eliminate a conditional branch on a
>>> particular path.  However, to do so the compiler has to duplicate code.
>>>
>>> Not surprisingly, there's a cost/benefit tradeoff here and the
>>> heuristics are largely driven by the real or estimated profile data as
>>> well as the coarser "optimize for code space".  So changing flags
>>> changes the output of those heuristics and ultimately can result in
>>> leaving paths in the CFG that can not be executed -- and that often
>>> leads to false positive may-be-uninitialized warnings and such.
>>>
>>> Long term I would like to find a good way to mark paths that are not
>>> executable, but are not profitable to eliminate, then utilize that
>>> information to prune various "may" warnings.  That would make those kind
>>> of warnings more stable across different optimization levels as well as
>>> more stable release-to-release.  But that's definitely in the "future
>>> work" area.
>>
>> I've spent a lot of time trying to eliminate -Wmaybe-uninitialized
>> warnings from the Linux kernel. Here are some data points that you
>> may find useful too:
>>
>> - Building with -Os causes many false positives starting with gcc-4.9,
>>   and I have disabled the warning for this specific flag. I believe
>>   this is due to the lack of the "-fschedule-insns" optimization step
>
> No this is false.  It is usually due to jump threading is not as
> aggressive at -O2 than -Os due to -Os not wanting to increase code
> size.
Correct.  The scheduler has nothing to do with this issue, it's 
primarily jump threading.  At -Os we don't allow as much block copying 
which results in many jump threads not being optimized.  The reduced 
jump threading leaves unexecutable paths in the CFG and results in false 
positive -Wuninitialized errors.

PGO can have similar effects as profiling data may indicate that a 
particular path is not worth duplicating to eliminate a conditional

Inlining goes both ways.  By exposing more code to the optimizers, we 
can often do a better job at eliminating false positives.  But it is 
also the case that inlining may remove the addressability property on an 
object which in turn exposes the object to these kinds of analysis.

Similarly for SRA, function splitting, etc etc.
Jeff

[PATCH v2] Fix -Os related build and test failures.

[Carlos O'Donell]

On 10/28/2016 09:18 AM, Carlos O'Donell wrote:
> I would rather follow Joseph's suggestion of adding optimization
> specific DIAG_* macros.
> 
> e.g.
> 	DIAG_IGNORE_O3_NEEDS_COMMENT
> 	DIAG_IGNORE_O2_NEEDS_COMMENT
> 	DIAG_IGNORE_O1_NEEDS_COMMENT
> 	DIAG_IGNORE_Os_NEEDS_COMMENT
> 	DIAG_IGNORE_Og_NEEDS_COMMENT
> 
> Where the diagnostic is only ignored for the relevant optimization
> mode.
> 
> This way the patch I just suggested would use the *_Os_* variants
> and not interfere with -O2 builds. Since the kinds of warnings
> generated are rather tightly coupled with the optimization passes
> that are enabled, it makes sense to specialize them a bit.

v2
- Use '5' only as the version identifier for the GCC impacted as
  requested by Andreas and noted in the comments for the diagnostic.
- Fix math/test-nan-overflow.c which uses malloc but doesn't include
  stdlib.h.
- 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.

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

	* include/libc-internal.h (DIAG_IGNORE_Ox_NEEDS_COMMENT): Define.
	(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.
	* math/test-nan-overflow.c: Include stdlib.h for malloc.

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..cba8c7c 100644
--- a/include/libc-internal.h
+++ b/include/libc-internal.h
@@ -111,4 +111,39 @@ 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 macros ignore the
+   diagnostic OPTION but only when:
+
+   - optimizations are enabled (DIAG_IGNORE_Ox_NEEDS_COMMENT) or 
+
+   - optimizations for size are enabled (DIAG_IGNORE_Os_NEEDS_COMMENT).
+
+   These are 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 in -Os, but at -O2 you would
+   still like the warning to be enabled to catch errors.  In this case
+   you could use DIAG_IGNORE_Os_NEEDS_COMMENT to disable the warning
+   only for -Os.
+
+   At present we don't actually distinguish between the various levels
+   of optimization because the compiler doesn't allow that easily, but
+   we do distinguish between -O[1,2,3,g] and -Os.  Therefore use
+   DIAG_IGNORE_Ox_NEEDS_COMMENT for diagnostics that should only be
+   ignored when any optimizations are present (including -Os), and
+   DIAG_IGNORE_Os_NEEDS_COMMENT for diagnostics that should only be
+   ignored when space optimizations are in effect (only -Os).  */
+#if __OPTIMIZE__
+#define DIAG_IGNORE_Ox_NEEDS_COMMENT(version, option)	\
+  _Pragma (_DIAG_STR (GCC diagnostic ignored option))
+#else
+#define DIAG_IGNORE_Ox_NEEDS_COMMENT(version, option)
+#endif
+
+#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/math/test-nan-overflow.c b/math/test-nan-overflow.c
index 40aae2e..745a044 100644
--- a/math/test-nan-overflow.c
+++ b/math/test-nan-overflow.c
@@ -20,6 +20,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <sys/resource.h>
+#include <stdlib.h>
 
 #define STACK_LIM 1048576
 #define STRING_SIZE (2 * STACK_LIM)
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.3, "-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.3, "-Wmaybe-uninitialized");
       len = weights[idx++];
+      DIAG_POP_NEEDS_COMMENT;
       /* Skip over indices of previous levels.  */
       for (int i = 0; i < pass; i++)
 	{
---

-- 
Cheers,
Carlos.

Re: [PATCH v2] Fix -Os related build and test failures.

[Joseph Myers]

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).

> - 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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix -Os related -Werror failures.

[Paul Eggert]

On 10/28/2016 01:12 AM, Arnd Bergmann wrote:
> I found that most often when gcc is confused about whether a variable is uninitialized or not, the source code tends to be confusing to a human reader as well and rewriting it differently results in better readability and better object code while avoiding the warning.

I'm afraid my experience has not been so good. Maybe 1/3 of the time rewriting is better, but otherwise rewriting doesn't help or even confuses the code. When that happens with -Wmaybe-uninitialized, in Emacs we typically use C declarations like this:

       ptrdiff_t offset2 UNINIT; /* The UNINIT works around GCC bug 
78081.  */

where UNINIT is defined something like this:

   #ifdef GCC_LINT
   # define UNINIT = {0,}
   #else
   # define UNINIT /* empty */
   #endif

and configuring with --enable-gcc-warnings compiles with something like 
'gcc -Wall -Werror -DGCC_LINT'.

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

[Carlos O'Donell]

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.

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++)
 	{
---

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Jeff Law]

On 10/28/2016 02:10 PM, Paul Eggert wrote:
> On 10/28/2016 01:12 AM, Arnd Bergmann wrote:
>> I found that most often when gcc is confused about whether a variable
>> is uninitialized or not, the source code tends to be confusing to a
>> human reader as well and rewriting it differently results in better
>> readability and better object code while avoiding the warning.
>
> I'm afraid my experience has not been so good. Maybe 1/3 of the time
> rewriting is better, but otherwise rewriting doesn't help or even
> confuses the code. When that happens with -Wmaybe-uninitialized, in
> Emacs we typically use C declarations like this:
>
>       ptrdiff_t offset2 UNINIT; /* The UNINIT works around GCC bug
> 78081.  */
And I would echo that markup indicating that the initializer is to work 
around a GCC false positive is something I wish we had strictly enforced 
within GCC itself when it was made Wuninitialized clean.

GCC has made significant strides in its jump threading and predicate 
analysis to significantly such false positives and many of these 
initializers could probably be removed at this point.

>
> where UNINIT is defined something like this:
>
>   #ifdef GCC_LINT
>   # define UNINIT = {0,}
>   #else
>   # define UNINIT /* empty */
>   #endif
>
> and configuring with --enable-gcc-warnings compiles with something like
> 'gcc -Wall -Werror -DGCC_LINT'.\
But I would caution against blindly using 0 as an initializer.  Each 
case has to be examined to determine what a safe value would be.  Often 
0 is appropriate, but there are certainly cases where it is not the safe 
initializer to use.

Jeff

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

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.

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

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 10/29/2016 01:35 PM, Joseph Myers wrote:
> 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.
 
Fixed.

I've committed the following after testing on i486 -Os and
x86_64 -O2.

v4
- Correct preprocessor indentation.

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.

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..1a386e7 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.  */
+#ifdef __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..4ec8c1a 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..5605dbf 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++)
 	{
---

-- 
Cheers,
Carlos.

Re: [PATCH] Fix -Os related -Werror failures.

[Paul Eggert]

Jeff Law wrote:
> I would caution against blindly using 0 as an initializer.

Yes, in Emacs we use 0 only when the value does not matter so all values are 
equally "safe". It's merely a convenience to use 0, as 0 is valid for pointers 
as well as for numbers so we can use the same macro for both. We're just trying 
to pacify GCC when warnings are enabled when developing; typically in production 
warnings are disabled and there is no initializer at all.

Re: [PATCH v4] Fix -Os related build and test failures.

[Andreas Schwab]

https://build.opensuse.org/project/monitor/home:Andreas_Schwab:glibc

In file included from strxfrm_l.c:48:0:
../locale/weight.h: In function ‘findidx’:
../locale/weight.h:69:4: error: ‘DIAG_PUSH_NEEDS_COMMENT’ undeclared (first use in this function)
    DIAG_PUSH_NEEDS_COMMENT;
    ^~~~~~~~~~~~~~~~~~~~~~~
../locale/weight.h:69:4: note: each undeclared identifier is reported only once for each function it appears in
../locale/weight.h:70:4: error: implicit declaration of function ‘DIAG_IGNORE_Os_NEEDS_COMMENT’ [-Werror=implicit-function-declaration]
    DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../locale/weight.h:74:4: error: ‘DIAG_POP_NEEDS_COMMENT’ undeclared (first use in this function)
    DIAG_POP_NEEDS_COMMENT;
    ^~~~~~~~~~~~~~~~~~~~~~

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 10/31/2016 04:33 AM, Andreas Schwab wrote:
> https://build.opensuse.org/project/monitor/home:Andreas_Schwab:glibc
> 
> In file included from strxfrm_l.c:48:0:
> ../locale/weight.h: In function ‘findidx’:
> ../locale/weight.h:69:4: error: ‘DIAG_PUSH_NEEDS_COMMENT’ undeclared (first use in this function)
>     DIAG_PUSH_NEEDS_COMMENT;
>     ^~~~~~~~~~~~~~~~~~~~~~~
> ../locale/weight.h:69:4: note: each undeclared identifier is reported only once for each function it appears in
> ../locale/weight.h:70:4: error: implicit declaration of function ‘DIAG_IGNORE_Os_NEEDS_COMMENT’ [-Werror=implicit-function-declaration]
>     DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../locale/weight.h:74:4: error: ‘DIAG_POP_NEEDS_COMMENT’ undeclared (first use in this function)
>     DIAG_POP_NEEDS_COMMENT;
>     ^~~~~~~~~~~~~~~~~~~~~~

I'm fixing this. I don't know why this didn't fail on my x86_64 build.

I'm moving the #include <libc-internal.h> into the weight header fragments.

Cheers,
Carlos.

Re: [PATCH v4] Fix -Os related build and test failures.

[Florian Weimer]

On 10/31/2016 10:16 AM, Carlos O'Donell wrote:

> I'm fixing this. I don't know why this didn't fail on my x86_64 build.

x86_64 includes <libc-internal.h> by default because the 
platform-specific headers need the cast_to_integer macro.

Florian

Re: [PATCH v4] Fix -Os related build and test failures.

[David Miller]

From: Carlos O'Donell <carlos@redhat.com>
Date: Mon, 31 Oct 2016 05:16:29 -0400

> On 10/31/2016 04:33 AM, Andreas Schwab wrote:
>> https://build.opensuse.org/project/monitor/home:Andreas_Schwab:glibc
>> 
>> In file included from strxfrm_l.c:48:0:
>> ../locale/weight.h: In function ‘findidx’:
>> ../locale/weight.h:69:4: error: ‘DIAG_PUSH_NEEDS_COMMENT’ undeclared (first use in this function)
>>     DIAG_PUSH_NEEDS_COMMENT;
>>     ^~~~~~~~~~~~~~~~~~~~~~~
>> ../locale/weight.h:69:4: note: each undeclared identifier is reported only once for each function it appears in
>> ../locale/weight.h:70:4: error: implicit declaration of function ‘DIAG_IGNORE_Os_NEEDS_COMMENT’ [-Werror=implicit-function-declaration]
>>     DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../locale/weight.h:74:4: error: ‘DIAG_POP_NEEDS_COMMENT’ undeclared (first use in this function)
>>     DIAG_POP_NEEDS_COMMENT;
>>     ^~~~~~~~~~~~~~~~~~~~~~
> 
> I'm fixing this. I don't know why this didn't fail on my x86_64 build.
> 
> I'm moving the #include <libc-internal.h> into the weight header fragments.

The thread debugging header nptl_db/thread_dbP.h needs it too.

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

[Steve Ellcey]

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

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

[Carlos O'Donell]

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.

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 10/31/2016 08:55 AM, David Miller wrote:
> From: Carlos O'Donell <carlos@redhat.com>
> Date: Mon, 31 Oct 2016 05:16:29 -0400
> 
>> On 10/31/2016 04:33 AM, Andreas Schwab wrote:
>>> https://build.opensuse.org/project/monitor/home:Andreas_Schwab:glibc
>>>
>>> In file included from strxfrm_l.c:48:0:
>>> ../locale/weight.h: In function Β‘findidxΒ’:
>>> ../locale/weight.h:69:4: error: Β‘DIAG_PUSH_NEEDS_COMMENTΒ’ undeclared (first use in this function)
>>>     DIAG_PUSH_NEEDS_COMMENT;
>>>     ^~~~~~~~~~~~~~~~~~~~~~~
>>> ../locale/weight.h:69:4: note: each undeclared identifier is reported only once for each function it appears in
>>> ../locale/weight.h:70:4: error: implicit declaration of function Β‘DIAG_IGNORE_Os_NEEDS_COMMENTΒ’ [-Werror=implicit-function-declaration]
>>>     DIAG_IGNORE_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ../locale/weight.h:74:4: error: Β‘DIAG_POP_NEEDS_COMMENTΒ’ undeclared (first use in this function)
>>>     DIAG_POP_NEEDS_COMMENT;
>>>     ^~~~~~~~~~~~~~~~~~~~~~
>>
>> I'm fixing this. I don't know why this didn't fail on my x86_64 build.
>>
>> I'm moving the #include <libc-internal.h> into the weight header fragments.
> 
> The thread debugging header nptl_db/thread_dbP.h needs it too.

Yes. I'm fixing any file to include the header as required.
I was too clever in thinking I could elide it for header
fragments included in other source files, I should have just
followed the rules we had in place "if it needs it it should
include it" since that fixed the messes we previously had.

I'm going to setup a non-x86_64 cross-build to test this.
I think I can do it easily enough on Fedora so I can keep
it up to date for upstream builds.

Cheers,
Carlos.
 

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

[Steve Ellcey]

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

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

[Carlos O'Donell]

On 10/31/2016 03:56 PM, Steve Ellcey wrote:
> 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
> 

This should fix it. Sorry for the lack of non-x86/x86_64 testing.

Like I said I'll set about setting up more crosses like the kernel
people do.

commit bb5badf17087099dd9140f812778f7a8615b2111
Author: Carlos O'Donell <carlos@redhat.com>
Date:   Mon Oct 31 16:46:57 2016 -0400

    Bug 20729: Include libc-internal.h where required.
    
    The original fix for bug 20729 failed to include
    libc-internal.h in the files that needed them and
    this caused build failures on machines that don't
    implicitly include this header. This commit fixes
    that by following the consensus rule that a header,
    if needed, should always be directly included.

diff --git a/locale/weight.h b/locale/weight.h
index 1f61f01..19b8e4a 100644
--- a/locale/weight.h
+++ b/locale/weight.h
@@ -19,6 +19,8 @@
 #ifndef _WEIGHT_H_
 #define _WEIGHT_H_	1
 
+#include <libc-internal.h>
+
 /* Find index of weight.  */
 static inline int32_t __attribute__ ((always_inline))
 findidx (const int32_t *table,
diff --git a/locale/weightwc.h b/locale/weightwc.h
index e42ce13..ae18965 100644
--- a/locale/weightwc.h
+++ b/locale/weightwc.h
@@ -19,6 +19,8 @@
 #ifndef _WEIGHTWC_H_
 #define _WEIGHTWC_H_	1
 
+#include <libc-internal.h>
+
 /* Find index of weight.  */
 static inline int32_t __attribute__ ((always_inline))
 findidx (const int32_t *table,
diff --git a/nptl_db/thread_dbP.h b/nptl_db/thread_dbP.h
index b53f1c1..f448547 100644
--- a/nptl_db/thread_dbP.h
+++ b/nptl_db/thread_dbP.h
@@ -30,6 +30,7 @@
 #include "../nptl/pthreadP.h"  	/* This is for *_BITMASK only.  */
 #include <list.h>
 #include <gnu/lib-names.h>
+#include <libc-internal.h>
 
 /* Indeces for the symbol names.  */
 enum
---

Cheers,
Carlos.

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

[Steve Ellcey]

On Mon, 2016-10-31 at 16:50 -0400, Carlos O'Donell wrote:
> 
> > 
> This should fix it. Sorry for the lack of non-x86/x86_64 testing.
> 
> Like I said I'll set about setting up more crosses like the kernel
> people do.


I checked out the new Top-of-tree and everything built fine.

Thanks,
Steve Ellcey

Re: [PATCH v4] Fix -Os related build and test failures.

[Andreas Schwab]

On Okt 30 2016, Carlos O'Donell <carlos@redhat.com> wrote:

> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index 6d46bb2..4ec8c1a 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];

That breaks powerpc and s390.

res_send.c: In function 'send_vc':
res_send.c:668:6: error: 'resplen' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  int resplen;
      ^~~~~~~

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v4] Fix -Os related build and test failures.

[Joseph Myers]

On Tue, 1 Nov 2016, Andreas Schwab wrote:

> On Okt 30 2016, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> > diff --git a/resolv/res_send.c b/resolv/res_send.c
> > index 6d46bb2..4ec8c1a 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];
> 
> That breaks powerpc and s390.
> 
> res_send.c: In function 'send_vc':
> res_send.c:668:6: error: 'resplen' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   int resplen;
>       ^~~~~~~

And the other change to the same file introduces a new use of 
DIAG_IGNORE_NEEDS_COMMENT with a comment that only mentions -Os.  Was the 
intent to edit the latter use to be DIAG_IGNORE_Os_NEEDS_COMMENT, with the 
former one edited by mistake instead?

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH v4] Fix -Os related build and test failures.

[Tamar Christina]

This also breaks ARM and AArch64,

Would it be worth reverting the commit until this is fixed?
It's currently blocking trunk builds.

Kind Regards,
Tamar

> -----Original Message-----
> From: libc-alpha-owner@sourceware.org [mailto:libc-alpha-
> owner@sourceware.org] On Behalf Of Joseph Myers
> Sent: 01 November 2016 11:13
> To: Andreas Schwab
> Cc: Carlos O'Donell; Florian Weimer; GNU C Library
> Subject: Re: [PATCH v4] Fix -Os related build and test failures.
> 
> On Tue, 1 Nov 2016, Andreas Schwab wrote:
> 
> > On Okt 30 2016, Carlos O'Donell <carlos@redhat.com> wrote:
> >
> > > diff --git a/resolv/res_send.c b/resolv/res_send.c index
> > > 6d46bb2..4ec8c1a 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];
> >
> > That breaks powerpc and s390.
> >
> > res_send.c: In function 'send_vc':
> > res_send.c:668:6: error: 'resplen' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> >   int resplen;
> >       ^~~~~~~
> 
> And the other change to the same file introduces a new use of
> DIAG_IGNORE_NEEDS_COMMENT with a comment that only mentions -Os.
> Was the intent to edit the latter use to be
> DIAG_IGNORE_Os_NEEDS_COMMENT, with the former one edited by
> mistake instead?
> 
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH v4] Fix -Os related build and test failures.

[David Miller]

From: Tamar Christina <Tamar.Christina@arm.com>
Date: Tue, 1 Nov 2016 15:58:20 +0000

> This also breaks ARM and AArch64,
> 
> Would it be worth reverting the commit until this is fixed?
> It's currently blocking trunk builds.

Mainline has been fixed already.

RE: [PATCH v4] Fix -Os related build and test failures.

[Tamar Christina]

Ah, OK. Hadn't noticed.

Thanks!

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: 01 November 2016 16:06
> To: Tamar Christina
> Cc: joseph@codesourcery.com; schwab@linux-m68k.org;
> carlos@redhat.com; fweimer@redhat.com; libc-alpha@sourceware.org; nd;
> Bin Cheng
> Subject: Re: [PATCH v4] Fix -Os related build and test failures.
> 
> From: Tamar Christina <Tamar.Christina@arm.com>
> Date: Tue, 1 Nov 2016 15:58:20 +0000
> 
> > This also breaks ARM and AArch64,
> >
> > Would it be worth reverting the commit until this is fixed?
> > It's currently blocking trunk builds.
> 
> Mainline has been fixed already.

Re: [PATCH v4] Fix -Os related build and test failures.

[Joseph Myers]

On Mon, 31 Oct 2016, Carlos O'Donell wrote:

> I'm going to setup a non-x86_64 cross-build to test this.
> I think I can do it easily enough on Fedora so I can keep
> it up to date for upstream builds.

FYI: I'm writing a script to run glibc builds (and the part of the tests 
that don't involve running host code), including building the cross 
compilers, for lots of configurations (a rough sort of equivalent to GCC's 
contrib/config-list.mk).  I think such a script would be useful to have in 
glibc, though it would take to long to use for testing most patches (it 
should aim to cover all ABI variants in 
<https://sourceware.org/glibc/wiki/ABIList> and enough other variants e.g. 
for CPUs using different sysdeps directories to test building each piece 
of code in glibc at least once) - but it might be useful to have a bot 
running it continuously and watching for regressions.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 11/01/2016 12:06 PM, David Miller wrote:
> From: Tamar Christina <Tamar.Christina@arm.com>
> Date: Tue, 1 Nov 2016 15:58:20 +0000
> 
>> This also breaks ARM and AArch64,
>>
>> Would it be worth reverting the commit until this is fixed?
>> It's currently blocking trunk builds.
> 
> Mainline has been fixed already.

No, there is one more issue left. My automated cleanup script
turned one of the DIAG's into an -Os diag by error. Joseph
noticed this and I'll fix it right now.

I'm going to push out a Fedora Rawhide build to verify this
for: arm, aarch64, ppc64le, ppc64, s390, s390x, x86, and
x86_64.

Cheers,
Carlos.

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 11/01/2016 06:58 PM, Joseph Myers wrote:
> On Mon, 31 Oct 2016, Carlos O'Donell wrote:
> 
>> I'm going to setup a non-x86_64 cross-build to test this.
>> I think I can do it easily enough on Fedora so I can keep
>> it up to date for upstream builds.
> 
> FYI: I'm writing a script to run glibc builds (and the part of the tests 
> that don't involve running host code), including building the cross 
> compilers, for lots of configurations (a rough sort of equivalent to GCC's 
> contrib/config-list.mk).  I think such a script would be useful to have in 
> glibc, though it would take to long to use for testing most patches (it 
> should aim to cover all ABI variants in 
> <https://sourceware.org/glibc/wiki/ABIList> and enough other variants e.g. 
> for CPUs using different sysdeps directories to test building each piece 
> of code in glibc at least once) - but it might be useful to have a bot 
> running it continuously and watching for regressions.

That would be excellent. Please publish the script so that others can use it.
I have a similar script, but not yet setup for cross-compiles.

Cheers,
Carlos.

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 11/01/2016 07:12 AM, Joseph Myers wrote:
> On Tue, 1 Nov 2016, Andreas Schwab wrote:
> 
>> On Okt 30 2016, Carlos O'Donell <carlos@redhat.com> wrote:
>>
>>> diff --git a/resolv/res_send.c b/resolv/res_send.c
>>> index 6d46bb2..4ec8c1a 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];
>>
>> That breaks powerpc and s390.
>>
>> res_send.c: In function 'send_vc':
>> res_send.c:668:6: error: 'resplen' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>   int resplen;
>>       ^~~~~~~
> 
> And the other change to the same file introduces a new use of 
> DIAG_IGNORE_NEEDS_COMMENT with a comment that only mentions -Os.  Was the 
> intent to edit the latter use to be DIAG_IGNORE_Os_NEEDS_COMMENT, with the 
> former one edited by mistake instead?

It was indeed a mistake. I used a script to find and fix the uses I'd added,
but here I made a mistake.

I'm running a rawhide build check here to verify the fix fixes things for
x86, x86_64, aarch64, arm, ppc64, ppc64le, and s390.

Cheers,
Carlos.

Re: [PATCH v4] Fix -Os related build and test failures.

[Carlos O'Donell]

On 11/02/2016 07:52 AM, Carlos O'Donell wrote:
> On 11/01/2016 12:06 PM, David Miller wrote:
>> From: Tamar Christina <Tamar.Christina@arm.com>
>> Date: Tue, 1 Nov 2016 15:58:20 +0000
>>
>>> This also breaks ARM and AArch64,
>>>
>>> Would it be worth reverting the commit until this is fixed?
>>> It's currently blocking trunk builds.
>>
>> Mainline has been fixed already.
> 
> No, there is one more issue left. My automated cleanup script
> turned one of the DIAG's into an -Os diag by error. Joseph
> noticed this and I'll fix it right now.
> 
> I'm going to push out a Fedora Rawhide build to verify this
> for: arm, aarch64, ppc64le, ppc64, s390, s390x, x86, and
> x86_64.

The changes to fix bug 20729 introduced an error which removed an
ignore diagnostic from -O2 by using the new -Os related macro.
This broke ppc64 builds. The following patch fixes the mistake.
 
Tested on x86, x86_64, ppc64, ppc64le, arm, aarch64, and s390x.
 
Checked in.

2016-11-02  Florian Weimer  <fweimer@redhat.com>
           Carlos O'Donell  <carlos@redhat.com>

       [Bug #20729]
       * resolv/res_send.c (send_vc): Revert DIAG_IGNORE_Os_NEEDS_COMMENT
       change to non -Os related diagnostic.  Use DIAG_IGNORE_Os_NEEDS_COMMENT
       for -Os related change.

diff --git a/resolv/res_send.c b/resolv/res_send.c
index 4ec8c1a..e96d5d4 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_Os_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
+       DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");
        int resplen;
        DIAG_POP_NEEDS_COMMENT;
        struct iovec iov[4];
@@ -937,7 +937,7 @@ reopen (res_state statp, int *terrno, int ns)
                   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");
+               DIAG_IGNORE_Os_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);
---

Cheers,
Carlos.