Since r338934, Clang emits an error when aligned allocation functions are used in conjunction with a system libc++ dylib that does not support those functions. This causes some tests to fail when testing against older libc++ dylibs. This commit marks those tests as XFAIL, and also documents the various reasons for the tests failing.
Details
- Reviewers
vsapsai EricWF mclow.lists - Commits
- rG9dc31a87e7e8: Merging r339743: --------------------------------------------------------------…
rG7c6baa21846e: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions
rL339854: Merging r339743:
rCXX339743: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions
rL339743: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions
Diff Detail
- Repository
- rL LLVM
Event Timeline
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp | ||
---|---|---|
15 ↗ | (On Diff #159333) | In what cases are we supposed to run these tests? Such extensive collection of unsupported C++ standards looks suspicious and I guess that is the reason why I haven't seen test failures with older libc++ dylibs. |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp | ||
---|---|---|
15 ↗ | (On Diff #159333) | That's an excellent question. I would assume those should be enabled in C++17 and above, I'm not sure why they're disabled. @mclow.lists was the one to introduce those tests, perhaps he can shed light on why they were disabled in C++17? |
I still think we should go forward with this change since the tests _are_ expected to fail on the provided OS X versions, which do not contain the required operators.
This works with all the dylibs I have locally, with various combinations of availability enabled/disabled and all standards. I've asked Marshall to check on his system, where he initially reported some failures.
libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp | ||
---|---|---|
18 ↗ | (On Diff #160457) | These should probably be UNSUPPORTED as well. |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/delete_align_val_t_replace.pass.cpp | ||
15 ↗ | (On Diff #160457) | These should probably be UNSUPPORTED as well (though this is less important) |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp | ||
15 ↗ | (On Diff #159333) | IIRC, this call - operator new(std::size_t, std::align_val_t); was introduced for C++17, and [[nodiscard]] was added for C++20. this test is making sure that we get a warning when you call this and don't use the return value. It requires C++20 and above, hence the massive UNSUPPORTED list |
libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp | ||
---|---|---|
18 ↗ | (On Diff #160457) | I think the correct thing for those is XFAIL, since this test is actually testing that the macro is defined or not defined for specific platforms. Equivalently, we could have two tests -- one aligned_allocation_macro.pass.cpp which is UNSUPPORTED on macosx10.7 ... macosx10.12, and one aligned_allocation_macro.fail.cpp which REQUIRES one of macosx10.7, ..., macosx10.12. Does that make sense? |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp | ||
15 ↗ | (On Diff #159333) | Wouldn't it be sufficient to list the standard required? Why do we have UNSUPPORTED compilers too? |
What about defining a feature for unsupported configurations? I've tried
if '__cpp_aligned_new' not in macros or \ intMacroValue(macros['__cpp_aligned_new']) < 201606: self.config.available_features.add('libcpp-no-aligned-new')
and // UNSUPPORTED: libcpp-no-aligned-new. Seems to be working but it's not sufficient. For example, clang-6 for old macOS versions would define __cpp_aligned_new but a test would fail. Still, we can use another feature, not necessarily macro-based. Though probably something like libcpp-no-aligned-new can replace // UNSUPPORTED: c++98, c++03, c++11, c++14.
I thought about something like this, but it would only be useful in a couple of places, and I still don't understand why the tests are marked as unsupported on some compilers at all. Also, there's already a feature called no-aligned-alloc, which tests whether -faligned-alloc is supported, so we should avoid confusing both.
After discussing with Volodymyr, it's not clear exactly how we might create a libcpp-no-aligned-new check that properly expresses when aligned allocations are available or not. It also seems like there are no other lit features based on such a combination of compiler/target platform/standard/system_cxx_lib. I think we're better off just keeping the duplication of UNSUPPORTED cases right now, and we can revisit this idea in the future if we find ourselves needing that feature again.
Is there any blocking comment on this patch? I'd like to get it through to see if it fixes our testers (and Marshall's local testing, too).