std::allocator should use sized-delete

[llvmbot]

Bugzilla Link	26945
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@jyknight,@mclow

Extended Description

This is the implementation of std::allocator::deallocate from http://llvm.org/svn/llvm-project/libcxx/trunk/include/memory :

_LIBCPP_INLINE_VISIBILITY void deallocate(pointer __p, size_type) _NOEXCEPT
    {_VSTD::__deallocate((void*)__p);}

This is suboptimal for toolchains that implement an optimized sized-delete (that is, ::operator delete(void *ptr, size_t s).)

I think this should invoke

_VSTD::deallocate((void *)p, size * sizeof(T))

or something similar (with the obvious implementation of sized __deallocate.)

[llvmbot]

I was looking in to helping on this bug and it looks like it might be already addressed in the latest versions.

I see

_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
void deallocate(_Tp* __p, size_t __n) _NOEXCEPT {
    if (__libcpp_is_constant_evaluated()) {
        ::operator delete(__p);
    } else {
        _VSTD::__libcpp_deallocate((void*)__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp));
    }
}

as the implementation which seems to indicate that the size-delete is getting invoked (when it is not constant evaluated).

Please let me know if I am missing something -- I'm new here!

Will

[llvmbot]

I think this shouldn't actually be an ABI break -- it seems to me like it just needs to be conditional on the target platform being a new enough version.

Yeah, that sounds more correct to me. We'll probably need to do something akin to what we're doing with Clang and detecting when aligned allocation is unavailable on OS X.

(I wasn't sure if the availability attribute's effect was detectable by sfinae, so I checked and it is: https://godbolt.org/g/kbwUAg)

Clever. Though I'm not sure this will give us the behavior we want. For the aligned versions of new, we allow users who explicitly specify -faligned-allocation to use the feature without warning even if it would be otherwise unavailable.

I'll work on moving the unavailable diagnostics into the compiler, and making them match up with the behavior specified in D45015.

[jyknight]

I think this shouldn't actually be an ABI break -- it seems to me like it just needs to be conditional on the target platform being a new enough version.

(That is only even relevant for those platforms in which it's considered sensible to build code against newer libc++ headers and then at runtime load an old libc++.so -- that is to say, only Apple at the moment.)

We already have an annotation for what OS version introduced the sized delete functions on their declarations in , so, I think we can actually just detect the "unavailability" of the overload, and avoid calling it if it's unavailable for the target version.

(I wasn't sure if the availability attribute's effect was detectable by sfinae, so I checked and it is: https://godbolt.org/g/kbwUAg)

[llvmbot]

So there are 2 main issues implementing this change:

1. Sized operator delete is only sometimes available: On platform such as OS X calling sized operator delete will result in non-backwards compatible programs since older libc++ dylibs do not contain a definition for operator delete(void*, size_t).

2. builtin_operator_delete does not provide a sized version: If we want to implement this change we need to add a sized builtin_operator_delete to clang.

Your suggested change is clearly an improvement, but it's likely limited to the ABI v2 version of libc++.

[philnik777]

std::allocator::deallocate uses sized deallocation in ToT.