Fix inconsistent operator delete usages

Message ID b8552210-03aa-2f5b-6acb-444f592c1f9c@gmail.com
State New
Headers show
Series
  • Fix inconsistent operator delete usages
Related show

Commit Message

François Dumont Jan. 31, 2019, 9:29 p.m.
I was writing a test which needed to override the std::nothrow 
versions of the operator new and delete to control get_temporary_buffer 
behavior and noticed that it is inconsistent with 
release_temporary_buffer in terms of new/delete operators.

     Grepping for other std::nothrow usages I found some others and 
especially one in libsupc++.

     I don't know how serious it is considering the Standard. As long as 
you stick to the libstdc++ operators it is fine. Only users overriding 
those operators will notice.

     Tested under Linux x86_64 normal mode with some failures but none 
related to this patch I think but of course you better check on your side.

     * libsupc++/atexit_thread.cc (run(void*)): Call std::nothrow delete
     operator.
     * include/bits/stl_tempbuf.h (return_temporary_buffer): Likewise.
     * include/profile/impl/profiler_trace.h
     (__trace_base<>::~__trace_base()): Likewise.
     (__trace_base<>::__add_object(__stack_t)): Likewise.
     (__trace_base<>::__retire_object(__stack_t)): Likewise.

Let me know if it is a go.

François

Comments

Jonathan Wakely Feb. 1, 2019, 1:34 p.m. | #1
On 31/01/19 22:29 +0100, François Dumont wrote:
>    I was writing a test which needed to override the std::nothrow 

>versions of the operator new and delete to control 

>get_temporary_buffer behavior and noticed that it is inconsistent with 

>release_temporary_buffer in terms of new/delete operators.

>

>    Grepping for other std::nothrow usages I found some others and 

>especially one in libsupc++.

>

>    I don't know how serious it is considering the Standard. As long 

>as you stick to the libstdc++ operators it is fine. Only users 

>overriding those operators will notice.

>

>    Tested under Linux x86_64 normal mode with some failures but none 

>related to this patch I think but of course you better check on your 

>side.

>

>    * libsupc++/atexit_thread.cc (run(void*)): Call std::nothrow delete

>    operator.

>    * include/bits/stl_tempbuf.h (return_temporary_buffer): Likewise.

>    * include/profile/impl/profiler_trace.h

>    (__trace_base<>::~__trace_base()): Likewise.

>    (__trace_base<>::__add_object(__stack_t)): Likewise.

>    (__trace_base<>::__retire_object(__stack_t)): Likewise.

>

>Let me know if it is a go.


Nope.

>François

>


>diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h b/libstdc++-v3/include/bits/stl_tempbuf.h

>index b6ad9ee6a46..e614a77bc4f 100644

>--- a/libstdc++-v3/include/bits/stl_tempbuf.h

>+++ b/libstdc++-v3/include/bits/stl_tempbuf.h

>@@ -110,7 +110,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

>   template<typename _Tp>

>     inline void

>     return_temporary_buffer(_Tp* __p)

>-    { ::operator delete(__p); }

>+    { ::operator delete(__p, std::nothrow); }


This change is harmless, but unnecessary.

The standard requires that the nothrow versions of operator new must
obtain memory from the same source as the normal version of operator
new (even if the user has replaced one or both versions of operator
new). That means you can always use the normal version of operator
delete instead of the nothrow one.

If your tests failed because of this, then your replacement versions
of operator new and operator delete were wrong.

See [new.delete.single] p7.

> 

>   /**

>diff --git a/libstdc++-v3/include/profile/impl/profiler_trace.h b/libstdc++-v3/include/profile/impl/profiler_trace.h

>index 261f3b3cc72..36822ef77ac 100644

>--- a/libstdc++-v3/include/profile/impl/profiler_trace.h

>+++ b/libstdc++-v3/include/profile/impl/profiler_trace.h

>@@ -200,7 +200,7 @@ namespace __gnu_profile

>       {

> 	for (typename __stack_table_t::iterator __it

> 	       = __stack_table.begin(); __it != __stack_table.end(); ++__it)

>-	  delete __it->first;

>+	  ::operator delete(__it->first, std::nothrow);


This introduces a bug. The previous code called the destructor before
deallocating the memory, after your change it would not run the
destructor. This is definitely not OK.

__it->__first is a pointer to a std::vector, so it's destructor
definitely must be run.

You're making the code *less* consistent, by changing it from
new/delete to new/operator delete.

Also Profile Mode was deprecated in gcc-7 and I think instead of
maintaining the code we should just remove it early in stage 1 for
gcc-10.

>diff --git a/libstdc++-v3/libsupc++/atexit_thread.cc b/libstdc++-v3/libsupc++/atexit_thread.cc

>index 25334250dab..d47d1654b28 100644

>--- a/libstdc++-v3/libsupc++/atexit_thread.cc

>+++ b/libstdc++-v3/libsupc++/atexit_thread.cc

>@@ -79,7 +79,7 @@ namespace {

> 	  FreeLibrary (e->dll);

> #endif

> 	e = e->next;

>-	delete (old_e);

>+	::operator delete(old_e, std::nothrow);


This type has a trivial destructor, so this doesn't actually cause a
change in behaviour, but it's still making it inconsistent.


>       }

>   }

>

Patch

diff --git a/libstdc++-v3/include/bits/stl_tempbuf.h b/libstdc++-v3/include/bits/stl_tempbuf.h
index b6ad9ee6a46..e614a77bc4f 100644
--- a/libstdc++-v3/include/bits/stl_tempbuf.h
+++ b/libstdc++-v3/include/bits/stl_tempbuf.h
@@ -110,7 +110,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp>
     inline void
     return_temporary_buffer(_Tp* __p)
-    { ::operator delete(__p); }
+    { ::operator delete(__p, std::nothrow); }
 
 
   /**
diff --git a/libstdc++-v3/include/profile/impl/profiler_trace.h b/libstdc++-v3/include/profile/impl/profiler_trace.h
index 261f3b3cc72..36822ef77ac 100644
--- a/libstdc++-v3/include/profile/impl/profiler_trace.h
+++ b/libstdc++-v3/include/profile/impl/profiler_trace.h
@@ -200,7 +200,7 @@  namespace __gnu_profile
       {
 	for (typename __stack_table_t::iterator __it
 	       = __stack_table.begin(); __it != __stack_table.end(); ++__it)
-	  delete __it->first;
+	  ::operator delete(__it->first, std::nothrow);
       }
 
       __object_info* __add_object(__stack_t __stack);
@@ -235,14 +235,14 @@  namespace __gnu_profile
 
       if (__max_mem() != 0 && __objects_byte_size >= __max_mem())
 	{
-	  delete __stack;
+	  ::operator delete(__stack, std::nothrow);
 	  return 0;
 	}
 
       __object_info* __ret = new(std::nothrow) __object_info(__stack);
       if (!__ret)
 	{
-	  delete __stack;
+	  ::operator delete(__stack, std::nothrow);
 	  return 0;
 	}
 
@@ -277,16 +277,16 @@  namespace __gnu_profile
 					     __stack_info(__info)));
 	    }
 	  else
-	    delete __stack;
+	    ::operator delete(__stack, std::nothrow);
 	}
       else
 	{
 	  // Merge object info into info summary for this call context.
 	  __stack_it->second.__merge(__info);
-	  delete __stack;
+	  ::operator delete(__stack, std::nothrow);
 	}
 
-      delete __obj_info;
+      ::operator delete(__obj_info, std::nothrow);
       __objects_byte_size -= sizeof(__object_info);
     }
 
diff --git a/libstdc++-v3/libsupc++/atexit_thread.cc b/libstdc++-v3/libsupc++/atexit_thread.cc
index 25334250dab..d47d1654b28 100644
--- a/libstdc++-v3/libsupc++/atexit_thread.cc
+++ b/libstdc++-v3/libsupc++/atexit_thread.cc
@@ -79,7 +79,7 @@  namespace {
 	  FreeLibrary (e->dll);
 #endif
 	e = e->next;
-	delete (old_e);
+	::operator delete(old_e, std::nothrow);
       }
   }