On Thu, 2020-03-05 at 19:51 +0100, Richard Biener wrote: > On March 5, 2020 3:55:57 PM GMT+01:00, Jeff Law <l...@redhat.com> wrote: > > On Thu, 2020-03-05 at 08:51 +0100, Richard Biener wrote: > > > On Thu, Mar 5, 2020 at 12:49 AM Jeff Law <l...@redhat.com> wrote: > > > > On Wed, 2020-03-04 at 09:22 -0700, Martin Sebor wrote: > > > > > I don't remember why the code in -Wrestrict unconditionally > > overwrites > > > > > the statement location rather than only when it's not available, > > but > > > > > I do remember adding conditional code like in your patch in > > r277076 > > > > > to deal with missing location on the statement. So either your > > fix > > > > > or something like the hunk below might be the right solution (if > > we > > > > > go with the code below, abstracting it into a utility function > > might > > > > > be nice). > > > > So there's several chunks that are fairly similar to what you > > referenced in > > > > maybe_warn_pointless_strcmp. Factoring all of them into a single > > location is > > > > pretty easy. > > > > > > > > That also gives us a nice place where we can experiment with "does > > extraction > > > > of > > > > location information from the expression ever help". The answer > > is, it > > > > doesn't, > > > > at least not within our testsuite when run on x86_64. > > > > > > > > I'm hesitant to remove the code that extracts the location out of > > the > > > > expression, > > > > but could be convinced to do so. > > > > > > > > Thoughts? > > > > > > Using anything but the actual stmt location is prone to end up at > > random places > > > due to tree sharing issues, CSE and copy propagation. Simply > > consider > > I'd tend to agree. My conservatism is due to being in stage4 and not > > knowing > > precisely why we have code to extract the location from the operand to > > begin > > with. > > > > > > > where we happily forward p = &one[0] to both uses injecting > > > a "faulty" location. Well, it's actually the correct location > > > computing &one[0] but irrelevant for the actual call. > > Exactly. > > > > > So the question is why we end up with UNKNOWN_LOCATION > > > for such call and if why we need to bother emit a diagnostic > > > at all (and why emitting it for another possibly random location is a > > good idea > > > instead of maybe simply emitting it without location). > > One might argue that scenario should be a gcc_unreachable rather than > > extracting > > a likely bogus location. I'm even more hesitant to do that for gcc-10, > > but it > > might sense for gcc-11. > > > > My first inclination would be do do the refactor, but leave in the code > > that > > extracts a location from the expression. We'd close out the regression > > BZ and > > open a new one to remove the expression handling bits for gcc-11 (or > > turn them > > into a gcc_unreachable) > > > > Does that work for you Richi? > > We should avoid regressing in other ways of course. Given Martin's followup > I'm > not sure what to do but eventually stop using ‰G and remove the odd expression > location code? Yea, I think that's the conclusion Martin and I came to today as well. Do the refactoring now, but leaving in the expression location bits, then remove the expression location bits in gcc-11 after further testing.
jeff
