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? Richard. >jeff
