On Thu, 2020-03-12 at 14:21 -0600, Sandra Loosemore wrote: > On 1/27/20 2:02 PM, Jeff Law wrote: > > [snip] > > > > While I think we've missed the boat for gcc-10, I think these > > patches > > should go forward in gcc-11. I'll own getting the paths sorted so > > that > > this problem is avoided.
> I recently retested these patches on trunk, For reference, the patches are here: cover letter: https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534142.html [1/2] https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534145.html [2/2] https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534144.html > and I found some new > regressions in the analyzer tests. > > FAIL: default/gcc.sum:gcc.dg/analyzer/edges-1.c (test for bogus > messages, line 16) > > This one may be a genuine analyzer bug? It is apparently failing to > prune the control flow paths per commit > 004f2c07b6d3fa543f0fe86c55a7b3c227de2bb6. I think that what's happened is that your patch improves the location information for the gimple stmts, and this uncovers a bug in how I wrote the test. Without your patch 2, edges-1.c's test_1 has this output: edges-1.c:19:6: warning: leak of FILE 'fp' [CWE-775] [-Wanalyzer-file-leak] edges-1.c:10:14: note: (1) opened here edges-1.c:12:6: note: (2) assuming 'fp' is non-NULL edges-1.c:12:6: note: (3) following 'false' branch (when 'fp' is non-NULL)... cc1: note: (4) ...to here edges-1.c:19:6: note: (5) following 'false' branch (when 'flag == 0')... cc1: note: (6) ...to here edges-1.c:19:6: note: (7) 'fp' leaks here; was opened at (1) With both of your patches I get: edges-1.c:19:6: warning: leak of FILE 'fp' [CWE-775] [-Wanalyzer-file-leak] edges-1.c:10:14: note: (1) opened here edges-1.c:12:6: note: (2) assuming 'fp' is non-NULL edges-1.c:12:6: note: (3) following 'false' branch (when 'fp' is non-NULL)... edges-1.c:16:10: note: (4) ...to here edges-1.c:19:6: note: (5) following 'false' branch (when 'flag == 0')... cc1: note: (6) ...to here edges-1.c:19:6: note: (7) 'fp' leaks here; was opened at (1) Note how the patch improves event (4) in the path from being at an unknown location to being at line 16 (the "while"). I think I underspecified the dg-bogus at line 16: my intent is for it to verify that we don't emit any events relating to the "while" test, but the above now has the "...to here". The following patch fixes that dg-bogus at line 16; the analyzer is still correctly not reporting the control flow for the while-loop itself, as that's not needed to trigger the leak (I verified that the updated dg-bogus is still triggered if I supply -fanalyzer-verbosity=3 to disable the purging of redundant control-flow). Note how event (6) still doesn't have a location; that's the destination of the final "if" that's at the end of the function (falling off the end). That may well be an issue with the analyzer, though, not your code. > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/explode-2.c (test for > warnings, line 39) > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/explode-2.c (test for > warnings, line 46) > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/explode-2.c (test for > excess errors) > > This one is hitting the "--Wanalyzer-too-complex" diagnostic and > could > probably be fixed by adjusting the parameters at the top of the > testcase. I tried doubling the limits but it didn't help; something unexpected seems to be going on - but I'm in the middle of a rewrite of this code, so I'd prefer to postpone looking into this for now. > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/loop-4.c (test for > warnings, line 28) > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/loop-4.c (test for > warnings, line 37) > PASS -> FAIL: default/gcc.sum:gcc.dg/analyzer/loop-4.c (test for > excess > errors) > > It's now printing "2 processed enodes" in both warnings instead of 3 > or > 4 (respectively). At first glance, it's not obvious what the > significance of the the things being tested here is, so I'm not sure > if > this is an analyzer bug or just patterns that are too fragile. The test is trying to measure how many nodes we create at each measured program point in the exploded_graph - ideally the fewer the better; it seems that the patch improves the analyzer here, and the test should be updated to reflect that. Here's a lightly-tested patch which fixes the edges-1.c and loop-4.c failures for me. David gcc/testsuite/ChangeLog: * gcc.dg/analyzer/edges-1.c: Strengthen the dg-bogus directives to check for the word "branch" rather than any message, since test_1's if's "...to here" message now has the location of the "while" stmt. * gcc.dg/analyzer/loop-4.c: Reduce number of expected enodes in two places. --- gcc/testsuite/gcc.dg/analyzer/edges-1.c | 4 ++-- gcc/testsuite/gcc.dg/analyzer/loop-4.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gcc/testsuite/gcc.dg/analyzer/edges-1.c b/gcc/testsuite/gcc.dg/analyzer/edges-1.c index 6b53ddddc05..3e6a1ae6dd4 100644 --- a/gcc/testsuite/gcc.dg/analyzer/edges-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/edges-1.c @@ -13,7 +13,7 @@ void test_1 (const char *path, int flag) return; /* We shouldn't report this control flow. */ - while (foo ()) /* { dg-bogus "" } */ + while (foo ()) /* { dg-bogus "branch" } */ bar (); if (flag) /* { dg-message "when 'flag == 0'" "branch event" } */ @@ -26,7 +26,7 @@ void test_2 (const char *path, int flag) FILE *fp = fopen (path, "r"); /* We shouldn't report this control flow. */ - if (foo ()) /* { dg-bogus "" } */ + if (foo ()) /* { dg-bogus "branch" } */ bar (); else bar (); diff --git a/gcc/testsuite/gcc.dg/analyzer/loop-4.c b/gcc/testsuite/gcc.dg/analyzer/loop-4.c index e5767de1f96..3f3287ca468 100644 --- a/gcc/testsuite/gcc.dg/analyzer/loop-4.c +++ b/gcc/testsuite/gcc.dg/analyzer/loop-4.c @@ -25,7 +25,7 @@ void test(void) __analyzer_eval (j < 256); /* { dg-warning "TRUE" } */ - __analyzer_dump_exploded_nodes (0); /* { dg-warning "3 processed enodes" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enodes" } */ for (k=0; k<256; k++) { @@ -34,7 +34,7 @@ void test(void) __analyzer_eval (k < 256); /* { dg-warning "TRUE" } */ - __analyzer_dump_exploded_nodes (0); /* { dg-warning "4 processed enodes" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enodes" } */ } } } -- 2.21.0
