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

[Jonathan Wakely <jwakely at redhat dot com>]

On 23/12/15 18:35 +0100, Torvald Riegel wrote:
> > 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.
>
> OK.  I would have to write the wrappers for the new strings too then,
> and I wanted to avoid that for now.

Really? When the dual ABI is disabled there are no new strings, and
when dual ABI is enabled but you're compiling with the old ABI new
strings shouldn't be involved here either. Maybe I'm missing
something.

> > >+  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.
>
> Won't we get an undefined warning when using -Wundef if we remove the
> first part?  That's what I wanted to avoid.

Only when -Wsystem-headers is also used. We don't care about -Wundef
in libstdc++ (I don't care about it anyway, we already have loads of
such uses of macros and nobody has ever complained).

> > 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.
>
> Yes.  I added this comment to _ZGTtNKSt9exception4whatEv and also
> referenced it from a comment in _ZGTtNKSt13bad_exception4whatEv.
>
>  // We really want the non-virtual call here.  We already executed the
>  // indirect call representing the virtual call, and the TM runtime or the
>  // compiler resolved it to this transactional clone.  In the clone, we want
>  // to do the same as for the nontransactional original, so we just call it.

Gotcha, I was misunderstanding when we ended up calling the clone

> > 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 wasn't aware that a different std::allocator can be hooked in at
> compile time.  Can we check for this (I assume there must be some build
> flag for the new allocator?), and simply disable support for the TM TS
> if this is the case?

The build-time configuration is done by GLIBCXX_ENABLE_ALLOCATOR in
libstdc++-v3/acinclude.m4 but it doesn't currently set anything that
can be used to test whether the chosen config is new_allocator or not.

> An alternative would be to call the transactional clones of the
> std::allocator allocate/deallocate functions -- but even if we did that,
> and created txnal clones for those of new_allocator so that we
> eventually reach the txnal global new/delete functions that I'm calling
> directly in this patch, we'd still need to check whether other
> allocators provide them.
>
> What would you like to see?  Or should we just document that limitation
> somewhere for now?

I'm OK with only providing TM support for the default configuration.

We might need to add something to acinclude.m4 that says we're using
new_allocator and therefore it's OK to enable things that rely on it.

> > I assume we want to avoid making a txnal std::allocator.
>
> I'm not quite sure what you mean, but the TM support should work with
> the default std::allocator at least.
>
> > >+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.
>
> What would you like to see?

Disable TM support if _GLIBCXX_FULLY_DYNAMIC_STRING is defined to a
non-zero value.

> Also, can you give me some guidance on the remaining FIXME's in this
> patch, please?  In particular, these:
>
> +// FIXME copy over libitm's configury for MANGLE_SIZE_T?
> Is there some way to get this from libstdc++ already, or how do you
> prefer we handle this?
>
> +// If there is no support for weak, create dummies.
> +// FIXME really needed for libstdc++?
> +//#if !defined (HAVE_ELF_STYLE_WEAKREF)
> Can I assume weak refs to be supported, or how do I check for whether
> they are?  What's your preference?

Again, I'm OK with TM being unsupported where it's painful to do.

> +  // FIXME make a true compile-time choice to prevent warnings.
> +  if (sizeof(uint64_t)== sizeof(void*))
> +    return (void*)_ITM_RU8((const uint64_t*)ptr);
> +  else
> +    return (void*)_ITM_RU4((const uint32_t*)ptr);

These MAX values are pre-defined by the compiler:

#if __UINTPTR_MAX__ == __UINT64_MAX__

> I've attached a patch with changes for V2 of this (for ease of review,
> this is on top of the patch I sent).  This now makes the other
> exceptions besides logic_error transaction-safe too.  Please have a
> look.

I won't be able to review this properly until the new year.