Re: [PATCH] have -Wnonnull print inlining stack (PR 83369)

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2017-12-14 at 16:07 -0700, Martin Sebor wrote:
> On 12/14/2017 12:24 PM, Jeff Law wrote:
> > On 12/11/2017 03:18 PM, Martin Sebor wrote:
> > > On 12/11/2017 02:08 PM, David Malcolm wrote:
> > > > On Mon, 2017-12-11 at 09:51 -0700, Martin Sebor wrote:
> > > > > Bug 83369 - Missing diagnostics during inlining, notes that
> > > > > when
> > > > > -Wnonnull is issued for an inlined call to a built-in
> > > > > function,
> > > > > GCC doesn't print the inlining stack, making it hard to debug
> > > > > where the problem comes from.
> > > > > 
> > > > > When the -Wnonnull warning was introduced into the middle-end
> > > > > the diagnostic machinery provided no way to print the
> > > > > inlining
> > > > > stack (analogous to %K for trees).  Since then GCC has gained
> > > > > support for the %G directive which does just that.  The
> > > > > attached
> > > > > patch makes use of the directive to print the inlining
> > > > > context
> > > > > for -Wnonnull.
> > > > > 
> > > > > The patch doesn't include a test because the DejaGnu
> > > > > framework
> > > > > provides no mechanism to validate this part of GCC output
> > > > > (see
> > > > > also bug 83336).
> > > > > 
> > > > > Tested on x86_64-linux with no regressions.
> > > > > 
> > > > > Martin
> > > > 
> > > > I'm wondering if we should eliminate %K and %G altogether, and
> > > > make
> > > > tree-diagnostic.c and friends automatically print the inlining
> > > > stack
> > > > -they just need a location_t (the issue is with system headers,
> > > > I
> > > > suppose, but maybe we can just make that smarter: perhaps only
> > > > suppress
> > > > if every location in the chain is in a system header?).  I
> > > > wonder if
> > > > that would be GCC 9 material at this point though?
> > > 
> > > Getting rid of %G and %K sounds fine to me.  I can't think of
> > > a use case for suppressing middle end diagnostics in system
> > > headers so unless someone else can it might be a non-issue.
> > > Since the change would fix a known bug it seems to me that it
> > > should be acceptable even at this stage.
> > 
> > My recollection is we suppress warnings from bits in system headers
> > because neither we nor the end users necessarily have control over
> > the
> > contents of the system headers -- in which case we'd be issuing
> > warnings
> > for something that only the vendor can fix.
> > 
> > 
> > I believe default suppression of warnings from system headers
> > probably
> > needs to stay.
> 
> I agree. I wasn't suggesting to get rid of -Wsystem altogether.
> Rather, I meant that unlike front-end warnings, I don't know
> and can't think of middle-end warnings (and only those) that
> should be suppressed for system header code.   They indicate
> bugs in the emitted and (in most cases) reachable object code.
> There should be no such code in system headers, and the middle
> end warnings I've worked on go out of their way to make sure
> they are emitted regardless.  (It's easy to forget this part
> and end up with Glibc macros like strncpy suppressing warnings
> for serious bugs in user code.)
> 
>  > Is there any kind of consensus on what we want to do here -- do we
> want
>  > to try to tackle %G/%K removal for gcc-8 or defer it?  If we defer
> it,
>  > how do we want to handle printing the inline stack in the interm?
> 
> I would support it but I don't expect to have the cycles to
> do the work myself at this stage.  I view it as a nice design
> improvement but give its minimal impact on functionality (IIUC)
> it's lower priority than the bugs I'd like to fix. 

Yes.  The %K and %G feel a design wart to me, but doing it right is
fiddly, and I think something to punt to gcc 9.

> I don't
> have the impression that improving the detail included in
> the inlining stack is predicated on this API change, but I'll
> let David speak to that.

The patch is fine; I'd just prefer it if there was a test case.  I
think you can reuse the approach used by gcc.dg/tm/pr52141.c, which has
just a:

  /* { dg-message "inlined from \'f\'" "" { target *-*-* } 0 } */

to verify that *something* was printed about the inlining, without
overspecifying exactly what.

Dave