Remove std::shared_ptr::make_shared and std::shared_ptr::allocate_shared

[zoecarver]

Summary

This patch removes std::shared_ptr::make_shared and std::shared_ptr::allocate_shared as they are not part of the standard. This is a change originally created in D62233. This patch also adds __create_with_cntrl_block, which is a help function for std::allocate_shared and std::make_shared.

[ldionne]

I checked that the variadic versions of std::allocate_shared and std::make_shared were good. If you did the same mechanical transformation for the non-variadic ones (which it looks like you did), this LGTM.

The next step would be to remove the C++03 non-variadic versions of std::make_shared and std::allocate_shared.

[ldionne]

Please make sure this passes tests in C++03, C++11, C++14, and C++17.

[zoecarver]

Tests pass in all versions. Committed as rL368885.

[phosek]

I started seeing segfaults in some of our tests after the latests Clang roll and bisecting narrowed it down to this change. I also tried reverting this change locally and the segfaults no longer occur so I'm pretty confident it's this change. What's confusing is that it's not happening in regular Clang builds, it's only affecting GCC builds (we're using GCC 8.3) that are using libc++ and ASan builds. It's also don't see any of the modified functions on the stack which makes it even more confusing but I'm suspecting memory corruption that's happens earlier.

Here's one of the symbolized logs:

[230.404] 01385.01538> <== fatal exception: process /boot/test/sys/fit-test[563923] thread initial-thread[563937]
[230.404] 01385.01538> <== read not-present page fault, PC at 0x74f079c4d197
[230.404] 01385.01538>  CS:                   0 RIP:     0x74f079c4d197 EFL:            0x10202 CR2:               0x20
[230.404] 01385.01538>  RAX:               0x20 RBX:     0x74f079cff2e0 RCX:     0x3c5ec54ec8f0 RDX:                  0
[230.404] 01385.01538>  RSI:     0x324048b387c0 RDI:               0x30 RBP:     0x3c5ec54ecdb0 RSP:     0x3c5ec54ec7e8
[230.404] 01385.01538>   R8:                  0  R9: 0xfffff00000ffffff R10:     0x24f17e394991 R11:     0x324048b38780
[230.404] 01385.01538>  R12:     0x696b9aa4a040 R13:     0x3c5ec54ec870 R14:     0x74f079d00ba0 R15:     0x3c5ec54ec900
[230.404] 01385.01538>  fs.base:     0x36bebfd9cb38 gs.base:                  0
[230.404] 01385.01538>  errc:               0x4
[230.404] 01385.01538> bottom of user stack:
[230.404] 01385.01538> 0x00003c5ec54ec7e8: 79c71b5e 000074f0 c54ecbe0 00003c5e |^..y.t....N.^<..|
[230.405] 01385.01538> 0x00003c5ec54ec7f8: c54ec810 00003c5e c54ec8d0 00003c5e |..N.^<....N.^<..|
[230.405] 01385.01538> 0x00003c5ec54ec808: c54ec8b0 00003c5e 20202020 20202020 |..N.^<..        |
[230.405] 01385.01538> 0x00003c5ec54ec818: 20202020 20202020 20202020 20202020 |                |
[230.405] 01385.01538> 0x00003c5ec54ec828: 20202020 20202020 20202020 20202020 |                |
[230.405] 01385.01538> 0x00003c5ec54ec838: 20202020 00000000 00000000 00000000 |    ............|
[230.405] 01385.01538> 0x00003c5ec54ec848: c54ec910 00003c5e c54ec890 00003c5e |..N.^<....N.^<..|
[230.405] 01385.01538> 0x00003c5ec54ec858: c54ec8f0 00003c5e 00000001 00000000 |..N.^<..........|
[230.405] 01385.01538> 0x00003c5ec54ec868: 00000000 00000000 79cff2e0 000074f0 |...........y.t..|
[230.405] 01385.01538> 0x00003c5ec54ec878: 00000000 00000000 00000000 00000000 |................|
[230.405] 01385.01538> 0x00003c5ec54ec888: 00000000 00000000 79cff2e0 000074f0 |...........y.t..|
[230.405] 01385.01538> 0x00003c5ec54ec898: 00000000 00000000 00000000 00000000 |................|
[230.405] 01385.01538> 0x00003c5ec54ec8a8: 00000000 00000000 79cff2e0 000074f0 |...........y.t..|
[230.405] 01385.01538> 0x00003c5ec54ec8b8: 00000000 00000000 00000000 00000000 |................|
[230.405] 01385.01538> 0x00003c5ec54ec8c8: 00000000 00000000 79cff2e0 000074f0 |...........y.t..|
[230.405] 01385.01538> 0x00003c5ec54ec8d8: 00000000 00000000 00000000 00000000 |................|
[230.405] 01385.01538> arch: x86_64
[230.405] 01385.01538> dso: id=c2d8143ea2273b1ffed2fb43f0047a8dcc93f14a base=0x74f079bfd000 name=app:/boot/test/sys/fit-test
[230.405] 01385.01538> dso: id=917cbd24458d8ac1421a04344846e76fcd0bf4c4 base=0x6d7b61144000 name=<vDSO>
[230.405] 01385.01538> dso: id=aa81cd39e8cca017198bdb628a513a94e296acae base=0x696b9aa43000 name=libunittest.so
[230.405] 01385.01538> dso: id=8a80a62e95a4ced9ca0480796662a90104c505cd base=0x5e934f165000 name=libfdio.so
[230.405] 01385.01538> dso: id=09aee3d7f0e1245e8a304de42f0f45cbac1a25ee base=0x4437b334a000 name=libc.so
[230.405] 01385.01538> dso: id=0bb8728bb0bbaa3b base=0x2ab900e61000 name=libc++abi.so.1
[230.405] 01385.01538> dso: id=b216543249a796e8 base=0x292be29b5000 name=libc++.so.2
[230.405] 01385.01538> dso: id=6635fc5124abcdd4 base=0x1b1d07b59000 name=libunwind.so.1
[230.406] 01385.01538>    #1.1  0x000074f079c4d197 in fit::function_impl<16ul, false, void ()>* fit::internal::function_base<16ul, false, void ()>::target<fit::function_impl<16ul, false, void ()> >(bool) ../../out/default.zircon/../../zircon/system/ulib/fit/include/lib/fit/function_internal.h:227 <<VMO#563915=/boot/test/sys/fit->+0x50197 sp 0x3c5ec54ec7e8
[230.406] 01385.01538>    #1    0x000074f079c4d197 in fit::internal::target<fit::function_impl<16ul, false, void ()>, false, true, void>::get(void*) ../../out/default.zircon/../../zircon/system/ulib/fit/include/lib/fit/function_internal.h:153 <<VMO#563915=/boot/test/sys/fit->+0x50197 sp 0x3c5ec54ec7e8
[230.406] 01385.01538>    #2.2  0x000074f079c71b5e in void fit::internal::function_base<16ul, false, void ()>::share_with<fit::function_impl<16ul, false, void ()> >(fit::function_impl<16ul, false, void ()>&) ../../out/default.zircon/../../zircon/system/ulib/fit/include/lib/fit/function_internal.h:263 <<VMO#563915=/boot/test/sys/fit->+0x74b5e sp 0x3c5ec54ec7f0
[230.406] 01385.01538>    #2.1  0x000074f079c71b5e in fit::function_impl<16ul, false, void ()>::share() ../../out/default.zircon/../../zircon/system/ulib/fit/include/lib/fit/function.h:283 <<VMO#563915=/boot/test/sys/fit->+0x74b5e sp 0x3c5ec54ec7f0
[230.406] 01385.01538>    #2    0x000074f079c71b5e in (anonymous namespace)::sharing() ../../out/default.zircon/../../zircon/system/utest/fit/function_tests.cc:617 <<VMO#563915=/boot/test/sys/fit->+0x74b5e sp 0x3c5ec54ec7f0
[230.407] 01385.01538>    #3    0x0000696b9aa45157 in unittest_run_test(char const*, bool (*)(), test_info**, bool*) ../../out/default.zircon/../../zircon/system/ulib/unittest/unittest.cc:268 <<VMO#563950=libunittest.so>>+0x2157 sp 0x3c5ec54ecdc0
[230.407] 01385.01538>    #4.2  0x0000696b9aa45792 in operator() ../../out/default.zircon/../../zircon/system/ulib/unittest/unittest.cc:296 <<VMO#563950=libunittest.so>>+0x2792 sp 0x3c5ec54ece00
[230.407] 01385.01538>    #4.1  0x0000696b9aa45792 in run_with_watchdog<unittest_run_named_test(char const*, bool (*)(), test_type_t, test_info**, bool*)::<lambda()> > ../../out/default.zircon/../../zircon/system/ulib/unittest/unittest.cc:285 <<VMO#563950=libunittest.so>>+0x2792 sp 0x3c5ec54ece00
[230.407] 01385.01538>    #4    0x0000696b9aa45792 in unittest_run_named_test ../../out/default.zircon/../../zircon/system/ulib/unittest/unittest.cc:295 <<VMO#563950=libunittest.so>>+0x2792 sp 0x3c5ec54ece00
[230.407] 01385.01538>    #5    0x000074f079c4e32a in function_tests(bool, char const*) ../../out/default.zircon/../../zircon/system/utest/fit/function_tests.cc:945 <<VMO#563915=/boot/test/sys/fit->+0x5132a sp 0x3c5ec54ece40
[230.408] 01385.01538>    #6.1  0x0000696b9aa44dc0 in unittest_run_all_tests_etc ../../out/default.zircon/../../zircon/system/ulib/unittest/all-tests.cc:46 <<VMO#563950=libunittest.so>>+0x1dc0 sp 0x3c5ec54ece80
[230.408] 01385.01538>    #6    0x0000696b9aa44dc0 in unittest_run_all_tests ../../out/default.zircon/../../zircon/system/ulib/unittest/all-tests.cc:207 <<VMO#563950=libunittest.so>>+0x1dc0 sp 0x3c5ec54ece80
[230.409] 01385.01538>    #7    0x0000696b9aa44ac9 in main ../../out/default.zircon/../../zircon/system/ulib/unittest/unittest-main.cc:15 <<VMO#563950=libunittest.so>>+0x1ac9 sp 0x3c5ec54ecef0
[230.409] 01385.01538>    #8    0x00004437b337274c in start_main ../../out/default.zircon/../../zircon/third_party/ulib/musl/src/env/__libc_start_main.c:93 <<VMO#563929=ld.so.1>>+0x2874c sp 0x3c5ec54ecf00
[230.409] 01385.01538>    #9    0x0000000000000000 in <>+0x0 sp 0x3c5ec54ed000

[ldionne]

In D66178#1634795, @phosek wrote:

I started seeing segfaults in some of our tests after the latests Clang roll and bisecting narrowed it down to this change. I also tried reverting this change locally and the segfaults no longer occur so I'm pretty confident it's this change.

Thanks a lot for the heads up. Do you think you'd be able to get us a reproducer?

@zoecarver We should revert this patch until we can find what the problem is.

What's confusing is that it's not happening in regular Clang builds, it's only affecting GCC builds (we're using GCC 8.3) that are using libc++ and ASan builds. It's also don't see any of the modified functions on the stack which makes it even more confusing but I'm suspecting memory corruption that's happens earlier.

Here's one of the symbolized logs:
...

Yeah, the stack trace isn't too helpful.

[zoecarver]

Reverted as rL369270.

[zoecarver]

@phosek I looked around a bit. Seems like this is probably where the issue is. Still debugging and trying to find what caused the segmentation fault. Any more information is welcome. A reproducer would be fantastic.

[zoecarver]

@phosek I spent some more time trying to reproduce the segfault. I still haven't been able to reproduce the issue. Any more information you could share would be great (even, at least, the failing build so I can try to reproduce with that). I would really like to get this re-committed.

[ldionne]

In order to make progress on this, we could split this patch. I suggest roughly three patches:

1. Remove the faux-variadics for C++03 but still use the static member functions
2. Remove std::shared_ptr<T>::allocate_shared
3. Remove std::shared_ptr<T>::make_shared (I suspect this is what triggers the segfault).

By doing it in steps, we should be able to make some progress, and when we hit the segfault again, it might be clearer what the problem is.

[zoecarver]

• Remove std::shared_ptr::allocate_shared

[zoecarver]

• Remove std::shared_ptr::allocate_shared

[zoecarver]

• Use __create_with_control_block in allocate_shared

[zoecarver]

@ldionne friendly ping. This patch now only removes std::shared_ptr::allocate_shared. Should be an easy change. And I think we fixed @phosek's issue by breaking __hold2.get()->get() into a variable so that compilers that parse args backwards still work here.

[ldionne]

Thanks!

clang/include/clang/Basic/TargetInfo.h

public:
   // cpu_specific/cpu_dispatch so that it can be passed to llvm as optimization
   // options.
   virtual void getCPUSpecificCPUDispatchFeatures(
       StringRef Name, llvm::SmallVectorImpl<StringRef> &Features) const {
     llvm_unreachable(
         "cpu_specific Multiversioning not implemented on this target");
   }
 
+  // Get the cache line size of a given cpu. This method switches over
+  // the given cpu and returns `0` if the CPU is not found.
+  virtual Optional<unsigned> getCPUCacheLineSize() const { return None; }
+
   // Returns maximal number of args passed in registers.
   unsigned getRegParmMax() const {
     assert(RegParmMax < 7 && "RegParmMax value is larger than AST can handle");
     return RegParmMax;
   }
 
   /// Whether the target supports thread-local storage.
   bool isTLSSupported() const {

clang/lib/Basic/Targets/X86.h

public:
   bool validateCPUSpecificCPUDispatch(StringRef Name) const override;
 
   char CPUSpecificManglingCharacter(StringRef Name) const override;
 
   void getCPUSpecificCPUDispatchFeatures(
       StringRef Name,
       llvm::SmallVectorImpl<StringRef> &Features) const override;
 
+  Optional<unsigned> getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *&Name,
                              TargetInfo::ConstraintInfo &info) const override;
 
   bool validateGlobalRegisterVariable(StringRef RegName, unsigned RegSize,
                                       bool &HasSizeMismatch) const override {
     // esp and ebp are the only 32-bit registers the x86 backend can currently
     // handle.
     if (RegName.equals("esp") || RegName.equals("ebp")) {

clang/lib/Basic/Targets/X86.cpp

if (auto Len = matchAsmCCConstraint(Name)) {
       Name += Len - 1;
       Info.setAllowsRegister();
       return true;
     }
     return false;
   }
 }
 
+// Below is based on the following information:
+// +------------------------------------+-------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------+
+// |           Processor Name           | Cache Line Size (Bytes) |                                                                            Source                                                                            |
+// +------------------------------------+-------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------+
+// | i386                               |                      64 | https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf                                          |
+// | i486                               |                      16 | "four doublewords" (doubleword = 32 bits, 4 bits * 32 bits = 16 bytes) https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.126.4216&rep=rep1&type=pdf (page 29) |
+// | i586/Pentium MMX                   |                      32 | https://www.7-cpu.com/cpu/P-MMX.html                                                                                                                         |
+// | i686/Pentium                       |                      32 | https://www.7-cpu.com/cpu/P6.html                                                                                                                            |
+// | Netburst/Pentium4                  |                      64 | https://www.7-cpu.com/cpu/P4-180.html                                                                                                                        |
+// | Atom                               |                      64 | https://www.7-cpu.com/cpu/Atom.html                                                                                                                          |
+// | Westmere                           |                      64 | https://en.wikichip.org/wiki/intel/microarchitectures/sandy_bridge_(client) "Cache Architecture"                                                             |
+// | Sandy Bridge                       |                      64 | https://en.wikipedia.org/wiki/Sandy_Bridge and https://www.7-cpu.com/cpu/SandyBridge.html                                                                    |
+// | Ivy Bridge                         |                      64 | https://blog.stuffedcow.net/2013/01/ivb-cache-replacement/ and https://www.7-cpu.com/cpu/IvyBridge.html                                                      |
+// | Haswell                            |                      64 | https://www.7-cpu.com/cpu/Haswell.html                                                                                                                       |
+// | Bradwell                           |                      64 | https://www.7-cpu.com/cpu/Broadwell.html                                                                                                                     |
+// | Skylake (including skylake-avx512) |                      64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"                                                                       |
+// | Cascade Lake                       |                      64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"                                                                  |
+// | Skylake                            |                      64 | https://en.wikichip.org/wiki/intel/microarchitectures/kaby_lake "Memory Hierarchy"                                                                           |
+// | Ice Lake                           |                      64 | https://www.7-cpu.com/cpu/Ice_Lake.html                                                                                                                      |
+// | Knights Landing                    |                      64 | https://software.intel.com/en-us/articles/intel-xeon-phi-processor-7200-family-memory-management-optimizations "The Intel® Xeon Phi™ Processor Architecture" |
+// | Knights Mill                       |                      64 | https://software.intel.com/sites/default/files/managed/9e/bc/64-ia-32-architectures-optimization-manual.pdf?countrylabel=Colombia "2.5.5.2 L1 DCache "       |
+// +------------------------------------+-------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------+
+Optional<unsigned> X86TargetInfo::getCPUCacheLineSize() const {
+  switch (CPU) {
+    // i486
+    case CK_i486:
+    case CK_WinChipC6:
+    case CK_WinChip2:
+    case CK_C3:
+    // Lakemont
+    case CK_Lakemont:
+      return 16;
+    // i586
+    case CK_i586:
+    case CK_Pentium:
+    case CK_PentiumMMX:
+    // i686
+    case CK_PentiumPro:
+    case CK_i686:
+    case CK_Pentium2:
+    case CK_Pentium3:
+    case CK_PentiumM:
+    case CK_C3_2:
+    // K6
+    case CK_K6:
+    case CK_K6_2:
+    case CK_K6_3:
+    // Geode
+    case CK_Geode:
+      return 32;
+      
+    // i386
+    case CK_i386:
+    // Netburst
+    case CK_Pentium4:
+    case CK_Prescott:
+    case CK_Nocona:
+    // Atom
+    case CK_Bonnell:
+    case CK_Silvermont:
+    case CK_Goldmont:
+    case CK_GoldmontPlus:
+    case CK_Tremont:
+
+    case CK_Westmere:
+    case CK_SandyBridge:
+    case CK_IvyBridge:
+    case CK_Haswell:
+    case CK_Broadwell:
+    case CK_SkylakeClient:
+    case CK_SkylakeServer:
+    case CK_Cascadelake:
+    case CK_Nehalem:
+    case CK_Cooperlake:
+    case CK_Cannonlake:
+    case CK_Tigerlake:
+    case CK_IcelakeClient:
+    case CK_IcelakeServer:
+    case CK_KNL:
+    case CK_KNM:
+    // K7
+    case CK_Athlon:
+    case CK_AthlonXP:
+    // K8
+    case CK_K8:
+    case CK_K8SSE3:
+    case CK_AMDFAM10:
+    // Bobcat
+    case CK_BTVER1:
+    case CK_BTVER2:
+    // Bulldozer
+    case CK_BDVER1:
+    case CK_BDVER2:
+    case CK_BDVER3:
+    case CK_BDVER4:
+    // Zen
+    case CK_ZNVER1:
+    case CK_ZNVER2:
+    // Deprecated
+    case CK_x86_64:
+    case CK_Yonah:
+    case CK_Penryn:
+      return 64;
+
+    // The following currently have unknown cache line sizes (but they are probably all 64):
+    // Core
+    case CK_Core2:
+    case CK_Generic:
+      return None;
+  }
+}
+
 bool X86TargetInfo::validateOutputSize(const llvm::StringMap<bool> &FeatureMap,
                                        StringRef Constraint,
                                        unsigned Size) const {
   // Strip off constraint modifiers.
   while (Constraint[0] == '=' || Constraint[0] == '+' || Constraint[0] == '&')
     Constraint = Constraint.substr(1);
 
   return validateOperandSize(FeatureMap, Constraint, Size);

libcxx/include/memory

#endif  // _LIBCPP_NO_RTTI
     {
         shared_ptr<_Tp> __r;
         __r.__ptr_ = __p;
         __r.__cntrl_ = __cntrl;
         __r.__enable_weak_this(__r.__ptr_, __r.__ptr_);
         return __r;
     }
 
-    template<class _Alloc, class ..._Args>
-        static
-        shared_ptr<_Tp>
-        allocate_shared(const _Alloc& __a, _Args&& ...__args);
-
 private:
     template <class _Yp, bool = is_function<_Yp>::value>
         struct __shared_ptr_default_allocator
         {
             typedef allocator<_Yp> type;
         };
 
     template <class _Yp>
#endif
                                      _AllocT > _CntrlBlk;
         __cntrl_ = new _CntrlBlk(__r.get(), ref(__r.get_deleter()), _AllocT());
         __enable_weak_this(__r.get(), __r.get());
     }
     __r.release();
 }
 
 template<class _Tp>
-template<class _Alloc, class ..._Args>
-shared_ptr<_Tp>
-shared_ptr<_Tp>::allocate_shared(const _Alloc& __a, _Args&& ...__args)
-{
-    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared" );
-    typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
-    typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
-    typedef __allocator_destructor<_A2> _D2;
-    _A2 __a2(__a);
-    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-    ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
-        _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
-    shared_ptr<_Tp> __r;
-    __r.__ptr_ = __hold2.get()->get();
-    __r.__cntrl_ = _VSTD::addressof(*__hold2.release());
-    __r.__enable_weak_this(__r.__ptr_, __r.__ptr_);
-    return __r;
-}
-
-template<class _Tp>
 shared_ptr<_Tp>::~shared_ptr()
 {
     if (__cntrl_)
         __cntrl_->__release_shared();
 }
 
 template<class _Tp>
 inline

 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>
 >::type
 allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
-    return shared_ptr<_Tp>::allocate_shared(__a, _VSTD::forward<_Args>(__args)...);
+    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
+
+    typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
+    typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
+    typedef __allocator_destructor<_A2> _D2;
+
+    _A2 __a2(__a);
+    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
+    ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
+        _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
+
+    shared_ptr<_Tp> __r;
+    __r.__ptr_ = __hold2.get()->get();
+    __r.__cntrl_ = _VSTD::addressof(*__hold2.release());
+    __r.__enable_weak_this(__r.__ptr_, __r.__ptr_);
+    return __r;
 }
 
 template<class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY
 bool
 operator==(const shared_ptr<_Tp>& __x, const shared_ptr<_Up>& __y) _NOEXCEPT
 {
     return __x.get() == __y.get();