Re: [PATCH] PR libstdc++/88782 avoid ODR problems in std::make_shared

[Jonathan Wakely <jwakely at redhat dot com>]

On 21/01/19 09:13 +0100, Richard Biener wrote:
> On Fri, Jan 18, 2019 at 10:29 PM Jonathan Wakely <jwakely@redhat.com> wrote:
> > The old version of _Sp_counted_ptr_inplace::_M_get_deleter (up to GCC
> > 8.2.0) expects to be passed a real std::typeinfo object, so mixing that
> > with the new definition of the __shared_ptr constructor (which always
> > passes the fake tag) leads to accessing the fake object as a real
> > std::typeinfo. Instead of trying to make it safe to mix the old and new
> > definitions, just stop using that function. By passing a reference to
> > __shared_ptr::_M_ptr to the __shared_count constructor it can be set
> > directly, without needing to obtain the pointer via the _M_get_deleter
> > back-channel. This avoids a virtual dispatch (which fixes PR 87514).
> >
> > This means that code built against new libstdc++ headers doesn't use
> > _M_get_deleter at all, and so make_shared works the same whether RTTI is
> > enabled or not.
> >
> > Also change _M_get_deleter so that it checks for a real type_info object
> > even when RTTI is disabled, by calling a library function. Unless
> > libstdc++ itself is built without RTTI that library function will be
> > able to test if it's the right type_info. This means the new definition
> > of _M_get_deleter can handle both the fake type_info tag and a real
> > type_info object, even if built without RTTI.
> >
> > If linking to objects built against older versions of libstdc++ then if
> > all objects use -frtti or all use -fno-rtti, then the caller of
> > _M_get_deleter and the definition of _M_get_deleter will be consistent
> > and it will work. If mixing -frtti with -fno-rtti it can still fail if
> > the linker picks an old definition of _M_get_deleter and an old
> > __shared_ptr constructor that are incompatible. In that some or all
> > objects might need to be recompiled.
>
> Can you try summarizing whatever caveats result from this in
> the 8.3/9 changes.html parts?  I have a hard time extracting that
> myself from the above ;)  (small example what kind of simplest code might
> run into this helps)

There are *fewer* caveats compared to previous releases.

There's an example in PR 87520 showing problems mixing -frtti and
-fno-rtti code, although it's a contrived example that uses explicit
instantiation of shared_ptr internals to demonstrate the problem. The
real issue reported privately to me is harder to reproduce, and will
be even harder with this fix committed. I'll try to document this
potential problem mixing -frtti and -fno-rtti (there are still others,
e.g. PR 43105 lists one).

The problems described in PR 88782 only happen if you use 8.2.1
snapshots post-20181122 and mix code compiled by that snapshot with
code compiled by earlier releases. If you only use 8.3.0 (or 9.1.0)
and no snapshots post-20181122 then, the problem can't happen. I don't
think we need to document transient issues that only existed for two
months with snapshots.

> Richard.
>
> >         PR libstdc++/87514
> >         PR libstdc++/87520
> >         PR libstdc++/88782
> >         * config/abi/pre/gnu.ver (GLIBCXX_3.4.26): Export new symbol.
> >         * include/bits/shared_ptr.h
> >         (shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...))
> >         (allocate_shared): Change to use new tag type.
> >         * include/bits/shared_ptr_base.h (_Sp_make_shared_tag::_S_eq):
> >         Declare new member function.
> >         (_Sp_alloc_shared_tag): Define new type.
> >         (_Sp_counted_ptr_inplace): Declare __shared_count<_Lp> as a friend.
> >         (_Sp_counted_ptr_inplace::_M_get_deleter) [!__cpp_rtti]: Use
> >         _Sp_make_shared_tag::_S_eq to check type_info.
> >         (__shared_count(Ptr, Deleter),__shared_count(Ptr, Deleter, Alloc)):
> >         Constrain to prevent being called with _Sp_alloc_shared_tag.
> >         (__shared_count(_Sp_make_shared_tag, const _Alloc&, Args&&...)):
> >         Replace constructor with ...
> >         (__shared_count(Tp*&, _Sp_alloc_shared_tag<_Alloc>, Args&&...)): Use
> >         reference parameter so address of the new object can be returned to
> >         the caller. Obtain the allocator from the tag type.
> >         (__shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...)): Replace
> >         constructor with ...
> >         (__shared_ptr(_Sp_alloc_shared_tag<Alloc>, Args&&...)): Pass _M_ptr
> >         to the __shared_count constructor.
> >         (__allocate_shared): Change to use new tag type.
> >         * src/c++11/shared_ptr.cc (_Sp_make_shared_tag::_S_eq): Define.
> >
> > Tested powerpc64le-linux, committed to trunk.
> >
> > I'll backport this to gcc-8-branch without the new symbol in the library.