Fix inconsistent operator delete usages

[Jonathan Wakely]

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.


>       }
>   }
>