Re: [r14-3823 Regression] FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++98 (test for warnings, line 72) on Linux/x86_64
Hi,
Thanks for the report,
After investigation it seems the location of the new dejagnu directive for
C++ differs depending on the configuration.
The expected warning is still emitted, but its location differ slightly.
I expect it to be not an issue per se of the analyzer, but a divergence in
the FE between the two configurations.
Need further investigation.
Best,
Benjamin.
On Mon, Sep 11, 2023 at 10:03 AM Jiang, Haochen
wrote:
> On Linux/x86_64,
>
> 50b5199cff690891726877e1c00ac53dfb7cc1c8 is the first bad commit
> commit 50b5199cff690891726877e1c00ac53dfb7cc1c8
> Author: benjamin priour
> Date: Sat Sep 9 18:03:56 2023 +0200
>
> analyzer: Move gcc.dg/analyzer tests to c-c++-common (2) [PR96395]
>
> caused
>
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++14 (test for
> excess errors)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++14 (test for
> warnings, line 72)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++17 (test for
> excess errors)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++17 (test for
> warnings, line 72)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++20 (test for
> excess errors)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++20 (test for
> warnings, line 72)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++98 (test for
> excess errors)
> FAIL: c-c++-common/analyzer/compound-assignment-1.c -std=c++98 (test for
> warnings, line 72)
>
> with GCC configured with
>
> ../../gcc/configure
> --prefix=/export/users/haochenj/src/gcc-bisect/master/master/r14-3823/usr
> --enable-clocale=gnu --with-system-zlib --with-demangler-in-ld
> --with-fpmath=sse --enable-languages=c,c++,fortran --enable-cet
> --without-isl --enable-libmpx x86_64-linux --disable-bootstrap
>
> To reproduce:
>
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="analyzer.exp=c-c++-common/analyzer/compound-assignment-1.c
> --target_board='unix{-m32}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="analyzer.exp=c-c++-common/analyzer/compound-assignment-1.c
> --target_board='unix{-m32\ -march=cascadelake}'"
>
> (If you met problems with cascadelake related, disabling AVX512F in
> command line might save that.)
> (However, please make sure that there is no potential problems with
> AVX512.)
>
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.
[PATCH v2] analyzer: Call off a superseding when diagnostics are unrelated [PR110830]
From: benjamin priour
Hi,
Second version of this patch after David's suggestions.
Thanks David for pointing out how I could implement it using sedges.
I hadn't thought of them being independent of the exploded path taken,
and unique for a conditional block's outcome. I had mistaken them with
eedges, those that we see in the *exploded*-graph, therefore since the
two saved_diagnostics can have arbitrary different paths I had to
use a nested loop.
It is much more efficient this way.
Regstrapped off trunk a7d052b3200c7928d903a0242b8cfd75d131e374 on
x86_64-linux-gnu.
Is it ready for trunk ?
Thanks,
Benjamin.
Patch below.
---
Before this patch, a saved_diagnostic would supersede another at
the same statement if and only its vfunc supercedes_p returned true
for the other diagnostic's kind.
That both warning were unrelated - i.e. resolving one would not fix
the other - was not considered in making the above choice.
This patch makes it so that two saved_diagnostics taking a different
outcome of at least one common conditional branching cannot supersede
each other.
Signed-off-by: benjamin priour
Co-authored-by: david malcolm
gcc/analyzer/ChangeLog:
PR analyzer/110830
* diagnostic-manager.cc
(compatible_epaths_p): New function.
(saved_diagnostic::supercedes_p): Now calls the above
to determine if the diagnostics do overlap and the superseding
may proceed.
gcc/testsuite/ChangeLog:
PR analyzer/110830
* c-c++-common/analyzer/pr110830.c: New test.
---
gcc/analyzer/diagnostic-manager.cc| 90 +-
.../c-c++-common/analyzer/pr110830.c | 111 ++
2 files changed, 200 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/c-c++-common/analyzer/pr110830.c
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index 10fea486b8c..90c56a350e7 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -887,6 +887,88 @@ saved_diagnostic::add_duplicate (saved_diagnostic *other)
m_duplicates.safe_push (other);
}
+/* Walk up the sedges of each of the two paths.
+ If the two sequences of sedges do not perfectly correspond,
+ then paths are incompatible.
+ If there is at least one sedge that either cannot be paired up
+ or its counterpart is not equal, then the paths are incompatible
+ and this function returns FALSE.
+ Otherwise return TRUE.
+
+ Incompatible paths:
+
+
+ / \
+ /\
+true false
+ | |
+......
+ | |
+... stmt x
+ |
+ stmt x
+
+ Both LHS_PATH and RHS_PATH final enodes should be
+ over the same gimple statement. */
+
+static bool
+compatible_epath_p (const exploded_path *lhs_path,
+ const exploded_path *rhs_path)
+{
+ gcc_assert (lhs_path);
+ gcc_assert (rhs_path);
+ gcc_assert (rhs_path->length () > 0);
+ gcc_assert (rhs_path->length () > 0);
+ int lhs_eedge_idx = lhs_path->length () -1;
+ int rhs_eedge_idx = rhs_path->length () -1;
+ const exploded_edge *lhs_eedge;
+ const exploded_edge *rhs_eedge;
+
+ while (lhs_eedge_idx >= 0 && rhs_eedge_idx >= 0)
+{
+ while (lhs_eedge_idx >= 0)
+ {
+ /* Find LHS_PATH's next superedge. */
+ lhs_eedge = lhs_path->m_edges[lhs_eedge_idx];
+ if (lhs_eedge->m_sedge)
+ break;
+ else
+ lhs_eedge_idx--;
+ }
+ while (rhs_eedge_idx >= 0)
+ {
+ /* Find RHS_PATH's next superedge. */
+ rhs_eedge = rhs_path->m_edges[rhs_eedge_idx];
+ if (rhs_eedge->m_sedge)
+ break;
+ else
+ rhs_eedge_idx--;
+ }
+
+ if (lhs_eedge->m_sedge && rhs_eedge->m_sedge)
+ {
+ if (lhs_eedge->m_sedge != rhs_eedge->m_sedge)
+ /* Both superedges do not match.
+ Superedges are not dependent on the exploded path, so even
+ different epaths will have similar sedges if they follow
+ the same outcome of a conditional node. */
+ return false;
+
+ lhs_eedge_idx--;
+ rhs_eedge_idx--;
+ continue;
+ }
+ else if (lhs_eedge->m_sedge == nullptr && rhs_eedge->m_sedge == nullptr)
+ /* Both paths were drained up entirely.
+ No discriminant was found. */
+ return true;
+
+ /* A superedge was found for only one of the two paths. */
+ return false;
+}
+}
+
+
/* Return true if this diagnostic supercedes OTHER, and that OTHER should
therefore not be emitted. */
@@ -896,7 +978,13 @@ saved_diagnostic::supercedes_p (const saved_diagnostic
) const
/* They should be at the same stmt. */
if (m_stmt != other.m_stmt)
return false;
- return m_d->supercedes_p (*other.m_d);
+ /* return early if OTHER won't be superseded anyway. */
+ if (!m_d->supercedes_p (*other.m_d))
+return false;
+
+ /* If the two
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
[PATCH] c++: Additional warning for name-hiding [PR12341]
From: benjamin priour
Hi,
This patch was the first I wrote and had been
at that time returned to me because ill-formatted.
Getting busy with other things, I forgot about it.
I've now fixed the formatting.
Succesfully regstrapped on x86_64-linux-gnu off trunk
a7d052b3200c7928d903a0242b8cfd75d131e374.
Is it OK for trunk ?
Thanks,
Benjamin.
Patch below.
---
Add a new warning for name-hiding. When a class's field
is named similarly to one inherited, a warning should
be issued.
This new warning is controlled by the existing Wshadow.
gcc/cp/ChangeLog:
PR c++/12341
* search.cc (lookup_member):
New optional parameter to preempt processing the
inheritance tree deeper than necessary.
(lookup_field): Likewise.
(dfs_walk_all): Likewise.
* cp-tree.h: Update the above declarations.
* class.cc: (warn_name_hiding): New function.
(finish_struct_1): Call warn_name_hiding if -Wshadow.
gcc/testsuite/ChangeLog:
PR c++/12341
* g++.dg/pr12341-1.C: New file.
* g++.dg/pr12341-2.C: New file.
Signed-off-by: benjamin priour
---
gcc/cp/class.cc | 75
gcc/cp/cp-tree.h | 9 ++--
gcc/cp/search.cc | 28
gcc/testsuite/g++.dg/pr12341-1.C | 65 +++
gcc/testsuite/g++.dg/pr12341-2.C | 34 +++
5 files changed, 200 insertions(+), 11 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/pr12341-1.C
create mode 100644 gcc/testsuite/g++.dg/pr12341-2.C
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 778759237dc..b1c59c392a0 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,79 @@ warn_hidden (tree t)
}
}
+/* Warn about non-static fields name hiding. */
+
+static void
+warn_name_hiding (tree t)
+{
+ if (is_empty_class (t) || CLASSTYPE_NEARLY_EMPTY_P (t))
+return;
+
+ for (tree field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
+{
+ /* Skip if field is not an user-defined non-static data member. */
+ if (TREE_CODE (field) != FIELD_DECL || DECL_ARTIFICIAL (field))
+ continue;
+
+ unsigned j;
+ tree name = DECL_NAME (field);
+ /* Skip if field is anonymous. */
+ if (!name || !identifier_p (name))
+ continue;
+
+ auto_vec base_vardecls;
+ tree binfo;
+ tree base_binfo;
+ /* Iterate through all of the base classes looking for possibly
+shadowed non-static data members. */
+ for (binfo = TYPE_BINFO (t), j = 0;
+ BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
+ {
+ tree basetype = BINFO_TYPE (base_binfo);
+ tree candidate = lookup_field (basetype, name,
+/* protect */ 2,
+/* want_type */ 0,
+/* once_suffices */ true);
+ if (candidate)
+ {
+ /* If we went up the hierarchy to a base class with multiple
+inheritance, there could be multiple matches in which case
+a TREE_LIST is returned. */
+ if (TREE_TYPE (candidate) == error_mark_node)
+ {
+ for (; candidate; candidate = TREE_CHAIN (candidate))
+ {
+ tree candidate_field = TREE_VALUE (candidate);
+ tree candidate_klass = DECL_CONTEXT (candidate_field);
+ if (accessible_base_p (t, candidate_klass, true))
+ base_vardecls.safe_push (candidate_field);
+ }
+ }
+ else if (accessible_base_p (t, DECL_CONTEXT (candidate), true))
+ base_vardecls.safe_push (candidate);
+ }
+ }
+
+ /* Field was not found among the base classes. */
+ if (base_vardecls.is_empty ())
+ continue;
+
+ /* Emit a warning for each field similarly named found
+in the base class hierarchy. */
+ for (tree base_vardecl : base_vardecls)
+ {
+ if (base_vardecl)
+ {
+ auto_diagnostic_group d;
+ if (warning_at (location_of (field), OPT_Wshadow,
+ "%qD might shadow %qD", field, base_vardecl))
+ inform (location_of (base_vardecl),
+ " %qD name already in use here", base_vardecl);
+ }
+ }
+}
+}
+
/* Recursive helper for finish_struct_anon. */
static void
@@ -7654,6 +7727,8 @@ finish_struct_1 (tree t)
if (warn_overloaded_virtual)
warn_hidden (t);
+ if (warn_shadow)
+warn_name_hiding (t);
/* Class layout, assignment of virtual table slots, etc., is now
complete. Give the back end a chance to tweak the visibility of
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 3ca011c61c8..890326f0fd8 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7554,11 +7554,13 @@
[PATCH] analyzer: call off a superseding when diagnostics are unrelated [PR110830]
From: benjamin priour
Hi,
Patch succesfully regstrapped off trunk 7f2ed06ddc825e8a4e0edfd1d66b5156e6dc1d34
on x86_64-linux-gnu.
Is it OK for trunk ?
Thanks,
Benjamin.
Patch below.
---
Before this patch, a saved_diagnostic would supersede another at
the same statement if and only its vfunc supercedes_p returned true
for the other diagnostic's kind.
That both warning were unrelated, that is resolving one would not fix
the other was not considered in making the above choice.
This patch makes it so that two saved_diagnostics taking a different
outcome of at least one common conditional branching cannot supersede
each other.
Signed-off-by: benjamin priour
gcc/analyzer/ChangeLog:
PR analyzer/110830
* diagnostic-manager.cc
(compatible_epaths_p): New function.
(saved_diagnostic::supercedes_p): Now calls the above
to determine if the diagnostics do overlap and the superseding
may proceed.
gcc/testsuite/ChangeLog:
PR analyzer/110830
* c-c++-common/analyzer/pr110830.c: New test.
---
gcc/analyzer/diagnostic-manager.cc| 89 +-
.../c-c++-common/analyzer/pr110830.c | 111 ++
2 files changed, 199 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/c-c++-common/analyzer/pr110830.c
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index 10fea486b8c..7cf181e7972 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -887,6 +887,87 @@ saved_diagnostic::add_duplicate (saved_diagnostic *other)
m_duplicates.safe_push (other);
}
+/* Walk up the two paths to each of their common conditional
+ branching. At each branching, make sure both diagnostics'
+ paths branched similarly. If there is at least one where
+ both paths go down a different outcome, then the paths
+ are incompatible and this function returns FALSE.
+ Otherwise return TRUE.
+
+ Incompatible paths:
+
+
+ / \
+ /\
+true false
+ | |
+......
+ | |
+... stmt x
+ |
+ stmt x
+
+ Both LHS_PATH and RHS_PATH final enodes should be
+ over the same gimple statement. */
+
+static bool
+compatible_epath_p (const exploded_path *lhs_path,
+ const exploded_path *rhs_path)
+{
+ gcc_assert (lhs_path);
+ gcc_assert (rhs_path);
+ int i;
+ const exploded_edge *outer_eedge;
+ FOR_EACH_VEC_ELT_REVERSE (lhs_path->m_edges, i, outer_eedge)
+{
+ const superedge *outer_sedge = outer_eedge->m_sedge;
+ if (!outer_sedge || !outer_eedge->m_src)
+ continue;
+ const program_point _src_point = outer_eedge->m_src->get_point ();
+ switch (outer_src_point.get_kind ())
+ {
+ case PK_AFTER_SUPERNODE:
+ if (const cfg_superedge *cfg_outer_sedge
+ = outer_sedge->dyn_cast_cfg_superedge ())
+ {
+ int j;
+ const exploded_edge *inner_eedge;
+ FOR_EACH_VEC_ELT_REVERSE (rhs_path->m_edges, j, inner_eedge)
+ {
+ const superedge *inner_sedge = inner_eedge->m_sedge;
+ if (!inner_sedge || !inner_eedge->m_src)
+ continue;
+ const program_point _src_point
+ = inner_eedge->m_src->get_point ();
+ switch (inner_src_point.get_kind ())
+ {
+ case PK_AFTER_SUPERNODE:
+ if (inner_src_point.get_stmt ()
+ != outer_src_point.get_stmt ())
+ continue;
+ if (const cfg_superedge *cfg_inner_sedge
+ = inner_sedge->dyn_cast_cfg_superedge ())
+ {
+ if (cfg_inner_sedge->true_value_p ()
+ != cfg_outer_sedge->true_value_p ())
+ return false;
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
+}
+return true;
+}
+
+
/* Return true if this diagnostic supercedes OTHER, and that OTHER should
therefore not be emitted. */
@@ -896,7 +977,13 @@ saved_diagnostic::supercedes_p (const saved_diagnostic
) const
/* They should be at the same stmt. */
if (m_stmt != other.m_stmt)
return false;
- return m_d->supercedes_p (*other.m_d);
+ /* return early if OTHER won't be superseded anyway. */
+ if (!m_d->supercedes_p (*other.m_d))
+return false;
+
+ /* If the two saved_diagnostics' path are not compatible
+ then they cannot supersede one another. */
+ return compatible_epath_p (m_best_epath.get (), other.m_best_epath.get ());
}
/*
Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948,PR94355]
Patch has been updated as per your suggestions and successfully regstrapped
on x86_64-linux-gnu.
call_details::maybe_get_arg_region is now
/* If argument IDX's svalue at the callsite is of pointer type,
return the region it points to.
Otherwise return NULL. */
const region *
call_details::deref_ptr_arg (unsigned idx) const
{
const svalue *ptr_sval = get_arg_svalue (idx);
return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
}
New test is
+
+void test_binop ()
+{
+ char *p = (char *) malloc (4);
+ if (!p)
+return;
+ int32_t *i = ::new (p + 1) int32_t; /* { dg-warning "heap-based buffer
overflow" } */
+ *i = 42; /* { dg-warning "heap-based buffer overflow" } */
+ free (p);
+}
Is it OK for trunk ?
I didn't resend the whole patch as it otherwise was OK.
Thanks,
Benjamin.
On Fri, Sep 1, 2023 at 12:07 PM Benjamin Priour wrote:
> Hi David,
>
> On Fri, Sep 1, 2023 at 1:59 AM David Malcolm wrote:
>
>> On Fri, 2023-09-01 at 00:04 +0200, priour...@gmail.com wrote:
>>
>>
> [..snip..]
>
>
>> ...which will only fire if arg 1 is a region_svalue. This won't
>> trigger if you have e.g. a binop_svalue for pointer arithmetic.
>>
>> What happens e.g. for this one-off-the-end bug:
>>
>> void *p = malloc (4);
>> if (!p)
>> return;
>> int32_t *i = ::new (p + 1) int32_t;
>> *i = 42;
>>
>> So maybe call_details::maybe_get_arg_region should instead be:
>>
>> /* Return the region that argument IDX points to. */
>>
>> const region *
>> call_details::deref_ptr_arg (unsigned idx) const
>> {
>> const svalue *ptr_sval = get_arg_svalue (idx);
>> return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
>> }
>>
>> (caveat: I didn't test this)
>>
>> > + const region *base_reg = ptr_reg->get_base_region ();
>> > + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
>> > + const region *sized_new_reg
>> > + = mgr->get_sized_region (base_reg,
>> > + cd.get_lhs_type (),
>> > + num_bytes_sval);
>>
>> Why do you use the base_reg here, rather than just ptr_reg?
>>
>> In the example above, the *(p + 1) has base region
>> heap_allocated_region, but the ptr_reg is one byte higher; hence
>> check_region_for_write of 4 bytes ought to detect a problem with
>> writing 4 bytes to *(p + 1), but wouldn't complain about the write to
>> *p.
>>
>> ...assuming that I'm reading this code correctly.
>>
>> > + model->check_region_for_write (sized_new_reg,
>> > +nullptr,
>> > +ctxt);
>> > + const svalue *ptr_sval
>> > + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
>> > + cd.maybe_set_lhs (ptr_sval);
>> > + }
>> > + }
>>
>> [...snip...]
>>
>> The patch is OK for trunk as is; but please can you look into the
>> above.
>>
>>
> Thanks for the test case David, it exposed a missing heap-based over write
> when on the placement new statement.
> I've updated the code as per your suggestions, and it now works properly.
>
>
>> If the above is a problem, you can either do another version of the
>> patch, or do it as a followup patch (whichever you're more comfortable
>> with, but it might be best to get the patch into trunk as-is, given
>> that the GSoC period is nearly over).
>>
>> Thanks
>> Dave
>>
>>
> I will update the patch and regstrap it, so that it is done at once.
> I've compared the new test case to a "C" version of it, resulting in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111266
>
> I will attempt to fix it while I'm regstrapping everything else,
> I still have 4 patches in queue.
> It will give me a brief break from transitioning the tests :)
>
> Thanks for the review,
> Benjamin.
>
Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948,PR94355]
Hi David,
On Fri, Sep 1, 2023 at 1:59 AM David Malcolm wrote:
> On Fri, 2023-09-01 at 00:04 +0200, priour...@gmail.com wrote:
>
>
[..snip..]
> ...which will only fire if arg 1 is a region_svalue. This won't
> trigger if you have e.g. a binop_svalue for pointer arithmetic.
>
> What happens e.g. for this one-off-the-end bug:
>
> void *p = malloc (4);
> if (!p)
> return;
> int32_t *i = ::new (p + 1) int32_t;
> *i = 42;
>
> So maybe call_details::maybe_get_arg_region should instead be:
>
> /* Return the region that argument IDX points to. */
>
> const region *
> call_details::deref_ptr_arg (unsigned idx) const
> {
> const svalue *ptr_sval = get_arg_svalue (idx);
> return m_model->deref_rvalue (ptr_sval, get_arg_tree (idx), m_ctxt);
> }
>
> (caveat: I didn't test this)
>
> > + const region *base_reg = ptr_reg->get_base_region ();
> > + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
> > + const region *sized_new_reg
> > + = mgr->get_sized_region (base_reg,
> > + cd.get_lhs_type (),
> > + num_bytes_sval);
>
> Why do you use the base_reg here, rather than just ptr_reg?
>
> In the example above, the *(p + 1) has base region
> heap_allocated_region, but the ptr_reg is one byte higher; hence
> check_region_for_write of 4 bytes ought to detect a problem with
> writing 4 bytes to *(p + 1), but wouldn't complain about the write to
> *p.
>
> ...assuming that I'm reading this code correctly.
>
> > + model->check_region_for_write (sized_new_reg,
> > +nullptr,
> > +ctxt);
> > + const svalue *ptr_sval
> > + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
> > + cd.maybe_set_lhs (ptr_sval);
> > + }
> > + }
>
> [...snip...]
>
> The patch is OK for trunk as is; but please can you look into the
> above.
>
>
Thanks for the test case David, it exposed a missing heap-based over write
when on the placement new statement.
I've updated the code as per your suggestions, and it now works properly.
> If the above is a problem, you can either do another version of the
> patch, or do it as a followup patch (whichever you're more comfortable
> with, but it might be best to get the patch into trunk as-is, given
> that the GSoC period is nearly over).
>
> Thanks
> Dave
>
>
I will update the patch and regstrap it, so that it is done at once.
I've compared the new test case to a "C" version of it, resulting in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111266
I will attempt to fix it while I'm regstrapping everything else,
I still have 4 patches in queue.
It will give me a brief break from transitioning the tests :)
Thanks for the review,
Benjamin.
[PATCH] analyzer: Add support of placement new and improved operator new [PR105948, PR94355]
From: benjamin priour Hi, Succesfully regstrapped off trunk 7f2ed06ddc825e8a4e0edfd1d66b5156e6dc1d34 on x86_64-linux-gnu. Is it OK for trunk ? Thanks, Benjamin. Patch below. --- Fixed spurious possibly-NULL warning always tagging along throwing operator new despite it never returning NULL. Now operator new is correctly recognized as possibly returning NULL if and only if it is non-throwing or exceptions have been disabled. Different standard signatures of operator new are now properly recognized. Added support of placement new, so that it is now properly recognized, and a 'heap_allocated' region is no longer created for it. Placement new size is also checked and a 'Wanalyzer-allocation-size' is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'. 'operator new' non-throwing variants are detected y checking the types of the parameters. Indeed, in a call to new (std::nothrow) () the chosen overload has signature 'operator new (void*, std::nothrow_t&)', where the second parameter is a reference. In a placement new, the second parameter will always be a void pointer. Prior to this patch, some buffers first allocated with 'new', then deleted an thereafter used would result in a 'Wanalyzer-user-after-free' warning. However the wording was "use after 'free'" instead of the expected "use after 'delete'". This patch fixes this by introducing a new kind of poisoned value, namely POISON_KIND_DELETED. Due to how the analyzer sees calls to non-throwing variants of operator new, dereferencing a pointer freshly allocated in this fashion caused both a 'Wanalyzer-use-of-uninitialized-value' and a 'Wanalyzer-null-dereference' to be emitted, while only the latter was relevant. As a result, 'null-dereference' now supersedes 'use-of-uninitialized'. Signed-off-by: benjamin priour gcc/analyzer/ChangeLog: PR analyzer/105948 PR analyzer/94355 * analyzer.h (is_placement_new_p): New declaration. * call-details.cc (call_details::maybe_get_arg_region): New function. Returns the region of the argument at given index if possible. * call-details.h: Declaration of the above function. * kf-lang-cp.cc (is_placement_new_p): Returns true if the gcall is recognized as a placement new. (kf_operator_delete::impl_call_post): Unbinding a region and its descendents now poisons with POISON_KIND_DELETED. (register_known_functions_lang_cp): Known function "operator delete" is now registered only once independently of its number of arguments. * region-model.cc (region_model::eval_condition): Now recursively calls itself if any of the operand is wrapped in a cast. * sm-malloc.cc (malloc_state_machine::on_stmt): Add placement new recognition. * svalue.cc (poison_kind_to_str): Wording for the new PK. * svalue.h (enum poison_kind): Add value POISON_KIND_DELETED. gcc/testsuite/ChangeLog: PR analyzer/105948 PR analyzer/94355 * g++.dg/analyzer/out-of-bounds-placement-new.C: Added a directive. * g++.dg/analyzer/placement-new.C: Added tests. * g++.dg/analyzer/new-2.C: New test. * g++.dg/analyzer/noexcept-new.C: New test. * g++.dg/analyzer/placement-new-size.C: New test. --- gcc/analyzer/analyzer.h | 1 + gcc/analyzer/call-details.cc | 11 ++ gcc/analyzer/call-details.h | 1 + gcc/analyzer/kf-lang-cp.cc| 117 +++--- gcc/analyzer/region-model.cc | 36 ++ gcc/analyzer/sm-malloc.cc | 37 -- gcc/analyzer/svalue.cc| 2 + gcc/analyzer/svalue.h | 3 + gcc/testsuite/g++.dg/analyzer/new-2.C | 70 +++ gcc/testsuite/g++.dg/analyzer/noexcept-new.C | 48 +++ .../analyzer/out-of-bounds-placement-new.C| 2 +- .../g++.dg/analyzer/placement-new-size.C | 27 gcc/testsuite/g++.dg/analyzer/placement-new.C | 90 +- 13 files changed, 417 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/g++.dg/analyzer/new-2.C create mode 100644 gcc/testsuite/g++.dg/analyzer/noexcept-new.C create mode 100644 gcc/testsuite/g++.dg/analyzer/placement-new-size.C diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 9b351b5ed56..208b85026fc 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -423,6 +423,7 @@ extern bool is_std_named_call_p (const_tree fndecl, const char *funcname, const gcall *call, unsigned int num_args); extern bool is_setjmp_call_p (const gcall *call); extern bool is_longjmp_call_p (const gcall *call); +extern bool is_placement_new_p (const gcall *call); extern const char *get_user_facing_name (const gcall *call); diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc index
Re: [PATCH v2] analyzer: Move gcc.dg/analyzer tests to c-c++-common (1) [PR96395]
Hi Prathamesh,
Thanks for the report, I am running on a x86_64_linux_gnu and never had to
cross-compile before.
I've tried to set it up and reproduce your errors, please tell me if I'm
off the mark.
I configured gcc as:
../gcc/configure --prefix $CONTROL_PFX --disable-bootstrap
--host=x86_64-linux-gnu --target=arm-eabi --enable-languages=c --with-newlib
Then from the patch 7997f0d35efca8a24d1b0ceae5066b1019d633d7 prior to mine,
*I ran the tests manually as arm-sim refused to work*:
$GCC_SRC/control/eabi_arm_build/gcc/xgcc
-B$GCC_SRC/control//eabi_arm_build/gcc \
../../gcc/gcc/testsuite/gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
\
-fanalyzer -Wanalyzer-too-complex -fanalyzer-call-summaries
and
$GCC_SRC/control/eabi_arm_build/gcc/xgcc
-B$GCC_SRC/control//eabi_arm_build/gcc \
../../gcc/gcc/testsuite/gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
\
-fanalyzer -Wanalyzer-too-complex -fanalyzer-call-summaries -O2
>From my patch, I ran similar commands, only s/gcc.dg/c-c++-common/ as the
patch
moved the tests there.
The output were *identical*, for both pre/post-patched.
I do agree with you that there is an excess error compared to
x86_64-linux-gnu and the test expectations.
../../gcc/gcc/testsuite/gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:63:65:
warning: converting a packed ‘enum obj_type’ pointer (alignment 1) to a
‘struct connection’ pointer (alignment 4) may result in an unaligned
pointer value [-Waddress-of-packed-member]
63 | return ((struct connection *)(((void *)(t)) - ((long)&((struct
connection *)0)->obj_type)));
|
^~
../../gcc/gcc/testsuite/gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:31:6:
note: defined here
31 | enum obj_type {
| ^~~~
../../gcc/gcc/testsuite/gcc.dg/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:47:8:
note: defined here
47 | struct connection {
|^~
Therefore I do also see a regression, but it does not come from
55f6a7d949abc708d1c6ebc01eb3053f96d1472b.
It seems like an easy fix though and I will add it to the second part of
this patch.
However I did run the tests manually, rather than with make check-gcc as it
failed to run - an issue with stdio not found.
I'm attending this issue and will further update you in case the conclusion
of this mail changes.
Thanks again,
Benjamin.
On Tue, Aug 29, 2023 at 8:47 AM Prathamesh Kulkarni <
prathamesh.kulka...@linaro.org> wrote:
> On Sat, 26 Aug 2023 at 18:02, Benjamin Priour via Gcc-patches
> wrote:
> >
> > From: benjamin priour
> >
> > Hi,
> >
> > Updated version of the patch, regstrapping the changes described in
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628455.html.
> >
> > Regstrapped off trunk 66be6ed81f369573824f1a8f5a3538a63472292f
> > on x86_64-linux-gnu.
> Hi Benjamin,
> It seems your patch committed in 55f6a7d949abc708d1c6ebc01eb3053f96d1472b
> caused following regressions on arm-eabi config:
> Running gcc:gcc.dg/analyzer/analyzer.exp ...
> FAIL:
> c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
> (test for excess errors)
> FAIL:
> c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
> (test for excess errors)
>
> Please let me know if you need help to reproduce these failures.
>
> Thanks,
> Prathamesh
>
>
Re: [PATCH] analyzer: Move gcc.dg/analyzer tests to c-c++-common (1).
Hi David,
Thanks for the review.
On Fri, Aug 25, 2023 at 2:12 AM David Malcolm wrote:
> > From: benjamin priour
> >
> > Hi,
> >
> > Below the first batch of a serie of patches to transition
> > the analyzer testsuite from gcc.dg/analyzer to c-c++-common/analyzer.
> > I do not know how long this serie will be, thus the patch was not
> > numbered.
> >
> > For the grand majority of the tests, the transition only required some
> > adjustement over the syntax and casts to be C++-friendly, or to adjust
> > the warnings regexes to fit the C++ FE.
> >
> > The most noteworthy change is in the handling of known_functions,
> > as described in the below patch.
>
> Hi Benjamin.
>
> Many thanks for putting this together, it looks like it was a lot of
> work.
>
> > Successfully regstrapped on x86_64-linux-gnu off trunk
> > 18befd6f050e70f11ecca1dd58624f0ee3c68cc7.
>
> Did you compare the before/after results from DejaGnu somehow?
>
> Note that I've pushed 9 patches to the analyzer since
> 18befd6f050e70f11ecca1dd58624f0ee3c68cc7 and some of those touch the
> files below, so it's worth rebasing and double-checking the results.
>
>
Thanks for the info, I've rebased off it and I'm regstrapping.
For tests I didn't touch the warnings, I have checked they still pass in C
and C++.
Except for pr96841.c, C tests most notable update was to add a { target c }.
Every previous PASS and XFAIL remained as such in gcc.dg output.
For C++ I went with a no failure policy. Some tests weren't making sense
in C++,
such as transparent_unions. I've just skipped those.
For some tests C++ fpermissive would throw out an error. These tests I've
tried
to smuggle them out with const_cast and the likes, but never disabled
fpermissive.
> Please add
> PR analyzer/96395
> to the ChangeLog entries, and [PR96395] to the end of the Subject of
> the commit, so that these get tracked within that bug as they get
> pushed.
>
>
Nods.
[...snip...]
> I confess I'm still a little hazy as to the whole builtin_kf logic, but
> I trust you that this is needed.
>
> Please can you add a paragraph to this comment to explain the
> motivation here (perhaps giving examples?)
>
> > +
> > +const builtin_known_function *
> > +region_model::get_builtin_kf (const gcall *call,
> > +region_model_context *ctxt /* = NULL */)
> const
> > +{
> > + region_model *mut_this = const_cast (this);
> > + tree callee_fndecl = mut_this->get_fndecl_for_call (call, ctxt);
> > + if (! callee_fndecl)
> > +return NULL;
> > +
> > + call_details cd (call, mut_this, ctxt);
> > + if (const known_function *kf = get_known_function (callee_fndecl, cd))
> > +return kf->dyn_cast_builtin_kf ();
> > +
> > + return NULL;
> > +}
> > +
>
>
The new comment is as follow:
/* Get any builtin_known_function for CALL and emit any warning to CTXT
if not NULL.
The call must match all assumptions made by the known_function (such as
e.g. "argument 1's type must be a pointer type").
Return NULL if no builtin_known_function is found, or it does
not match the assumption(s).
Internally calls get_known_function to find a known_function and cast it
to a builtin_known_function.
For instance, calloc is a C builtin, defined in gcc/builtins.def
by the DEF_LIB_BUILTIN macro. Such builtins are recognized by the
analyzer by their name, so that even in C++ or if the user redeclares
them but mismatch their signature, they are still recognized as builtins.
Cases when a supposed builtin is not flagged as one by the FE:
The C++ FE does not recognize calloc as a builtin if it has not been
included from a standard header, but the C FE does. Hence in C++ if
CALL comes from a calloc and stdlib is not included,
gcc/tree.h:fndecl_built_in_p (CALL) would be false.
In C code, a __SIZE_TYPE__ calloc (__SIZE_TYPE__, __SIZE_TYPE__) user
declaration has obviously a mismatching signature from the standard, and
its function_decl tree won't be unified by
gcc/c-decl.cc:match_builtin_function_types.
Yet in both cases the analyzer should treat the calls as a builtin calloc
so that extra attributes unspecified by the standard but added by GCC
(e.g. sprintf attributes in gcc/builtins.def), useful for the detection
of
dangerous behavior, are indeed processed.
Therefore for those cases when a "builtin flag" is not added by the FE,
builtins' kf are derived from builtin_known_function, whose method
builtin_known_function::builtin_decl returns the builtin's
function_decl tree as defined in gcc/builtins.def, with all the extra
attributes. */
I hope it clarifies the new kf subclass's purpose.
[...snip...]
>
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/aliasing-3.c
> b/gcc/testsuite/c-c++-common/analyzer/aliasing-3.c
> > similarity index 85%
> > rename from gcc/testsuite/gcc.dg/analyzer/aliasing-3.c
> > rename to gcc/testsuite/c-c++-common/analyzer/aliasing-3.c
> > index 003077ad5c1..f78fea64fbe
Re: [WIP RFC v2] analyzer: Add support of placement new and improved operator new [PR105948]
On Thu, Aug 17, 2023 at 12:34 AM David Malcolm wrote:
> On Wed, 2023-08-16 at 14:19 +0200, priour...@gmail.com wrote:
> > From: benjamin priour
> >
> > Hi,
> > (s/we/the analyzer/)
>
> Hi Benjamin, thanks for the updated patch.
>
> >
> > I've been continuing my patch of supporting operator new variants
> > in the analyzer, and have added a few more test cases.
> >
> >
> > > > If "y" is null then the allocation failed and dereferencing
> > "y" will
> > > > cause
> > > > a segfault, not a "use-of-uninitialized-value".
> > > > Thus we should stick to 'dereference of NULL 'y'" only.
> > > > If "y" is non-null then the allocation succeeded and "*y" is
> > > > initialized
> > > > since we are calling a default initialization with the empty
> > > > parenthesis.
> > >
> > > I *think* it's possible to have the region_model have y
> > pointing to a
> > > heap_allocated_region of sizeof(int) size that's been
> > initialized, but
> > > still have the malloc state machine part of the program_state
> > say that
> > > the pointer is maybe-null.
> >
> > By maybe-null are you implying a new sm-malloc state ?
>
> Sorry, I was too vague here.
>
> I was referring to the "unchecked" state in sm-malloc.cc, which
> represents a pointer that's been returned from an allocator function,
> where the pointer hasn't yet been checked for being null/non-null.
>
> Oh I see then. Unfortunately I don't think initializing the
heap_allocated_region
while having he unchecked state is doable here. I could do it in the
kf_operator_new::on_call_{pre,post} hook, but it's not actually operator
new that
initiliaze the allocated region.
For calls such as "A a = new (nothrow) A;", then 'a' is actually never
initialized,
therefore we need the heap_allocated_region to reflect that.
> > I am not sure to follow on that front.
> >
> >
> > >
> > > > This led me to consider having "null-dereference" supersedes
> > > > "use-of-uninitialized-value", but
> > > > new PR 110830 made me reexamine it.
> > > >
> > > > I believe fixing PR 110830 is thus required before submitting
> > this
> > > > patch,
> > > > or we would have some extra irrelevant warnings.
> > >
> > > How bad would the problem be? PR 110830 looks a little
> > involved, so is
> > > there a way to get the current patch in without dragging that
> > extra
> > > complexity in?
> >
> > Having "null-dereference" supersedes "use-of-uninitialized-value"
> > would
> > cause false negative upon conditional return statement (similarly as
> > demonstrated
> > in PR 110830).
> >
> > Since PR 110830 is off for the moment, I have tried solving this
> > differently.
> > I have considered using known NULL constraints on
> > heap_allocated_region
> > as "initialized_value".
> >
> > You can see below in the diff of region_model::get_store_value
> > two versions of this approach. The version commented out proved to
> > solve
> > the issue of the spurious "use-of-unitialized-value" tagging along
> > calls to
> > "new(std::nothrow) ()". However, this version also shortcircuits the
> > diagnostics of the "null-dereference" warning.
> >
> > Given
> > /* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer-
> > suppress-followups" } */
> > #include
> >
> > struct A
> > {
> > int x;
> > int y;
> > };
> >
> > void test_nonthrowing ()
> > {
> > A* y = new(std::nothrow) A();
> > int z = y->x + 2; /* { dg-warning "dereference of NULL 'y'" }
> > */
> > /* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *-
> > *-* } .-1 } */
> >
> > delete y;
> > }
> >
> > The analyzer sees gimple
> >
> >:
> > _7 = operator new (8, );
> > if (_7 != 0B)
> > goto ; [INV]
> > else
> > goto ; [INV]
>
> I would have thought that at each branch of this conditional that
> region_model::add_constraint would be called, and within that we'd
> reach this code:
>
> 4339 /* Notify the context, if any. This exists so that the state
> machines
> 4340 in a program_state can be notified about the condition, and
> so can
> 4341 set sm-state for e.g. unchecked->checked, both for cfg-edges,
> and
> 4342 when synthesizing constraints as above. */
> 4343 if (ctxt)
> 4344ctxt->on_condition (lhs, op, rhs);
>
> This ought to call impl_region_model_context::on_condition in
> engine.cc, which ought to call malloc_state_machine::on_condition in
> sm-malloc.cc, and this ought to transition the sm-state of _7.
>
> Is something going wrong somewhere in the things I mentioned above?
>
> Nope. Everything's happening as you say.
> >
> >:
> > MEM[(struct A *)_7].x = 0;
> > MEM[(struct A *)_7].y = 0;
> > iftmp.0_11 = _7;
> > goto ; [INV]
> >
> >:
> > iftmp.0_8 = _7;
> >
> >:
> > # iftmp.0_2 = PHI
> > y_12 = iftmp.0_2;
> > _1 = y_12->x;
>
> ...and at this point we have a deref from y_12, which on the
[WIP RFC v2] analyzer: Add support of placement new and improved operator new [PR105948]
From: benjamin priour
Hi,
(s/we/the analyzer/)
I've been continuing my patch of supporting operator new variants
in the analyzer, and have added a few more test cases.
> > If "y" is null then the allocation failed and dereferencing "y" will
> > cause
> > a segfault, not a "use-of-uninitialized-value".
> > Thus we should stick to 'dereference of NULL 'y'" only.
> > If "y" is non-null then the allocation succeeded and "*y" is
> > initialized
> > since we are calling a default initialization with the empty
> > parenthesis.
>
> I *think* it's possible to have the region_model have y pointing to a
> heap_allocated_region of sizeof(int) size that's been initialized, but
> still have the malloc state machine part of the program_state say that
> the pointer is maybe-null.
By maybe-null are you implying a new sm-malloc state ?
I am not sure to follow on that front.
>
> > This led me to consider having "null-dereference" supersedes
> > "use-of-uninitialized-value", but
> > new PR 110830 made me reexamine it.
> >
> > I believe fixing PR 110830 is thus required before submitting this
> > patch,
> > or we would have some extra irrelevant warnings.
>
> How bad would the problem be? PR 110830 looks a little involved, so is
> there a way to get the current patch in without dragging that extra
> complexity in?
Having "null-dereference" supersedes "use-of-uninitialized-value" would
cause false negative upon conditional return statement (similarly as
demonstrated
in PR 110830).
Since PR 110830 is off for the moment, I have tried solving this
differently.
I have considered using known NULL constraints on heap_allocated_region
as "initialized_value".
You can see below in the diff of region_model::get_store_value
two versions of this approach. The version commented out proved to solve
the issue of the spurious "use-of-unitialized-value" tagging along calls to
"new(std::nothrow) ()". However, this version also shortcircuits the
diagnostics of the "null-dereference" warning.
Given
/* { dg-additional-options "-O0 -fno-exceptions
-fno-analyzer-suppress-followups" } */
#include
struct A
{
int x;
int y;
};
void test_nonthrowing ()
{
A* y = new(std::nothrow) A();
int z = y->x + 2; /* { dg-warning "dereference of NULL 'y'" } */
/* { dg-bogus "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1
} */
delete y;
}
The analyzer sees gimple
:
_7 = operator new (8, );
if (_7 != 0B)
goto ; [INV]
else
goto ; [INV]
:
MEM[(struct A *)_7].x = 0;
MEM[(struct A *)_7].y = 0;
iftmp.0_11 = _7;
goto ; [INV]
:
iftmp.0_8 = _7;
:
# iftmp.0_2 = PHI
y_12 = iftmp.0_2;
_1 = y_12->x;
z_13 = _1 + 2;
y.1_14 = y_12;
if (y.1_14 != 0B)
goto ; [INV]
else
goto ; [INV]
:
*y.1_14 ={v} {CLOBBER};
operator delete (y.1_14, 8);
The injurious path, causing the "use-of-uninit" warning is as follows:
:
_7 = operator new (8, );
if (_7 != 0B)
...
else <- Takes false branch
goto ; [INV]
...
:
iftmp.0_8 = _7; <- MEM[(struct A*) _7] is left uninit in this bb
:
# iftmp.0_2 = PHI <- iftmp.0_2 = iftmp.0_8(4)
y_12 = iftmp.0_2;
_1 = y_12->x; // deref of null y_12, use of uninit y_12->x
z_13 = _1 + 2; // check_for_poison sets _1 to unknown_svalue
y.1_14 = y_12;
if (y.1_14 != 0B)
goto ; [INV]
else
goto ; [INV]
Then using the "commented-out" fix, iftmp.0_8 which had an uninit value is
forcibly set to constant_svalue(0), since the analyzer detects a NULL constraint
on _allocated_region.
Unfortunately, this loses all clusters binding on _7 and the followings
variables, such as when we arrive at "_1 = y_12->x", we emit a
"null_deref" not because the heap_allocated_region is in a null state,
but because we are dereferencing a constant "0".
Thus the analysis path no longer tracks down the creation of this
region, and the genese event is "iftmp.0_8 = _7".
As you guess, this loss of information fails a lot of regression tests,
although it achieves the goal of removing the "use-of-uninit" warning.
The second attempt (see get_store_value diff below, the non-commented
out block), actually does nothing, which as I understood through
debugging was to be expected. We are doing the same "constraints" check
as the former version, but only as a last resort before resorting to
creating an initial or unknown svalue.
And instead of creating a constant_svalue(0) as before, now a NULL
constraint only prevents the creation of a poisoned_svalue(uninit)
by setting "check_poisoned" to false.
However in
+ if (reg->get_kind () == RK_FIELD || reg->get_kind () == RK_ELEMENT)
+{
+ const region *base_reg = reg->get_base_region ();
+ const svalue *base_sval
+ = m_store.get_any_binding (m_mgr->get_store_manager (), base_reg);
+ if (base_sval)
+ {
+...
+ }
[PATCH] testsuite: Remove unused dg-line in ce8cdf5bcf96a2db6d7b9f656fc9ba58d7942a83
From: benjamin priour
Yet another blunder.
Succesfully regstrapped against ce8cdf5bcf96a2db6d7b9f656fc9ba58d7942a83
on x86_64-linux-gnu.
OK to push on trunk ?
Sorry,
Benjamin.
Fixup below.
---
Test case g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C
introduced by patch ce8cdf5bcf96a2db6d7b9f656fc9ba58d7942a83
emitted a warning for an unused dg-line variable.
This fixes up the blunder.
Signed-off-by: benjamin priour
gcc/testsuite/ChangeLog:
* g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C:
Remove dg-line var declare_a.
---
.../g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C
b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C
index 4cc93d129f0..aa964f93563 100644
--- a/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C
+++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C
@@ -6,7 +6,7 @@
struct A {int x; int y;};
int main () { /* { dg-message "\\(1\\) entry to 'main'" "telltale event that
we are going within a deeper frame than 'main'" } */
- std::shared_ptr a; /* { dg-line declare_a } */
+ std::shared_ptr a;
a->x = 4; /* { dg-line deref_a } */
/* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */
--
2.34.1
[PATCH v2] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]
From: benjamin priour Plenty useful, thanks David. I've adjusted some few things, especially the artifacts of earlier versions I missed when building the commit. I didn't how to test for warnings within , I couldn't figure a portable test. I cannot pinpoint the line the warning is issued at in an inline DejaGNU directive, nor can I safely say the stack depth if I check a multiline-output (nor the methods names) In the end, I found out an alternative, I am checking for the presence of event "entry of 'main'". Indeed, diagnostic_manager::finish_pruning comment's reads If all we're left with is in one function, then filter function entry events. The provided test case can only goes into main and std::* frames, so if "entry of 'main'" exists, it means we are also going into std::* frames. I've also adjusted the comment of prune_system_headers, analyzer.opt and added an entry to invoker.texi. Successfully regstrapped off trunk 54be338589ea93ad4ff53d22adde476a0582537b on x86_64-linux-gnu. Thanks, Benjamin. Patch below. This patch introduces -fanalyzer-show-events-in-system-headers, disabled by default. This option reduces the noise of the analyzer emitted diagnostics when dealing with system headers. The new option only affects the display of the diagnostics, but doesn't hinder the actual analysis. Given a diagnostics path diving into a system header in the form [ prefix events..., system header call, system header entry, events within system headers..., system header return, suffix events... ] then disabling the option (either by default or explicitly) will shorten the path into: [ prefix events..., system header call, system header return, suffix events... ] Signed-off-by: benjamin priour gcc/analyzer/ChangeLog: PR analyzer/110543 * analyzer.opt: Add new option. * diagnostic-manager.cc (diagnostic_manager::prune_path): Call prune_system_headers. (prune_frame): New function that deletes all events in a frame. (diagnostic_manager::prune_system_headers): New function. * diagnostic-manager.h: Add prune_system_headers declaration. gcc/ChangeLog: PR analyzer/110543 * doc/invoke.texi: Add documentation of fanalyzer-show-events-in-system-headers gcc/testsuite/ChangeLog: PR analyzer/110543 * g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C: New test. * g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C: New test. * g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C: New test. --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/diagnostic-manager.cc| 96 +++ gcc/analyzer/diagnostic-manager.h | 1 + gcc/doc/invoke.texi | 9 ++ ...er-show-events-in-system-headers-default.C | 18 ...nalyzer-show-events-in-system-headers-no.C | 19 .../fanalyzer-show-events-in-system-headers.C | 14 +++ 7 files changed, 161 insertions(+) create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers.C diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 2760aaa8151..7917473d122 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -290,6 +290,10 @@ fanalyzer-transitivity Common Var(flag_analyzer_transitivity) Init(0) Enable transitivity of constraints during analysis. +fanalyzer-show-events-in-system-headers +Common Var(flag_analyzer_show_events_in_system_headers) Init(0) +Show events within system headers in analyzer execution paths. + fanalyzer-call-summaries Common Var(flag_analyzer_call_summaries) Init(0) Approximate the effect of function calls to simplify analysis. diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index cfca305d552..430c4dc3d58 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see #include "system.h" #include "coretypes.h" #include "tree.h" +#include "input.h" #include "pretty-print.h" #include "gcc-rich-location.h" #include "gimple-pretty-print.h" @@ -2281,6 +2282,8 @@ diagnostic_manager::prune_path (checker_path *path, path->maybe_log (get_logger (), "path"); prune_for_sm_diagnostic (path, sm, sval, state); prune_interproc_events (path); + if (! flag_analyzer_show_events_in_system_headers) +prune_system_headers (path); consolidate_conditions (path); finish_pruning (path); path->maybe_log (get_logger (), "pruned"); @@ -2667,6 +2670,99 @@ diagnostic_manager::prune_interproc_events (checker_path *path) const while (changed); } +/* Remove everything within [call
Re: [PATCH] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]
I forgot to mention that this has been successfully regstrapped off trunk 54be338589ea93ad4ff53d22adde476a0582537b on x86_64-linux-gnu. Is it OK for trunk ? Thanks, Benjamin.
[PATCH] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]
From: benjamin priour
This patch introduces -fanalyzer-show-events-in-system-headers,
disabled by default.
This option reduce the noise of the analyzer emitted diagnostics
when dealing with system headers.
The new option only affects the display of the diagnostics,
but doesn't hinder the actual analysis.
Given a diagnostics path diving into a system header in the form
[
prefix events...,
system header call,
system header entry,
events within system headers...,
system header return,
suffix events...
]
then disabling the option (either by default or explicitly)
will shorten the path into:
[
prefix events...,
system header call,
system header return,
suffix events...
]
Signed-off-by: benjamin priour
gcc/analyzer/ChangeLog:
PR analyzer/110543
* analyzer.cc (is_std_function_p): No longer static.
* analyzer.h (is_std_function_p): Add declaration.
* analyzer.opt: Add new option.
* diagnostic-manager.cc
(INCLUDE_VECTOR): Include vector from system.h
(diagnostic_manager::prune_path): Call prune_system_headers.
(prune_frame): New function that deletes all events in a frame.
(diagnostic_manager::prune_system_headers): New function.
* diagnostic-manager.h: Add prune_system_headers declaration.
gcc/testsuite/ChangeLog:
PR analyzer/110543
* g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C:
New test.
* g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C:
New test.
---
gcc/analyzer/analyzer.cc | 2 +-
gcc/analyzer/analyzer.h | 1 +
gcc/analyzer/analyzer.opt | 4 ++
gcc/analyzer/diagnostic-manager.cc| 65 +++
gcc/analyzer/diagnostic-manager.h | 1 +
...er-show-events-in-system-headers-default.C | 19 ++
...nalyzer-show-events-in-system-headers-no.C | 19 ++
7 files changed, 110 insertions(+), 1 deletion(-)
create mode 100644
gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C
create mode 100644
gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C
diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc
index 5091fb7a583..b27d8e359db 100644
--- a/gcc/analyzer/analyzer.cc
+++ b/gcc/analyzer/analyzer.cc
@@ -274,7 +274,7 @@ is_named_call_p (const_tree fndecl, const char *funcname)
Compare with cp/typeck.cc: decl_in_std_namespace_p, but this doesn't
rely on being the C++ FE (or handle inline namespaces inside of std). */
-static inline bool
+bool
is_std_function_p (const_tree fndecl)
{
tree name_decl = DECL_NAME (fndecl);
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index 579517c23e6..31597079153 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -386,6 +386,7 @@ extern bool is_special_named_call_p (const gcall *call,
const char *funcname,
extern bool is_named_call_p (const_tree fndecl, const char *funcname);
extern bool is_named_call_p (const_tree fndecl, const char *funcname,
const gcall *call, unsigned int num_args);
+extern bool is_std_function_p (const_tree fndecl);
extern bool is_std_named_call_p (const_tree fndecl, const char *funcname);
extern bool is_std_named_call_p (const_tree fndecl, const char *funcname,
const gcall *call, unsigned int num_args);
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 2760aaa8151..d97cd569f52 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -290,6 +290,10 @@ fanalyzer-transitivity
Common Var(flag_analyzer_transitivity) Init(0)
Enable transitivity of constraints during analysis.
+fanalyzer-show-events-in-system-headers
+Common Var(flag_analyzer_show_events_in_system_headers) Init(0)
+Trim diagnostics path that are too long before emission.
+
fanalyzer-call-summaries
Common Var(flag_analyzer_call_summaries) Init(0)
Approximate the effect of function calls to simplify analysis.
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index cfca305d552..2a9705a464f 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -20,9 +20,11 @@ along with GCC; see the file COPYING3. If not see
#include "config.h"
#define INCLUDE_MEMORY
+#define INCLUDE_VECTOR
#include "system.h"
#include "coretypes.h"
#include "tree.h"
+#include "input.h"
#include "pretty-print.h"
#include "gcc-rich-location.h"
#include "gimple-pretty-print.h"
@@ -2281,6 +2283,8 @@ diagnostic_manager::prune_path (checker_path *path,
path->maybe_log (get_logger (), "path");
prune_for_sm_diagnostic (path, sm, sval, state);
prune_interproc_events (path);
+ if (! flag_analyzer_show_events_in_system_headers)
+prune_system_headers (path);
consolidate_conditions (path);
finish_pruning (path);
path->maybe_log
Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]
Hi Dave,
On Fri, Jul 21, 2023 at 10:10 PM David Malcolm wrote:
[...]
It looks like something's gone wrong with the indentation in the above:
> previously we had tab characters, but now I'm seeing a pair of spaces,
> which means this wouldn't line up properly. This might be a glitch
> somewhere in our email workflow, but please check it in your editor
> (with visual whitespace enabled).
>
I'll double check that before submitting.
> [...snip...]
>
> Some comments on the test cases:
>
> > diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C
> b/gcc/testsuite/g++.dg/analyzer/new-2.C
> > new file mode 100644
> > index 000..4e696040a54
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C
> > @@ -0,0 +1,50 @@
> > +// { dg-additional-options "-O0" }
> > +
> > +struct A
> > +{
> > + int x;
> > + int y;
> > +};
> > +
> > +void test_spurious_null_warning_throwing ()
> > +{
> > + int *x = new int; /* { dg-bogus "dereference of possibly-NULL" } */
> > + int *y = new int (); /* { dg-bogus "dereference of possibly-NULL"
> "non-throwing" } */
> > + int *arr = new int[3]; /* { dg-bogus "dereference of possibly-NULL" }
> */
> > + A *a = new A (); /* { dg-bogus "dereference of possibly-NULL"
> "throwing new cannot be null" } */
> > +
> > + int z = *y + 2;
> > + z = *x + 4; /* { dg-bogus "dereference of possibly-NULL 'x'" } */
> > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > + z = arr[0] + 4; /* { dg-bogus "dereference of possibly-NULL" } */
> > +
> > + delete a;
> > + delete y;
> > + delete x;
> > + delete[] arr;
> > +}
> > +
> > +void test_default_initialization ()
> > +{
> > +int *y = ::new int;
> > +int *x = ::new int (); /* { dg-bogus "dereference of possibly-NULL
> 'operator new" } */
> > +
> > +int b = *x + 3; /* { dg-bogus "dereference of possibly-NULL" } */
> > +/* { dg-bogus "use of uninitialized ‘*x’" "" { target *-*-* } .-1 }
> */
> > +int a = *y + 2; /* { dg-bogus "dereference of possibly-NULL 'y'" }
> */
> > +/* { dg-warning "use of uninitialized value '\\*y'" "no default
> init" { target *-*-* } .-1 } */
> > +
> > +delete x;
> > +delete y;
> > +}
> > +
> > +/* From clang core.uninitialized.NewArraySize
> > +new[] should not be called with an undefined size argument */
> > +
> > +void test_garbage_new_array ()
> > +{
> > + int n;
> > + int *arr = ::new int[n]; /* { dg-warning "use of uninitialized value
> 'n'" } */
> > + arr[0] = 7;
> > + ::delete[] arr; /* no warnings emitted here either */
> > +}
> > diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> > new file mode 100644
> > index 000..7699cd99cff
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> > @@ -0,0 +1,48 @@
> > +/* { dg-additional-options "-O0 -fno-exceptions
> -fno-analyzer-suppress-followups" } */
> > +#include
> > +
> > +/* Test non-throwing variants of operator new */
> > +
> > +struct A
> > +{
> > + int x;
> > + int y;
> > +};
> > +
> > +void test_throwing ()
> > +{
> > + int* x = new int;
> > + int* y = new int(); /* { dg-warning "dereference of possibly-NULL" }
> */
> > + int* arr = new int[10];
> > + A *a = new A(); /* { dg-warning "dereference of possibly-NULL" } */
> > +
> > + int z = *y + 2;
> > + z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
> > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > + z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"
> } */
> > + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target
> *-*-* } .-1 } */
> > + a->y = a->x + 3;
> > +
> > + delete a;
> > + delete y;
> > + delete x;
> > + delete[] arr;
> > +}
> > +
> > +void test_nonthrowing ()
> > +{
> > + int* x = new(std::nothrow) int;
> > + int* y = new(std::nothrow) int();
> > + int* arr = new(std::nothrow) int[10];
> > +
> > + int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
> > + /* { dg-warning "use of uninitialized value '\\*y'" "" { target *-*-*
> } .-1 } */
> > + z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
> > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > + z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"
> } */
> > + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target
> *-*-* } .-1 } */
> > +
> > + delete y;
> > + delete x;
> > + delete[] arr;
> > +}
>
> I see that we have test coverage for:
> noexcept-new.C: -fno-exceptions with new vs nothrow-new
> whereas:
> new-2.C has (implicitly) -fexceptions with new
>
> It seems that of the four combinations for:
> - exceptions enabled or disabled
> and:
> - throwing versus non-throwing new
> this is covering three of the cases but is missing the case of nothrow-
> new when exceptions are enabled.
> Presumably new-2.C should gain test coverage for this case. Or am
Re: [WIP RFC] analyzer: Add optional trim of the analyzer diagnostics going too deep [PR110543]
On Sat, Jul 22, 2023 at 12:04 AM David Malcolm wrote:
> On Fri, 2023-07-21 at 17:35 +0200, Benjamin Priour wrote:
> > Hi,
> >
> > Upon David's request I've joined the in progress patch to the below
> > email.
> > I hope it makes more sense now.
> >
> > Best,
> > Benjamin.
>
> Thanks for posting the work-in-progress patch; it makes the idea
> clearer.
>
> Some thoughts about this:
>
> - I like the idea of defaulting to *not* showing events within system
> headers, which the patch achieves
> - I don't like the combination of never/system with maxdepth, in that
> it seems complicated and I don't think a user is likely to experiment
> with different depths.
> - Hence I think it would work better as a simple boolean, perhaps
> "-fanalyzer-show-events-in-system-headers"
> or somesuch? It seems like the sort of thing that we want to provide
> a sensible default for, but have the option of turning off for
> debugging the analyzer itself, but I don't expect an end-user to touch
> that option.
>
A boolean sounds good, I will trust your experience with the end-user here,
especially since and "never" had some overlap, it could have
been confusing.
> FWIW the patch seems to have been mangled somewhat via email, so I
> don't have a sense of what the actual output from patched analyzer
> looks like. What should we output to the user with -fanalyzer and no
> other options for the case in PR 110543? Currently, for
> https://godbolt.org/z/sb9dM9Gqa trunk emits 12 events, of which
> probably only this last one is useful:
>
> (12) dereference of NULL 'a.std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->()'
>
> What does the output look like with your patch?
>
The plan with this patch was to get events :
(1) entry to 'main'
(2) calling 'std::__shared_ptr_access::operator->' from 'main'
(12) dereference of NULL 'a.std::__shared_ptr_access::operator->()'
(11) returning to 'main' from 'std::__shared_ptr_access::operator->'
This way, we get the entry and exit point to the system headers ( (2) and
(11) ), and the actual injurious event ( (12) ).
We could however go as you suggest, with an even more succint path and only
keep (1) and (12).
Thanks,
Benjamin
> Thanks
> Dave
>
>
> >
> > -- Forwarded message -
> > From: Benjamin Priour
> > Date: Tue, Jul 18, 2023 at 3:30 PM
> > Subject: [RFC] analyzer: Add optional trim of the analyzer
> > diagnostics
> > going too deep [PR110543]
> > To: , David Malcolm
> >
> >
> > Hi,
> >
> > I'd like to request comments on a patch I am writing for PR110543.
> > The goal of this patch is to reduce the noise of the analyzer emitted
> > diagnostics when dealing with
> > system headers, or simply diagnostic paths that are too long. The new
> > option only affects the display
> > of the diagnostics, but doesn't hinder the actual analysis.
> >
> > I've defaulted the new option to "system", thus preventing the
> > diagnostic
> > paths from showing system headers.
> > "never" corresponds to the pre-patch behavior, whereas you can also
> > specify
> > an unsigned value
> > that prevents paths to go deeper than frames.
> >
> > fanalyzer-trim-diagnostics=
> > > Common Joined RejectNegative ToLower
> > > Var(flag_analyzer_trim_diagnostics)
> > > Init("system")
> > > -fanalyzer-trim-diagnostics=[never|system|] Trim
> > > diagnostics
> > > path that are too long before emission.
> > >
> >
> > Does it sounds reasonable and user-friendly ?
> >
> > Regstrapping was a success against trunk, although one of the newly
> > added
> > test case fails for c++14.
> > Note that the test case below was done with "never", thus behaves
> > exactly
> > as the pre-patch analyzer
> > on x86_64-linux-gnu.
> >
> > /* { dg-additional-options "-fdiagnostics-plain-output
> > > -fdiagnostics-path-format=inline-events -fanalyzer-trim-
> > > diagnostics=never"
> > > } */
> > > /* { dg-skip-if "" { c++98_only } } */
> > >
> > > #include
> > > struct A {int x; int y;};
> > >
> > > int main () {
> > > std::shared_ptr a;
> > > a->x = 4; /* { dg-line deref_a } */
> > > /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a
> > > } */
> > >
> > > return 0;
> > > }
> > >
> > > /* { dg-begin-multiline-output "" }
> > > 'int main()': events 1-2
> > > |
> > > |
> > > +--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> > >
> > > > ::element_type* std::__shared_ptr_access<_Tp, _Lp, ,
> > > >::operator->() const [with _Tp = A;
> > > __gnu_cxx::_Lock_policy
> > > _Lp = __gnu_cxx::_S_atomic; bool = false; bool
> > > =
> > > false]': events 3-4
> > >|
> > >|
> > >+--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> > > >::element_type* std::__shared_ptr_access<_Tp, _Lp,
> > > , >::_M_get() const [with _Tp = A;
> > > __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool
> > > =
> > > false; bool = false]': events 5-6
> > > |
> > > |
> > > +--> 'std::__shared_ptr<_Tp,
[WIP RFC] analyzer: Add optional trim of the analyzer diagnostics going too deep [PR110543]
Hi,
Upon David's request I've joined the in progress patch to the below email.
I hope it makes more sense now.
Best,
Benjamin.
-- Forwarded message -
From: Benjamin Priour
Date: Tue, Jul 18, 2023 at 3:30 PM
Subject: [RFC] analyzer: Add optional trim of the analyzer diagnostics
going too deep [PR110543]
To: , David Malcolm
Hi,
I'd like to request comments on a patch I am writing for PR110543.
The goal of this patch is to reduce the noise of the analyzer emitted
diagnostics when dealing with
system headers, or simply diagnostic paths that are too long. The new
option only affects the display
of the diagnostics, but doesn't hinder the actual analysis.
I've defaulted the new option to "system", thus preventing the diagnostic
paths from showing system headers.
"never" corresponds to the pre-patch behavior, whereas you can also specify
an unsigned value
that prevents paths to go deeper than frames.
fanalyzer-trim-diagnostics=
> Common Joined RejectNegative ToLower Var(flag_analyzer_trim_diagnostics)
> Init("system")
> -fanalyzer-trim-diagnostics=[never|system|] Trim diagnostics
> path that are too long before emission.
>
Does it sounds reasonable and user-friendly ?
Regstrapping was a success against trunk, although one of the newly added
test case fails for c++14.
Note that the test case below was done with "never", thus behaves exactly
as the pre-patch analyzer
on x86_64-linux-gnu.
/* { dg-additional-options "-fdiagnostics-plain-output
> -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=never"
> } */
> /* { dg-skip-if "" { c++98_only } } */
>
> #include
> struct A {int x; int y;};
>
> int main () {
> std::shared_ptr a;
> a->x = 4; /* { dg-line deref_a } */
> /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */
>
> return 0;
> }
>
> /* { dg-begin-multiline-output "" }
> 'int main()': events 1-2
> |
> |
> +--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp, ,
> >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy
> _Lp = __gnu_cxx::_S_atomic; bool = false; bool =
> false]': events 3-4
>|
>|
>+--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp,
> , >::_M_get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool =
> false; bool = false]': events 5-6
> |
> |
> +--> 'std::__shared_ptr<_Tp, _Lp>::element_type*
> std::__shared_ptr<_Tp, _Lp>::get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]': events 7-8
> |
> |
> <--+
> |
> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp,
> , >::_M_get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool =
> false; bool = false]': event 9
> |
> |
><--+
>|
> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp, ,
> >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy
> _Lp = __gnu_cxx::_S_atomic; bool = false; bool =
> false]': event 10
>|
>|
> <--+
> |
> 'int main()': events 11-12
> |
> |
>{ dg-end-multiline-output "" } */
>
The first events "'int main()': events 1-2" vary in c++14 (get events 1-3).
>
> // c++14 with fully detailed output
> ‘int main()’: events 1-3
> |
> |8 | int main () {
> | | ^~~~
> | | |
> | | (1) entry to ‘main’
> |9 | std::shared_ptr a;
> | | ~
> | | |
> | | (2)
> ‘a.std::shared_ptr::.std::__shared_ptr __gnu_cxx::_S_atomic>::_M_ptr’ is NULL
> | 10 | a->x = 4; /* { dg-line deref_a } */
> | |~~
> | ||
> | |(3) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’
>
whereas c++17 and posterior give
> // c++17 with fully detailed output
>
// ./xg++ -fanalyzer
> ../../gcc/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C
> -B. -shared-libgcc -fanalyzer-trim-diagnostics=never -std=c++17
>
‘int main()’: events 1-2
> |
> |8 | int main () {
> | | ^~~~
> | | |
> | | (1) entry to ‘main’
> |9 | std::shared_ptr a;
> | 10 | a->x = 4; /* { dg-line deref_a } */
> | |~~
> | ||
> | |(2) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’
>
Is there a way to make dg-multiline-output check for a regex ? Or would
checking the multiline-output only for c++17 and c++20 be acceptable ?
This
[RFC] analyzer: Add optional trim of the analyzer diagnostics going too deep [PR110543]
Hi,
I'd like to request comments on a patch I am writing for PR110543.
The goal of this patch is to reduce the noise of the analyzer emitted
diagnostics when dealing with
system headers, or simply diagnostic paths that are too long. The new
option only affects the display
of the diagnostics, but doesn't hinder the actual analysis.
I've defaulted the new option to "system", thus preventing the diagnostic
paths from showing system headers.
"never" corresponds to the pre-patch behavior, whereas you can also specify
an unsigned value
that prevents paths to go deeper than frames.
fanalyzer-trim-diagnostics=
> Common Joined RejectNegative ToLower Var(flag_analyzer_trim_diagnostics)
> Init("system")
> -fanalyzer-trim-diagnostics=[never|system|] Trim diagnostics
> path that are too long before emission.
>
Does it sounds reasonable and user-friendly ?
Regstrapping was a success against trunk, although one of the newly added
test case fails for c++14.
Note that the test case below was done with "never", thus behaves exactly
as the pre-patch analyzer
on x86_64-linux-gnu.
/* { dg-additional-options "-fdiagnostics-plain-output
> -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=never"
> } */
> /* { dg-skip-if "" { c++98_only } } */
>
> #include
> struct A {int x; int y;};
>
> int main () {
> std::shared_ptr a;
> a->x = 4; /* { dg-line deref_a } */
> /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */
>
> return 0;
> }
>
> /* { dg-begin-multiline-output "" }
> 'int main()': events 1-2
> |
> |
> +--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp, ,
> >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy
> _Lp = __gnu_cxx::_S_atomic; bool = false; bool =
> false]': events 3-4
>|
>|
>+--> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp,
> , >::_M_get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool =
> false; bool = false]': events 5-6
> |
> |
> +--> 'std::__shared_ptr<_Tp, _Lp>::element_type*
> std::__shared_ptr<_Tp, _Lp>::get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]': events 7-8
> |
> |
> <--+
> |
> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp,
> , >::_M_get() const [with _Tp = A;
> __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool =
> false; bool = false]': event 9
> |
> |
><--+
>|
> 'std::__shared_ptr_access<_Tp, _Lp, ,
> >::element_type* std::__shared_ptr_access<_Tp, _Lp, ,
> >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy
> _Lp = __gnu_cxx::_S_atomic; bool = false; bool =
> false]': event 10
>|
>|
> <--+
> |
> 'int main()': events 11-12
> |
> |
>{ dg-end-multiline-output "" } */
>
The first events "'int main()': events 1-2" vary in c++14 (get events 1-3).
>
> // c++14 with fully detailed output
> ‘int main()’: events 1-3
> |
> |8 | int main () {
> | | ^~~~
> | | |
> | | (1) entry to ‘main’
> |9 | std::shared_ptr a;
> | | ~
> | | |
> | | (2)
> ‘a.std::shared_ptr::.std::__shared_ptr __gnu_cxx::_S_atomic>::_M_ptr’ is NULL
> | 10 | a->x = 4; /* { dg-line deref_a } */
> | |~~
> | ||
> | |(3) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’
>
whereas c++17 and posterior give
> // c++17 with fully detailed output
>
// ./xg++ -fanalyzer
> ../../gcc/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C
> -B. -shared-libgcc -fanalyzer-trim-diagnostics=never -std=c++17
>
‘int main()’: events 1-2
> |
> |8 | int main () {
> | | ^~~~
> | | |
> | | (1) entry to ‘main’
> |9 | std::shared_ptr a;
> | 10 | a->x = 4; /* { dg-line deref_a } */
> | |~~
> | ||
> | |(2) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’
>
Is there a way to make dg-multiline-output check for a regex ? Or would
checking the multiline-output only for c++17 and c++20 be acceptable ?
This divergence results from two slightly different IPA:
// c++14 -fdump-ipa-analyzer
> // std::shared_ptr a; becomes
> a.D.29392._M_ptr = 0B;
> a.D.29392._M_refcount._M_pi = 0B;
>
whereas in c++17
> // c++17 -fdump-ipa-analyzer
> // std::shared_ptr a; becomes
> a = {};
>
I know shared_ptr limited support is a coincidence more than a feature,
Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]
As per David's suggestion.
- Improved leading comment of "is_placement_new_p"
- "kf_operator_new::matches_call_types_p" now checks that arg 0 is of
integral type and that arg 1, if any, is of pointer type.
- Changed ambiguous "int" to "int8_t" and "int64_t" in placement-new-size.C
to trigger a target independent out-of-bounds warning.
Other OOB tests were not based on the size of types, but on the number
elements, so them using "int" didn't lead to any ambiguity.
contrib/check_GNU_style.sh still complains about a space before square
brackets in string "operator new []", but as before, this one space is
mandatory for a correct recognition of the function.
Changes succesfully regstrapped on x86_64-linux-gnu against trunk
3c776fdf1a8.
Is it OK for trunk ?
Thanks again,
Benjamin.
---
Fixed spurious possibly-NULL warning always tagging along throwing
operator new despite it never returning NULL.
Now, operator new is correctly recognized as possibly returning
NULL if and only if it is non-throwing or exceptions have been disabled.
Different standard signatures of operator new are now properly recognized.
Added support of placement new, so that is now properly recognized,
and a 'heap_allocated' region is no longer created for it.
Placement new size is also checked and a 'Wanalyzer-allocation-size'
is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'.
gcc/analyzer/ChangeLog:
PR analyzer/105948
* analyzer.h (is_placement_new_p): New declaration.
* call-details.cc
(call_details::maybe_get_arg_region): New function.
Returns the region of the argument at given index if possible.
* call-details.h: Declaration of above function.
* kf-lang-cp.cc (is_placement_new_p): Returns true if the
gcall is recognized as a placement new.
* region-model.cc (region_model::eval_condition):
Now recursively call itself if one the operand is wrapped in a cast.
* sm-malloc.cc (malloc_state_machine::on_stmt): Added
recognition of placement new.
gcc/testsuite/ChangeLog:
PR analyzer/105948
* g++.dg/analyzer/out-of-bounds-placement-new.C: Added a directive.
* g++.dg/analyzer/placement-new.C: Added tests.
* g++.dg/analyzer/new-2.C: New test.
* g++.dg/analyzer/noexcept-new.C: New test.
* g++.dg/analyzer/placement-new-size.C: New test.
Signed-off-by: benjamin priour
---
gcc/analyzer/analyzer.h | 1 +
gcc/analyzer/call-details.cc | 11 ++
gcc/analyzer/call-details.h | 1 +
gcc/analyzer/kf-lang-cp.cc| 105 +-
gcc/analyzer/region-model.cc | 21
gcc/analyzer/sm-malloc.cc | 17 ++-
gcc/testsuite/g++.dg/analyzer/new-2.C | 50 +
gcc/testsuite/g++.dg/analyzer/noexcept-new.C | 48
.../analyzer/out-of-bounds-placement-new.C| 2 +-
.../g++.dg/analyzer/placement-new-size.C | 27 +
gcc/testsuite/g++.dg/analyzer/placement-new.C | 63 ++-
11 files changed, 332 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/analyzer/new-2.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/noexcept-new.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/placement-new-size.C
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index 579517c23e6..b86e5cac74d 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -391,6 +391,7 @@ extern bool is_std_named_call_p (const_tree fndecl, const
char *funcname,
const gcall *call, unsigned int num_args);
extern bool is_setjmp_call_p (const gcall *call);
extern bool is_longjmp_call_p (const gcall *call);
+extern bool is_placement_new_p (const gcall *call);
extern const char *get_user_facing_name (const gcall *call);
diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc
index 17edaf26276..01f061d774e 100644
--- a/gcc/analyzer/call-details.cc
+++ b/gcc/analyzer/call-details.cc
@@ -152,6 +152,17 @@ call_details::get_arg_svalue (unsigned idx) const
return m_model->get_rvalue (arg, m_ctxt);
}
+/* If argument IDX's svalue at the callsite is a region_svalue,
+ return the region it points to.
+ Otherwise return NULL. */
+
+const region *
+call_details::maybe_get_arg_region (unsigned idx) const
+{
+ const svalue *sval = get_arg_svalue (idx);
+ return sval->maybe_get_region ();
+}
+
/* Attempt to get the string literal for argument IDX, or return NULL
otherwise.
For use when implementing "__analyzer_*" functions that take
diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h
index 14a206ff5d6..aac2b7d33d8 100644
--- a/gcc/analyzer/call-details.h
+++ b/gcc/analyzer/call-details.h
@@ -55,6 +55,7 @@ public:
tree get_arg_tree (unsigned idx) const;
tree get_arg_type (unsigned idx) const;
const svalue *get_arg_svalue (unsigned
Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]
Hi David,
On 05/07/2023 22:59, David Malcolm wrote:
diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc
index 393b4f25e79..258d92919d7 100644
--- a/gcc/analyzer/kf-lang-cp.cc
+++ b/gcc/analyzer/kf-lang-cp.cc
@@ -35,6 +35,34 @@ along with GCC; see the file COPYING3. If not see
#if ENABLE_ANALYZER
+/* Return TRUE if CALL is non-allocating operator new or operator
new[]*/
+
+bool is_placement_new_p (const gcall *call)
Please can you extend the leading comment, giving the expected
signatures of the functions, and a link to cppreference.org.
In particular, there's some special-casing here of "nothrow_t" which
would make more sense with a comment up here.
I've now extended the leading comment of is_placement_new_p so that the
special cases appears clearer.
Leading comment is now:
/* Return true if CALL is a non-allocating operator new or operator
new []
that contains no user-defined args, i.e. having any signature of:
- void* operator new ( std::size_t count, void* ptr );
- void* operator new[]( std::size_t count, void* ptr );
See https://en.cppreference.com/w/cpp/memory/new/operator_new . */
Whereas above the "nothrow_t" special case now reads
/* We must distinguish between an allocating non-throwing new
and a non-allocating new.
The former might have one of the following signatures :
void* operator new ( std::size_t count, const std::nothrow_t&
tag );
void* operator new[]( std::size_t count, const std::nothrow_t&
tag );
However, debugging has shown that TAG is actually a POINTER_TYPE,
not a REFERENCE_TYPE.
Thus, we cannot easily differentiate the types, but we instead
have to
check if the second argument's type identifies as nothrow_t. */
+{
+ gcc_assert (call);
+
+ tree fndecl = gimple_call_fndecl (call);
+ if (!fndecl)
+return false;
+
+ if (!is_named_call_p (fndecl, "operator new", call, 2)
+&& !is_named_call_p (fndecl, "operator new []", call, 2))
+return false;
+ tree arg1 = gimple_call_arg (call, 1);
+
+ if (!POINTER_TYPE_P (TREE_TYPE (arg1)))
+return false;
+
+ /* Sadly, for non-throwing new, the second argument type
+is not REFERENCE_TYPE but also POINTER_TYPE
+so a simple check is out of the way. */
+ tree identifier = TYPE_IDENTIFIER (TREE_TYPE (TREE_TYPE (arg1)));
+ if (!identifier)
+return true;
+ const char *name = IDENTIFIER_POINTER (identifier);
+ return 0 != strcmp (name, "nothrow_t");
+}
+
namespace ana {
/* Implementations of specific functions. */
@@ -46,7 +74,7 @@ class kf_operator_new : public known_function
public:
bool matches_call_types_p (const call_details ) const final
override
{
-return cd.num_args () == 1;
+return cd.num_args () == 1 || cd.num_args () == 2;
Looks like we should also check that arg 0 is of integral type, and
that arg 1 is of pointer type.
Well technically some standard signatures use an align_val_t as a second
argument,
which is a size_t value. But since we don't handle such signatures
properly yet, I'm going
with your suggestion.
}
void impl_call_pre (const call_details ) const final override
@@ -54,13 +82,60 @@ public:
region_model *model = cd.get_model ();
region_model_manager *mgr = cd.get_manager ();
const svalue *size_sval = cd.get_arg_svalue (0);
-const region *new_reg
- = model->get_or_create_region_for_heap_alloc (size_sval,
cd.get_ctxt ());
-if (cd.get_lhs_type ())
+region_model_context *ctxt = cd.get_ctxt ();
+const gcall *call = cd.get_call_stmt ();
+
+/* If the call is an allocating new, then create a heap
allocated
+region. */
+if (!is_placement_new_p (call))
+ {
You have:
if (!condition)
suite_a;
else
suite_b; // this is implicitly a double negative
Please change it to:
if (condition)
suite_b;
else
suite_a;
to avoid the implicit double negative.
(nods)
diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C
b/gcc/testsuite/g++.dg/analyzer/new-2.C
new file mode 100644
index 000..4e696040a54
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/new-2.C
@@ -0,0 +1,50 @@
+// { dg-additional-options "-O0" }
+
+struct A
+{
+ int x;
+ int y;
+};
We've run into issues with bounds-checking testcases when using types
like "int" that have target-specific sizes.
Please use in these test cases, and types with explicit
sizes, such as int32_t, to avoid the behavior of the test cases being
affected by sizeof the various types.
Thanks, I've now changed it in placement-new-size.C
[..snip...]
Other than those issues, looks good
Thanks again
Dave
Thanks for the review !
I'll submit the updated patch tomorrow on the mail list.
Benjamin
[PATCH] analyzer: Add support of placement new and improved operator new [PR105948]
From: benjamin priour
Script contrib/check_GNU_style.sh complains about there being a space
before a left square bracket ("operator new []").
Though, it is actually within a literal string, and the space
is required to correctly detect the function.
Succesfully regstrapped on x86_64-linux-gnu against trunk 3c776fdf1a8.
Is it OK for trunk ?
Benjamin.
---
Fixed spurious possibly-NULL warning always tagging along throwing
operator new despite it never returning NULL.
Now, operator new is correctly recognized as possibly returning
NULL if and only if it is non-throwing or exceptions have been disabled.
Different standard signatures of operator new are now properly recognized.
Added support of placement new, so that is now properly recognized,
and a 'heap_allocated' region is no longer created for it.
Placement new size is also checked and a 'Wanalyzer-allocation-size'
is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'.
gcc/analyzer/ChangeLog:
PR analyzer/105948
* analyzer.h (is_placement_new_p): New declaration.
* call-details.cc
(call_details::maybe_get_arg_region): New function.
Returns the region of the argument at given index if possible.
* call-details.h: Declaration of above function.
* kf-lang-cp.cc (is_placement_new_p): Returns true if the
gcall is recognized as a placement new.
* region-model.cc (region_model::eval_condition):
Now recursively call itself if one the operand is wrapped in a cast.
* sm-malloc.cc (malloc_state_machine::on_stmt): Added
recognition of placement new.
gcc/testsuite/ChangeLog:
PR analyzer/105948
* g++.dg/analyzer/out-of-bounds-placement-new.C: New test.
* g++.dg/analyzer/placement-new.C: Added tests.
* g++.dg/analyzer/new-2.C: New test.
* g++.dg/analyzer/noexcept-new.C: New test.
* g++.dg/analyzer/placement-new-size.C: New test.
Signed-off-by: benjamin priour
---
gcc/analyzer/analyzer.h | 1 +
gcc/analyzer/call-details.cc | 11 +++
gcc/analyzer/call-details.h | 1 +
gcc/analyzer/kf-lang-cp.cc| 85 +--
gcc/analyzer/region-model.cc | 21 +
gcc/analyzer/sm-malloc.cc | 17 ++--
gcc/testsuite/g++.dg/analyzer/new-2.C | 50 +++
gcc/testsuite/g++.dg/analyzer/noexcept-new.C | 48 +++
.../analyzer/out-of-bounds-placement-new.C| 2 +-
.../g++.dg/analyzer/placement-new-size.C | 27 ++
gcc/testsuite/g++.dg/analyzer/placement-new.C | 63 +-
11 files changed, 312 insertions(+), 14 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/analyzer/new-2.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/noexcept-new.C
create mode 100644 gcc/testsuite/g++.dg/analyzer/placement-new-size.C
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index 579517c23e6..b86e5cac74d 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -391,6 +391,7 @@ extern bool is_std_named_call_p (const_tree fndecl, const
char *funcname,
const gcall *call, unsigned int num_args);
extern bool is_setjmp_call_p (const gcall *call);
extern bool is_longjmp_call_p (const gcall *call);
+extern bool is_placement_new_p (const gcall *call);
extern const char *get_user_facing_name (const gcall *call);
diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc
index 17edaf26276..01f061d774e 100644
--- a/gcc/analyzer/call-details.cc
+++ b/gcc/analyzer/call-details.cc
@@ -152,6 +152,17 @@ call_details::get_arg_svalue (unsigned idx) const
return m_model->get_rvalue (arg, m_ctxt);
}
+/* If argument IDX's svalue at the callsite is a region_svalue,
+ return the region it points to.
+ Otherwise return NULL. */
+
+const region *
+call_details::maybe_get_arg_region (unsigned idx) const
+{
+ const svalue *sval = get_arg_svalue (idx);
+ return sval->maybe_get_region ();
+}
+
/* Attempt to get the string literal for argument IDX, or return NULL
otherwise.
For use when implementing "__analyzer_*" functions that take
diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h
index 14a206ff5d6..aac2b7d33d8 100644
--- a/gcc/analyzer/call-details.h
+++ b/gcc/analyzer/call-details.h
@@ -55,6 +55,7 @@ public:
tree get_arg_tree (unsigned idx) const;
tree get_arg_type (unsigned idx) const;
const svalue *get_arg_svalue (unsigned idx) const;
+ const region *maybe_get_arg_region (unsigned idx) const;
const char *get_arg_string_literal (unsigned idx) const;
tree get_fndecl_for_call () const;
diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc
index 393b4f25e79..258d92919d7 100644
--- a/gcc/analyzer/kf-lang-cp.cc
+++ b/gcc/analyzer/kf-lang-cp.cc
@@ -35,6 +35,34 @@ along with GCC; see the file COPYING3. If not see
#if ENABLE_ANALYZER
+/*
Re: [PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [PR110198]
From: benjamin priour
See below formatting updates on my patch.
In mail https://gcc.gnu.org/pipermail/gcc-patches/2023-June/623140.html,
David Malcolm says regtesting failed for him.
So I did it once more this morning rebased on fresh trunk dc93a0f633b
and target x86_64-linux-gnu, the output was similar to last time:
# from gcc_sources_testing
gcc/contrib/compare_tests ../gcc_sources_control/build build
# Comparing directories
## Dir1=../gcc_sources_control/build: 8 sum files
## Dir2=build: 8 sum files
# Comparing 8 common sum files
## /bin/sh gcc/contrib/compare_tests /tmp/gxx-sum1.750468
/tmp/gxx-sum2.750468
Tests that now work, but didn't before (3 tests):
g++.dg/analyzer/pr100244.C -std=c++14 (test for warnings, line 17)
g++.dg/analyzer/pr100244.C -std=c++17 (test for warnings, line 17)
g++.dg/analyzer/pr100244.C -std=c++20 (test for warnings, line 17)
# No differences found in 8 common sum files
Can you confirm formatting of the patch below, and perhaps regtest it ?
Thanks a lot, as this regression fix is now long due.
Benjamin.
---
g++.dg/analyzer/PR100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon
out-of-bounds read that was preventing further checks. Now instead,
a boolean value check_poisoned goes to false when a OOB is detected,
and is later on given to get_or_create_initial_value.
gcc/analyzer/ChangeLog:
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Take an
optional boolean value to bypass poisoning checks
* region-model-manager.h: Update declaration of the above function.
* region-model.cc (region_model::get_store_value): No longer returns
on OOB, but rather gives a boolean to get_or_create_initial_value.
(region_model::check_region_access): Update docstring.
(region_model::check_region_for_write): Update docstring.
Signed-off-by: benjamin priour
---
gcc/analyzer/region-model-manager.cc | 5 +++--
gcc/analyzer/region-model-manager.h | 3 ++-
gcc/analyzer/region-model.cc | 15 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue (tree type)
necessary. */
const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+ bool check_poisoned)
{
- if (!reg->can_have_initial_svalue_p ())
+ if (!reg->can_have_initial_svalue_p () && check_poisoned)
return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
reg->get_type ());
diff --git a/gcc/analyzer/region-model-manager.h
b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
tree type);
const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
tree type);
- const svalue *get_or_create_initial_value (const region *reg);
+ const svalue *get_or_create_initial_value (const region *reg,
+bool check_poisoned = true);
const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
const svalue *get_or_create_unaryop (tree type, enum tree_code op,
const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
if (reg->empty_p ())
return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
+ bool check_poisoned = true;
if (check_region_for_read (reg, ctxt))
-return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+check_poisoned = false;
/* Special-case: handle var_decls in the constant pool. */
if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
== RK_GLOBALS)
return get_initial_value_for_global (reg);
- return m_mgr->get_or_create_initial_value (reg);
+ return m_mgr->get_or_create_initial_value (reg, check_poisoned);
}
/* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ region_model::get_string_size (const region *reg) const
/* If CTXT is non-NULL, use it to warn about any problems accessing REG,
using DIR to
PING: Re: [PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]
Hi,
Pinging that regression fix.
Is everything OK for trunk ?
Thanks,
Benjamin
On Thu, Jun 22, 2023 at 9:57 PM wrote:
From: benjamin priour
Resend with proper subject line ...
Hi,
Below is the fix to regression bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
Was bootstrapped and regtested successfully on x86_64-linux-gnu
Considering mishap from last patch, I would appreciate if you could
also regtest it, to be sure :)
Thanks,
Benjamin.
g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon
out-of-bounds read that
was preventing further checks. Now instead, a boolean value
check_poisoned goes to false when
a OOB is detected, and is later on given to get_or_create_initial_value.
gcc/analyzer/ChangeLog:
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Take an
optional boolean value to bypass poisoning checks
* region-model-manager.h: Update declaration of the above
function.
* region-model.cc (region_model::get_store_value): No longer
returns on OOB, but rather gives a boolean to
get_or_create_initial_value.
(region_model::check_region_access): Update docstring.
(region_model::check_region_for_write): Update docstring.
Signed-off-by: benjamin priour
---
gcc/analyzer/region-model-manager.cc | 5 +++--
gcc/analyzer/region-model-manager.h | 3 ++-
gcc/analyzer/region-model.cc | 15 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue
(tree type)
necessary. */
const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+ bool check_poisoned)
{
- if (!reg->can_have_initial_svalue_p ())
+ if (!reg->can_have_initial_svalue_p () && check_poisoned)
return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
reg->get_type ());
diff --git a/gcc/analyzer/region-model-manager.h
b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
tree type);
const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
tree type);
- const svalue *get_or_create_initial_value (const region *reg);
+ const svalue *get_or_create_initial_value (const region *reg,
+ bool check_poisoned =
true);
const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
const svalue *get_or_create_unaryop (tree type, enum tree_code op,
const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
if (reg->empty_p ())
return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
+ bool check_poisoned = true;
if (check_region_for_read (reg, ctxt))
- return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+ check_poisoned = false;
/* Special-case: handle var_decls in the constant pool. */
if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
== RK_GLOBALS)
return get_initial_value_for_global (reg);
- return m_mgr->get_or_create_initial_value (reg);
+ return m_mgr->get_or_create_initial_value (reg, check_poisoned);
}
/* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ region_model::get_string_size (const region
*reg) const
/* If CTXT is non-NULL, use it to warn about any problems
accessing REG,
using DIR to determine if this access is a read or write.
- Return TRUE if an UNKNOWN_SVALUE needs be created.
+ Return TRUE if an OOB access was detected.
If SVAL_HINT is non-NULL, use it as a hint in diagnostics
about the value that would be written to REG. */
@@ -2804,10 +2805,10 @@ region_model::check_region_access (const
region *reg,
[PATCH] analyzer: Fix regression bug after r14-1632-g9589a46ddadc8b [pr110198]
From: benjamin priour
Resend with proper subject line ...
Hi,
Below is the fix to regression bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
Was bootstrapped and regtested successfully on x86_64-linux-gnu
Considering mishap from last patch, I'd would appreciate if you could
also regtest it, to be sure :)
Thanks,
Benjamin.
g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon
out-of-bounds read that
was preventing further checks. Now instead, a boolean value check_poisoned goes
to false when
a OOB is detected, and is later on given to get_or_create_initial_value.
gcc/analyzer/ChangeLog:
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Take an
optional boolean value to bypass poisoning checks
* region-model-manager.h: Update declaration of the above function.
* region-model.cc (region_model::get_store_value): No longer
returns on OOB, but rather gives a boolean to
get_or_create_initial_value.
(region_model::check_region_access): Update docstring.
(region_model::check_region_for_write): Update docstring.
Signed-off-by: benjamin priour
---
gcc/analyzer/region-model-manager.cc | 5 +++--
gcc/analyzer/region-model-manager.h | 3 ++-
gcc/analyzer/region-model.cc | 15 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue (tree type)
necessary. */
const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+ bool check_poisoned)
{
- if (!reg->can_have_initial_svalue_p ())
+ if (!reg->can_have_initial_svalue_p () && check_poisoned)
return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
reg->get_type ());
diff --git a/gcc/analyzer/region-model-manager.h
b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
tree type);
const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
tree type);
- const svalue *get_or_create_initial_value (const region *reg);
+ const svalue *get_or_create_initial_value (const region *reg,
+bool check_poisoned = true);
const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
const svalue *get_or_create_unaryop (tree type, enum tree_code op,
const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
if (reg->empty_p ())
return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
+ bool check_poisoned = true;
if (check_region_for_read (reg, ctxt))
-return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+check_poisoned = false;
/* Special-case: handle var_decls in the constant pool. */
if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
== RK_GLOBALS)
return get_initial_value_for_global (reg);
- return m_mgr->get_or_create_initial_value (reg);
+ return m_mgr->get_or_create_initial_value (reg, check_poisoned);
}
/* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ region_model::get_string_size (const region *reg) const
/* If CTXT is non-NULL, use it to warn about any problems accessing REG,
using DIR to determine if this access is a read or write.
- Return TRUE if an UNKNOWN_SVALUE needs be created.
+ Return TRUE if an OOB access was detected.
If SVAL_HINT is non-NULL, use it as a hint in diagnostics
about the value that would be written to REG. */
@@ -2804,10 +2805,10 @@ region_model::check_region_access (const region *reg,
if (!ctxt)
return false;
- bool need_unknown_sval = false;
+ bool oob_access_detected = false;
check_region_for_taint (reg, dir, ctxt);
if (!check_region_bounds (reg, dir, sval_hint, ctxt))
-need_unknown_sval = true;
+oob_access_detected = true;
switch (dir)
{
@@ -2820,7 +2821,7 @@ region_model::check_region_access (const region *reg,
check_for_writable_region (reg, ctxt);
break;
}
[no subject]
Hi,
Below is the fix to regression bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
Was bootstrapped and regtested successfully on x86_64-linux-gnu
Considering mishap from last patch, I'd would appreciate if you could
also regtest it, to be sure :)
Thanks,
Benjamin.
>From 04186f04a3f172d7ccf9824cc71faca489eb39af Mon Sep 17 00:00:00 2001
From: benjamin priour
Date: Thu, 22 Jun 2023 21:39:05 +0200
Subject: [PATCH] [PATCH] analyzer: Fix regression bug after
r14-1632-g9589a46ddadc8b [pr110198]
g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon
out-of-bounds read that
was preventing further checks. Now instead, a boolean value check_poisoned goes
to false when
a OOB is detected, and is later on given to get_or_create_initial_value.
gcc/analyzer/ChangeLog:
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Take an
optional boolean value to bypass poisoning checks
* region-model-manager.h: Update declaration of the above function.
* region-model.cc (region_model::get_store_value): No longer
returns on OOB, but rather gives a boolean to
get_or_create_initial_value.
(region_model::check_region_access): Update docstring.
(region_model::check_region_for_write): Update docstring.
Signed-off-by: benjamin priour
---
gcc/analyzer/region-model-manager.cc | 5 +++--
gcc/analyzer/region-model-manager.h | 3 ++-
gcc/analyzer/region-model.cc | 15 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue (tree type)
necessary. */
const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+ bool check_poisoned)
{
- if (!reg->can_have_initial_svalue_p ())
+ if (!reg->can_have_initial_svalue_p () && check_poisoned)
return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
reg->get_type ());
diff --git a/gcc/analyzer/region-model-manager.h
b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
tree type);
const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
tree type);
- const svalue *get_or_create_initial_value (const region *reg);
+ const svalue *get_or_create_initial_value (const region *reg,
+bool check_poisoned = true);
const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
const svalue *get_or_create_unaryop (tree type, enum tree_code op,
const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
if (reg->empty_p ())
return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
+ bool check_poisoned = true;
if (check_region_for_read (reg, ctxt))
-return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+check_poisoned = false;
/* Special-case: handle var_decls in the constant pool. */
if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
== RK_GLOBALS)
return get_initial_value_for_global (reg);
- return m_mgr->get_or_create_initial_value (reg);
+ return m_mgr->get_or_create_initial_value (reg, check_poisoned);
}
/* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ region_model::get_string_size (const region *reg) const
/* If CTXT is non-NULL, use it to warn about any problems accessing REG,
using DIR to determine if this access is a read or write.
- Return TRUE if an UNKNOWN_SVALUE needs be created.
+ Return TRUE if an OOB access was detected.
If SVAL_HINT is non-NULL, use it as a hint in diagnostics
about the value that would be written to REG. */
@@ -2804,10 +2805,10 @@ region_model::check_region_access (const region *reg,
if (!ctxt)
return false;
- bool need_unknown_sval = false;
+ bool oob_access_detected = false;
check_region_for_taint (reg, dir, ctxt);
if (!check_region_bounds (reg, dir, sval_hint, ctxt))
-need_unknown_sval = true;
+oob_access_detected = true;
Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
Hi Maxim, I managed to nail the bug on the failing test pr100244.C, as I did too observe a divergence after my patch. For pr101962.c, it was simply a dg-note I forgot to remove, that made it fail, since the related warning is no longer relevant. The behavior otherwise is as expected before and after the patch. It is now fixed. Thanks again a lot for you attention, Benjamin. On Fri, Jun 9, 2023 at 2:19 AM Benjamin Priour wrote: > Hi David, > > So first real committed patch actually was a misstep. So I'm currently > fixing that. > The issue is that the original idea, to return a boolean and create a > unknown_svalue on OOB access to prevent further "use-of-uninitialized-value" > caused a loss of information on the location of the buffer. So now, when a > buffer is on the stack, we lose that information by returning an > unknown_svalue > from get_store_value. Therefore further checks from 'sm_malloc' won't be > able to detect erroneous operations expecting heap-allocated buffer, e.g. a > delete. > It does not trouble successive out_of_bounds checks, since the checks are > done on the boundaries of the initial buffer, that > The issue is from sm_state_map::get_state, since an unknown_svalue cannot > hold any state, then in the checker is misled. > I thought to artificially add a state, but since the unknown_svalue are > singleton per type, it is not right. > Therefore I'm considering something, to make it so > can_have_initial_svalue_p holds true for OOB heap access as it is for the > stack, instead of creating an unknown_svalue. > I'll do **PROPER** testing tomorrow, now that I have the compile farm, to > check if doing so won't introduce any further issue. > This way we would keep all the relevant information about the region, > without making it poisoned, and OOB checks are done with the initial byte > size of the buffer, > so this should not be disturbed. > > I briefly tried the above as a proof of concept. Doing so fixed PR100244.C > mentioned by Maxim, while still passing my new test cases for PR109439. > I will regtest this configuration tomorrow morning on the farm, I am > getting sleepy, except if you can already see problems this would cause. > > Sorry again I have somewhat managed to fail my first commit, and pushed it. > > Thanks, > Benjamin. > > -- Forwarded message - > From: Benjamin Priour > Date: Thu, Jun 8, 2023 at 8:18 PM > Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439] > To: Maxim Kuvyrkov > Cc: , Benjamin Priour , < > dmalc...@redhat.com> > > > Hi, > > Yes of course, I tested many days ago since regtesting takes several days > on my box, I should have retested ! > But I got an account for the compile farm today, so I'm on it immediately, > I also see a divergence in the warnings on my box. > > Thanks for the report ! > Sincerely sorry, > Benjamin. > > On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov > wrote: > >> > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches < >> gcc-patches@gcc.gnu.org> wrote: >> > >> > From: Benjamin Priour >> > >> > This patch enchances -Wanalyzer-out-of-bounds that is no longer paired >> > with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. >> > >> > This also fixes PR analyzer/109437. >> > Before there could always be at most one OOB-read warning per frame >> because >> > -Wanalyzer-use-of-uninitialized-value always terminates the analysis >> > path. >> > >> > PR analyzer/109439 >> > >> > gcc/analyzer/ChangeLog: >> > >> > * bounds-checking.cc (region_model::check_symbolic_bounds): >> > Returns whether the BASE_REG region access was OOB. >> > (region_model::check_region_bounds): Likewise. >> > * region-model.cc (region_model::get_store_value): Creates an >> > unknown svalue on OOB-read access to REG. >> > (region_model::check_region_access): Returns whether an >> > unknown svalue needs be created. >> > (region_model::check_region_for_read): Passes >> > check_region_access return value. >> > * region-model.h: Update prior function definitions. >> > >> > gcc/testsuite/ChangeLog: >> > >> > * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for >> > uninitialized-value warning. >> > * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. >> > * gcc.dg/analyzer/pr101962.c: Likewise. >> > * gcc.dg/analyzer/realloc-5.c: Likewise. >> > * gcc.dg/analyzer/pr109439.c: New test. >> > --- >> >> Hi Benjamin, >> >> This patch ma
Fwd: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
Hi David, So first real committed patch actually was a misstep. So I'm currently fixing that. The issue is that the original idea, to return a boolean and create a unknown_svalue on OOB access to prevent further "use-of-uninitialized-value" caused a loss of information on the location of the buffer. So now, when a buffer is on the stack, we lose that information by returning an unknown_svalue from get_store_value. Therefore further checks from 'sm_malloc' won't be able to detect erroneous operations expecting heap-allocated buffer, e.g. a delete. It does not trouble successive out_of_bounds checks, since the checks are done on the boundaries of the initial buffer, that The issue is from sm_state_map::get_state, since an unknown_svalue cannot hold any state, then in the checker is misled. I thought to artificially add a state, but since the unknown_svalue are singleton per type, it is not right. Therefore I'm considering something, to make it so can_have_initial_svalue_p holds true for OOB heap access as it is for the stack, instead of creating an unknown_svalue. I'll do **PROPER** testing tomorrow, now that I have the compile farm, to check if doing so won't introduce any further issue. This way we would keep all the relevant information about the region, without making it poisoned, and OOB checks are done with the initial byte size of the buffer, so this should not be disturbed. I briefly tried the above as a proof of concept. Doing so fixed PR100244.C mentioned by Maxim, while still passing my new test cases for PR109439. I will regtest this configuration tomorrow morning on the farm, I am getting sleepy, except if you can already see problems this would cause. Sorry again I have somewhat managed to fail my first commit, and pushed it. Thanks, Benjamin. -- Forwarded message - From: Benjamin Priour Date: Thu, Jun 8, 2023 at 8:18 PM Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439] To: Maxim Kuvyrkov Cc: , Benjamin Priour , < dmalc...@redhat.com> Hi, Yes of course, I tested many days ago since regtesting takes several days on my box, I should have retested ! But I got an account for the compile farm today, so I'm on it immediately, I also see a divergence in the warnings on my box. Thanks for the report ! Sincerely sorry, Benjamin. On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov wrote: > > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches < > gcc-patches@gcc.gnu.org> wrote: > > > > From: Benjamin Priour > > > > This patch enchances -Wanalyzer-out-of-bounds that is no longer paired > > with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. > > > > This also fixes PR analyzer/109437. > > Before there could always be at most one OOB-read warning per frame > because > > -Wanalyzer-use-of-uninitialized-value always terminates the analysis > > path. > > > > PR analyzer/109439 > > > > gcc/analyzer/ChangeLog: > > > > * bounds-checking.cc (region_model::check_symbolic_bounds): > > Returns whether the BASE_REG region access was OOB. > > (region_model::check_region_bounds): Likewise. > > * region-model.cc (region_model::get_store_value): Creates an > > unknown svalue on OOB-read access to REG. > > (region_model::check_region_access): Returns whether an > > unknown svalue needs be created. > > (region_model::check_region_for_read): Passes > > check_region_access return value. > > * region-model.h: Update prior function definitions. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for > > uninitialized-value warning. > > * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. > > * gcc.dg/analyzer/pr101962.c: Likewise. > > * gcc.dg/analyzer/realloc-5.c: Likewise. > > * gcc.dg/analyzer/pr109439.c: New test. > > --- > > Hi Benjamin, > > This patch makes two tests fail on arm-linux-gnueabihf. Probably, they > need to be updated as well. Would you please investigate? Let me know if > it doesn't easily reproduce for you, and I'll help. > > === g++ tests === > > Running g++:g++.dg/analyzer/analyzer.exp ... > FAIL: g++.dg/analyzer/pr100244.C -std=c++14 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=c++17 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=c++20 (test for warnings, line 17) > > === gcc tests === > > Running gcc:gcc.dg/analyzer/analyzer.exp ... > FAIL: gcc.dg/analyzer/pr101962.c (test for warnings, line 19) > > Thanks, > > -- > Maxim Kuvyrkov > https://www.linaro.org > > > > > gcc/analyzer/bounds-checking.cc | 30 +-- > > gcc/analyzer/region-model.cc | 22
Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
Hi, Yes of course, I tested many days ago since regtesting takes several days on my box, I should have retested ! But I got an account for the compile farm today, so I'm on it immediately, I also see a divergence in the warnings on my box. Thanks for the report ! Sincerely sorry, Benjamin. On Thu, Jun 8, 2023 at 7:53 PM Maxim Kuvyrkov wrote: > > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches < > gcc-patches@gcc.gnu.org> wrote: > > > > From: Benjamin Priour > > > > This patch enchances -Wanalyzer-out-of-bounds that is no longer paired > > with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. > > > > This also fixes PR analyzer/109437. > > Before there could always be at most one OOB-read warning per frame > because > > -Wanalyzer-use-of-uninitialized-value always terminates the analysis > > path. > > > > PR analyzer/109439 > > > > gcc/analyzer/ChangeLog: > > > > * bounds-checking.cc (region_model::check_symbolic_bounds): > > Returns whether the BASE_REG region access was OOB. > > (region_model::check_region_bounds): Likewise. > > * region-model.cc (region_model::get_store_value): Creates an > > unknown svalue on OOB-read access to REG. > > (region_model::check_region_access): Returns whether an > > unknown svalue needs be created. > > (region_model::check_region_for_read): Passes > > check_region_access return value. > > * region-model.h: Update prior function definitions. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for > > uninitialized-value warning. > > * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. > > * gcc.dg/analyzer/pr101962.c: Likewise. > > * gcc.dg/analyzer/realloc-5.c: Likewise. > > * gcc.dg/analyzer/pr109439.c: New test. > > --- > > Hi Benjamin, > > This patch makes two tests fail on arm-linux-gnueabihf. Probably, they > need to be updated as well. Would you please investigate? Let me know if > it doesn't easily reproduce for you, and I'll help. > > === g++ tests === > > Running g++:g++.dg/analyzer/analyzer.exp ... > FAIL: g++.dg/analyzer/pr100244.C -std=c++14 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=c++17 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=c++20 (test for warnings, line 17) > > === gcc tests === > > Running gcc:gcc.dg/analyzer/analyzer.exp ... > FAIL: gcc.dg/analyzer/pr101962.c (test for warnings, line 19) > > Thanks, > > -- > Maxim Kuvyrkov > https://www.linaro.org > > > > > gcc/analyzer/bounds-checking.cc | 30 +-- > > gcc/analyzer/region-model.cc | 22 +- > > gcc/analyzer/region-model.h | 8 ++--- > > .../gcc.dg/analyzer/out-of-bounds-2.c | 1 - > > .../gcc.dg/analyzer/out-of-bounds-5.c | 2 -- > > gcc/testsuite/gcc.dg/analyzer/pr101962.c | 1 - > > gcc/testsuite/gcc.dg/analyzer/pr109439.c | 12 > > gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 1 - > > 8 files changed, 51 insertions(+), 26 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c > > > > diff --git a/gcc/analyzer/bounds-checking.cc > b/gcc/analyzer/bounds-checking.cc > > index 3bf542a8eba..479b8e4b88d 100644 > > --- a/gcc/analyzer/bounds-checking.cc > > +++ b/gcc/analyzer/bounds-checking.cc > > @@ -767,9 +767,11 @@ public: > > } > > }; > > > > -/* Check whether an access is past the end of the BASE_REG. */ > > +/* Check whether an access is past the end of the BASE_REG. > > + Return TRUE if the access was valid, FALSE otherwise. > > +*/ > > > > -void > > +bool > > region_model::check_symbolic_bounds (const region *base_reg, > > const svalue *sym_byte_offset, > > const svalue *num_bytes_sval, > > @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region > *base_reg, > > offset_tree, > > num_bytes_tree, > > capacity_tree)); > > +return false; > > break; > > case DIR_WRITE: > > ctxt->warn (make_unique (base_reg, > > @@ -807,9 +810,11 @@ region_model::check_symbolic_bounds (const region > *base_reg, > > offset_tree, > > num_bytes_tree, > > capacity_tree)); > > +return false; > > break; > > } > > } > > + return true; > > } > > > > static tree > > @@ -822,9 +827,11 @@ maybe_get_integer_cst_tree (const svalue *sval) > > return NULL_TREE; >
[COMMITTED] analyzer: Standalone OOB-warning, formatting fixed [PR109437, PR109439]
From: Benjamin Priour For the record, below is the previous patch I submitted, with the little formatting issues fixed - multiline docstring no ends on a newline. It was otherwise validated by David Malcolm, so I already committed it. This patch enhances -Wanalyzer-out-of-bounds that is no longer paired with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. This also fixes PR analyzer/109437. Before there could always be at most one OOB-read warning per frame because -Wanalyzer-use-of-uninitialized-value always terminates the analysis path. PR analyzer/109439 gcc/analyzer/ChangeLog: * bounds-checking.cc (region_model::check_symbolic_bounds): Returns whether the BASE_REG region access was OOB. (region_model::check_region_bounds): Likewise. * region-model.cc (region_model::get_store_value): Creates an unknown svalue on OOB-read access to REG. (region_model::check_region_access): Returns whether an unknown svalue needs be created. (region_model::check_region_for_read): Passes check_region_access return value. * region-model.h: Update prior function definitions. gcc/testsuite/ChangeLog: * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for uninitialized-value warning. * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. * gcc.dg/analyzer/pr101962.c: Likewise. * gcc.dg/analyzer/realloc-5.c: Likewise. * gcc.dg/analyzer/pr109439.c: New test. --- gcc/analyzer/bounds-checking.cc | 28 +-- gcc/analyzer/region-model.cc | 22 +-- gcc/analyzer/region-model.h | 8 +++--- .../gcc.dg/analyzer/out-of-bounds-2.c | 1 - .../gcc.dg/analyzer/out-of-bounds-5.c | 2 -- gcc/testsuite/gcc.dg/analyzer/pr101962.c | 1 - gcc/testsuite/gcc.dg/analyzer/pr109439.c | 12 gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 1 - 8 files changed, 49 insertions(+), 26 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc index 3bf542a8eba..a5692cf9319 100644 --- a/gcc/analyzer/bounds-checking.cc +++ b/gcc/analyzer/bounds-checking.cc @@ -767,9 +767,10 @@ public: } }; -/* Check whether an access is past the end of the BASE_REG. */ +/* Check whether an access is past the end of the BASE_REG. + Return TRUE if the access was valid, FALSE otherwise. */ -void +bool region_model::check_symbolic_bounds (const region *base_reg, const svalue *sym_byte_offset, const svalue *num_bytes_sval, @@ -800,6 +801,7 @@ region_model::check_symbolic_bounds (const region *base_reg, offset_tree, num_bytes_tree, capacity_tree)); + return false; break; case DIR_WRITE: ctxt->warn (make_unique (base_reg, @@ -807,9 +809,11 @@ region_model::check_symbolic_bounds (const region *base_reg, offset_tree, num_bytes_tree, capacity_tree)); + return false; break; } } + return true; } static tree @@ -822,9 +826,10 @@ maybe_get_integer_cst_tree (const svalue *sval) return NULL_TREE; } -/* May complain when the access on REG is out-of-bounds. */ +/* May complain when the access on REG is out-of-bounds. + Return TRUE if the access was valid, FALSE otherwise. */ -void +bool region_model::check_region_bounds (const region *reg, enum access_direction dir, region_model_context *ctxt) const @@ -839,14 +844,14 @@ region_model::check_region_bounds (const region *reg, (e.g. because the analyzer did not see previous offsets on the latter, it might think that a negative access is before the buffer). */ if (base_reg->symbolic_p ()) -return; + return true; /* Find out how many bytes were accessed. */ const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr); tree num_bytes_tree = maybe_get_integer_cst_tree (num_bytes_sval); /* Bail out if 0 bytes are accessed. */ if (num_bytes_tree && zerop (num_bytes_tree)) -return; + return true; /* Get the capacity of the buffer. */ const svalue *capacity = get_capacity (base_reg); @@ -877,13 +882,13 @@ region_model::check_region_bounds (const region *reg, } else byte_offset_sval = reg_offset.get_symbolic_byte_offset (); - check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sval, + return
Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
On Tue, Jun 6, 2023 at 8:37 PM David Malcolm wrote: > > On Tue, 2023-06-06 at 18:05 +0200, Benjamin Priour wrote: [...] > [Looks like you droppped the mailing list from the recipients; was that > intentional?] > Not at all, just me missing the reply all button. > > > > I indeed bootstrapped and regtested on linux-x86_64, but it was last > > week, since I'm still using my laptop, which is painfully slow (1 > > night per step), my tests are always a few days old. > > Thanks. The patch is OK for trunk once the minor formatting nits are > fixed (you don't have to bother with a full test run for that). We > might want to backport it to gcc 13 as well, but let's let it "soak" in > trunk for some time first. > > > We discussed it already but yes, in the end I believe an account on > > the compile farm will be necessary for me. > > Let me know if you need any help with that. I'm not certain about what to put under "Contributions" in the account creation form. I'm still green behind the ears, and wouldn't count my current count of 2 patches *not yet pushed to trunk* as anything remarkable. > > I'll correct the formatting of the comments and resend it, and double > > check the indentation. > > Thanks. I said that but actually I am unsure about the indentation format. Is it spaces up to 6 characters them morph them into tabs ? It was looking like that in the code, although some portion were breaking this rule. I went with the same indentation rules as already shown within each function. > > > I'm still writing custom formatting rules for > > my gcc subfolders, > > but the formatter is sometimes switching back to my default rules > > instead of the workspace's. > > Which formatter are these rules for, BTW? > I'm using vscode default C/Cpp extension's formatter. [...] Thanks, Benjamin
[PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
From: Benjamin Priour This patch enchances -Wanalyzer-out-of-bounds that is no longer paired with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. This also fixes PR analyzer/109437. Before there could always be at most one OOB-read warning per frame because -Wanalyzer-use-of-uninitialized-value always terminates the analysis path. PR analyzer/109439 gcc/analyzer/ChangeLog: * bounds-checking.cc (region_model::check_symbolic_bounds): Returns whether the BASE_REG region access was OOB. (region_model::check_region_bounds): Likewise. * region-model.cc (region_model::get_store_value): Creates an unknown svalue on OOB-read access to REG. (region_model::check_region_access): Returns whether an unknown svalue needs be created. (region_model::check_region_for_read): Passes check_region_access return value. * region-model.h: Update prior function definitions. gcc/testsuite/ChangeLog: * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for uninitialized-value warning. * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. * gcc.dg/analyzer/pr101962.c: Likewise. * gcc.dg/analyzer/realloc-5.c: Likewise. * gcc.dg/analyzer/pr109439.c: New test. --- gcc/analyzer/bounds-checking.cc | 30 +-- gcc/analyzer/region-model.cc | 22 +- gcc/analyzer/region-model.h | 8 ++--- .../gcc.dg/analyzer/out-of-bounds-2.c | 1 - .../gcc.dg/analyzer/out-of-bounds-5.c | 2 -- gcc/testsuite/gcc.dg/analyzer/pr101962.c | 1 - gcc/testsuite/gcc.dg/analyzer/pr109439.c | 12 gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 1 - 8 files changed, 51 insertions(+), 26 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc index 3bf542a8eba..479b8e4b88d 100644 --- a/gcc/analyzer/bounds-checking.cc +++ b/gcc/analyzer/bounds-checking.cc @@ -767,9 +767,11 @@ public: } }; -/* Check whether an access is past the end of the BASE_REG. */ +/* Check whether an access is past the end of the BASE_REG. + Return TRUE if the access was valid, FALSE otherwise. +*/ -void +bool region_model::check_symbolic_bounds (const region *base_reg, const svalue *sym_byte_offset, const svalue *num_bytes_sval, @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region *base_reg, offset_tree, num_bytes_tree, capacity_tree)); +return false; break; case DIR_WRITE: ctxt->warn (make_unique (base_reg, @@ -807,9 +810,11 @@ region_model::check_symbolic_bounds (const region *base_reg, offset_tree, num_bytes_tree, capacity_tree)); +return false; break; } } + return true; } static tree @@ -822,9 +827,11 @@ maybe_get_integer_cst_tree (const svalue *sval) return NULL_TREE; } -/* May complain when the access on REG is out-of-bounds. */ +/* May complain when the access on REG is out-of-bounds. + Return TRUE if the access was valid, FALSE otherwise. +*/ -void +bool region_model::check_region_bounds (const region *reg, enum access_direction dir, region_model_context *ctxt) const @@ -839,14 +846,14 @@ region_model::check_region_bounds (const region *reg, (e.g. because the analyzer did not see previous offsets on the latter, it might think that a negative access is before the buffer). */ if (base_reg->symbolic_p ()) -return; +return true; /* Find out how many bytes were accessed. */ const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr); tree num_bytes_tree = maybe_get_integer_cst_tree (num_bytes_sval); /* Bail out if 0 bytes are accessed. */ if (num_bytes_tree && zerop (num_bytes_tree)) -return; +return true; /* Get the capacity of the buffer. */ const svalue *capacity = get_capacity (base_reg); @@ -877,13 +884,13 @@ region_model::check_region_bounds (const region *reg, } else byte_offset_sval = reg_offset.get_symbolic_byte_offset (); - check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sval, + return check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sval, capacity, dir, ctxt); - return; } /* Otherwise continue to check with concrete values. */ byte_range out (0, 0); + bool oob_safe
[COMMITTED]: New entry to MAINTAINERS.
From: Benjamin Priour ChangeLog: * MAINTAINERS: New entry. --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index e4dee76e2df..b1d174af280 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -600,6 +600,7 @@ Antoniu Pop Siddhesh Poyarekar Vidya Praveen Thomas Preud'homme +Benjamin Priour Vladimir Prus Hafiz Abid Qadeer Yao Qi -- 2.34.1
[PATCH] c++: Additional warning for name-hiding [PR12341]
Hi everyone,
My first patch, and I don't have write access yet.
This patch add a new warning under -Wshadow, to warn when a class
field hides another inherited.
At the moment, I'm looking for a similarly
named field independently of its visibility (whether it is public,
protected or private within the base class).
However, if the inheritance itself is not visible from the current
class, then I dismiss the warning
(e.g. Grandparent class is inherited privately by Parent, then Child
won't collide).
Note that ChangeLogs were generated without the script `git gcc-mklog`.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
Diff is with 20230413-trunk, I checked in with today 20230416 trunk though.
I tried to follow the contribute page down to the letter, still if
I've missed anything, please tell me how I could improve the
submission.
Great thanks,
Benjamin.
c++: Additional warning for name-hiding [PR12341]
Add a new warning for name-hiding. When a class's field
is named similarly to one inherited, a warning should
be issued.
2023-04-13 Benjamin Priour vutlk...@gcc.gnu.org
gcc/cp/ChangeLog:
* search.cc (lookup_member):
New optional parameter to preempt too deep inheritance
tree processing.
(lookup_field): Likewise.
(dfs_walk_all): Likewise.
* cp-tree.h: Complete the above declarations.
* class.cc (warn_name_hiding): New function.
(finish_struct_1): Call warn_name_hiding if -Wshadow.
gcc/testsuite/ChangeLog:
* g++.dg/warn/pr12341-1.C: New file.
* g++.dg/warn/pr12341-2.C: New file.
---
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 68b62086340..1e3efc028a6 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,80 @@ warn_hidden (tree t)
}
}
+/* Warn about non-static fields name hiding. */
+static void
+warn_name_hiding (tree t)
+{
+ if (is_empty_class (t) || CLASSTYPE_NEARLY_EMPTY_P (t))
+return;
+
+ for (tree field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
+{
+/* Skip if field is not a user-defined non-static data member. */
+if (TREE_CODE (field) != FIELD_DECL || DECL_ARTIFICIAL (field))
+ continue;
+
+unsigned j;
+tree name = DECL_NAME (field);
+/* Skip if field is anonymous. */
+if (!name || !identifier_p (name))
+ continue;
+
+auto_vec base_vardecls;
+tree binfo;
+tree base_binfo;
+ /* Iterate through all of the base classes looking for possibly
+ shadowed non-static data members. */
+for (binfo = TYPE_BINFO (t), j = 0;
+BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
+{
+ tree basetype = BINFO_TYPE (base_binfo);
+ tree candidate = lookup_field (basetype,
+name,
+/* protect */ 2,
+/* want_type */ 0,
+/* once_suffices */ true);
+ if (candidate)
+ {
+/*
+if we went up the hierarchy to a base class with multiple inheritance,
+there could be multiple matches, in which case a TREE_LIST is returned
+*/
+if (TREE_TYPE (candidate) == error_mark_node)
+{
+ for (; candidate; candidate = TREE_CHAIN (candidate))
+ {
+tree candidate_field = TREE_VALUE (candidate);
+tree candidate_klass = DECL_CONTEXT (candidate_field);
+if (accessible_base_p (t, candidate_klass, true))
+ base_vardecls.safe_push (candidate_field);
+ }
+}
+else if (accessible_base_p (t, DECL_CONTEXT (candidate), true))
+ base_vardecls.safe_push (candidate);
+ }
+}
+
+/* field was not found among the base classes */
+if (base_vardecls.is_empty ())
+ continue;
+
+/* Emit a warning for each field similarly named
+found in the base class hierarchy */
+for (tree base_vardecl : base_vardecls)
+ if (base_vardecl)
+ {
+auto_diagnostic_group d;
+if (warning_at (location_of (field),
+OPT_Wshadow,
+"%qD might shadow %qD", field, base_vardecl))
+inform (location_of (base_vardecl),
+" %qD name already in use here", base_vardecl);
+ }
+}
+}
+
+
/* Recursive helper for finish_struct_anon. */
static void
@@ -7654,6 +7728,8 @@ finish_struct_1 (tree t)
if (warn_overloaded_virtual)
warn_hidden (t);
+ if (warn_shadow)
+warn_name_hiding (t);
/* Class layout, assignment of virtual table slots, etc., is now
complete. Give the back end a chance to tweak the visibility of
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 622752ae4e6..e56e85d93cc 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7519,11 +7519,11 @@ extern tree lookup_base
(tree, tree, base_access,
extern tree dcast_base_hint(tree, tree);
extern int accessible_p(tree, tree, bool);
extern int accessible_in_template_p(tree, tree);
-extern tree lookup_field (tree, tree, int, bool);
+extern
[RFC] c++/new-warning: Additional warning for name-hiding [PR12341]
I've tried my hands on this first patch, to add new warnings for
name-hiding, i.e.
when a derived class's field shares the name of a base class's field.
I have currently put it under -Wshadow, but I could instead add a
-Wname-hiding warning, what do you think about this ?
At the moment, I'm using protect = 2 in a call to
cp/search.cc:lookup_field, meaning that I'm looking for a similarly
named field independently of its visibility (whether it is public,
protected or private within the base class) (1).
However, if the inheritance itself is not visible from the current
class, then I dismiss the warning (2).
I justify (1) with the code below.
class Base {
public:
friend void polymorphic_parameter_friend(Base *);
private:
int x;
int z;
};
class Derived : private Base {
public:
int x; // warning emitted
int y;
};
/* polymorphic_parameter_friend(new Derived()) ambiguous access to field x,
thus the necessity of the warning */
Extending the code (1) shows behavior of (2):
class Grand : public Derived {
float x; // issue warning from Derived only.
float z; // no warning issued because Grand doesn't know of Base.
};
Anonymous bit fields, and members of anonymous unions are correctly
dealt with (see pr12341-2.C)
I've also taken Jason's previous feedback and now use lookup_field.
I've also added an optional parameter 'once_suffices'. I describe as
follow in the docstring:
> ONCE_SUFFICES is 1 when we should return upon first find of the member in a
> branch of the
> inheritance hierarchy tree, rather than collect all similarly named members
> further in that branch.
> Does not impede other parallel branches of the tree.
I very welcome your comments on this patch, any feedback to change or
improve something. Bootstrapped OK on today's trunk. Regression tests
will run through the night.
The git gcc-mklog alias turns infinitely on my machine, so I generated
the Changelog below by hand.
Changelog:
PR c++/12341
* search.cc (lookup_member):
New optional parameter to preempt too deep inheritance
tree processing.
(lookup_field): Likewise.
(dfs_walk_all): Likewise.
* cp-tree.h: Complete the above declarations.
* class.cc (warn_name_hiding): New function.
(finish_struct_1): Call warn_name_hiding if -Wshadow.
* testsuite/g++.dg/warn/pr12341-1.C: New file.
* testsuite/g++.dg/warn/pr12341-2.C: New file.
The mail is already long enough as it is, thus I'm attaching the git
diff in a text file.
Thanks,
Benjamin.
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 68b62086340..1e3efc028a6 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,80 @@ warn_hidden (tree t)
}
}
+/* Warn about non-static fields name hiding. */
+static void
+warn_name_hiding (tree t)
+{
+ if (is_empty_class (t) || CLASSTYPE_NEARLY_EMPTY_P (t))
+return;
+
+ for (tree field = TYPE_FIELDS (t); field; field = DECL_CHAIN (field))
+{
+/* Skip if field is not a user-defined non-static data member. */
+if (TREE_CODE (field) != FIELD_DECL || DECL_ARTIFICIAL (field))
+ continue;
+
+unsigned j;
+tree name = DECL_NAME (field);
+/* Skip if field is anonymous. */
+if (!name || !identifier_p (name))
+ continue;
+
+auto_vec base_vardecls;
+tree binfo;
+tree base_binfo;
+ /* Iterate through all of the base classes looking for possibly
+ shadowed non-static data members. */
+for (binfo = TYPE_BINFO (t), j = 0;
+BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
+{
+ tree basetype = BINFO_TYPE (base_binfo);
+ tree candidate = lookup_field (basetype,
+name,
+/* protect */ 2,
+/* want_type */ 0,
+/* once_suffices */ true);
+ if (candidate)
+ {
+/*
+if we went up the hierarchy to a base class with multiple inheritance,
+there could be multiple matches, in which case a TREE_LIST is returned
+*/
+if (TREE_TYPE (candidate) == error_mark_node)
+{
+ for (; candidate; candidate = TREE_CHAIN (candidate))
+ {
+tree candidate_field = TREE_VALUE (candidate);
+tree candidate_klass = DECL_CONTEXT (candidate_field);
+if (accessible_base_p (t, candidate_klass, true))
+ base_vardecls.safe_push (candidate_field);
+ }
+}
+else if (accessible_base_p (t, DECL_CONTEXT (candidate), true))
+ base_vardecls.safe_push (candidate);
+ }
+}
+
+/* field was not found among the base classes */
+if (base_vardecls.is_empty ())
+ continue;
+
+/* Emit a warning for each field similarly named
+found in the base class hierarchy */
+for (tree base_vardecl : base_vardecls)
+ if (base_vardecl)
+ {
+auto_diagnostic_group d;
+if (warning_at (location_of (field),
+OPT_Wshadow,
+"%qD might shadow %qD", field, base_vardecl))
+inform (location_of (base_vardecl),
+
[RFC] Fix for c++/PR12341
Hi, below is my first patch ever. I ran the testsuites against trunk
20230322, everything seems OK to me, but as it is my first submission I'd
like to be sure of it.
Thanks a lot for the review !
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 68b62086340..147a7458488 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3080,6 +3080,109 @@ warn_hidden (tree t)
}
}
+
+/* Lookup the inheritance hierarchy of T for any non-static field that have
+the indicated NAME. */
+static void
+lookup_basevardecls (tree name, tree t, vec *base_vardecls)
+{
+ /* Find non-static fields in T with the indicated NAME. */
+ for (tree field = TYPE_FIELDS(t); field; field = DECL_CHAIN(field))
+{
+ /* Going with the same policy as warn_hidden with base class's
members
+ non visible from child, i.e. do not distinguish.
+ A justification might be to not omit warning in the following case:
+
+ class Ancestor {
+ friend void foo_accessor(Ancestor *);
+
+ private:
+int x;
+ };
+
+ class Descendant : public Ancestor {
+ public:
+int x;
+ }
+
+ void foo_accessor(Ancestor *anc) {
+...
+anc->x;
+...
+ }
+
+ foo_accessor(new Descendant());
+ */
+ if (TREE_CODE (field) == FIELD_DECL
+&& !DECL_ARTIFICIAL(field)
+&& DECL_NAME(field) == name)
+{
+ base_vardecls->safe_push (field);
+/* Return upon first match, as there cannot be two data members
+named equally in the same RECORD_TYPE.
+Moreover, avoid redundant warnings by not diving deeper into
+T inheritance hierarchy. */
+return;
+}
+}
+
+ int n_baseclasses = BINFO_N_BASE_BINFOS (TYPE_BINFO (t));
+ /* Go one step up the inheritance hierarchy. */
+ for (int i = 0; i < n_baseclasses; i++)
+{
+ tree basetype = BINFO_TYPE (BINFO_BASE_BINFO (TYPE_BINFO (t), i));
+ lookup_basevardecls (name, basetype, base_vardecls);
+}
+}
+
+
+/* Warn about non-static fields name hiding. */
+static void
+warn_name_hiding(tree t)
+{
+ if (is_empty_class(t) || CLASSTYPE_NEARLY_EMPTY_P(t))
+return;
+
+ for (tree field = TYPE_FIELDS(t); field; field = DECL_CHAIN(field))
+{
+/* Skip if field is not a user-defined non-static data member. */
+if (TREE_CODE(field) != FIELD_DECL || DECL_ARTIFICIAL(field))
+ continue;
+
+unsigned j;
+tree name = DECL_NAME(field);
+/* Not sure about the size parameter of auto_vec */
+auto_vec base_vardecls;
+tree binfo;
+tree base_binfo;
+ /* Iterate through all of the base classes looking for possibly
+ shadowed non-static data members. */
+for (binfo = TYPE_BINFO (t), j = 0;
+BINFO_BASE_ITERATE (binfo, j, base_binfo); j++)
+ {
+tree basetype = BINFO_TYPE(base_binfo);
+lookup_basevardecls(name, basetype, _vardecls);
+ }
+
+/* field was not found among the base classes */
+if (base_vardecls.is_empty())
+ continue;
+
+/* Emit a warning for each field similarly named
+found in the base class hierarchy */
+for (tree base_vardecl : base_vardecls)
+ if (base_vardecl)
+{
+ auto_diagnostic_group d;
+ if (warning_at (location_of (field),
+ OPT_Wshadow,
+ "%qD might shadow %qD.", field, base_vardecl))
+ inform (location_of (base_vardecl), " %qD name already in use
here.", base_vardecl);
+ }
+}
+}
+
+
/* Recursive helper for finish_struct_anon. */
static void
@@ -7654,6 +7757,8 @@ finish_struct_1 (tree t)
if (warn_overloaded_virtual)
warn_hidden (t);
+ if (warn_shadow)
+warn_name_hiding(t);
/* Class layout, assignment of virtual table slots, etc., is now
complete. Give the back end a chance to tweak the visibility of
diff --git a/gcc/testsuite/g++.dg/warn/pr12341-1.C
b/gcc/testsuite/g++.dg/warn/pr12341-1.C
new file mode 100644
index 000..2c8f63d3b4f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/pr12341-1.C
@@ -0,0 +1,51 @@
+// PR c++/12341
+/* { dg-do compile } */
+/* { dg-options -Wshadow } */
+
+class A {
+protected:
+ int aaa; /* { dg-line A_def_aaa } */
+};
+
+class B : public A {
+public:
+ int aaa; /* { dg-line B_shadowing_aaa } */
+ /* { dg-warning "'B::aaa' might shadow 'A::aaa'." "" { target *-*-* }
.-1 } */
+ /* { dg-note "'A::aaa' name already in use here." "" { target *-*-* }
A_def_aaa } */
+private:
+ int bbb; /* { dg-line B_def_bbb } */
+};
+
+class C : public B {
+public:
+ int bbb; /* { dg-warning "'C::bbb' might shadow 'B::bbb'." } */
+ /* { dg-note "'B::bbb' name already in use here." "" { target *-*-* }
B_def_bbb } */
+};
+
+class D {
+protected:
+ int bbb; /* { dg-line D_def_bbb } */
+ int ddd; /* { dg-line D_def_ddd } */
+};
+
+class E : protected D {
+private:
+ int eee;
+};
+
+// all first-level base classes must be considered.
+class Bi : protected B, private E {
