| Message ID | 07d67ec3-918f-ba7c-08cf-6c523b09b086@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | have -Wnonnull print inlining stack (PR 83369) | expand |
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? Coming back to this patch: regarding tests, would you be able to use the techniques of: https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00646.html to build a test case? Dave
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. > Coming back to this patch: regarding tests, would you be able to use > the techniques of: > https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00646.html > to build a test case? I think so. If I'm reading it right, it depends on the prune.exp changes and could be as simple as testsuite/gcc.dg/plugin/diagnostic-test-inlining-4.c, right? Does it need to be in the plugin directory or can any test use this approach? Martin
On Mon, 2017-12-11 at 15:18 -0700, 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. There's the "artificial" attribute, which as far as I can tell was introduced as a kind of workaround for the "inlined code now looks like it's in a system header" issue (but may also related to debuginfo???) > > Coming back to this patch: regarding tests, would you be able to > > use > > the techniques of: > > https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00646.html > > to build a test case? > > I think so. If I'm reading it right, it depends on the prune.exp > changes and could be as simple as > testsuite/gcc.dg/plugin/diagnostic-test-inlining-4.c, right? > > Does it need to be in the plugin directory or can any test > use this approach? Any test could use that approach, without needing to use the plugin; that plugin exists purely to separate the testing of the "print inlining information for middle-end warnings" out from any particular middle-end warning. Now that I think about it a bit more, that approach has an issue, in that it hardcodes a lot of what we expect the output format to be. That's OK for the patch I linked to, since that's the purpose of that test. But for your purposes, I think that all you need is the approach Aldy used in 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
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. jeff
On 12/11/2017 06:16 PM, David Malcolm wrote: > On Mon, 2017-12-11 at 15:18 -0700, 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. 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? jeff
On 12/11/2017 09:51 AM, 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. So thinking more about this, I don't see a strong reason not to move forward with this fix in the immediate term. If we ultimately remove %K/%G, then obviously we'll revisit this chunk of code in the process. OK for the trunk. jeff
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. 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. Martin > > jeff
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
On 12/14/2017 06:53 PM, David Malcolm wrote: > 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. Yes, I'm planning on add something like before committing. Thanks for pointing out the way to do it within the framework. I didn't think it was possible. Martin
PR c/83369 - Missing diagnostics during inlining gcc/ChangeLog: PR c/83369 * tree-ssa-ccp.c (pass_post_ipa_warn::execute): Use %G to print inlining context. diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c index 3acddf9..8a405fc 100644 --- a/gcc/tree-ssa-ccp.c +++ b/gcc/tree-ssa-ccp.c @@ -3457,8 +3457,8 @@ pass_post_ipa_warn::execute (function *fun) location_t loc = gimple_location (stmt); if (warning_at (loc, OPT_Wnonnull, - "argument %u null where non-null " - "expected", i + 1)) + "%Gargument %u null where non-null " + "expected", as_a <gcall *>(stmt), i + 1)) { tree fndecl = gimple_call_fndecl (stmt); if (fndecl && DECL_IS_BUILTIN (fndecl))