valarray: Fix delete in order to satisfy ASan

[fsub]

Particularly fixes 'AddressSanitizer: new-delete-type-mismatch' for std::valarray<int> on x64.

[cbezault]

This looks good to me. I took a look and I think that _Myptr will always be allocated with the global operator new.

Could you elaborate on how this is to satisfy ASan? Is it that this breaks when a user provides delete but global operator new is overridden by ASan? Or is it just that ASan detects the difference between operator delete and a delete expression?

[fsub]

@cbezault I can't see any reason why the original code could generate wrong instructions for std::valarray<int>. Asymmetric usage of new and delete smells, however, and can lead to real bugs, at least in presence of class-specific overloads.

Reduced repro of the issue reported by ASan (originally found in production code compiled with VS 2019 16.8.2 on x64):

$ cat repro.cxx 
int main(int argc, [[maybe_unused]] char** argv)
{
   auto p = static_cast<int*>(::operator new(argc * sizeof(int)));
   delete p;
}
$ g++ -std=c++17 -fsanitize=address repro.cxx 
$ ./a.out 
$ ./a.out boom
=================================================================
==4208==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x602000000010 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   8 bytes;
  size of the deallocated type: 4 bytes.
    #0 0x7f178fa25009 in operator delete(void*, unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:172
    #1 0x56448d26b1a2 in main (/tmp/a.out+0x11a2)
    #2 0x7f178f492151 in __libc_start_main (/usr/lib/libc.so.6+0x28151)
    #3 0x56448d26b09d in _start (/tmp/a.out+0x109d)

0x602000000010 is located 0 bytes inside of 8-byte region [0x602000000010,0x602000000018)
allocated by thread T0 here:
    #0 0x7f178fa23f41 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x56448d26b188 in main (/tmp/a.out+0x1188)
    #2 0x7f178f492151 in __libc_start_main (/usr/lib/libc.so.6+0x28151)

SUMMARY: AddressSanitizer: new-delete-type-mismatch /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:172 in operator delete(void*, unsigned long)
==4208==HINT: if you don't care about these errors you may set ASAN_OPTIONS=new_delete_type_mismatch=0
==4208==ABORTING

[cbezault]

That makes sense.

[CaseyCarter]

I pushed a clarification of the comment on line 40; looks good to me, too.

[cbezault]

I pushed the change that I believe @StephanTLavavej requested.
I didn't add a test to cover checking the mismatch yet. If @StephanTLavavej thinks it is necessary that should also be added.

[StephanTLavavej]

@cbezault I pushed a small change to clean up a pre-existing cosmetic issue that was being copied here. __cpp_aligned_new implies we're in C++17 mode or later (but not the converse, due to the /Zc:alignedNew- escape hatch), which implies that if constexpr is available. We already assume this elsewhere:

STL/stl/inc/xmemory

Lines 1383 to 1384 in 19c683d

	#ifdef __cpp_aligned_new
	if constexpr (alignof(_Ty) > __STDCPP_DEFAULT_NEW_ALIGNMENT__) {

I attempted to add test coverage for over-aligned types, but encountered error C2719: '_Val': formal parameter with requested alignment of 128 won't be aligned, due to how valarray member functions take _Ty by value. This might mean that over-aligned types have never actually worked with valarray.

@fsub I believe you're being prompted to sign our CLA because our commits editing additional lines have pushed your PR above a minimum threshold.

[fsub]

Thank you all for your reviews, comments, and improvements!
@StephanTLavavej Is it really necessary to sign the CLA? I'd rather avoid it. After all, my share of the work is insignificant. I've never been interested in an attribution. My only concern was to draw attention to the problem and find a solution quickly.

[CaseyCarter]

@StephanTLavavej Is it really necessary to sign the CLA? I'd rather avoid it. After all, my share of the work is insignificant. I've never been interested in an attribution. My only concern was to draw attention to the problem and find a solution quickly.

It's only necessary if you want us to merge your PR ;)

I'm not a lawyer, but my understanding is that the CLA is necessary to establish that both parties clearly understand what code contribution means, i.e., what rights over the changes you are sharing with Microsoft.

[fsub]

That's too bad. I hope that retracting the pull request will solve all legal issues. I'm sorry for the inconvenience.

[StephanTLavavej]

Since my last commit was cosmetic, I want to try reverting it and seeing if the CLA bot will accept the PR as minimal again. (I wouldn't have pushed that change if I knew this would happen.)

[StephanTLavavej]

@fsub We can proceed with merging your PR 😸

[StephanTLavavej]

Thanks for fixing this bug, and congratulations on your first microsoft/STL commit! This will ship in VS 2019 16.9 Preview 3. 😺 🚀

[fsub]

@StephanTLavavej Thank you for your efforts and your open-mindedness.
Keep up the great work on the C++ standard library! It is greatly appreciated.