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 <vultk...@gcc.gnu.org>
[...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 00000000000..b208f58f09f
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/compound-assignment-1.c
> @@ -0,0 +1,72 @@
> +#include <stdlib.h>
> +
> +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 00000000000..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 <stdlib.h>
>
> -#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:
> /* TODO: ideally we ought to figure out i > 0 after 1st
iteration. */
[...snip...]
> diff --git a/gcc/testsuite/c-c++-common/analyzer/pr94362-1.c
> b/gcc/testsuite/c-c++-common/analyzer/pr94362-1.c
> new file mode 100644
> index 00000000000..a184636073f
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/pr94362-1.c
Looks like this is another duplication that's meant to be a move, due
to missing the deletion of pr94362-1.c in the old location.
[...snip...]
> diff --git a/gcc/testsuite/c-c++-common/analyzer/scope-1.c
> b/gcc/testsuite/c-c++-common/analyzer/scope-1.c
> new file mode 100644
> index 00000000000..09e62d46df3
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/scope-1.c
[...snip...]
Likewise, this looks identical to gcc.dg/analyzer/scope-1.c, but shows
up as a new file here; presumably this was meant to be a move, but the
old file didn't get deleted.
> diff --git a/gcc/testsuite/c-c++-common/analyzer/setjmp-2.c
b/gcc/testsuite/c-c++-common/analyzer/setjmp-2.c
> new file mode 100644
> index 00000000000..731a17293fc
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/setjmp-2.c
[...snip...]
Likewise, this looks the same as gcc.dg/analyzer/setjmp-2.c (but with
c/c++ differences for the multiline output).
Presumably the old file needs to get deleted.
> diff --git a/gcc/testsuite/c-c++-common/analyzer/setjmp-5.c
b/gcc/testsuite/c-c++-common/analyzer/setjmp-5.c
> new file mode 100644
> index 00000000000..3133a473d51
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/setjmp-5.c
[...snip...]
Likewise here for setjmp-5.c
> diff --git a/gcc/testsuite/c-c++-common/analyzer/setjmp-9.c
> b/gcc/testsuite/c-c++-common/analyzer/setjmp-9.c
> new file mode 100644
> index 00000000000..f7e940ea605
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/setjmp-9.c
[...snip...]
Likewise here for setjmp-9.c
> diff --git a/gcc/testsuite/c-c++-common/analyzer/signal-4a.c
> b/gcc/testsuite/c-c++-common/analyzer/signal-4a.c
> new file mode 100644
> index 00000000000..b5c6012ec2e
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/signal-4a.c
[...snip...]
Likewise here for signal-4a.c
> diff --git a/gcc/testsuite/c-c++-common/analyzer/signal-4b.c
> b/gcc/testsuite/c-c++-common/analyzer/signal-4b.c
> new file mode 100644
> index 00000000000..d2b5db7b276
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/analyzer/signal-4b.c
[...snip...]
Likewise here for signal-4b.c
Thanks
Dave
