Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: >> On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: >>> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via >>>> Gcc-patches wrote: >>>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: >>>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: >>>>>>> Over the last couple of months quite a few warnings about uninitialized >>>>>>> variables were raised while building GCC. A reason why these warnings >>>>>>> show up on S/390 only is due to the aggressive inlining settings here. >>>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, >>>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by >>>>>>> initializing the corresponding variable. Since the latter reoccurs and >>>>>>> while bootstrapping such warnings are turned into errors bootstrapping >>>>>>> fails on S/390 consistently. Therefore, for the moment do not turn >>>>>>> those warnings into errors. >>>>>>> >>>>>>> config/ChangeLog: >>>>>>> >>>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into >>>>>>> errors >>>>>>> on S/390. >>>>>>> >>>>>>> fixincludes/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> gcc/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libcc1/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libcpp/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libdecnumber/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>> That change looks good to me. Could a global reviewer please comment! >>>>> Ping >>>> Ping >>> I think this would be a huge mistake to install. >> The root cause why those false positives show up on S/390 only seems to >> be of more aggressive inlining w.r.t. other architectures. Because of >> bigger caches and a rather huge function call overhead we greatly >> benefit from those inlining parameters. Thus: >> >> 1) Reverting those parameters would have a negative performance impact. >> >> 2) Fixing the maybe-uninitialized warnings analysis itself seems not to >> happen in the near future (assuming that it is fixable at all). >> >> 3) Silencing the warning by initialising the variable itself also seems >> to be undesired and feels like a fight against windmills ;-) >> >> 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. >> >> Option (4) has the least intrusive effect to me. At least then it is >> not necessary to bootstrap with --disable-werror and we would still >> treat all other warnings as errors. All maybe-uninitialized warnings >> which are triggered in common code with non-aggressive inlining are >> still caught by other architectures. Therefore, I'm wondering why this >> should be a huge mistake? What would you propose instead? > > I'm aware of all that. What I think it all argues is that y'all need to > address the issues because of how you've changed the tuning on the s390 > port. Simply disabling things like you've suggested is, IMHO, horribly > wrong. > > > Improve the analysis, dummy initializers, pragmas all seem viable. But > again, it feels like it's something the s390 maintainers will have to > take the lead on because of how you've retuned the port. > > > And note that this isn't just an issue with uninitialized warnings, the > changes in inlining heuristics can impact all the middle end warnings.
To play devil's advocate: it seems like a reasonable workaround to me. (I didn't want to approve a potentially controversial patch for “another port” so was staying silent. :-)) Isn't this just the known downside of using maybe-used-uninitialised warnings? AIUI, it's accepted that the option has false positives that vary based on the amount of optimisation that previous passes have or haven't done. So I don't think it's an issue of “fixing” the analysis: the current implementation doesn't seem like it is going to be (and perhaps it isn't meant to be) predictable from a user's perspective. I got the impression this was a deliberate trade-off we'd made in order to let fewer false negatives slip by. We already disable maybe-used-uninitialized warnings when bootstrapping with anything other than the default -O2 -g configuration. (I remember when looking at -Og a while back that there were a large number of unsuppressed false positives when bootstrapping with that.) ISTM that s390 is effectively using non-standard bootstrap options, in a similar way to --with-build-config=, and so turning these errors back into warnings is reasonable here too. Thanks, Richard
