[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-dev
Subject:    Re: [cfe-dev] [LibC++] Bug in implementation of 'std::shared_ptr'
From:       "Martin J. O'Riordan via cfe-dev" <cfe-dev () lists ! llvm ! org>
Date:       2017-04-30 19:32:23
Message-ID: 005f01d2c1e8$7f457ec0$7dd07c40$ () movidius ! com
[Download RAW message or body]

Hi Anton,

Sorry, I did 'Reply' instead of 'Reply All'

Actually, I should've added that the '__shared_ptr_pointer::__get_deleter' is also \
similarly affected, but is not qualified by '_LIBCPP_BUILD_STATIC', and that the same \
is true for the 'functional implementation although I have no test cases yet which \
illustrate this as a problem.

Thanks,

	MartinO

-----Original Message-----
From: Martin J. O'Riordan [mailto:martin.oriordan@movidius.com] 
Sent: 30 April 2017 20:22
To: 'Anton Korobeynikov' <anton@korobeynikov.info>
Subject: RE: [cfe-dev] [LibC++] Bug in implementation of 'std::shared_ptr'

Exactly.  When building the LibC++ library, '_LIBCPP_BUILD_STATIC' is defined.

But this means that the ABI for the 'vtable' is now a function of RTTI enabled vs \
disabled.

	MartinO

-----Original Message-----
From: Anton Korobeynikov [mailto:anton@korobeynikov.info]
Sent: 30 April 2017 18:39
To: Martin J. O'Riordan <martin.oriordan@movidius.com>
Cc: Clang Dev <cfe-dev@lists.llvm.org>
Subject: Re: [cfe-dev] [LibC++] Bug in implementation of 'std::shared_ptr'

Martin,

How the '_LIBCPP_BUILD_STATIC' define got set in your case? It should only appear \
when building libc++ itself.

On Thu, Apr 27, 2017 at 6:27 PM, Martin J. O'Riordan via cfe-dev \
<cfe-dev@lists.llvm.org> wrote:
> I have a test that has been failing for a long time, and finally got 
> around to investigating it.  The test is really simple:
> 
> 
> 
> #include <iostream>
> 
> #include <memory>
> 
> 
> 
> struct Test {
> 
> int n;
> 
> Test(int x) : n(x) { std::cerr << "Creating " << n << std::endl; }
> 
> ~Test() { std::cerr << "Deleting " << n << std::endl; }
> 
> };
> 
> 
> 
> int main () {
> 
> std::shared_ptr<Test> x(std::make_shared<Test>(42));
> 
> x.reset();
> 
> std::cerr << "Completed " << std::endl;
> 
> }
> 
> 
> 
> but it crashes immediately after printing the message from the 
> destructor, and before the message in ‘main'.
> 
> 
> 
> After investigating, I found that the test works perfectly with RTTI 
> enabled, but crashes if it is disabled which puzzled me.  My LibC++ 
> library is built with RTTI enabled as instructed on ‘libcxx.llvm.org'
> which says that the library must be built with RTTI enabled, though it 
> may be used with RTTI disabled.
> 
> 
> 
> What I found was that the ‘vtable' for the ‘shared_ptr' specialisation 
> is different depending on whether RTTI is enabled or disabled.  With 
> RTTI disabled it is:
> 
> 
> 
> _ZTVNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEEE:
> 
> .word      0
> 
> .word      0
> 
> .word      _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEED1Ev
> 
> .word      _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEED0Ev
> 
> .word
> _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEE16__on_zero_s
> haredEv
> 
> .word
> _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEE21__on_zero_s
> hared_weakEv
> 
> 
> 
> but with RTTI enabled it is:
> 
> 
> 
> _ZTVNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEEE:
> 
> .word      0
> 
> .word      _ZTINSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEEE
> 
> .word      _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEED1Ev
> 
> .word      _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEED0Ev
> 
> .word
> _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEE16__on_zero_s
> haredEv
> 
> .word      _ZNKSt3__119__shared_weak_count13__get_deleterERKSt9type_info
> 
> .word
> _ZNSt3__120__shared_ptr_emplaceI4TestNS_9allocatorIS1_EEE21__on_zero_s
> hared_weakEv
> 
> 
> 
> In the library, the implementation (in ‘memory.cpp' compiler with
> ‘-frtti') is attempting to call the function ‘__on_zero_shared_weak', 
> but using offset 24, and the ‘vtable' emitted in the test case 
> (compiled with ‘-fno-rtti') has this function at offset 20.
> 
> 
> 
> This is caused by the following lines in ‘<memory>':
> 
> 
> 
> // Define the function out only if we build static libc++ without RTTI.
> 
> // Otherwise we may break clients who need to compile their 
> projects with
> 
> // -fno-rtti and yet link against a libc++.dylib compiled
> 
> // without -fno-rtti.
> 
> #if !defined(_LIBCPP_NO_RTTI) || !defined(_LIBCPP_BUILD_STATIC)
> 
> virtual const void* __get_deleter(const type_info&) const 
> _NOEXCEPT;
> 
> #endif
> 
> 
> 
> and because the function is virtual, the layout of the ‘vtable' is 
> different between RTTI enabled and disabled (we are building a static 
> library, so ‘_LIBCPP_BUILD_STATIC' is true).
> 
> 
> 
> There are a couple of places in ‘<memory>' and ‘memory.cpp' where this 
> happens (always with ‘__get_deleter'), but after sanity checking the 
> other headers, I see that ‘<functional>' and ‘<__functional_03>' also 
> have similar issues where the layout of the ‘vtable' is different 
> depending on whether RTTI is enabled or not; though I don't have any tests which \
> show this. 
> 
> 
> I don't know what the best fix is for this because it was clearly 
> introduced to address some issue with dynamic libraries, but locally I 
> have decided to always define ‘__get_deleter', and make its implementation return \
> ‘nullptr' when RTTI is disabled.  This preserves the ‘vtable' layout \
> independent  of RTTI.
> 
> 
> 
> Thanks,
> 
> 
> 
> MartinO
> 
> 
> 
> 
> _______________________________________________
> cfe-dev mailing list
> cfe-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> 



--
With best regards, Anton Korobeynikov
Department of Statistical Modelling, Saint Petersburg State University

_______________________________________________
cfe-dev mailing list
cfe-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


[prev in list] [next in list] [prev in thread] [next in thread] 

Configure | About | News | Add a list | Sponsored by KoreLogic