This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix inconsistent operator delete usages


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.


      }
  }



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]