[PATCH] teach compute_objsize about placement new (PR 100876)

[PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor]

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

The two forms of placement operator new defined in <new> return their
pointer argument and may not be displaced by user-defined functions.
But because they are ordinary (not built-in) functions this property
isn't reflected in their declarations alone, and there's no user-
level attribute to annotate them with.  When they are inlined
the property is transparent in the IL but when they are not (without
inlining such as -O0), calls to the operators appear in the IL and
cause -Wmismatched-new-delete to try to match them with the functions
called to deallocate memory.  When the pointer to the memory was
obtained from a function that matches the deallocator but not
the placement new, the warning falsely triggers.

The attached patch solves this by detecting calls to placement new
and treating them the same as those to other pass-through calls (such
as memset).  In addition, it also teaches -Wfree-nonheap-object about
placement delete, for a similar reason as above.  Finally, it also
adds a test for attribute fn spec indicating a function returns its
argument.  It's not necessary for the fix (I had initially though
placement new might have the attribute) but it seems appropriate
to check.

Tested on x86_64-linux.

Martin

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

PR c++/100876 - -Wmismatched-new-delete should understand placement new when it's not inlined

gcc/ChangeLog:

	PR c++/100876
	* builtins.c (gimple_call_return_array): Check for attribute fn spec.
	Handle calls to placement new.
	(ndecl_dealloc_argno): Avoid placement delete.

gcc/testsuite/ChangeLog:

	PR c++/100876
	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
	* g++.dg/warn/Wstringop-overflow-7.C: New test.
	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
	* g++.dg/analyzer/placement-new.C: Prune out expected warning.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index af1fe49bb48..fb0717a0248 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -5159,11 +5159,43 @@ static tree
 gimple_call_return_array (gimple *stmt, offset_int offrng[2],
 			  range_query *rvals)
 {
-  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
-      || gimple_call_num_args (stmt) < 1)
+  {
+    /* Check for attribute fn spec to see if the function returns one
+       of its arguments.  */
+    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
+    unsigned int argno;
+    if (fnspec.returns_arg (&argno))
+      {
+	offrng[0] = offrng[1] = 0;
+	return gimple_call_arg (stmt, argno);
+      }
+  }
+
+  if (gimple_call_num_args (stmt) < 1)
     return NULL_TREE;
 
   tree fn = gimple_call_fndecl (stmt);
+  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
+    {
+      /* See if this is a call to placement new.  */
+      if (!fn
+	  || !DECL_IS_OPERATOR_NEW_P (fn)
+	  || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
+	return NULL_TREE;
+
+      tree fname = DECL_ASSEMBLER_NAME (fn);
+      const char *name = IDENTIFIER_POINTER (fname);
+      if (strcmp (name, "_ZnwmPv")       // ordinary form
+	  && strcmp (name, "_ZnamPv"))   // array form
+	return NULL_TREE;
+
+      if (gimple_call_num_args (stmt) != 2)
+	return NULL_TREE;
+
+      offrng[0] = offrng[1] = 0;
+      return gimple_call_arg (stmt, 1);
+    }
+
   switch (DECL_FUNCTION_CODE (fn))
     {
     case BUILT_IN_MEMCPY:
@@ -13285,7 +13317,18 @@ fndecl_dealloc_argno (tree fndecl)
 {
   /* A call to operator delete isn't recognized as one to a built-in.  */
   if (DECL_IS_OPERATOR_DELETE_P (fndecl))
-    return 0;
+    {
+      if (DECL_IS_REPLACEABLE_OPERATOR (fndecl))
+	return 0;
+
+      /* Avoid placement delete that's not been inlined.  */
+      tree fname = DECL_ASSEMBLER_NAME (fndecl);
+      const char *name = IDENTIFIER_POINTER (fname);
+      if (strcmp (name, "_ZdlPvS_") == 0       // ordinary form
+	  || strcmp (name, "_ZdaPvS_") == 0)   // array form
+	return UINT_MAX;
+      return 0;
+    }
 
   /* TODO: Handle user-defined functions with attribute malloc?  Handle
      known non-built-ins like fopen?  */
diff --git a/gcc/testsuite/g++.dg/analyzer/placement-new.C b/gcc/testsuite/g++.dg/analyzer/placement-new.C
index 8250f45b9d9..b648a428247 100644
--- a/gcc/testsuite/g++.dg/analyzer/placement-new.C
+++ b/gcc/testsuite/g++.dg/analyzer/placement-new.C
@@ -24,3 +24,5 @@ void test_3 (void)
   int *p = new(buf) int (42);
   delete p; // { dg-warning "memory not on the heap" }
 }
+
+// { dg-prune-output "-Wfree-nonheap-object" }
diff --git a/gcc/testsuite/g++.dg/warn/Wfree-nonheap-object-6.C b/gcc/testsuite/g++.dg/warn/Wfree-nonheap-object-6.C
new file mode 100644
index 00000000000..83b6ff9157c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wfree-nonheap-object-6.C
@@ -0,0 +1,45 @@
+/* { dg-do compile }
+   { dg-options "-O0 -Wall" } */
+
+#if __cplusplus < 201103L
+# define noexcept throw ()
+#endif
+
+void* operator new (__SIZE_TYPE__, void* __p) noexcept;
+void operator delete (void*, void*);
+
+void* operator new[] (__SIZE_TYPE__, void* __p) noexcept;
+void operator delete[] (void*, void*) noexcept;
+
+struct A { A (); ~A (); int i; };
+
+extern void *p;
+
+void nowarn_placement_new ()
+{
+  char a[sizeof (A)];
+  p = new (a) A ();           // { dg-bogus "-Wfree-nonheap-object" }
+}
+
+
+void warn_placement_new ()
+{
+  char a[sizeof (A)];
+  p = new (a + 1) A ();       // { dg-warning "\\\[-Wplacement-new" }
+                              // { dg-bogus "-Wfree-nonheap-object" "bogus" { target *-*-* } .-1 }
+}
+
+
+void nowarn_placement_new_array ()
+{
+  char a[sizeof (A)];
+  p = new (a) A[1];           // { dg-bogus "-Wfree-nonheap-object" }
+}
+
+
+void warn_placement_new_array ()
+{
+  char a[sizeof (A)];
+  p = new (a + 1) A[1];       // { dg-warning "\\\[-Wplacement-new" }
+                              // { dg-bogus "-Wfree-nonheap-object" "bogus" { target *-*-* } .-1 }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C
new file mode 100644
index 00000000000..4320181e4d7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C
@@ -0,0 +1,37 @@
+/* PR c++/100876 - -Wmismatched-new-delete should either look through
+   or ignore placement new
+   { dg-do compile }
+   { dg-options "-O0 -Wall" } */
+
+extern "C" {
+  void* malloc (__SIZE_TYPE__);
+  void free (void*);
+}
+
+void* operator new (__SIZE_TYPE__, void*);
+void* operator new[] (__SIZE_TYPE__, void*);
+
+void nowarn_placement_new ()
+{
+  free (new (malloc (sizeof (int))) int ());      // { dg-bogus "-Wmismatched-new-delete" }
+}
+
+void nowarn_placement_array_new ()
+{
+  free (new (malloc (sizeof (int) * 2)) int[2]);  // { dg-bogus "-Wmismatched-new-delete" }
+}
+
+
+void warn_placement_new ()
+{
+  void *p = malloc (sizeof (int));
+  int *q = new (p) int ();
+  delete q;                   // { dg-warning "-Wmismatched-new-delete" }
+}
+
+void warn_placement_array_new ()
+{
+  void *p = malloc (sizeof (int));
+  int *q = new (p) int[2];
+  delete q;                   // { dg-warning "-Wmismatched-new-delete" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-5.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-5.C
new file mode 100644
index 00000000000..92c75df40d9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-5.C
@@ -0,0 +1,37 @@
+/* PR c++/100876 - -Wmismatched-new-delete should either look through
+   or ignore placement new
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern "C" {
+  void* malloc (__SIZE_TYPE__);
+  void free (void*);
+}
+
+inline void* operator new (__SIZE_TYPE__, void *p) { return p; }
+inline void* operator new[] (__SIZE_TYPE__, void *p) { return p; }
+
+void nowarn_placement_new ()
+{
+  free (new (malloc (sizeof (int))) int ());      // { dg-bogus "-Wmismatched-new-delete" }
+}
+
+void nowarn_placement_array_new ()
+{
+  free (new (malloc (sizeof (int) * 2)) int[2]);  // { dg-bogus "-Wmismatched-new-delete" }
+}
+
+
+void warn_placement_new ()
+{
+  void *p = malloc (sizeof (int));
+  int *q = new (p) int ();
+  delete q;                   // { dg-warning "-Wmismatched-new-delete" }
+}
+
+void warn_placement_array_new ()
+{
+  void *p = malloc (sizeof (int));
+  int *q = new (p) int[2];
+  delete q;                   // { dg-warning "-Wmismatched-new-delete" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-7.C b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-7.C
new file mode 100644
index 00000000000..d3d28f43d0a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-7.C
@@ -0,0 +1,42 @@
+/* PR c++/100876 - -Wmismatched-new-delete should either look through
+   or ignore placement new
+   { dg-do compile }
+   { dg-options "-O0 -Wall -Wno-array-bounds" } */
+
+inline void* operator new (__SIZE_TYPE__, void *p) { return p; }
+inline void* operator new[] (__SIZE_TYPE__, void *p) { return p; }
+
+void* nowarn_placement_new_memset ()
+{
+  struct S { int i; };
+  void *p = __builtin_malloc (sizeof (S));
+  S *q = new (p) S;
+  __builtin_memset (q, 0, sizeof (S));
+  return q;
+}
+
+void* warn_placement_new_memset ()
+{
+  struct S { int i; };
+  void *p = __builtin_malloc (sizeof (S));
+  S *q = new (p) S;
+  __builtin_memset (q, 0, sizeof (S) + 1);  // { dg-warning "\\\[-Wstringop-overflow" }
+  return q;
+}
+
+void* nowarn_placement_new_array_strncpy (const char *s)
+{
+  void *p = __builtin_malloc (5);
+  char *q = new (p) char[5];
+  __builtin_strncpy (q, s, 5);
+  return q;
+
+}
+
+void* warn_placement_new_array_strncpy (const char *s)
+{
+  void *p = __builtin_malloc (4);
+  char *q = new (p) char[5];
+  __builtin_strncpy (q, s, 5);  // { dg-warning "\\\[-Wstringop-overflow" }
+  return q;
+}

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Marek Polacek]

On Wed, Jun 02, 2021 at 03:40:49PM -0600, Martin Sebor via Gcc-patches wrote:
> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
> +    {
> +      /* See if this is a call to placement new.  */
> +      if (!fn
> +	  || !DECL_IS_OPERATOR_NEW_P (fn)
> +	  || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
> +	return NULL_TREE;
> +
> +      tree fname = DECL_ASSEMBLER_NAME (fn);
> +      const char *name = IDENTIFIER_POINTER (fname);
> +      if (strcmp (name, "_ZnwmPv")       // ordinary form
> +	  && strcmp (name, "_ZnamPv"))   // array form
> +	return NULL_TREE;

Not a review, but you can use id_equal here and simplify things.

Marek

PING [PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor]

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571777.html

On 6/2/21 3:40 PM, Martin Sebor wrote:
> The two forms of placement operator new defined in <new> return their
> pointer argument and may not be displaced by user-defined functions.
> But because they are ordinary (not built-in) functions this property
> isn't reflected in their declarations alone, and there's no user-
> level attribute to annotate them with.  When they are inlined
> the property is transparent in the IL but when they are not (without
> inlining such as -O0), calls to the operators appear in the IL and
> cause -Wmismatched-new-delete to try to match them with the functions
> called to deallocate memory.  When the pointer to the memory was
> obtained from a function that matches the deallocator but not
> the placement new, the warning falsely triggers.
> 
> The attached patch solves this by detecting calls to placement new
> and treating them the same as those to other pass-through calls (such
> as memset).  In addition, it also teaches -Wfree-nonheap-object about
> placement delete, for a similar reason as above.  Finally, it also
> adds a test for attribute fn spec indicating a function returns its
> argument.  It's not necessary for the fix (I had initially though
> placement new might have the attribute) but it seems appropriate
> to check.
> 
> Tested on x86_64-linux.
> 
> Martin

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Jeff Law]

On 6/2/2021 3:40 PM, Martin Sebor via Gcc-patches wrote:
> The two forms of placement operator new defined in <new> return their
> pointer argument and may not be displaced by user-defined functions.
> But because they are ordinary (not built-in) functions this property
> isn't reflected in their declarations alone, and there's no user-
> level attribute to annotate them with.  When they are inlined
> the property is transparent in the IL but when they are not (without
> inlining such as -O0), calls to the operators appear in the IL and
> cause -Wmismatched-new-delete to try to match them with the functions
> called to deallocate memory.  When the pointer to the memory was
> obtained from a function that matches the deallocator but not
> the placement new, the warning falsely triggers.
>
> The attached patch solves this by detecting calls to placement new
> and treating them the same as those to other pass-through calls (such
> as memset).  In addition, it also teaches -Wfree-nonheap-object about
> placement delete, for a similar reason as above.  Finally, it also
> adds a test for attribute fn spec indicating a function returns its
> argument.  It's not necessary for the fix (I had initially though
> placement new might have the attribute) but it seems appropriate
> to check.
>
> Tested on x86_64-linux.
>
> Martin
>
> gcc-100876.diff
>
> PR c++/100876 - -Wmismatched-new-delete should understand placement new when it's not inlined
>
> gcc/ChangeLog:
>
> 	PR c++/100876
> 	* builtins.c (gimple_call_return_array): Check for attribute fn spec.
> 	Handle calls to placement new.
> 	(ndecl_dealloc_argno): Avoid placement delete.
>
> gcc/testsuite/ChangeLog:
>
> 	PR c++/100876
> 	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
> 	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
> 	* g++.dg/warn/Wstringop-overflow-7.C: New test.
> 	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
> 	* g++.dg/analyzer/placement-new.C: Prune out expected warning.
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index af1fe49bb48..fb0717a0248 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -5159,11 +5159,43 @@ static tree
>   gimple_call_return_array (gimple *stmt, offset_int offrng[2],
>   			  range_query *rvals)
>   {
> -  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
> -      || gimple_call_num_args (stmt) < 1)
> +  {
> +    /* Check for attribute fn spec to see if the function returns one
> +       of its arguments.  */
> +    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
> +    unsigned int argno;
> +    if (fnspec.returns_arg (&argno))
> +      {
> +	offrng[0] = offrng[1] = 0;
> +	return gimple_call_arg (stmt, argno);
> +      }
> +  }
> +
> +  if (gimple_call_num_args (stmt) < 1)
>       return NULL_TREE;
Nit.  You've got an unnecessary {} at the outer level of this hunk.

OK with the nit fixed.

THanks,
Jeff

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Bernhard Reutner-Fischer]

On 14 June 2021 01:45:36 CEST, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
>
>On 6/2/2021 3:40 PM, Martin Sebor via Gcc-patches wrote:
>> The two forms of placement operator new defined in <new> return their
>> pointer argument and may not be displaced by user-defined functions.
>> But because they are ordinary (not built-in) functions this property
>> isn't reflected in their declarations alone, and there's no user-
>> level attribute to annotate them with.  When they are inlined
>> the property is transparent in the IL but when they are not (without
>> inlining such as -O0), calls to the operators appear in the IL and
>> cause -Wmismatched-new-delete to try to match them with the functions
>> called to deallocate memory.  When the pointer to the memory was
>> obtained from a function that matches the deallocator but not
>> the placement new, the warning falsely triggers.
>>
>> The attached patch solves this by detecting calls to placement new
>> and treating them the same as those to other pass-through calls (such
>> as memset).  In addition, it also teaches -Wfree-nonheap-object about
>> placement delete, for a similar reason as above.  Finally, it also
>> adds a test for attribute fn spec indicating a function returns its
>> argument.  It's not necessary for the fix (I had initially though
>> placement new might have the attribute) but it seems appropriate
>> to check.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> gcc-100876.diff
>>
>> PR c++/100876 - -Wmismatched-new-delete should understand placement
>new when it's not inlined
>>
>> gcc/ChangeLog:
>>
>> 	PR c++/100876
>> 	* builtins.c (gimple_call_return_array): Check for attribute fn
>spec.
>> 	Handle calls to placement new.
>> 	(ndecl_dealloc_argno): Avoid placement delete.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/100876
>> 	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
>> 	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
>> 	* g++.dg/warn/Wstringop-overflow-7.C: New test.
>> 	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
>> 	* g++.dg/analyzer/placement-new.C: Prune out expected warning.
>>
>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>> index af1fe49bb48..fb0717a0248 100644
>> --- a/gcc/builtins.c
>> +++ b/gcc/builtins.c
>> @@ -5159,11 +5159,43 @@ static tree
>>   gimple_call_return_array (gimple *stmt, offset_int offrng[2],
>>   			  range_query *rvals)
>>   {
>> -  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
>> -      || gimple_call_num_args (stmt) < 1)
>> +  {
>> +    /* Check for attribute fn spec to see if the function returns
>one
>> +       of its arguments.  */
>> +    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
>> +    unsigned int argno;
>> +    if (fnspec.returns_arg (&argno))
>> +      {
>> +	offrng[0] = offrng[1] = 0;
>> +	return gimple_call_arg (stmt, argno);
>> +      }
>> +  }
>> +
>> +  if (gimple_call_num_args (stmt) < 1)
>>       return NULL_TREE;
>Nit.  You've got an unnecessary {} at the outer level of this hunk.

if (unsigned int = 1 && fnspec.returns_arg (&argno))

doesn't look too appealing though, I'd leave the curly braces, no?

cheers,
>
>OK with the nit fixed.
>
>THanks,
>Jeff

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Bernhard Reutner-Fischer]

On 14 June 2021 07:56:38 CEST, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>On 14 June 2021 01:45:36 CEST, Jeff Law via Gcc-patches
><gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>On 6/2/2021 3:40 PM, Martin Sebor via Gcc-patches wrote:
>>> The two forms of placement operator new defined in <new> return
>their
>>> pointer argument and may not be displaced by user-defined functions.
>>> But because they are ordinary (not built-in) functions this property
>>> isn't reflected in their declarations alone, and there's no user-
>>> level attribute to annotate them with.  When they are inlined
>>> the property is transparent in the IL but when they are not (without
>>> inlining such as -O0), calls to the operators appear in the IL and
>>> cause -Wmismatched-new-delete to try to match them with the
>functions
>>> called to deallocate memory.  When the pointer to the memory was
>>> obtained from a function that matches the deallocator but not
>>> the placement new, the warning falsely triggers.
>>>
>>> The attached patch solves this by detecting calls to placement new
>>> and treating them the same as those to other pass-through calls
>(such
>>> as memset).  In addition, it also teaches -Wfree-nonheap-object
>about
>>> placement delete, for a similar reason as above.  Finally, it also
>>> adds a test for attribute fn spec indicating a function returns its
>>> argument.  It's not necessary for the fix (I had initially though
>>> placement new might have the attribute) but it seems appropriate
>>> to check.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>> gcc-100876.diff
>>>
>>> PR c++/100876 - -Wmismatched-new-delete should understand placement
>>new when it's not inlined
>>>
>>> gcc/ChangeLog:
>>>
>>> 	PR c++/100876
>>> 	* builtins.c (gimple_call_return_array): Check for attribute fn
>>spec.
>>> 	Handle calls to placement new.
>>> 	(ndecl_dealloc_argno): Avoid placement delete.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	PR c++/100876
>>> 	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
>>> 	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
>>> 	* g++.dg/warn/Wstringop-overflow-7.C: New test.
>>> 	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
>>> 	* g++.dg/analyzer/placement-new.C: Prune out expected warning.
>>>
>>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>>> index af1fe49bb48..fb0717a0248 100644
>>> --- a/gcc/builtins.c
>>> +++ b/gcc/builtins.c
>>> @@ -5159,11 +5159,43 @@ static tree
>>>   gimple_call_return_array (gimple *stmt, offset_int offrng[2],
>>>   			  range_query *rvals)
>>>   {
>>> -  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
>>> -      || gimple_call_num_args (stmt) < 1)

Err, the if was removed, so right.
thanks,

>>> +  {
>>> +    /* Check for attribute fn spec to see if the function returns
>>one
>>> +       of its arguments.  */
>>> +    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
>>> +    unsigned int argno;
>>> +    if (fnspec.returns_arg (&argno))
>>> +      {
>>> +	offrng[0] = offrng[1] = 0;
>>> +	return gimple_call_arg (stmt, argno);
>>> +      }
>>> +  }
>>> +
>>> +  if (gimple_call_num_args (stmt) < 1)
>>>       return NULL_TREE;
>>Nit.  You've got an unnecessary {} at the outer level of this hunk.
>
>if (unsigned int = 1 && fnspec.returns_arg (&argno))
>
>doesn't look too appealing though, I'd leave the curly braces, no?
>
>cheers,
>>
>>OK with the nit fixed.
>>
>>THanks,
>>Jeff

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor]

On 6/13/21 5:45 PM, Jeff Law wrote:
> 
> 
> On 6/2/2021 3:40 PM, Martin Sebor via Gcc-patches wrote:
>> The two forms of placement operator new defined in <new> return their
>> pointer argument and may not be displaced by user-defined functions.
>> But because they are ordinary (not built-in) functions this property
>> isn't reflected in their declarations alone, and there's no user-
>> level attribute to annotate them with.  When they are inlined
>> the property is transparent in the IL but when they are not (without
>> inlining such as -O0), calls to the operators appear in the IL and
>> cause -Wmismatched-new-delete to try to match them with the functions
>> called to deallocate memory.  When the pointer to the memory was
>> obtained from a function that matches the deallocator but not
>> the placement new, the warning falsely triggers.
>>
>> The attached patch solves this by detecting calls to placement new
>> and treating them the same as those to other pass-through calls (such
>> as memset).  In addition, it also teaches -Wfree-nonheap-object about
>> placement delete, for a similar reason as above.  Finally, it also
>> adds a test for attribute fn spec indicating a function returns its
>> argument.  It's not necessary for the fix (I had initially though
>> placement new might have the attribute) but it seems appropriate
>> to check.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> gcc-100876.diff
>>
>> PR c++/100876 - -Wmismatched-new-delete should understand placement new when it's not inlined
>>
>> gcc/ChangeLog:
>>
>> 	PR c++/100876
>> 	* builtins.c (gimple_call_return_array): Check for attribute fn spec.
>> 	Handle calls to placement new.
>> 	(ndecl_dealloc_argno): Avoid placement delete.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c++/100876
>> 	* g++.dg/warn/Wmismatched-new-delete-4.C: New test.
>> 	* g++.dg/warn/Wmismatched-new-delete-5.C: New test.
>> 	* g++.dg/warn/Wstringop-overflow-7.C: New test.
>> 	* g++.dg/warn/Wfree-nonheap-object-6.C: New test.
>> 	* g++.dg/analyzer/placement-new.C: Prune out expected warning.
>>
>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>> index af1fe49bb48..fb0717a0248 100644
>> --- a/gcc/builtins.c
>> +++ b/gcc/builtins.c
>> @@ -5159,11 +5159,43 @@ static tree
>>   gimple_call_return_array (gimple *stmt, offset_int offrng[2],
>>   			  range_query *rvals)
>>   {
>> -  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)
>> -      || gimple_call_num_args (stmt) < 1)
>> +  {
>> +    /* Check for attribute fn spec to see if the function returns one
>> +       of its arguments.  */
>> +    attr_fnspec fnspec = gimple_call_fnspec (as_a <gcall *>(stmt));
>> +    unsigned int argno;
>> +    if (fnspec.returns_arg (&argno))
>> +      {
>> +	offrng[0] = offrng[1] = 0;
>> +	return gimple_call_arg (stmt, argno);
>> +      }
>> +  }
>> +
>> +  if (gimple_call_num_args (stmt) < 1)
>>       return NULL_TREE;
> Nit.  You've got an unnecessary {} at the outer level of this hunk.
> 
> OK with the nit fixed.

That's intentional, to minimize the scope of of the two new local
variables.

Martin

> 
> THanks,
> Jeff
> 

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor]

On 6/2/21 3:46 PM, Marek Polacek wrote:
> On Wed, Jun 02, 2021 at 03:40:49PM -0600, Martin Sebor via Gcc-patches wrote:
>> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
>> +    {
>> +      /* See if this is a call to placement new.  */
>> +      if (!fn
>> +	  || !DECL_IS_OPERATOR_NEW_P (fn)
>> +	  || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
>> +	return NULL_TREE;
>> +
>> +      tree fname = DECL_ASSEMBLER_NAME (fn);
>> +      const char *name = IDENTIFIER_POINTER (fname);
>> +      if (strcmp (name, "_ZnwmPv")       // ordinary form
>> +	  && strcmp (name, "_ZnamPv"))   // array form
>> +	return NULL_TREE;
> 
> Not a review, but you can use id_equal here and simplify things.

Okay, done.

Thanks
Martin

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Christophe Lyon]

Hi,

On Tue, 15 Jun 2021 at 00:58, Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 6/2/21 3:46 PM, Marek Polacek wrote:
> > On Wed, Jun 02, 2021 at 03:40:49PM -0600, Martin Sebor via Gcc-patches wrote:
> >> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
> >> +    {
> >> +      /* See if this is a call to placement new.  */
> >> +      if (!fn
> >> +      || !DECL_IS_OPERATOR_NEW_P (fn)
> >> +      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
> >> +    return NULL_TREE;
> >> +
> >> +      tree fname = DECL_ASSEMBLER_NAME (fn);
> >> +      const char *name = IDENTIFIER_POINTER (fname);
> >> +      if (strcmp (name, "_ZnwmPv")       // ordinary form
> >> +      && strcmp (name, "_ZnamPv"))   // array form
> >> +    return NULL_TREE;
> >
> > Not a review, but you can use id_equal here and simplify things.
>
> Okay, done.
>

On arm the new test Wmismatched-new-delete-4.C fails:
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C: In function
'void nowarn_placement_new()':
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:16:8: warning:
'void free(void*)' called on pointer returned from a mismatched
allocation function [-Wmismatched-new-delete]
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:16:42: note:
returned from 'void* operator new(unsigned int, void*)'
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C: In function
'void nowarn_placement_array_new()':
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:21:8: warning:
'void free(void*)' called on pointer returned from a mismatched
allocation function [-Wmismatched-new-delete]
/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:21:8: note:
returned from 'void* operator new [](unsigned int, void*)'
FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
bogus messages, line 16)
FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
bogus messages, line 21)
FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
warnings, line 29)
FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
warnings, line 36)


and Wstringop-overflow-7.C:
FAIL: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98  (test for
warnings, line 23)
FAIL: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98  (test for
warnings, line 40)
PASS: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98 (test for excess errors)

(no warning emitted)

Can you  check?

Thanks,

Christophe

> Thanks
> Martin

Re: [PATCH] teach compute_objsize about placement new (PR 100876)

[Martin Sebor]

On 6/15/21 4:58 AM, Christophe Lyon wrote:
> Hi,
> 
> On Tue, 15 Jun 2021 at 00:58, Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 6/2/21 3:46 PM, Marek Polacek wrote:
>>> On Wed, Jun 02, 2021 at 03:40:49PM -0600, Martin Sebor via Gcc-patches wrote:
>>>> +  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
>>>> +    {
>>>> +      /* See if this is a call to placement new.  */
>>>> +      if (!fn
>>>> +      || !DECL_IS_OPERATOR_NEW_P (fn)
>>>> +      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
>>>> +    return NULL_TREE;
>>>> +
>>>> +      tree fname = DECL_ASSEMBLER_NAME (fn);
>>>> +      const char *name = IDENTIFIER_POINTER (fname);
>>>> +      if (strcmp (name, "_ZnwmPv")       // ordinary form
>>>> +      && strcmp (name, "_ZnamPv"))   // array form
>>>> +    return NULL_TREE;
>>>
>>> Not a review, but you can use id_equal here and simplify things.
>>
>> Okay, done.
>>
> 
> On arm the new test Wmismatched-new-delete-4.C fails:
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C: In function
> 'void nowarn_placement_new()':
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:16:8: warning:
> 'void free(void*)' called on pointer returned from a mismatched
> allocation function [-Wmismatched-new-delete]
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:16:42: note:
> returned from 'void* operator new(unsigned int, void*)'
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C: In function
> 'void nowarn_placement_array_new()':
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:21:8: warning:
> 'void free(void*)' called on pointer returned from a mismatched
> allocation function [-Wmismatched-new-delete]
> /gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-4.C:21:8: note:
> returned from 'void* operator new [](unsigned int, void*)'
> FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
> bogus messages, line 16)
> FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
> bogus messages, line 21)
> FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
> warnings, line 29)
> FAIL: g++.dg/warn/Wmismatched-new-delete-4.C  -std=gnu++98  (test for
> warnings, line 36)
> 
> 
> and Wstringop-overflow-7.C:
> FAIL: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98  (test for
> warnings, line 23)
> FAIL: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98  (test for
> warnings, line 40)
> PASS: g++.dg/warn/Wstringop-overflow-7.C  -std=gnu++98 (test for excess errors)
> 
> (no warning emitted)
> 
> Can you  check?

I've pushed r12-1490 to adjust the code to check the mangling of
both size_t representations: unsigned int and unsigned long.

Martin