Avoid cast<T*> before T is constructed to pacify CFI checks

[rnk]

Summary

Fixes usage of shared_ptr with CFI enabled, which is llvm.org/pr48993.

[ldionne]

Ah, I just commented on the bug report, but your patch confirms what I thought. Thanks for the patch. So this is basically a CFI false positive, do we agree?

Another approach would be:

template <class _Pointer = _Tp*>
_Pointer __get_elem() _NOEXCEPT {
    _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
    typename _CompressedPair::_Base2* __second = _CompressedPair::__get_second_base(__as_pair);
    _Pointer __elem = reinterpret_cast<_Pointer>(__second);
    return __elem;
}

Then, we can use it as __get_elem<void*>() for the placement-new use case, and just __get_elem() for the normal case. What do you think?

libcxx/include/memory

2656

Indentation looks off.

[rnk]

In D95827#2535177, @ldionne wrote:

Ah, I just commented on the bug report, but your patch confirms what I thought. Thanks for the patch. So this is basically a CFI false positive, do we agree?

I think the code is fine, but I'm not 100% sure it isn't technically UB. I mean, I'd write it.

Then, we can use it as __get_elem<void*>() for the placement-new use case, and just __get_elem() for the normal case. What do you think?

That works, I tried it, but I realized the C++17+ cases are still broken.

libcxx/include/memory

2594

I realize that the same issue exists here. However, the prorotype of C++17 std::construct_at expects a _Tp* argument, not void*, so this change is really fighting uphill against future standards versions, and that makes me reconsider the whole effort. I guess I'll hold off on this change for now and go forward with the exclusion.

[ldionne]

We could add _LIBCPP_NO_CFI on the problematic method, like we do for std::addressof. Can you try that out?

[rnk]

• use attribute to disable CFI

[rnk]

In D95827#2536611, @ldionne wrote:

We could add _LIBCPP_NO_CFI on the problematic method, like we do for std::addressof. Can you try that out?

Yep, it works.

I haven't put together a test for this. Are there existing tests for CFI or sanitizers that I could use to build a test for this?

[ldionne]

In D95827#2537371, @rnk wrote:

In D95827#2536611, @ldionne wrote:

We could add _LIBCPP_NO_CFI on the problematic method, like we do for std::addressof. Can you try that out?

Yep, it works.

I haven't put together a test for this. Are there existing tests for CFI or sanitizers that I could use to build a test for this?

No, I don't think we have any. I see two options:

1. Add a buildbot that runs with CFI enabled.
2. Add a single .sh.cpp test that runs this specifically.

I think option (1) would be the best. Is it sufficient to add -fsanitize=cfi -flto when compiling a test file to reproduce the issue? If so, I think we could simply add a new CFI sanitizer configuration to Lit that does that, and add a corresponding job to run the whole test suite with CFI enabled. We can tackle that as a separate effort.

[rnk]

In D95827#2537382, @ldionne wrote:

I think option (1) would be the best. Is it sufficient to add -fsanitize=cfi -flto when compiling a test file to reproduce the issue? If so, I think we could simply add a new CFI sanitizer configuration to Lit that does that, and add a corresponding job to run the whole test suite with CFI enabled. We can tackle that as a separate effort.

Makes sense to me. I think CFI also requires -fvisibility=hidden to make its vtable checks work. -fno-sanitize-trap makes CFI violations produce diagnostics instead of simply trapping, so you would want to include it in a test config.

libcxx/include/memory

struct __shared_ptr_emplace
     template<class ..._Args>
     _LIBCPP_HIDE_FROM_ABI
     explicit __shared_ptr_emplace(_Alloc __a, _Args&& ...__args)
         : __storage_(_VSTD::move(__a))
     {
 #if _LIBCPP_STD_VER > 17
         using _TpAlloc = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
         _TpAlloc __tmp(*__get_alloc());
         allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), _VSTD::forward<_Args>(__args)...);
 #else
         ::new ((void*)__get_elem()) _Tp(_VSTD::forward<_Args>(__args)...);
 #endif
     }
 
     _LIBCPP_HIDE_FROM_ABI
     _Alloc* __get_alloc() _NOEXCEPT { return __storage_.__get_alloc(); }
 
struct _ALIGNAS_TYPE(_CompressedPair) _Storage {
             __get_alloc()->~_Alloc();
         }
         _Alloc* __get_alloc() _NOEXCEPT {
             _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
             typename _CompressedPair::_Base1* __first = _CompressedPair::__get_first_base(__as_pair);
             _Alloc *__alloc = reinterpret_cast<_Alloc*>(__first);
             return __alloc;
         }
-        _Tp* __get_elem() _NOEXCEPT {
+        _LIBCPP_NO_CFI _Tp* __get_elem() _NOEXCEPT {
             _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
             typename _CompressedPair::_Base2* __second = _CompressedPair::__get_second_base(__as_pair);
             _Tp *__elem = reinterpret_cast<_Tp*>(__second);
             return __elem;
         }
     };
 
     static_assert(_LIBCPP_ALIGNOF(_Storage) == _LIBCPP_ALIGNOF(_CompressedPair), "");
     static_assert(sizeof(_Storage) == sizeof(_CompressedPair), "");
     _Storage __storage_;
 };
 
 struct __shared_ptr_dummy_rebind_allocator_type;
 template <>