This is the mail archive of the
mailing list for the libstdc++ project.
Re: [PATCH] PR libstdc++/88782 avoid ODR problems in std::make_shared
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jonathan Wakely <jwakely at redhat dot com>
- Cc: "libstdc++" <libstdc++ at gcc dot gnu dot org>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 21 Jan 2019 12:16:25 +0100
- Subject: Re: [PATCH] PR libstdc++/88782 avoid ODR problems in std::make_shared
- References: <20190118212843.GA7905@redhat.com> <CAFiYyc0+Acx4mO_Z0KGXPgwA5DUNjnp3khdt_JK0y=_QaAiv2g@mail.gmail.com> <20190121110759.GT15627@redhat.com>
On Mon, Jan 21, 2019 at 12:08 PM Jonathan Wakely <firstname.lastname@example.org> wrote:
> On 21/01/19 09:13 +0100, Richard Biener wrote:
> >On Fri, Jan 18, 2019 at 10:29 PM Jonathan Wakely <email@example.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.
OK, I had the impression that with the fix in there's still more cases
that have issues than before whatever rev. triggered the original issue
on the GCC 8 branch. If that's not so then I'm fine.
I wasn't aware of any -f[no-]rtti mixing issues at all so documenting
those might still be nice.
> >> 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.