<functional>: is compatible with /Zc:alignedNew- now

[fsb4000]

fixes #2711

I also found that:

#include <iostream>

using namespace std;

int main() {
#ifdef __cpp_aligned_new
    cout << "has aligned new\n";
#else
    cout << "nothing\n";
#endif
}

works differently on cl.exe and clang-cl.exe

$ clang-cl main.cpp /std:c++14 /Zc:alignedNew /EHsc
$ main
has aligned new

$ cl main.cpp /std:c++14 /Zc:alignedNew /EHsc
$ main
nothing

this is why I replaced all

#ifdef __cpp_aligned_new

to

#if defined(__cpp_aligned_new) && _HAS_CXX17

Should I report about it to LLVM and add "Transition" comment?

[AlexGuteniev]

I'm concerned about changing existing code to rely on C++17 mode

[fsb4000]

cl defines __cpp_aligned_new for C++17 and up (with /Zc:alignedNew)
clang-cl defines __cpp_aligned_new for C++14 and up (with /Zc:alignedNew)
I added _HAS_CXX17 for compatibility between clang-cl and cl
Am I wrong?

[fsb4000]

Does clang-cl compile with /std:c++14 and /Zc:alignedNew before this change?

No, it doesn't compile as far as I can see.
I got the error(and other similar):

Command: "C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin\clang-cl.EXE" "C:\Dev\STL\tests\std\tests\GH_000690_overaligned_function\test.cpp" "-IC:\Dev\STL\out\build\x64\out\inc" "-IC:\Dev\STL\llvm-project\libcxx\test\support" "-IC:\Dev\STL\tests\std\include" "/nologo" "/Od" "/W4" "/w14061" "/w14242" "/w14265" "/w14582" "/w14583" "/w14587" "/w14588" "/w14749" "/w14841" "/w14842" "/w15038" "/w15214" "/w15215" "/w15216" "/w15217" "/sdl" "/WX" "/Zc:strictStrings" "/D_ENABLE_STL_INTERNAL_CHECK" "/bigobj" "/FIforce_include.hpp" "/w14365" "/D_ENFORCE_FACET_SPECIALIZATIONS=1" "/D_STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETER" "-fno-ms-compatibility" "-fno-delayed-template-parsing" "/EHsc" "/MD" "/std:c++14" "/w14640" "/Zc:threadSafeInit-" "/Zc:alignedNew" "-m64" "-FeC:\Dev\STL\out\build\x64\tests\std\tests\GH_000690_overaligned_function\Output\55\GH_000690_overaligned_function.exe" "-link" "-LIBPATH:C:\Dev\STL\out\build\x64\out\lib\amd64" "-LIBPATH:C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.33.31424\lib\x64" "/MANIFEST:EMBED"
Exit Code: 1
Standard Error:
--
In file included from C:\Dev\STL\tests\std\tests\GH_000690_overaligned_function\test.cpp:9:
In file included from C:\Dev\STL\out\build\x64\out\inc\functional:14:
C:\Dev\STL\out\build\x64\out\inc\xmemory(1425,60): error: cannot initialize a value of type 'std::align_val_t' with an rvalue of type 'unsigned long long'
                _Pbuf = ::operator new (_Size, align_val_t{alignof(_Ty)}, nothrow);
                                                           ^~~~~~~~~~~~
C:\Dev\STL\out\build\x64\out\inc\xmemory(1445,47): error: cannot initialize a value of type 'std::align_val_t' with an rvalue of type 'unsigned long long'
        ::operator delete (_Pbuf, align_val_t{alignof(_Ty)});
                                              ^~~~~~~~~~~~
2 errors generated.

only with clang-cl and c++14 configuration.

So every #ifdef __cpp_aligned_new which contained align_val_t didn't compile.

[AlexGuteniev]

Ok then I agree

[CaseyCarter]

works differently on cl.exe and clang-cl.exe

This is a separate issue, which should be handled in a separate PR (if at all). Feel free to report upstream to the clang-cl devs that cl ignores /Zc:alignedNew in pre-C++17 modes and that they could do so as well if they like.

[fsb4000]

I created: LLVM-55389

[strega-nil-ms]

I don't have an opinion on the testing changes (though they seem reasonable to me), and this makes the existing situation better; there may be some discussion left to be had wrt how we want to test /Zc:alignedNew-.

[StephanTLavavej]

FYI @strega-nil-ms I pushed minor changes after you approved.

I was initially concerned about mismatch - adding __cpp_aligned_new logic to otherwise unguarded code would create an incompatibility between C++14 and C++17 mode (as C++17 implies this by default). However, the behavioral change here is specific to C++23 mode. Therefore, only mixing /std:c++latest and /std:c++latest /Zc:alignedNew- would be a problem, and this is already true for our other __cpp_aligned_new guards.

[Nick-Kooij]

Hello,
Thanks all for working on this issue. As the developer who raise the issue in the first place, I'd just like to clarify that my purpose in using /Zc:alignedNew- is really a workaround to eliminate the overhead of calling overaligned new/delete.

The ideal solution for me would be to add a compiler switch /Zc:defaultNewAlignment={std::align_val_t} where the __STDCPP_DEFAULT_NEW_ALIGNMENT__ compile-time alignment threshold where C++17 overaligned new/delete are called:

• void* operator new ( std::size_t count, std::align_val_t al );
• void operator delete ( void* ptr, std::align_val_t al ) noexcept;
• And similar array/noexcept overloads--anything taking an additional std::align_val_t parameter.

In other words, implement a switch symmetric to Clang's -fnew-alignment. See Add compiler switch to change STDCPP_DEFAULT_NEW_ALIGNMENT.

---

Some background:

1. I have overloaded global operator new/delete providing stronger alignment guarantees than the default __STDCPP_DEFAULT_NEW_ALIGNMENT__. Specifically alignment beyond a single 64 byte cache line is aligned to 128 bytes (two cache lines). (This meshes well with x64 prefetching logic and avoids false sharing.)

2. By default with C++17 compilation (without /Zc:alignedNew) new/delete expressions for types with alignment beyond __STDCPP_DEFAULT_NEW_ALIGNMENT__ will call the C++17 overaligned operator new/delete overloads:

• void* operator new ( std::size_t count, std::align_val_t al );
• void operator delete ( void* ptr, std::align_val_t al ) noexcept;
• ...

This is a fantastic new C++17 feature allowing over-aligned types to be dynamically allocated. Ideally I would like this feature enabled, rather than disabling it using /Zc:alignedNew-.

Consider allocation of a 256 byte type, with 128 byte alignment, by default without /Zc:alignedNew-. As __STDCPP_DEFAULT_NEW_ALIGNMENT__ can't be we have we now have runtime overhead which I wish to avoid:

• bloated operator new and operator delete call sites
• runtime overhead of calling an overaligned operator new

The goal is to eliminate this runtime overhead.

While the runtime overhead within the overloaded overaligned new/delete can be somewhat mitigated using [[likely]]/[[unlikely]] on the alignment check, nevertheless there is a runtime overhead due to unnecessary branches.

<public: static void __cdecl TaskSpawn<struct ChainTask>::spawn<int>(int &&)>:
               	push	rbx
               	sub	rsp, 32
               	mov	rbx, rcx
               	mov	edx, 128 // additional std::align_val_t parameter
               	mov	ecx, 256 // object size
               	call	 ??2@YAPEAX_KW4align_val_t@std@@@Z // overaligned operator new call
                ...

This bloats each over-aligned operator new/delete call site, and unnecessarily calls an overload that contains additional alignment logic.

Ideally we want this:

<public: static void __cdecl TaskSpawn<struct ChainTask>::spawn<int>(int &&)>:
               	push	rbx
               	sub	rsp, 32
               	mov	rbx, rcx
               	mov	ecx, 256 // object size
               	call	 ??2@YAPEAX_K@Z // regular operator new called
                ...

Best Regards,
Nick

[StephanTLavavej]

We talked about this at the weekly maintainer meeting - we believe that this PR should be changed to follow the same strategy as the rest of the STL currently uses for /Zc:alignedNew-, which is to directly call operator new and not attempt to add additional alignment. As @frederick-vs-ja mentioned, this will potentially not handle overaligned types correctly (in the case where operator new hasn't been replaced to provide such alignment), but this is consistent with existing practice, and attempting to add additional alignment would interfere with scenarios like @Nick-Kooij's. We conclude that if users want the STL to automatically handle overaligned types correctly, they should be using the C++17 Standard setting - by setting /Zc:alignedNew- they are responsible for their own destiny.

[StephanTLavavej]

I'm speculatively mirroring this to the MSVC-internal repo - further changes can be pushed, but please notify me.

[CaseyCarter]

Applied my suggested changes after testing locally.

[StephanTLavavej]

Thanks for aligning these new features with our existing tolerance of this option! 😹 🚀 🎉