[PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[priour.be]

From: benjamin priour <priour.be@gmail.com>

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 <priour.be@gmail.com>
---
 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
 
+/* Return TRUE if CALL is non-allocating operator new or 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;
+
+  /* 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 &cd) const final override
   {
-    return cd.num_args () == 1;
+    return cd.num_args () == 1 || cd.num_args () == 2;
   }
 
   void impl_call_pre (const call_details &cd) 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))
+      {
+	const region *new_reg
+	  = model->get_or_create_region_for_heap_alloc (size_sval, ctxt);
+	if (cd.get_lhs_type ())
+	  {
+	    const svalue *ptr_sval
+	      = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+	    cd.maybe_set_lhs (ptr_sval);
+	  }
+      }
+    /* If the call was actually a placement new, check that accessing
+    the buffer lhs is placed into does not result in out-of-bounds.  */
+    else
       {
+	const region *ptr_reg = cd.maybe_get_arg_region (1);
+	if (ptr_reg && cd.get_lhs_type ())
+	  {
+	    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);
+	    model->check_region_for_write (sized_new_reg,
+	      nullptr,
+	      ctxt);
 	const svalue *ptr_sval
-	  = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+	  = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
 	cd.maybe_set_lhs (ptr_sval);
+	  }
+      }
+  }
+
+  void impl_call_post (const call_details &cd) const final override
+  {
+    region_model *model = cd.get_model ();
+    region_model_manager *mgr = cd.get_manager ();
+    tree callee_fndecl = cd.get_fndecl_for_call ();
+    region_model_context *ctxt = cd.get_ctxt ();
+
+    /* If the call is guaranteed to return nonnull
+      then add a nonnull constraint to the allocated region.  */
+    if (!TREE_NOTHROW (TREE_TYPE (callee_fndecl)) && flag_exceptions)
+      {
+	const svalue *nonnull
+	  = mgr->get_or_create_null_ptr (cd.get_lhs_type ());
+	const svalue *result
+	  = model->get_store_value (cd.get_lhs_region (), ctxt);
+	model->add_constraint (result, NE_EXPR, nonnull, ctxt);
       }
   }
 };
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 187013a37cc..599178e3d86 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3479,6 +3479,27 @@ region_model::eval_condition (const svalue *lhs,
 	}
     }
 
+  /* Attempt to unwrap cast if there is one, and the types match.  */
+  tree lhs_type = lhs->get_type ();
+  tree rhs_type = rhs->get_type ();
+  if (lhs_type && rhs_type)
+  {
+    const unaryop_svalue *lhs_un_op = dyn_cast <const unaryop_svalue *> (lhs);
+    const unaryop_svalue *rhs_un_op = dyn_cast <const unaryop_svalue *> (rhs);
+    if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+      && rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs_un_op->get_arg (), op, rhs_un_op->get_arg ());
+
+    else if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs_un_op->get_arg (), op, rhs);
+
+    else if (rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs, op, rhs_un_op->get_arg ());
+  }
+
   /* Otherwise, try constraints.
      Cast to const to ensure we don't change the constraint_manager as we
      do this (e.g. by creating equivalence classes).  */
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,
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;
+}
diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
index d3076c3cf25..33872e6ddab 100644
--- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
+++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
@@ -14,6 +14,6 @@ struct int_and_addr {
 
 int test (int_container ic)
 {
-  int_and_addr *iaddr = new (ic.addr ()) int_and_addr;
+  int_and_addr *iaddr = new (ic.addr ()) int_and_addr; /* { dg-warning "stack-based buffer overflow" } */
   return iaddr->i;
 }
diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C
new file mode 100644
index 00000000000..810944cd137
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C
@@ -0,0 +1,27 @@
+/* { dg-additional-options "-Wno-placement-new -Wno-analyzer-use-of-uninitialized-value" } */
+
+#include <new>
+#include "../../gcc.dg/analyzer/analyzer-decls.h"
+
+extern int get_buf_size ();
+
+void var_too_short ()
+{
+  short s;
+  long *lp = new (&s) long; /* { dg-warning "stack-based buffer overflow" } */
+  /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" "" { target *-*-* } .-1 } */
+}
+
+void static_buffer_too_short ()
+{
+  int n = 16;
+  int buf[n];
+  int *p = new (buf) int[n + 1]; /* { dg-warning "stack-based buffer overflow" } */
+}
+
+void symbolic_buffer_too_short ()
+{
+  int n = get_buf_size ();
+  char buf[n];
+  char *p = new (buf) char[n + 10]; /* { dg-warning "stack-based buffer overflow" } */
+}
diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C
index 89055491a48..c0cf7ec4c48 100644
--- a/gcc/testsuite/g++.dg/analyzer/placement-new.C
+++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C
@@ -14,15 +14,74 @@ void test_2 (void)
 {
   char buf[sizeof(int) * 10];
   int *p = new(buf) int[10];
-}
+} // { dg-prune-output "-Wfree-nonheap-object" }
 
 /* Delete of placement new.  */
 
 void test_3 (void)
 {
   char buf[sizeof(int)]; // { dg-message "region created on stack here" }
-  int *p = new(buf) int (42);
+  int *p = new (buf) int (42);
   delete p; // { dg-warning "memory on the stack" }
 }
 
 // { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_4 (void)
+{
+  int buf[5]; // { dg-message "region created on stack here" }
+  int *p = new (&buf[2]) int (42);
+  delete p; // { dg-warning "memory on the stack" }
+}
+
+
+// { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_write_placement_after_delete (void)
+{
+  short *s = ::new short;
+  short *lp = ::new (s) short;
+  ::delete s;
+  *lp = 12; /* { dg-warning "use after 'delete' of 'lp'" "write placement new after buffer deletion" } */
+}
+
+void test_read_placement_after_delete (void)
+{
+  short *s = ::new short;
+  short *lp = ::new (s) short;
+  ::delete s;
+  short m = *lp; // { dg-warning "use after 'delete' of 'lp'" "read placement new after buffer deletion" }
+}
+
+struct A
+{
+  int x;
+  int y;
+};
+
+void test_use_placement_after_destruction (void)
+{
+  A a;
+  int *lp = ::new (&a.y) int;
+  *lp = 2; /* { dg-bogus "-Wanalyzer-use-of-uninitialized-value" } */
+  a.~A();
+  int m = *lp; /* { dg-warning "use of uninitialized value '\\*lp'" "use of placement after the underlying buffer was destructed." } */
+}
+
+void test_initialization_through_placement (void)
+{
+  int x;
+  int *p = ::new (&x) int;
+  *p = 10;
+  int z = x + 2; /* { dg-bogus "use of uninitialized value 'x'" "x has been initialized through placement pointer" } */
+}
+
+void test_partial_initialization_through_placement (void)
+{
+  char buf[4];
+  char *p = ::new (&buf[2]) char;
+  *p = 10;
+  char *y = ::new (&buf[0]) char;
+  char z = buf[2] + 2; /* { dg-bogus "use of uninitialized value" } */
+  z = *y + 2; /* { dg-warning "use of uninitialized value '\\*y'" "y has only been partially initialized" } */
+}
\ No newline at end of file
-- 
2.34.1

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[David Malcolm]

On Tue, 2023-07-04 at 18:25 +0200, priour.be@gmail.com wrote:
> From: benjamin priour <priour.be@gmail.com>
> 
> 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 ?

Thanks for the patch.

Overall, looks almost ready, but some nitpicks below,,,

[..snip...]


> 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.

> +{
> +  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 &cd) 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.


>    }
>  
>    void impl_call_pre (const call_details &cd) 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.


> +       const region *new_reg
> +         = model->get_or_create_region_for_heap_alloc (size_sval, ctxt);
> +       if (cd.get_lhs_type ())
> +         {
> +           const svalue *ptr_sval
> +             = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> +           cd.maybe_set_lhs (ptr_sval);
> +         }
> +      }
> +    /* If the call was actually a placement new, check that
> accessing
> +    the buffer lhs is placed into does not result in out-of-bounds. 
> */
> +    else
>        {
> +       const region *ptr_reg = cd.maybe_get_arg_region (1);
> +       if (ptr_reg && cd.get_lhs_type ())
> +         {
> +           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);
> +           model->check_region_for_write (sized_new_reg,
> +             nullptr,
> +             ctxt);
>         const svalue *ptr_sval
> -         = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> +         = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
>         cd.maybe_set_lhs (ptr_sval);
> +         }
> +      }
> +  }

[...snip...]

> 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;
> +};

We've run into issues with bounds-checking testcases when using types
like "int" that have target-specific sizes.

Please use <stdint.h> 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.

[..snip...]

Other than those issues, looks good

Thanks again
Dave

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[Benjamin Priour]

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 &cd) 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 &cd) 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 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;
>> +};
> We've run into issues with bounds-checking testcases when using types
> like "int" that have target-specific sizes.
>
> Please use <stdint.h> 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

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[priour.be]

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 <priour.be@gmail.com>
---
 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 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..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");
+}
+
 namespace ana {
 
 /* Implementations of specific functions.  */
@@ -46,7 +89,11 @@ class kf_operator_new : public known_function
 public:
   bool matches_call_types_p (const call_details &cd) const final override
   {
-    return cd.num_args () == 1;
+    return (cd.num_args () == 1
+      && cd.arg_is_size_p (0))
+      || (cd.num_args () == 2
+      && cd.arg_is_size_p (0)
+      && POINTER_TYPE_P (cd.get_arg_type (1)));
   }
 
   void impl_call_pre (const call_details &cd) const final override
@@ -54,13 +101,61 @@ 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 was actually a placement new, check that accessing
+    the buffer lhs is placed into does not result in out-of-bounds.  */
+    if (is_placement_new_p (call))
       {
+	const region *ptr_reg = cd.maybe_get_arg_region (1);
+	if (ptr_reg && cd.get_lhs_type ())
+	  {
+	    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);
+	    model->check_region_for_write (sized_new_reg,
+	      nullptr,
+	      ctxt);
 	const svalue *ptr_sval
-	  = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+	  = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
 	cd.maybe_set_lhs (ptr_sval);
+	  }
+      }
+    /* If the call is an allocating new, then create a heap allocated
+    region.  */
+    else
+      {
+	const region *new_reg
+	  = model->get_or_create_region_for_heap_alloc (size_sval, ctxt);
+	if (cd.get_lhs_type ())
+	  {
+	    const svalue *ptr_sval
+	      = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+	    cd.maybe_set_lhs (ptr_sval);
+	  }
+      }
+
+  }
+
+  void impl_call_post (const call_details &cd) const final override
+  {
+    region_model *model = cd.get_model ();
+    region_model_manager *mgr = cd.get_manager ();
+    tree callee_fndecl = cd.get_fndecl_for_call ();
+    region_model_context *ctxt = cd.get_ctxt ();
+
+    /* If the call is guaranteed to return nonnull
+      then add a nonnull constraint to the allocated region.  */
+    if (!TREE_NOTHROW (TREE_TYPE (callee_fndecl)) && flag_exceptions)
+      {
+	const svalue *nonnull
+	  = mgr->get_or_create_null_ptr (cd.get_lhs_type ());
+	const svalue *result
+	  = model->get_store_value (cd.get_lhs_region (), ctxt);
+	model->add_constraint (result, NE_EXPR, nonnull, ctxt);
       }
   }
 };
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 187013a37cc..599178e3d86 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3479,6 +3479,27 @@ region_model::eval_condition (const svalue *lhs,
 	}
     }
 
+  /* Attempt to unwrap cast if there is one, and the types match.  */
+  tree lhs_type = lhs->get_type ();
+  tree rhs_type = rhs->get_type ();
+  if (lhs_type && rhs_type)
+  {
+    const unaryop_svalue *lhs_un_op = dyn_cast <const unaryop_svalue *> (lhs);
+    const unaryop_svalue *rhs_un_op = dyn_cast <const unaryop_svalue *> (rhs);
+    if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+      && rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs_un_op->get_arg (), op, rhs_un_op->get_arg ());
+
+    else if (lhs_un_op && CONVERT_EXPR_CODE_P (lhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs_un_op->get_arg (), op, rhs);
+
+    else if (rhs_un_op && CONVERT_EXPR_CODE_P (rhs_un_op->get_op ())
+      && lhs_type == rhs_type)
+      return eval_condition (lhs, op, rhs_un_op->get_arg ());
+  }
+
   /* Otherwise, try constraints.
      Cast to const to ensure we don't change the constraint_manager as we
      do this (e.g. by creating equivalence classes).  */
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,
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;
+}
diff --git a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
index d3076c3cf25..33872e6ddab 100644
--- a/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
+++ b/gcc/testsuite/g++.dg/analyzer/out-of-bounds-placement-new.C
@@ -14,6 +14,6 @@ struct int_and_addr {
 
 int test (int_container ic)
 {
-  int_and_addr *iaddr = new (ic.addr ()) int_and_addr;
+  int_and_addr *iaddr = new (ic.addr ()) int_and_addr; /* { dg-warning "stack-based buffer overflow" } */
   return iaddr->i;
 }
diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new-size.C b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C
new file mode 100644
index 00000000000..4536d361b4a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/placement-new-size.C
@@ -0,0 +1,27 @@
+/* { dg-additional-options "-Wno-placement-new -Wno-analyzer-use-of-uninitialized-value" } */
+
+#include <new>
+#include <stdint.h>
+
+extern int get_buf_size ();
+
+void var_too_short ()
+{
+  int8_t s;
+  int64_t *lp = new (&s) int64_t; /* { dg-warning "stack-based buffer overflow" } */
+  /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" "" { target *-*-* } .-1 } */
+}
+
+void static_buffer_too_short ()
+{
+  int n = 16;
+  int buf[n];
+  int *p = new (buf) int[n + 1]; /* { dg-warning "stack-based buffer overflow" } */
+}
+
+void symbolic_buffer_too_short ()
+{
+  int n = get_buf_size ();
+  char buf[n];
+  char *p = new (buf) char[n + 10]; /* { dg-warning "stack-based buffer overflow" } */
+}
diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C
index 89055491a48..c0cf7ec4c48 100644
--- a/gcc/testsuite/g++.dg/analyzer/placement-new.C
+++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C
@@ -14,15 +14,74 @@ void test_2 (void)
 {
   char buf[sizeof(int) * 10];
   int *p = new(buf) int[10];
-}
+} // { dg-prune-output "-Wfree-nonheap-object" }
 
 /* Delete of placement new.  */
 
 void test_3 (void)
 {
   char buf[sizeof(int)]; // { dg-message "region created on stack here" }
-  int *p = new(buf) int (42);
+  int *p = new (buf) int (42);
   delete p; // { dg-warning "memory on the stack" }
 }
 
 // { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_4 (void)
+{
+  int buf[5]; // { dg-message "region created on stack here" }
+  int *p = new (&buf[2]) int (42);
+  delete p; // { dg-warning "memory on the stack" }
+}
+
+
+// { dg-prune-output "-Wfree-nonheap-object" }
+
+void test_write_placement_after_delete (void)
+{
+  short *s = ::new short;
+  short *lp = ::new (s) short;
+  ::delete s;
+  *lp = 12; /* { dg-warning "use after 'delete' of 'lp'" "write placement new after buffer deletion" } */
+}
+
+void test_read_placement_after_delete (void)
+{
+  short *s = ::new short;
+  short *lp = ::new (s) short;
+  ::delete s;
+  short m = *lp; // { dg-warning "use after 'delete' of 'lp'" "read placement new after buffer deletion" }
+}
+
+struct A
+{
+  int x;
+  int y;
+};
+
+void test_use_placement_after_destruction (void)
+{
+  A a;
+  int *lp = ::new (&a.y) int;
+  *lp = 2; /* { dg-bogus "-Wanalyzer-use-of-uninitialized-value" } */
+  a.~A();
+  int m = *lp; /* { dg-warning "use of uninitialized value '\\*lp'" "use of placement after the underlying buffer was destructed." } */
+}
+
+void test_initialization_through_placement (void)
+{
+  int x;
+  int *p = ::new (&x) int;
+  *p = 10;
+  int z = x + 2; /* { dg-bogus "use of uninitialized value 'x'" "x has been initialized through placement pointer" } */
+}
+
+void test_partial_initialization_through_placement (void)
+{
+  char buf[4];
+  char *p = ::new (&buf[2]) char;
+  *p = 10;
+  char *y = ::new (&buf[0]) char;
+  char z = buf[2] + 2; /* { dg-bogus "use of uninitialized value" } */
+  z = *y + 2; /* { dg-warning "use of uninitialized value '\\*y'" "y has only been partially initialized" } */
+}
\ No newline at end of file
-- 
2.34.1

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[David Malcolm]

On Thu, 2023-07-06 at 16:43 +0200, priour.be@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

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[Benjamin Priour]

[-- Attachment #1: Type: text/plain, Size: 6869 bytes --]

Hi Dave,

On Fri, Jul 21, 2023 at 10:10 PM David Malcolm <dmalcolm@redhat.com> 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 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?
>
>
Thanks a lot for spotting that, the new test pointed out an issue with the
detection of nothrow.
It has been fixed and now both test cases behave similarly.
However, this highlighted a faulty test case I had written.

int* y = new(std::nothrow) int();
int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
/* { dg-warning "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1
} */ // (#) should be a bogus
delete y;

The test labelled (#) is wrong and should be a bogus instead.
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.

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.

I've also fixed the wording of "use-after-free" when using a placement
pointer after deallocation of its underlying buffer.

A *a = ::new A ();
int *lp = ::new (&a->y) int;
delete a;
int m = *lp; /* { dg-bogus "use after 'free' of 'lp'" "PREVIOUSLY" } */
int m = *lp; /* { dg-warning "use after 'delete' of 'lp'" "CURRENTLY" } */

Thanks
Benjamin

Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[David Malcolm]

On Mon, 2023-07-31 at 13:46 +0200, Benjamin Priour wrote:
> Hi Dave,
> 
> On Fri, Jul 21, 2023 at 10:10 PM David Malcolm <dmalcolm@redhat.com>
> wrote:

[...snip...]

> > 
> > 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?
> > 
> > 
> Thanks a lot for spotting that, the new test pointed out an issue
> with the
> detection of nothrow.
> It has been fixed and now both test cases behave similarly.
> However, this highlighted a faulty test case I had written.
> 
> int* y = new(std::nothrow) int();
> int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
> /* { dg-warning "use of uninitialized value '\\*y'" "" { xfail *-*-*
> } .-1
> } */ // (#) should be a bogus
> delete y;
> 
> The test labelled (#) is wrong and should be a bogus instead.

Am I right in thinking that by this you mean that with the patch, the
analyzer complains about "use of uninitialized value '*y'" ? (which
would be an incorrect warning)

> 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.

What does the gimple look like and what does the program_state look
like after the assignment to y?

> 
> 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?


[...snip...]

Thanks
Dave