* [PATCH] Fix -Os related -Werror failures. @ 2016-10-28 4:48 Carlos O'Donell 2016-10-28 6:25 ` Andreas Schwab 2016-10-28 6:32 ` Florian Weimer 0 siblings, 2 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-28 4:48 UTC (permalink / raw) To: GNU C Library, Florian Weimer 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 4:48 [PATCH] Fix -Os related -Werror failures Carlos O'Donell @ 2016-10-28 6:25 ` Andreas Schwab 2016-10-28 6:32 ` Florian Weimer 1 sibling, 0 replies; 43+ messages in thread From: Andreas Schwab @ 2016-10-28 6:25 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library, Florian Weimer 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." ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 4:48 [PATCH] Fix -Os related -Werror failures Carlos O'Donell 2016-10-28 6:25 ` Andreas Schwab @ 2016-10-28 6:32 ` Florian Weimer 2016-10-28 6:44 ` Jeff Law ` (2 more replies) 1 sibling, 3 replies; 43+ messages in thread From: Florian Weimer @ 2016-10-28 6:32 UTC (permalink / raw) To: Carlos O'Donell, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 6:32 ` Florian Weimer @ 2016-10-28 6:44 ` Jeff Law 2016-10-28 8:12 ` Arnd Bergmann 2016-10-28 12:09 ` Carlos O'Donell 2016-10-28 12:49 ` Joseph Myers 2 siblings, 1 reply; 43+ messages in thread From: Jeff Law @ 2016-10-28 6:44 UTC (permalink / raw) To: Florian Weimer, Carlos O'Donell, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 6:44 ` Jeff Law @ 2016-10-28 8:12 ` Arnd Bergmann 2016-10-28 8:17 ` Andrew Pinski 2016-10-28 20:10 ` Paul Eggert 0 siblings, 2 replies; 43+ messages in thread From: Arnd Bergmann @ 2016-10-28 8:12 UTC (permalink / raw) To: libc-alpha; +Cc: Jeff Law, Florian Weimer, Carlos O'Donell 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 8:12 ` Arnd Bergmann @ 2016-10-28 8:17 ` Andrew Pinski 2016-10-28 13:28 ` Jeff Law 2016-10-28 20:10 ` Paul Eggert 1 sibling, 1 reply; 43+ messages in thread From: Andrew Pinski @ 2016-10-28 8:17 UTC (permalink / raw) To: Arnd Bergmann Cc: GNU C Library, Jeff Law, Florian Weimer, Carlos O'Donell 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 8:17 ` Andrew Pinski @ 2016-10-28 13:28 ` Jeff Law 0 siblings, 0 replies; 43+ messages in thread From: Jeff Law @ 2016-10-28 13:28 UTC (permalink / raw) To: Andrew Pinski, Arnd Bergmann Cc: GNU C Library, Florian Weimer, Carlos O'Donell 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 8:12 ` Arnd Bergmann 2016-10-28 8:17 ` Andrew Pinski @ 2016-10-28 20:10 ` Paul Eggert 2016-10-29 3:03 ` Jeff Law 1 sibling, 1 reply; 43+ messages in thread From: Paul Eggert @ 2016-10-28 20:10 UTC (permalink / raw) To: Arnd Bergmann, libc-alpha; +Cc: Jeff Law, Florian Weimer, Carlos O'Donell 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'. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 20:10 ` Paul Eggert @ 2016-10-29 3:03 ` Jeff Law 2016-10-30 4:25 ` Paul Eggert 0 siblings, 1 reply; 43+ messages in thread From: Jeff Law @ 2016-10-29 3:03 UTC (permalink / raw) To: Paul Eggert, Arnd Bergmann, libc-alpha Cc: Florian Weimer, Carlos O'Donell 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-29 3:03 ` Jeff Law @ 2016-10-30 4:25 ` Paul Eggert 0 siblings, 0 replies; 43+ messages in thread From: Paul Eggert @ 2016-10-30 4:25 UTC (permalink / raw) To: Jeff Law, Arnd Bergmann, libc-alpha; +Cc: Florian Weimer, Carlos O'Donell 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 6:32 ` Florian Weimer 2016-10-28 6:44 ` Jeff Law @ 2016-10-28 12:09 ` Carlos O'Donell 2016-10-28 12:43 ` Florian Weimer ` (2 more replies) 2016-10-28 12:49 ` Joseph Myers 2 siblings, 3 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-28 12:09 UTC (permalink / raw) To: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 12:09 ` Carlos O'Donell @ 2016-10-28 12:43 ` Florian Weimer 2016-10-28 13:04 ` Joseph Myers 2016-10-28 13:07 ` Carlos O'Donell 2 siblings, 0 replies; 43+ messages in thread From: Florian Weimer @ 2016-10-28 12:43 UTC (permalink / raw) To: Carlos O'Donell, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 12:09 ` Carlos O'Donell 2016-10-28 12:43 ` Florian Weimer @ 2016-10-28 13:04 ` Joseph Myers 2016-10-28 13:07 ` Carlos O'Donell 2 siblings, 0 replies; 43+ messages in thread From: Joseph Myers @ 2016-10-28 13:04 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 12:09 ` Carlos O'Donell 2016-10-28 12:43 ` Florian Weimer 2016-10-28 13:04 ` Joseph Myers @ 2016-10-28 13:07 ` Carlos O'Donell 2 siblings, 0 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-28 13:07 UTC (permalink / raw) To: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 6:32 ` Florian Weimer 2016-10-28 6:44 ` Jeff Law 2016-10-28 12:09 ` Carlos O'Donell @ 2016-10-28 12:49 ` Joseph Myers 2016-10-28 12:55 ` Florian Weimer 2 siblings, 1 reply; 43+ messages in thread From: Joseph Myers @ 2016-10-28 12:49 UTC (permalink / raw) To: Florian Weimer; +Cc: Carlos O'Donell, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 12:49 ` Joseph Myers @ 2016-10-28 12:55 ` Florian Weimer 2016-10-28 13:18 ` Carlos O'Donell 0 siblings, 1 reply; 43+ messages in thread From: Florian Weimer @ 2016-10-28 12:55 UTC (permalink / raw) To: Joseph Myers; +Cc: Carlos O'Donell, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] Fix -Os related -Werror failures. 2016-10-28 12:55 ` Florian Weimer @ 2016-10-28 13:18 ` Carlos O'Donell 2016-10-28 13:58 ` [PATCH v2] Fix -Os related build and test failures Carlos O'Donell 0 siblings, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-10-28 13:18 UTC (permalink / raw) To: Florian Weimer, Joseph Myers; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2] Fix -Os related build and test failures. 2016-10-28 13:18 ` Carlos O'Donell @ 2016-10-28 13:58 ` Carlos O'Donell 2016-10-28 14:17 ` Joseph Myers 0 siblings, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-10-28 13:58 UTC (permalink / raw) To: Florian Weimer, Joseph Myers; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2] Fix -Os related build and test failures. 2016-10-28 13:58 ` [PATCH v2] Fix -Os related build and test failures Carlos O'Donell @ 2016-10-28 14:17 ` Joseph Myers 2016-10-29 2:59 ` [PATCH v3] " Carlos O'Donell 0 siblings, 1 reply; 43+ messages in thread From: Joseph Myers @ 2016-10-28 14:17 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3] Fix -Os related build and test failures. 2016-10-28 14:17 ` Joseph Myers @ 2016-10-29 2:59 ` Carlos O'Donell 2016-10-29 3:26 ` Carlos O'Donell ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-29 2:59 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-29 2:59 ` [PATCH v3] " Carlos O'Donell @ 2016-10-29 3:26 ` Carlos O'Donell 2016-10-29 17:35 ` Joseph Myers 2016-10-31 18:38 ` [PATCH v3] " Steve Ellcey 2 siblings, 0 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-29 3:26 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-29 2:59 ` [PATCH v3] " Carlos O'Donell 2016-10-29 3:26 ` Carlos O'Donell @ 2016-10-29 17:35 ` Joseph Myers 2016-10-30 3:51 ` [PATCH v4] " Carlos O'Donell 2016-10-31 18:38 ` [PATCH v3] " Steve Ellcey 2 siblings, 1 reply; 43+ messages in thread From: Joseph Myers @ 2016-10-29 17:35 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4] Fix -Os related build and test failures. 2016-10-29 17:35 ` Joseph Myers @ 2016-10-30 3:51 ` Carlos O'Donell 2016-10-31 8:33 ` Andreas Schwab 2016-11-01 9:17 ` Andreas Schwab 0 siblings, 2 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-30 3:51 UTC (permalink / raw) To: Joseph Myers; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-30 3:51 ` [PATCH v4] " Carlos O'Donell @ 2016-10-31 8:33 ` Andreas Schwab 2016-10-31 9:16 ` Carlos O'Donell 2016-11-01 9:17 ` Andreas Schwab 1 sibling, 1 reply; 43+ messages in thread From: Andreas Schwab @ 2016-10-31 8:33 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Joseph Myers, Florian Weimer, GNU C Library 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." ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-31 8:33 ` Andreas Schwab @ 2016-10-31 9:16 ` Carlos O'Donell 2016-10-31 9:22 ` Florian Weimer 2016-10-31 12:56 ` David Miller 0 siblings, 2 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-10-31 9:16 UTC (permalink / raw) To: Andreas Schwab; +Cc: Joseph Myers, Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-31 9:16 ` Carlos O'Donell @ 2016-10-31 9:22 ` Florian Weimer 2016-10-31 12:56 ` David Miller 1 sibling, 0 replies; 43+ messages in thread From: Florian Weimer @ 2016-10-31 9:22 UTC (permalink / raw) To: Carlos O'Donell, Andreas Schwab; +Cc: Joseph Myers, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-31 9:16 ` Carlos O'Donell 2016-10-31 9:22 ` Florian Weimer @ 2016-10-31 12:56 ` David Miller 2016-10-31 19:56 ` Carlos O'Donell 1 sibling, 1 reply; 43+ messages in thread From: David Miller @ 2016-10-31 12:56 UTC (permalink / raw) To: carlos; +Cc: schwab, joseph, fweimer, libc-alpha 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-31 12:56 ` David Miller @ 2016-10-31 19:56 ` Carlos O'Donell 2016-11-01 22:59 ` Joseph Myers 0 siblings, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-10-31 19:56 UTC (permalink / raw) To: David Miller; +Cc: schwab, joseph, fweimer, libc-alpha 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-31 19:56 ` Carlos O'Donell @ 2016-11-01 22:59 ` Joseph Myers 2016-11-02 12:52 ` Carlos O'Donell 0 siblings, 1 reply; 43+ messages in thread From: Joseph Myers @ 2016-11-01 22:59 UTC (permalink / raw) To: Carlos O'Donell; +Cc: David Miller, schwab, fweimer, libc-alpha 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 22:59 ` Joseph Myers @ 2016-11-02 12:52 ` Carlos O'Donell 0 siblings, 0 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-11-02 12:52 UTC (permalink / raw) To: Joseph Myers; +Cc: David Miller, schwab, fweimer, libc-alpha 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-10-30 3:51 ` [PATCH v4] " Carlos O'Donell 2016-10-31 8:33 ` Andreas Schwab @ 2016-11-01 9:17 ` Andreas Schwab 2016-11-01 11:13 ` Joseph Myers 1 sibling, 1 reply; 43+ messages in thread From: Andreas Schwab @ 2016-11-01 9:17 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Joseph Myers, Florian Weimer, GNU C Library 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." ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 9:17 ` Andreas Schwab @ 2016-11-01 11:13 ` Joseph Myers 2016-11-01 15:58 ` Tamar Christina 2016-11-02 13:22 ` Carlos O'Donell 0 siblings, 2 replies; 43+ messages in thread From: Joseph Myers @ 2016-11-01 11:13 UTC (permalink / raw) To: Andreas Schwab; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 11:13 ` Joseph Myers @ 2016-11-01 15:58 ` Tamar Christina 2016-11-01 16:06 ` David Miller 2016-11-02 13:22 ` Carlos O'Donell 1 sibling, 1 reply; 43+ messages in thread From: Tamar Christina @ 2016-11-01 15:58 UTC (permalink / raw) To: Joseph Myers, Andreas Schwab Cc: Carlos O'Donell, Florian Weimer, GNU C Library, nd, Bin Cheng 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 15:58 ` Tamar Christina @ 2016-11-01 16:06 ` David Miller 2016-11-01 16:15 ` Tamar Christina 2016-11-02 11:53 ` Carlos O'Donell 0 siblings, 2 replies; 43+ messages in thread From: David Miller @ 2016-11-01 16:06 UTC (permalink / raw) To: Tamar.Christina Cc: joseph, schwab, carlos, fweimer, libc-alpha, nd, Bin.Cheng 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 16:06 ` David Miller @ 2016-11-01 16:15 ` Tamar Christina 2016-11-02 11:53 ` Carlos O'Donell 1 sibling, 0 replies; 43+ messages in thread From: Tamar Christina @ 2016-11-01 16:15 UTC (permalink / raw) To: David Miller; +Cc: joseph, schwab, carlos, fweimer, libc-alpha, nd, Bin Cheng 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 16:06 ` David Miller 2016-11-01 16:15 ` Tamar Christina @ 2016-11-02 11:53 ` Carlos O'Donell 2016-11-02 17:03 ` Carlos O'Donell 1 sibling, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-11-02 11:53 UTC (permalink / raw) To: David Miller, Tamar.Christina Cc: joseph, schwab, fweimer, libc-alpha, nd, Bin.Cheng 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-02 11:53 ` Carlos O'Donell @ 2016-11-02 17:03 ` Carlos O'Donell 0 siblings, 0 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-11-02 17:03 UTC (permalink / raw) To: David Miller, Tamar.Christina Cc: joseph, schwab, fweimer, libc-alpha, nd, Bin.Cheng 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4] Fix -Os related build and test failures. 2016-11-01 11:13 ` Joseph Myers 2016-11-01 15:58 ` Tamar Christina @ 2016-11-02 13:22 ` Carlos O'Donell 1 sibling, 0 replies; 43+ messages in thread From: Carlos O'Donell @ 2016-11-02 13:22 UTC (permalink / raw) To: Joseph Myers, Andreas Schwab; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-29 2:59 ` [PATCH v3] " Carlos O'Donell 2016-10-29 3:26 ` Carlos O'Donell 2016-10-29 17:35 ` Joseph Myers @ 2016-10-31 18:38 ` Steve Ellcey 2016-10-31 19:50 ` Carlos O'Donell 2 siblings, 1 reply; 43+ messages in thread From: Steve Ellcey @ 2016-10-31 18:38 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-31 18:38 ` [PATCH v3] " Steve Ellcey @ 2016-10-31 19:50 ` Carlos O'Donell 2016-10-31 19:57 ` Steve Ellcey 0 siblings, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-10-31 19:50 UTC (permalink / raw) To: Steve Ellcey; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-31 19:50 ` Carlos O'Donell @ 2016-10-31 19:57 ` Steve Ellcey 2016-10-31 20:50 ` Carlos O'Donell 0 siblings, 1 reply; 43+ messages in thread From: Steve Ellcey @ 2016-10-31 19:57 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-31 19:57 ` Steve Ellcey @ 2016-10-31 20:50 ` Carlos O'Donell 2016-10-31 21:00 ` Steve Ellcey 0 siblings, 1 reply; 43+ messages in thread From: Carlos O'Donell @ 2016-10-31 20:50 UTC (permalink / raw) To: Steve Ellcey; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3] Fix -Os related build and test failures. 2016-10-31 20:50 ` Carlos O'Donell @ 2016-10-31 21:00 ` Steve Ellcey 0 siblings, 0 replies; 43+ messages in thread From: Steve Ellcey @ 2016-10-31 21:00 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library 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 ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2016-11-02 17:03 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-28 4:48 [PATCH] Fix -Os related -Werror failures Carlos O'Donell 2016-10-28 6:25 ` Andreas Schwab 2016-10-28 6:32 ` Florian Weimer 2016-10-28 6:44 ` Jeff Law 2016-10-28 8:12 ` Arnd Bergmann 2016-10-28 8:17 ` Andrew Pinski 2016-10-28 13:28 ` Jeff Law 2016-10-28 20:10 ` Paul Eggert 2016-10-29 3:03 ` Jeff Law 2016-10-30 4:25 ` Paul Eggert 2016-10-28 12:09 ` Carlos O'Donell 2016-10-28 12:43 ` Florian Weimer 2016-10-28 13:04 ` Joseph Myers 2016-10-28 13:07 ` Carlos O'Donell 2016-10-28 12:49 ` Joseph Myers 2016-10-28 12:55 ` Florian Weimer 2016-10-28 13:18 ` Carlos O'Donell 2016-10-28 13:58 ` [PATCH v2] Fix -Os related build and test failures Carlos O'Donell 2016-10-28 14:17 ` Joseph Myers 2016-10-29 2:59 ` [PATCH v3] " Carlos O'Donell 2016-10-29 3:26 ` Carlos O'Donell 2016-10-29 17:35 ` Joseph Myers 2016-10-30 3:51 ` [PATCH v4] " Carlos O'Donell 2016-10-31 8:33 ` Andreas Schwab 2016-10-31 9:16 ` Carlos O'Donell 2016-10-31 9:22 ` Florian Weimer 2016-10-31 12:56 ` David Miller 2016-10-31 19:56 ` Carlos O'Donell 2016-11-01 22:59 ` Joseph Myers 2016-11-02 12:52 ` Carlos O'Donell 2016-11-01 9:17 ` Andreas Schwab 2016-11-01 11:13 ` Joseph Myers 2016-11-01 15:58 ` Tamar Christina 2016-11-01 16:06 ` David Miller 2016-11-01 16:15 ` Tamar Christina 2016-11-02 11:53 ` Carlos O'Donell 2016-11-02 17:03 ` Carlos O'Donell 2016-11-02 13:22 ` Carlos O'Donell 2016-10-31 18:38 ` [PATCH v3] " Steve Ellcey 2016-10-31 19:50 ` Carlos O'Donell 2016-10-31 19:57 ` Steve Ellcey 2016-10-31 20:50 ` Carlos O'Donell 2016-10-31 21:00 ` Steve Ellcey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).