Re: [PATCH][WIP] libstdc++: Make certain exceptions transaction_safe.

[Jonathan Wakely <jwakely at redhat dot com>]

Sorry for the delay finishing this review, some of the code kept
melting my brain ;-)


On 14/11/15 20:45 +0100, Torvald Riegel wrote:
> diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
> index 1b3184a..d902b03 100644
> --- a/libstdc++-v3/config/abi/pre/gnu.ver
> +++ b/libstdc++-v3/config/abi/pre/gnu.ver
> @@ -1876,6 +1876,12 @@ GLIBCXX_3.4.22 {
>     _ZNSt6thread6_StateD[012]Ev;
>     _ZNSt6thread15_M_start_threadESt10unique_ptrINS_6_StateESt14default_deleteIS1_EEPFvvE;
>
> +    # Support for the Transactional Memory TS (N4514)
> +    _ZGTtNSt11logic_errorC1EPKc;
> +    _ZGTtNSt11logic_errorC1ERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE;
> +    _ZGTtNKSt11logic_error4whatEv;
> +    _ZGTtNSt11logic_errorD1Ev;
> +
> } GLIBCXX_3.4.21;

This is OK but ...

> # Symbols in the support library (libsupc++) have their own tag.
> @@ -2107,6 +2113,12 @@ CXXABI_1.3.9 {
>     # operator delete[](void*, std::size_t)
>     _ZdaPv[jmy];
>
> +    # Support for the Transactional Memory TS (N4514)
> +    _ZGTtNKSt9exceptionD1Ev;
> +    _ZGTtNKSt9exception4whatEv;
> +    _ZGTtNKSt13bad_exceptionD1Ev;
> +    _ZGTtNKSt13bad_exception4whatEv;
> +
> } CXXABI_1.3.8;

That symbol version was already used in the gcc-5 release and so is
frozen, you'll need CXXABI_1.3.10 for these new symbols (similar to
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg00258.html so if
Catherine's already added that version you can just add them there).


> diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
> index 723feb1..0e66bb0 100644
> --- a/libstdc++-v3/include/bits/c++config
> +++ b/libstdc++-v3/include/bits/c++config
> @@ -481,6 +481,17 @@ namespace std
> # define _GLIBCXX_BEGIN_EXTERN_C extern "C" {
> # define _GLIBCXX_END_EXTERN_C }
>
> +// Conditionally enable annotations for the Transactional Memory TS on C++11.
> +#if __cplusplus >= 201103L && \
> +  _GLIBCXX_USE_CXX11_ABI && _GLIBCXX_USE_DUAL_ABI && \

It's possible we can make this work for the old ABI too, but this is
OK for now. The old ABI always uses COW strings, but that's what the
code you've written deals with anyway.

> +  defined(__cpp_transactional_memory) && __cpp_transactional_memory >= 201505L

The defined(__cpp_transactional_memory) check is redundant, isn't it?

Users aren't allowed to define it, so it will either be defined to an
integer value or undefined and evaluate to zero.

> +#define _GLIBCXX_TXN_SAFE transaction_safe
> +#define _GLIBCXX_TXN_SAFE_DYN transaction_safe_dynamic
> +#else
> +#define _GLIBCXX_TXN_SAFE
> +#define _GLIBCXX_TXN_SAFE_DYN
> +#endif
> +
> #else // !__cplusplus
> # define _GLIBCXX_BEGIN_EXTERN_C
> # define _GLIBCXX_END_EXTERN_C


> @@ -44,7 +46,36 @@ std::exception::what() const _GLIBCXX_USE_NOEXCEPT
> }
>
> const char* 
> -std::bad_exception::what() const _GLIBCXX_USE_NOEXCEPT
> +std::bad_exception::what() const _GLIBCXX_TXN_SAFE_DYN _GLIBCXX_USE_NOEXCEPT
> {
>   return "std::bad_exception";
> }
> +
> +// Transactional clones for the destructors and what().
> +// what() is effectively transaction_pure, but we do not want to annotate it
> +// as such; thus, we call exactly the respective nontransactional function.
> +extern "C" {
> +
> +void
> +_ZGTtNKSt9exceptionD1Ev(const std::exception*)
> +{ }
> +
> +const char*
> +_ZGTtNKSt9exception4whatEv(const std::exception* that)
> +{
> +  return that->std::exception::what();
> +}

This makes a non-virtual call, is that correct?

If users derive from std::exception and override what() they will
expect a call to what() to dispatch to their override in the derived
class, but IIUC in a transactional block they would call this
function, which would call the base what(), not their override.

> +_ZGTtNKSt13bad_exception4whatEv(
> +    const std::bad_exception* that)
> +{
> +  return that->std::bad_exception::what();
> +}

Ditto.

> @@ -151,3 +164,220 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> _GLIBCXX_END_NAMESPACE_VERSION
> } // namespace
> +
> +// Support for the Transactional Memory TS (N4514).
> +//
> +// logic_error and runtime_error both carry a message in the form of a COW
> +// string.  This COW string is never made visible to users of the exception
> +// because what() returns a C string.  The COW string can be constructed as
> +// either a copy of a COW string of another logic_error/runtime_error, or
> +// using a C string or SSO string; thus, the COW string's _Rep is only
> +// accessed by logic_error operations.  We control all txnal clones of those
> +// operations and thus can ensure that _Rep is never accessed transactionally.
> +// Furthermore, _Rep will always have been allocated or deallocated via
> +// global new or delete, so nontransactional writes we do to _Rep cannot

Hmm, will it always be global new/delete? It uses std::allocator,
which by default uses new/delete but libstdc++ can be configured to
use a different std::allocator implementation. If they always use new
at some point maybe we're OK, but I'd have to check the alternative
allocators. Maybe we just say only new_allocator is supported for TM.

I assume we want to avoid making a txnal std::allocator.

> +extern "C" {
> +
> +void
> +_ZGTtNSt11logic_errorC1EPKc (std::logic_error *that, const char* s)
> +{
> +  // This will use the singleton _Rep for an empty string and just point
> +  // to it instead of allocating memory.  Thus, we can use it as source, copy
> +  // it into the object we are constructing, and then construct the COW string
> +  // in the latter manually.
> +  std::logic_error le("");
> +  _ITM_memcpyRnWt(that, &le, sizeof(std::logic_error));
> +  _txnal_cow_string_C1_for_exceptions(_txnal_logic_error_get_msg(that),
> +				      s, that);

The shared empty _Rep is also only used conditionally, it can be
disabled with --enable-fully-dynamic-string. Maybe another thing we
just don't deal with for now.

Otherwise this looks good to me.