Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
Hi David, Thanks for the review. On Tue, Sep 5, 2023 at 1:53 PM David Malcolm wrote: > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote: > > [...snip...] > All of these "new" tests (apart from the "-noexcept" ones) look like > they're meant to be existing tests that were moved, but where the copy > of the test in gcc.dg/analyzer didn't get deleted, so they show up as a > duplicate. See the details below. > > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test. > > When duplicating a test like this, the test isn't entirely "new", so > please say something like this in the ChangeLog entry, to make it clear > where it came from: > > I actually wasn't sure about these -noexcept tests. They were part of gcc.dg/analyzer, thus only gcc was running them. Exceptions were not disabled *explicitly*, but since it was C, they weren't enabled either. Therefore, the -noexcept tests are basically a copy, but with an explicit -fno-exceptions specification. When I duplicated them in that way I was thinking about making it clear that these tests fail in C++ with exceptions enabled, so that we would already have easy-to-spot failing tests to challenge a future exception support. Though perhaps *not* duplicating the tests but rather simply specify -fno-exceptions, with a comment "Fails with exceptions" may be better. > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test, > based on gcc.dg/analyzer/file-pr58237.c. > > > * c-c++-common/analyzer/fopen-2.c: New test. > > Looks fopen-2.c is a move of the parts of gcc.dg/analyzer/fopen-1.c > that can also be C++, so please state that in the ChangeLog. > Nods. [...snip...] > Nice - thanks. > > Can this be combined with the EQ_EXPR and NE_EXPR cases? (possibly > updating the comment) The code looks identical to me, but I might be > misreading it. > > Totally, I forgot to move it after making sure it worked. Thanks. > [...snip...] > > > diff --git a/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > > new file mode 100644 > > index 000..b208f58f09f > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > > @@ -0,0 +1,72 @@ > > +#include > > + > > +struct ptr_wrapper > > +{ > > + int *ptr; > > +}; > > + > > +struct ptr_wrapper > > +test_1 (void) > > +{ > > + struct ptr_wrapper r; > > + r.ptr = (int *) malloc (sizeof (int)); > > + return r; > > +} > > This looks the same as gcc.dg/analyzer/compound-assignment-1.c > > Should this be a move, rather than a new file? i.e. is the patch > missing a deletion of the file in the old location? > > [...snip...] > > > diff --git a/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c > b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c > > new file mode 100644 > > index 000..6b7d25cfabe > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c > > Likewise here for infinite-recursion.c. > > [...snip...] > > > diff --git > a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > > similarity index 97% > > rename from > gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > > rename to > gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > > index 0172c9b324c..1b657697ef4 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > > +++ > b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > > @@ -1,6 +1,6 @@ > > #include > > > > -#include "analyzer-decls.h" > > +#include "../../gcc.dg/analyzer/analyzer-decls.h" > > > > struct iter > > { > > @@ -45,7 +45,7 @@ void test(int n) > >struct iter *it = iter_new (0, n, 1); > >while (!iter_done_p (it)) > > { > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE" "true" { > xfail *-*-* } } */ > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE" "true" } */ > >/* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > >/* TODO(xfail^^^): ideally we ought to figure out i > 0 after 1st > iteration. */ > > > > Presumably due to the change to > region_model::add_constraints_from_binop, right? > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN here, or > can that line be removed? > If so, then the 3rd comment can presumably become: > > The bogus here still make sense - without it there is an excess error -. I had checked for it because I too thought it could be removed. If I remember it correctly, we get UNKNOWN during the widening pass. > >/* TODO: ideally we ought to figure out i > 0 after 1st > iteration. */ > > [...snip...] > > > [...snip...] Thanks for spotting the files I forgot to remove from gcc.dg. Sorry about them, I had messed up my test folder when checking for arm-eabi, and I apparently missed some duplicates when re
Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
On Wed, 2023-09-06 at 15:50 +0200, Benjamin Priour wrote: > Hi David, > Thanks for the review. > > > > On Tue, Sep 5, 2023 at 1:53 PM David Malcolm > wrote: > > > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote: > > > > > [...snip...] > > > > All of these "new" tests (apart from the "-noexcept" ones) look > > like > > they're meant to be existing tests that were moved, but where the > > copy > > of the test in gcc.dg/analyzer didn't get deleted, so they show up > > as a > > duplicate. See the details below. > > > > > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test. > > > > When duplicating a test like this, the test isn't entirely "new", > > so > > please say something like this in the ChangeLog entry, to make it > > clear > > where it came from: > > > > > I actually wasn't sure about these -noexcept tests. They were part > of gcc.dg/analyzer, thus only gcc was running them. Exceptions > were not disabled *explicitly*, but since it was C, they weren't > enabled > either. > > Therefore, the -noexcept tests are basically a copy, but with an > explicit > -fno-exceptions specification. > When I duplicated them in that way I was thinking about making it > clear > that these tests fail in C++ with exceptions enabled, so that we > would > already > have easy-to-spot failing tests to challenge a future exception > support. Ah, OK; let's go with your approach. > > Though perhaps *not* duplicating the tests but rather simply specify > -fno-exceptions, > with a comment "Fails with exceptions" may be better. [...snip...] > > > @@ -45,7 +45,7 @@ void test(int n) > > > struct iter *it = iter_new (0, n, 1); > > > while (!iter_done_p (it)) > > > { > > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE" > > > "true" { > > xfail *-*-* } } */ > > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE" > > > "true" } */ > > > /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > > > /* TODO(xfail^^^): ideally we ought to figure out i > 0 > > > after 1st > > iteration. */ > > > > > > > Presumably due to the change to > > region_model::add_constraints_from_binop, right? > > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN > > here, or > > can that line be removed? > > If so, then the 3rd comment can presumably become: > > > > > The bogus here still make sense - without it there is an excess error > -. > I had checked for it because I too thought it could be removed. > If I remember it correctly, we get UNKNOWN during the widening pass. (nods) > > > > > /* TODO: ideally we ought to figure out i > 0 after 1st > > iteration. */ > > > > [...snip...] > > > > > > > [...snip...] > > Thanks for spotting the files I forgot to remove from gcc.dg. > Sorry about them, I had messed up my test folder when checking for > arm-eabi, > and I apparently missed some duplicates when retrieving my save. > > As for the files the likes of inlining-*.c, i.e. noted as Moved > to/...here. > at the end of the ChangeLog, some tests checking for multiline > outputs > are so heavily rewritten than git marks them as Removed/New test > instead of moved. I've manually edited that, but perhaps I shouldn't > ? > > I have successfully regstrapped the improvements you suggested. Thanks. Did you want me to doublecheck the updated patch? Otherwise feel free to push it to trunk. > Part 3 of this serie of patches I hope will be regstrapped for > Friday. Thanks; I'm impressed at how much progress you've made on this problem. Dave
Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
Hi! On Wed, 6 Sept 2023 at 22:22, David Malcolm via Gcc-patches < gcc-patches@gcc.gnu.org> wrote: > On Wed, 2023-09-06 at 15:50 +0200, Benjamin Priour wrote: > > Hi David, > > Thanks for the review. > > > > > > > > On Tue, Sep 5, 2023 at 1:53 PM David Malcolm > > wrote: > > > > > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote: > > > > > > > > [...snip...] > > > > > > > All of these "new" tests (apart from the "-noexcept" ones) look > > > like > > > they're meant to be existing tests that were moved, but where the > > > copy > > > of the test in gcc.dg/analyzer didn't get deleted, so they show up > > > as a > > > duplicate. See the details below. > > > > > > > > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test. > > > > > > When duplicating a test like this, the test isn't entirely "new", > > > so > > > please say something like this in the ChangeLog entry, to make it > > > clear > > > where it came from: > > > > > > > > I actually wasn't sure about these -noexcept tests. They were part > > of gcc.dg/analyzer, thus only gcc was running them. Exceptions > > were not disabled *explicitly*, but since it was C, they weren't > > enabled > > either. > > > > Therefore, the -noexcept tests are basically a copy, but with an > > explicit > > -fno-exceptions specification. > > When I duplicated them in that way I was thinking about making it > > clear > > that these tests fail in C++ with exceptions enabled, so that we > > would > > already > > have easy-to-spot failing tests to challenge a future exception > > support. > > Ah, OK; let's go with your approach. > > > > > Though perhaps *not* duplicating the tests but rather simply specify > > -fno-exceptions, > > with a comment "Fails with exceptions" may be better. > > [...snip...] > > > > > @@ -45,7 +45,7 @@ void test(int n) > > > >struct iter *it = iter_new (0, n, 1); > > > >while (!iter_done_p (it)) > > > > { > > > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE" > > > > "true" { > > > xfail *-*-* } } */ > > > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE" > > > > "true" } */ > > > >/* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > > > >/* TODO(xfail^^^): ideally we ought to figure out i > 0 > > > > after 1st > > > iteration. */ > > > > > > > > > > Presumably due to the change to > > > region_model::add_constraints_from_binop, right? > > > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN > > > here, or > > > can that line be removed? > > > If so, then the 3rd comment can presumably become: > > > > > > > > The bogus here still make sense - without it there is an excess error > > -. > > I had checked for it because I too thought it could be removed. > > If I remember it correctly, we get UNKNOWN during the widening pass. > > (nods) > > > > > > > > >/* TODO: ideally we ought to figure out i > 0 after 1st > > > iteration. */ > > > > > > [...snip...] > > > > > > > > > > > [...snip...] > > > > Thanks for spotting the files I forgot to remove from gcc.dg. > > Sorry about them, I had messed up my test folder when checking for > > arm-eabi, > > and I apparently missed some duplicates when retrieving my save. > > > > As for the files the likes of inlining-*.c, i.e. noted as Moved > > to/...here. > > at the end of the ChangeLog, some tests checking for multiline > > outputs > > are so heavily rewritten than git marks them as Removed/New test > > instead of moved. I've manually edited that, but perhaps I shouldn't > > ? > > > > I have successfully regstrapped the improvements you suggested. > > Thanks. Did you want me to doublecheck the updated patch? Otherwise > feel free to push it to trunk. > > Was this patch committed as r14-3823-g50b5199cff6908? Our CI noticed regression after that revision, on arm-eabi. I looked at the logs for out-of-bounds-diagram-11.c, where we have: FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c expected multiline pattern lines 46-61 FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c 2 blank line(s) in output FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c (test for excess errors) After visual inspection, I noticed that we emit: write of '(int32_t) 42' but expect: write of '(int) 42' (that is, we emit 'int32_t' and expect 'int') I didn't check all the regressions, but does that ring a bell? Thanks, Christophe > > Part 3 of this serie of patches I hope will be regstrapped for > > Friday. > > Thanks; I'm impressed at how much progress you've made on this problem. > > Dave > >
Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
Hi Christophe, On Mon, Sep 11, 2023 at 4:23 PM Christophe Lyon wrote: > Hi! > > > On Wed, 6 Sept 2023 at 22:22, David Malcolm via Gcc-patches < > gcc-patches@gcc.gnu.org> wrote: > >> On Wed, 2023-09-06 at 15:50 +0200, Benjamin Priour wrote: >> > Hi David, >> > Thanks for the review. >> > >> > >> > >> > On Tue, Sep 5, 2023 at 1:53 PM David Malcolm >> > wrote: >> > >> > > On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote: >> > > >> > > >> > [...snip...] >> > >> > >> > > All of these "new" tests (apart from the "-noexcept" ones) look >> > > like >> > > they're meant to be existing tests that were moved, but where the >> > > copy >> > > of the test in gcc.dg/analyzer didn't get deleted, so they show up >> > > as a >> > > duplicate. See the details below. >> > > >> > >> > > > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test. >> > > >> > > When duplicating a test like this, the test isn't entirely "new", >> > > so >> > > please say something like this in the ChangeLog entry, to make it >> > > clear >> > > where it came from: >> > > >> > > >> > I actually wasn't sure about these -noexcept tests. They were part >> > of gcc.dg/analyzer, thus only gcc was running them. Exceptions >> > were not disabled *explicitly*, but since it was C, they weren't >> > enabled >> > either. >> > >> > Therefore, the -noexcept tests are basically a copy, but with an >> > explicit >> > -fno-exceptions specification. >> > When I duplicated them in that way I was thinking about making it >> > clear >> > that these tests fail in C++ with exceptions enabled, so that we >> > would >> > already >> > have easy-to-spot failing tests to challenge a future exception >> > support. >> >> Ah, OK; let's go with your approach. >> >> > >> > Though perhaps *not* duplicating the tests but rather simply specify >> > -fno-exceptions, >> > with a comment "Fails with exceptions" may be better. >> >> [...snip...] >> >> > > > @@ -45,7 +45,7 @@ void test(int n) >> > > >struct iter *it = iter_new (0, n, 1); >> > > >while (!iter_done_p (it)) >> > > > { >> > > > - __analyzer_eval (it->val < n); /* { dg-warning "TRUE" >> > > > "true" { >> > > xfail *-*-* } } */ >> > > > + __analyzer_eval (it->val < n); /* { dg-warning "TRUE" >> > > > "true" } */ >> > > >/* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ >> > > >/* TODO(xfail^^^): ideally we ought to figure out i > 0 >> > > > after 1st >> > > iteration. */ >> > > > >> > > >> > > Presumably due to the change to >> > > region_model::add_constraints_from_binop, right? >> > > Looking at that dg-bogus "UNKNOWN", do we still get an UNKNOWN >> > > here, or >> > > can that line be removed? >> > > If so, then the 3rd comment can presumably become: >> > > >> > > >> > The bogus here still make sense - without it there is an excess error >> > -. >> > I had checked for it because I too thought it could be removed. >> > If I remember it correctly, we get UNKNOWN during the widening pass. >> >> (nods) >> >> > >> > >> > > >/* TODO: ideally we ought to figure out i > 0 after 1st >> > > iteration. */ >> > > >> > > [...snip...] >> > > >> > > >> > > >> > [...snip...] >> > >> > Thanks for spotting the files I forgot to remove from gcc.dg. >> > Sorry about them, I had messed up my test folder when checking for >> > arm-eabi, >> > and I apparently missed some duplicates when retrieving my save. >> > >> > As for the files the likes of inlining-*.c, i.e. noted as Moved >> > to/...here. >> > at the end of the ChangeLog, some tests checking for multiline >> > outputs >> > are so heavily rewritten than git marks them as Removed/New test >> > instead of moved. I've manually edited that, but perhaps I shouldn't >> > ? >> > >> > I have successfully regstrapped the improvements you suggested. >> >> Thanks. Did you want me to doublecheck the updated patch? Otherwise >> feel free to push it to trunk. >> >> Was this patch committed as r14-3823-g50b5199cff6908? > > Indeed. > Our CI noticed regression after that revision, on arm-eabi. > I looked at the logs for out-of-bounds-diagram-11.c, where we have: > FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c expected multiline > pattern lines 46-61 > FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c 2 blank line(s) in > output > FAIL: c-c++-common/analyzer/out-of-bounds-diagram-11.c (test for excess > errors) > > After visual inspection, I noticed that we emit: > write of '(int32_t) 42' > but expect: > write of '(int) 42' > (that is, we emit 'int32_t' and expect 'int') > > I didn't check all the regressions, but does that ring a bell? > > Thanks, > > Christophe > > Thanks for spotting that. I checked the diff of the aforementioned patch. The variable type has not been changed nor the expected multiline-output. Do you observe the regression in C and/or C++ ? Thanks, Benjamin.
Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
On Mon, 2023-09-04 at 20:00 +0200, priour...@gmail.com wrote: > Hi, > > The second patch of this serie. > Regstrapped on x86_64-linux-gnu off trunk > a7d052b3200c7928d903a0242b8cfd75d131e374. Thanks for the patch. Overall, looks like great work, but there are a few nitpicks to be fixed, see below... [...snip...] > Second batch of moving tests from under gcc.dg/analyzer into > c-c++-common/analyzer. > > Prior to this patch the analyzer was not unwrapping ordering > binop_svalue, such as LT_EXPR, when evaluating conditions. > > Therefore when an ordering conditional was stored, the analyzer > was missing out on some constraints, which led to false positives. > > Signed-off-by: benjamin priour [...snip...] > * gcc.dg/analyzer/inlining-7.c: Moved to... > * c-c++-common/analyzer/inlining-7.c: ...here. > * c-c++-common/analyzer/compound-assignment-1.c: New test. All of these "new" tests (apart from the "-noexcept" ones) look like they're meant to be existing tests that were moved, but where the copy of the test in gcc.dg/analyzer didn't get deleted, so they show up as a duplicate. See the details below. > * c-c++-common/analyzer/file-pr58237-noexcept.c: New test. When duplicating a test like this, the test isn't entirely "new", so please say something like this in the ChangeLog entry, to make it clear where it came from: * c-c++-common/analyzer/file-pr58237-noexcept.c: New test, based on gcc.dg/analyzer/file-pr58237.c. > * c-c++-common/analyzer/fopen-2.c: New test. Looks fopen-2.c is a move of the parts of gcc.dg/analyzer/fopen-1.c that can also be C++, so please state that in the ChangeLog. > * c-c++-common/analyzer/infinite-recursion.c: New test. > * c-c++-common/analyzer/malloc-paths-9-noexcept.c: New test. Likewise, please say where the -noexcept.c test came from. > * c-c++-common/analyzer/pr109577-noexcept.c: New test. Likewise for this -noexcept test. > * c-c++-common/analyzer/pr93355-localealias-feasibility-noexcept.c: New > test. Likewise for this -noexcept test. > * c-c++-common/analyzer/pr94362-1.c: New test. > * c-c++-common/analyzer/pr99193-1-noexcept.c: New test. Likewise for this -noexcept test. > * c-c++-common/analyzer/scope-1.c: New test. > * c-c++-common/analyzer/setjmp-2.c: New test. > * c-c++-common/analyzer/setjmp-5.c: New test. > * c-c++-common/analyzer/setjmp-9.c: New test. > * c-c++-common/analyzer/signal-4a.c: New test. > * c-c++-common/analyzer/signal-4b.c: New test. [...snip...] > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc > index 82bc3b2c382..43b4bc1cc5b 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -4486,6 +4486,14 @@ region_model::add_constraints_from_binop (const svalue > *outer_lhs, > return true; > } >return false; > +case GE_EXPR: > +case GT_EXPR: > +case LE_EXPR: > +case LT_EXPR: > + if (!is_true) > + inner_op = invert_tree_comparison (inner_op, false /* honor_nans */); > + *out = add_constraint (inner_lhs, inner_op, inner_rhs, ctxt); > + return true; > } > } > Nice - thanks. Can this be combined with the EQ_EXPR and NE_EXPR cases? (possibly updating the comment) The code looks identical to me, but I might be misreading it. [...snip...] > diff --git a/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > new file mode 100644 > index 000..b208f58f09f > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c > @@ -0,0 +1,72 @@ > +#include > + > +struct ptr_wrapper > +{ > + int *ptr; > +}; > + > +struct ptr_wrapper > +test_1 (void) > +{ > + struct ptr_wrapper r; > + r.ptr = (int *) malloc (sizeof (int)); > + return r; > +} This looks the same as gcc.dg/analyzer/compound-assignment-1.c Should this be a move, rather than a new file? i.e. is the patch missing a deletion of the file in the old location? [...snip...] > diff --git a/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c > b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c > new file mode 100644 > index 000..6b7d25cfabe > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/analyzer/infinite-recursion.c Likewise here for infinite-recursion.c. [...snip...] > diff --git > a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > similarity index 97% > rename from gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > rename to > gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > index 0172c9b324c..1b657697ef4 100644 > --- a/gcc/testsuite/gcc.dg/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > +++ b/gcc/testsuite/c-c++-common/analyzer/loop-0-up-to-n-by-1-with-iter-obj.c > @@ -1,6