On 10/30/20 4:08 AM, Stefan Schulze Frielinghaus wrote: > On Wed, Oct 28, 2020 at 11:34:53AM -0600, Jeff Law wrote: >> 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. > Fixing the analysis is of course the best option. However, this sounds > like a non-trivial task to me and I'm missing a lot of context here, > i.e., I'm not sure what the initial goals were and if it is possible to > meet those with the requirements which are necessary to solve those > false positives (currently having PR96564 in mind where it was mentioned > that alias info is not enough but also flow-based info is required; does > this imply that we would have to reschedule the analysis at later time > which was not desired in the first place etc.).
There are going to be cases we can't solve with just improvements in the analysis. My point is that we have several tools in our toolbox and we should be looking at those to solve the problem rather than just disabling the warning. > > In the past I tried to come up with some dummy initializers which were > tough to get accepted (which I can understand up to some degree). For > example, this one is still open (I would be happy if you could have a > look at it and accept/reject): > https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547063.html > > Then there is at least one unreported case (similar to PR96564) where we > are not talking about a variable of scalar type but of an aggregate > where only one struct member must be initialized in order to silence the > warning. Not sure whether a patch would be accepted where I initialize > the whole structure or just a single member. > > Thus I'm still willing to come up with dummy initializer patches, > though, I'm not sure whether they are really accepted by the community > or not. They can be after analysis of the events that lead to the diagnostic. Please make sure those are attached to the -Wuninitialized meta-bug. I don't know if I'm going to have much time to look at them this stage3/stage4, but that meta bug is where I start. > >> And note that this isn't just an issue with uninitialized warnings, the >> changes in inlining heuristics can impact all the middle end warnings. > Just curious, is this a hypothetical problem or did we have other > problems with those inlining parameters in the past? If there are > further concrete problems with those parameters I would be really > interested to look into those. We've bumped up against it regularly in Fedora. Essentially any middle end warning is potentially perturbed by changes in the inlining heuristics. Sometimes the extra inlining gives enough context to make a false positive go away other times it introduces false positives. Sometimes the extra inlining allows us to catch an case that was previously missed (which is a good thing obviously). I would probably go far enough to say that these are the most common source of FTBFS issues in Fedora when we drop in new compilers -- s390 builds failing because they emitted a false positive diagnostic that no other target has (due to the differences in inlining) and the package is using -Werror. > Furthermore, I'm still wondering, if those parameters are that > controversial whether we should document that. It is tempting to take > the documented ranges literally (although admitted we took almost a > limit value for param_inline_min_speedup ;-)). Maybe only a certain > subrange is meant for production? Anyhow, I did a quick test for > param_max_inline_insns_auto which reveals that for values greater than > 18 (default is 15) warnings are emitted. > > I will make a couple of benchmarks in the following days in order to > find a parameter set where no false positive is thrown. What I fear > most is that we get outperformed by other compilers due to less inlining > just because we lifted false positive warnings to errors which feels > really bad to me. There's so many things that go into these decisions. What I keep coming back to is the design principle that, to the extent possible, gimple should be reasonably comparable across different targets. That gives predictability for the analysis and optimizers which solves one of the *huge* problems with RTL. While we never 100% achieved that, it's a good guiding principle. Jeff
