This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix mismatches between aligned operator new and std::free
ClosedPublic

Authored by mstorsjo on Jul 13 2021, 11:34 PM.

Details

Summary

The XFAIL comments about VCRuntime not providing aligned operator new
are outdated; these days VCRuntime does provide them.

However, the tests used to fail on Windows, as the pointers allocated
with an aligned operator new (which is implemented with _aligned_malloc
on Windows) can't be freed using std::free() on Windows (but they need
to be freed with the corresponding function _aligned_free instead).

Instead override the aligned operator new to return a dummy suitably
aligned pointer instead, like other tests that override aligned operator
new.

Also override operator delete[] instead of plain operator delete
in the array testcase; the fallback from operator delete[] to
user defined operator delete doesn't work in all DLL build
configurations on Windows.

By providing the aligned operator new within the tests, this also makes
these test cases pass when testing back deployment on macOS 10.9.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Jul 13 2021, 11:34 PM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 11:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
61

Your commit summary mentions that the old test relied on

the fallback from operator delete[] to user defined operator delete

Are you sure that this was intentional, and not just a typo in the test? (Personally I didn't know that any such fallback rule existed! And wouldn't that be more of a compiler issue than a libc++ issue, anyway?)
I'd feel better if we did the delete->delete[] renaming in one commit/PR and then whatever Windows fixup is still needed, in a separate PR.

70

This DoNotOptimize seems unnecessary. I think you can just un-name the parameter.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
33

Any intuition for whether this use of __STDCPP_DEFAULT_NEW_ALIGNMENT__ is correct, or if it should just be alignof(std::max_align_t)? (Related to my request-for-investigation on D105905.)

51

Here and line 76, the DoNotOptimize seems unnecessary.

ldionne requested changes to this revision.Jul 14 2021, 9:34 AM
ldionne added a subscriber: ldionne.

LGTM in essence, with a few comments. Thanks for looking into this.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
47

I don't think we need TEST_THROW_SPEC at all, since this isn't tested in C++03 mode.

Same goes for TEST_NOEXCEPT -- it can just be turned into noexcept.

This revision now requires changes to proceed.Jul 14 2021, 9:34 AM
mstorsjo added inline comments.Jul 14 2021, 2:00 PM
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
47

Ok, I'll simplify those while touching the code.

61

I presume it was just a typo in the test, yes. (There's such a fallback, but we shouldn't be testing that implicitly everywhere but only in a test set out to specifically test that, yeah, as not all systems might support it.)

@ldionne WDYT, keep the delete -> delete[] change here or split to a separate patch?

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/delete_align_val_t_replace.pass.cpp
33

I'm not entirely sure about the finer details about which one matters where, but I think the most correct one here might be something like std::max(__STDCPP_DEFAULT_NEW_ALIGNMENT__, alignof(std::max_align_t)) * 2, to make sure it really is more aligned than what otherwise is provided. (But this is unrelated to the patch at hand here anyway.)

51

I guess so yeah. FWIW I copied inspiration for these from new_align_val_t_replace.pass.cpp in the same dir, where you might be able to remove some such extra calls.

mstorsjo updated this revision to Diff 358744.Jul 14 2021, 2:02 PM

Removed DoNotOptimize, simplified TEST_THROW_SPEC and TEST_NOEXCEPT assuming the test only runs on C++17.

ldionne accepted this revision.Jul 15 2021, 10:59 AM
ldionne added inline comments.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp
61

I'm fine with it being done here, since you called it out in the commit message. Pedantically, a separate patch would be better, but I'm trying to balance pedantic correctness and productivity :-).

This revision is now accepted and ready to land.Jul 15 2021, 10:59 AM