[PATCH] diagnose more new/delete mismatches (PR 101791)

[PATCH] diagnose more new/delete mismatches (PR 101791)

[Martin Sebor]

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

-Wmismatched-new-delete partly relies on valid_new_delete_pair_p()
to detect matching calls to operator new and delete.  The function
returns a conservative result which, when indicating a mismatch,
the warning then works to make more accurate before it triggers.
As it turns out, the logic is inadvertently overly permissive in
the most common case of a mismatch: between the array and scalar
forms of the global operators new and delete such as in:

   delete new int[2];   // should be delete[]

The attached patch solves the problem by adding a new argument to
valid_new_delete_pair_p() for the function to set to indicate when
its negative result is definitive rather than a conservative guess.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-101791.diff --]
[-- Type: text/x-patch, Size: 10600 bytes --]

Diagnose mismatches between array and scalar new and delete [PR101791].

Resolves:
PR middle-end/101791 - missing warning on a mismatch between scalar and array forms of new and delete


gcc/ChangeLog:

	PR middle-end/101791
	* gimple-ssa-warn-access.cc (new_delete_mismatch_p): Use new argument
	to valid_new_delete_pair_p.
	* tree.c (valid_new_delete_pair_p): Add argument.
	* tree.h (valid_new_delete_pair_p): Same.

gcc/testsuite/ChangeLog:

	PR middle-end/101791
	* g++.dg/warn/Wmismatched-new-delete-6.C: New test.
	* g++.dg/warn/Wmismatched-new-delete-7.C: New test.

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index e4d98b2ec28..c1ced98780f 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -1233,9 +1782,14 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
 
   /* valid_new_delete_pair_p() returns a conservative result (currently
      it only handles global operators).  A true result is reliable but
-     a false result doesn't necessarily mean the operators don't match.  */
-  if (valid_new_delete_pair_p (new_name, delete_name))
+     a false result doesn't necessarily mean the operators don't match
+     unless CERTAIN is set.  */
+  bool certain;
+  if (valid_new_delete_pair_p (new_name, delete_name, &certain))
     return false;
+  /* CERTAIN is set when the negative result is certain.  */
+  if (certain)
+    return true;
 
   /* For anything not handled by valid_new_delete_pair_p() such as member
      operators compare the individual demangled components of the mangled
diff --git a/gcc/tree.c b/gcc/tree.c
index e923e67b694..5c747321002 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -14311,17 +14311,28 @@ verify_type_context (location_t loc, type_context_kind context,
 	  || targetm.verify_type_context (loc, context, type, silent_p));
 }
 
-/* Return that NEW_ASM and DELETE_ASM name a valid pair of new and
-   delete operators.  */
+/* Return true if NEW_ASM and DELETE_ASM name a valid pair of new and
+   delete operators.  Return false if they may or may not name such
+   a pair and, when nonnull, set *PCERTAIN to true if they certainly
+   do not.  */
 
 bool
-valid_new_delete_pair_p (tree new_asm, tree delete_asm)
+valid_new_delete_pair_p (tree new_asm, tree delete_asm,
+			 bool *pcertain /* = NULL */)
 {
+  bool certain;
+  if (!pcertain)
+    pcertain = &certain;
+
   const char *new_name = IDENTIFIER_POINTER (new_asm);
   const char *delete_name = IDENTIFIER_POINTER (delete_asm);
   unsigned int new_len = IDENTIFIER_LENGTH (new_asm);
   unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
 
+  /* The following failures are due to invalid names so they're not
+     considered certain mismatches.  */
+  *pcertain = false;
+
   if (new_len < 5 || delete_len < 6)
     return false;
   if (new_name[0] == '_')
@@ -14334,11 +14345,19 @@ valid_new_delete_pair_p (tree new_asm, tree delete_asm)
     ++delete_name, --delete_len;
   if (new_len < 4 || delete_len < 5)
     return false;
+
+  /* The following failures are due to names of user-defined operators
+     so they're also not considered certain mismatches.  */
+
   /* *_len is now just the length after initial underscores.  */
   if (new_name[0] != 'Z' || new_name[1] != 'n')
     return false;
   if (delete_name[0] != 'Z' || delete_name[1] != 'd')
     return false;
+
+  /* The following failures are certain mismatches.  */
+  *pcertain = true;
+
   /* _Znw must match _Zdl, _Zna must match _Zda.  */
   if ((new_name[2] != 'w' || delete_name[2] != 'l')
       && (new_name[2] != 'a' || delete_name[2] != 'a'))
@@ -14377,6 +14396,9 @@ valid_new_delete_pair_p (tree new_asm, tree delete_asm)
 	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
 	return true;
     }
+
+  /* The negative result is conservative.  */
+  *pcertain = false;
   return false;
 }
 
diff --git a/gcc/tree.h b/gcc/tree.h
index 972ceb370f8..69d12193686 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5396,7 +5396,9 @@ extern bool gimple_canonical_types_compatible_p (const_tree, const_tree,
 extern bool type_with_interoperable_signedness (const_tree);
 extern bitmap get_nonnull_args (const_tree);
 extern int get_range_pos_neg (tree);
-extern bool valid_new_delete_pair_p (tree, tree);
+
+/* Return true for a valid pair of new and delete operators.  */
+extern bool valid_new_delete_pair_p (tree, tree, bool * = NULL);
 
 /* Return simplified tree code of type that is used for canonical type
    merging.  */
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-6.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-6.C
new file mode 100644
index 00000000000..e19d00058b3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-6.C
@@ -0,0 +1,158 @@
+/* PR middle-end/101791 - missing warning on a mismatch between scalar
+   and array forms of new and delete
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std
+{
+#if __cplusplus >= 201703L
+enum class align_val_t: size_t { };
+#else
+enum align_val_t { };
+#endif
+
+struct nothrow_t { };
+const nothrow_t nothrow = { };
+
+}
+
+void* operator new (size_t);
+void* operator new (size_t, std::align_val_t);
+void* operator new (size_t, std::nothrow_t) throw ();
+void* operator new (size_t, std::align_val_t, std::nothrow_t) throw ();
+
+void* operator new[] (size_t);
+void* operator new[] (size_t, std::align_val_t);
+void* operator new[] (size_t, std::nothrow_t) throw ();
+void* operator new[] (size_t, std::align_val_t, std::nothrow_t) throw ();
+
+void operator delete (void*);
+void operator delete (void*, size_t);
+void operator delete (void*, std::align_val_t);
+void operator delete (void*, size_t, std::align_val_t);
+void operator delete (void*, std::nothrow_t) throw ();
+void operator delete (void*, std::align_val_t, std::nothrow_t) throw ();
+
+void operator delete[] (void*);
+void operator delete[] (void*, size_t);
+void operator delete[] (void*, std::align_val_t);
+void operator delete[] (void*, size_t, std::align_val_t);
+void operator delete[] (void*, std::nothrow_t) throw ();
+void operator delete[] (void*, std::align_val_t, std::nothrow_t) throw ();
+
+
+void sink (void*, ...);
+
+
+void nowarn_scalar_scalar ()
+{
+  {
+    int *p = new int;
+    sink (p);
+    delete p;
+  }
+
+  {
+    int *p = new (std::align_val_t (8)) int;
+    sink (p);
+    delete p;
+  }
+
+  {
+    int *p = new (std::nothrow) int;
+    sink (p);
+    delete p;
+  }
+
+  {
+    int *p = new (std::align_val_t (8), std::nothrow) int;
+    sink (p);
+    delete p;
+  }
+}
+
+void nowarn_array_array ()
+{
+  {
+    int *p = new int[__LINE__];
+    sink (p);
+    delete[] p;
+  }
+
+  {
+    int *p = new (std::align_val_t (8)) int[__LINE__];
+    sink (p);
+    delete[] p;
+  }
+
+  {
+    int *p = new (std::nothrow) int[__LINE__];
+    sink (p);
+    delete[] p;
+  }
+
+  {
+    int *p = new (std::align_val_t (8), std::nothrow) int[__LINE__];
+    sink (p);
+    delete[] p;
+  }
+}
+
+
+
+void nowarn_scalar_array ()
+{
+  {
+    int *p = new int;   // { dg-message "returned from" }
+    sink (p);
+    delete[] p;         // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::align_val_t (8)) int;
+    sink (p);
+    delete[] p;         // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::nothrow) int;
+    sink (p);
+    delete[] p;         // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::align_val_t (8), std::nothrow) int;
+    sink (p);
+    delete[] p;         // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+}
+
+
+void nowarn_array_scalar ()
+{
+  {
+    int *p = new int[__LINE__];
+    sink (p);
+    delete p;           // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::align_val_t (8)) int[__LINE__];
+    sink (p);
+    delete p;           // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::nothrow) int[__LINE__];
+    sink (p);
+    delete p;           // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+
+  {
+    int *p = new (std::align_val_t (8), std::nothrow) int[__LINE__];
+    sink (p);
+    delete p;           // { dg-warning "\\\[-Wmismatched-new-delete" }
+  }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-7.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-7.C
new file mode 100644
index 00000000000..67a5354b91d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-7.C
@@ -0,0 +1,91 @@
+/* PR middle-end/101791 - missing warning on a mismatch between scalar
+   and array forms of new and delete
+   Verify that likely safe calls to technically mismatched member operator
+   new and delete are not diagnosed.  This test might need to be adjusted
+   if it turns out the assumptions GCC makes are overly conservative.
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std
+{
+#if __cplusplus >= 201703L
+enum class align_val_t: size_t { };
+#else
+enum align_val_t { };
+#endif
+
+struct nothrow_t { };
+const nothrow_t nothrow = { };
+
+}
+
+void sink (void*, ...);
+
+struct X { } x;
+struct Y { } y;
+
+struct A
+{
+  void* operator new (size_t);
+  void* operator new (size_t, std::align_val_t);
+
+  void* operator new (size_t, X);
+  void* operator new (size_t, Y);
+
+  void* operator new (size_t, std::align_val_t, X);
+  void* operator new (size_t, std::nothrow_t, Y);
+
+  /* A single operator delete callable on the result of calls to any
+     of the operator new overloads above (this may be too optimistic).  */
+  void operator delete (void*);
+};
+
+A* nowarn_align ()
+{
+  /* The following are likely okay given A's definition above but would
+     not be if A also defined an align_val_t overload of operator delete.  */
+  A *p = new (std::align_val_t (8)) A;
+  delete p;
+
+  return new (std::align_val_t (16)) A;
+}
+
+A* nowarn_X ()
+{
+  /* The following are also likely okay given A's definition above but
+     also would not be if A also defined an overload of operator delete
+     for X.  */
+  A *p = new (x) A;
+  delete p;
+  return new (x) A;
+}
+
+A* nowarn_Y ()
+{
+  // Same as above.
+  A *p = new (y) A;
+  delete p;
+  return new (y) A;
+}
+
+
+A* nowarn_align_X ()
+{
+  // Same as above.
+  A *p = new (std::align_val_t (32), x) A;
+  delete p;
+
+  return new (std::align_val_t (64), x) A;
+}
+
+
+A* nowarn_nothrow_Y ()
+{
+  // Same as above.
+  A *p = new (std::nothrow, y) A;
+  delete p;
+  return new (std::nothrow, y) A;
+}
+

Re: [PATCH] diagnose more new/delete mismatches (PR 101791)

[Jeff Law]

On 8/5/2021 4:02 PM, Martin Sebor via Gcc-patches wrote:
> -Wmismatched-new-delete partly relies on valid_new_delete_pair_p()
> to detect matching calls to operator new and delete.  The function
> returns a conservative result which, when indicating a mismatch,
> the warning then works to make more accurate before it triggers.
> As it turns out, the logic is inadvertently overly permissive in
> the most common case of a mismatch: between the array and scalar
> forms of the global operators new and delete such as in:
>
>   delete new int[2];   // should be delete[]
>
> The attached patch solves the problem by adding a new argument to
> valid_new_delete_pair_p() for the function to set to indicate when
> its negative result is definitive rather than a conservative guess.
>
> Tested on x86_64-linux.
>
> Martin
>
> gcc-101791.diff
>
> Diagnose mismatches between array and scalar new and delete [PR101791].
>
> Resolves:
> PR middle-end/101791 - missing warning on a mismatch between scalar and array forms of new and delete
>
>
> gcc/ChangeLog:
>
> 	PR middle-end/101791
> 	* gimple-ssa-warn-access.cc (new_delete_mismatch_p): Use new argument
> 	to valid_new_delete_pair_p.
> 	* tree.c (valid_new_delete_pair_p): Add argument.
> 	* tree.h (valid_new_delete_pair_p): Same.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/101791
> 	* g++.dg/warn/Wmismatched-new-delete-6.C: New test.
> 	* g++.dg/warn/Wmismatched-new-delete-7.C: New test.
OK
jeff