On Thu, 2023-07-06 at 16:43 +0200, priour...@gmail.com wrote: > 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.
Hi Benjamin, thanks for the updated patch. As before, this looks close to being ready, but I have some further comments: [...snip...] > diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc > index 393b4f25e79..ef057da863f 100644 > --- a/gcc/analyzer/kf-lang-cp.cc > +++ b/gcc/analyzer/kf-lang-cp.cc > @@ -35,6 +35,49 @@ along with GCC; see the file COPYING3. If not see > > #if ENABLE_ANALYZER > > +/* 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. */ > + > +bool is_placement_new_p (const gcall *call) > +{ > + 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; > + > + /* 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. */ > + 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"); > +} > + If we're looking for a simple "void *", wouldn't it be simpler and cleaner to check for arg1 being a pointer to a type that's VOID_TYPE_P, rather than this name comparison? [...snip...] > diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc > index a8c63eb1ce8..41c313c07dd 100644 > --- a/gcc/analyzer/sm-malloc.cc > +++ b/gcc/analyzer/sm-malloc.cc > @@ -754,7 +754,7 @@ public: > override > { > if (change.m_old_state == m_sm.get_start_state () > - && unchecked_p (change.m_new_state)) > + && (unchecked_p (change.m_new_state) || nonnull_p (change.m_new_state))) > // TODO: verify that it's the allocation stmt, not a copy > return label_text::borrow ("allocated here"); > if (unchecked_p (change.m_old_state) > @@ -1910,11 +1910,16 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt, > return true; > } > > - if (is_named_call_p (callee_fndecl, "operator new", call, 1)) > - on_allocator_call (sm_ctxt, call, &m_scalar_delete); > - else if (is_named_call_p (callee_fndecl, "operator new []", call, 1)) > - on_allocator_call (sm_ctxt, call, &m_vector_delete); > - else if (is_named_call_p (callee_fndecl, "operator delete", call, 1) > + if (!is_placement_new_p (call)) > + { > + bool returns_nonnull = !TREE_NOTHROW (callee_fndecl) && flag_exceptions; > + if (is_named_call_p (callee_fndecl, "operator new")) > + on_allocator_call (sm_ctxt, call, &m_scalar_delete, returns_nonnull); > + else if (is_named_call_p (callee_fndecl, "operator new []")) > + on_allocator_call (sm_ctxt, call, &m_vector_delete, returns_nonnull); > + } > + > + if (is_named_call_p (callee_fndecl, "operator delete", call, 1) > || is_named_call_p (callee_fndecl, "operator delete", call, 2)) > { > on_deallocator_call (sm_ctxt, node, call, 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). [...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 00000000000..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 00000000000..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 <new> > + > +/* 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 I missing something here? Am I right in thinking that it's possible for the user to use nothrow new when exceptions are enabled to get a new that can fail and return nullptr? Or is that not possible? [...snip...] Hope this makes sense; thanks again for the patch Dave
