This is an archive of the discontinued LLVM Phabricator instance.

Differential D102798

[SystemZ][z/OS][libcxx] mark aligned allocation tests UNSUPPORTED on z/OS
ClosedPublic

Authored by DanielMcIntosh-IBM on May 19 2021, 12:08 PM.

Details

Reviewers
ldionne
abhina.sreeskantharajan
fanbo-meng
zibi
Group Reviewers
Restricted Project
Commits
rGd0fe294729a2: [SystemZ][z/OS][libcxx] mark aligned allocation tests UNSUPPORTED on z/OS
Summary

zOS doesn't support aligned allocation, so these tests are failing.
For more details on aligned allocation in zOS, see
https://reviews.llvm.org/D87611 and https://reviews.llvm.org/D90178

Diff Detail

Unit TestsFailed

TimeTest
600 mslibcxx CI GCC-next/C++20 > libc++.std/atomics/atomics_types_operations/atomics_types_operations_req::atomic_is_lock_free.pass.cpp
580 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_requirements/iterator_concepts/iterator_concept_random_access::contiguous_iterator.compile.pass.cpp
600 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_requirements/iterator_concepts/iterator_concept_random_access::random_access_iterator.compile.pass.cpp
560 mslibcxx CI GCC-next/C++20 > libc++.std/ranges/range_access/range_access_begin::begin.pass.cpp
610 mslibcxx CI GCC-next/C++20 > libc++.std/ranges/range_adaptors::range.all.pass.cpp
View Full Test Results (15 Failed)

Event Timeline

DanielMcIntosh-IBM requested review of this revision.May 19 2021, 12:08 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 12:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
Harbormaster completed remote builds in B105286: Diff 346529.May 19 2021, 2:09 PM
DanielMcIntosh-IBM added reviewers: abhina.sreeskantharajan, fanbo-meng, zibi.Jun 1 2021, 3:31 PM
DanielMcIntosh-IBM added a comment.Jun 9 2021, 8:33 AM

Ping

ldionne requested changes to this revision.Jun 9 2021, 10:28 AM
ldionne added inline comments.
libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
23

What is the -zos Lit feature? Is this using Lit's partial matching of triples?

This looks incorrect to me, since you only want to mark this as XFAIL when running against your system library (well, that or libc++ when configured as-if it were your system library).

This revision now requires changes to proceed.Jun 9 2021, 10:28 AM
zibi added inline comments.Jun 9 2021, 11:15 AM
libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
23

Daniel, I think we want to use target-zos. You might want to wait until this is available upstream.

DanielMcIntosh-IBM updated this revision to Diff 351946.Jun 14 2021, 11:44 AM

Update to address Louis's comments.

As per Zibi's comments, this PR depends on other changes that aren't yet
submitted for review. I will update again once those changes have been merged.

Harbormaster completed remote builds in B109156: Diff 351946.Jun 14 2021, 3:29 PM
DanielMcIntosh-IBM updated this revision to Diff 352823.Jun 17 2021, 12:30 PM

Updating with related changes instead of creating new PRs

Specifically, the downstream change this PR depended on was too small to warrant it's
own PR, and there were also a couple more align_val_t tests we'd disabled downstream

The changes still fit within the original PR description, so there's no need to update that

Herald added a subscriber: arichardson. · View Herald TranscriptJun 17 2021, 12:30 PM
ldionne accepted this revision.Jun 17 2021, 1:03 PM

LGTM, but just out of curiosity, will this stay unsupported forever, or will this be implemented eventually?

Normally, we try to support things unless we have a fundamental reason why it can't be done (for example supporting exceptions on an embedded system). But it seems a bit strange to me to mark aligned allocation function tests as unsupported based on the *architecture* - instead I would have expected it to be marked as unsupported based on specific OS versions that do not implement the feature yet.

This revision is now accepted and ready to land.Jun 17 2021, 1:03 PM
Harbormaster completed remote builds in B109790: Diff 352823.Jun 18 2021, 1:42 AM
SeanP added a subscriber: SeanP.Jun 18 2021, 11:14 AM
In D102798#2825509, @ldionne wrote:

LGTM, but just out of curiosity, will this stay unsupported forever, or will this be implemented eventually?

Normally, we try to support things unless we have a fundamental reason why it can't be done (for example supporting exceptions on an embedded system). But it seems a bit strange to me to mark aligned allocation function tests as unsupported based on the *architecture* - instead I would have expected it to be marked as unsupported based on specific OS versions that do not implement the feature yet.

As of today z/OS does not support aligned allocation. Once it is supported we will be able to say it is only supported in z/OS X.Y and above. The tag "s390-zos" is more than an architecture tag. It is more of an OS tag. We have zLinux on s390 as well. The s390-zos tag only covers z/OS, not zLinux. We can change that name if it helps clarify some confusion.

DanielMcIntosh-IBM added a comment.EditedJun 18 2021, 11:43 AM
In D102798#2827705, @SeanP wrote:
In D102798#2825509, @ldionne wrote:

LGTM, but just out of curiosity, will this stay unsupported forever, or will this be implemented eventually?

Normally, we try to support things unless we have a fundamental reason why it can't be done (for example supporting exceptions on an embedded system). But it seems a bit strange to me to mark aligned allocation function tests as unsupported based on the *architecture* - instead I would have expected it to be marked as unsupported based on specific OS versions that do not implement the feature yet.

As of today z/OS does not support aligned allocation. Once it is supported we will be able to say it is only supported in z/OS X.Y and above. The tag "s390-zos" is more than an architecture tag. It is more of an OS tag. We have zLinux on s390 as well. The s390-zos tag only covers z/OS, not zLinux. We can change that name if it helps clarify some confusion.

I think better than changing the name is changing both the name and regular expression - instead of matching zos on the s390 architecture (and variants), just match zos, regardless of architecture. @ldionne Hopefully you can give some insight as to what is most clear for those not familiar with z/OS

DanielMcIntosh-IBM updated this revision to Diff 356789.EditedJul 6 2021, 11:38 AM

Update PR based on internal discussions about whether to match targets based on
just zos or both s390 and zos.

Also update the way we specify UNSUPPORTED now that Lit supports regular
expressions inside UNSUPPORTED. Similar to the changes in commit
c360553c15a8e5aa94d2236eb73e7dfeab9543e5.

DanielMcIntosh-IBM marked 2 inline comments as done.Jul 6 2021, 11:43 AM
Harbormaster completed remote builds in B112665: Diff 356789.Jul 6 2021, 12:57 PM
DanielMcIntosh-IBM retitled this revision from [SystemZ][z/OS] XFAIL aligned allocation tests on z/OS to [SystemZ][z/OS][libcxx] mark aligned allocation tests UNSUPPORTED on z/OS.Jul 7 2021, 7:52 AM
Closed by commit rGd0fe294729a2: [SystemZ][z/OS][libcxx] mark aligned allocation tests UNSUPPORTED on z/OS (authored by DanielMcIntosh-IBM). · Explain WhyJul 7 2021, 7:54 AM
This revision was automatically updated to reflect the committed changes.
DanielMcIntosh-IBM added a commit: rGd0fe294729a2: [SystemZ][z/OS][libcxx] mark aligned allocation tests UNSUPPORTED on z/OS.
ldionne added a comment.Jul 7 2021, 8:10 AM

As of today z/OS does not support aligned allocation. Once it is supported we will be able to say it is only supported in z/OS X.Y and above. The tag "s390-zos" is more than an architecture tag. It is more of an OS tag. We have zLinux on s390 as well. The s390-zos tag only covers z/OS, not zLinux. We can change that name if it helps clarify some confusion.

Thanks for the explanation. I think the approach taken here with the regular expression is by far the cleanest. Thanks.

Revision Contents

PathSize
libcxx/
test/
libcxx/
language.support/
support.dynamic/
4 lines
4 lines

Diff 351946

libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp

Show All 14 Lines
// The dylibs shipped before macosx10.13 do not contain the aligned allocation // The dylibs shipped before macosx10.13 do not contain the aligned allocation
// functions, so trying to force using those with -faligned-allocation results // functions, so trying to force using those with -faligned-allocation results
// in a link error. // in a link error.
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.12 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.12
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.11 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.11
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.10 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.10
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.9 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.9
ldionneUnsubmitted
Done
ReplyInline Actions

What is the -zos Lit feature? Is this using Lit's partial matching of triples?

This looks incorrect to me, since you only want to mark this as XFAIL when running against your system library (well, that or libc++ when configured as-if it were your system library).

ldionne: What is the `-zos` Lit feature? Is this using Lit's partial matching of triples? This looks…
zibiUnsubmitted
Done
ReplyInline Actions

Daniel, I think we want to use target-zos. You might want to wait until this is available upstream.

zibi: Daniel, I think we want to use target-zos. You might want to wait until this is available…
// Libcxx when built for z/OS doesn't contain the aligned allocation funcitons,
// nor does the dynamic library shipped with z/OS.
// XFAIL: s390-zos
// AppleClang < 10 incorrectly warns that aligned allocation is not supported // AppleClang < 10 incorrectly warns that aligned allocation is not supported
// even when it is supported. // even when it is supported.
// UNSUPPORTED: apple-clang-9 // UNSUPPORTED: apple-clang-9
// XFAIL: sanitizer-new-delete, ubsan // XFAIL: sanitizer-new-delete, ubsan
// GCC doesn't support the aligned-allocation flags. // GCC doesn't support the aligned-allocation flags.
// XFAIL: gcc // XFAIL: gcc
▲ Show 20 Lines • Show All 228 Lines • Show Last 20 Lines

libcxx/test/libcxx/language.support/support.dynamic/new_faligned_allocation.pass.cpp

Show All 15 Lines
// The dylibs shipped before macosx10.13 do not contain the aligned allocation // The dylibs shipped before macosx10.13 do not contain the aligned allocation
// functions, so trying to force using those with -faligned-allocation results // functions, so trying to force using those with -faligned-allocation results
// in a link error. // in a link error.
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.12 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.12
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.11 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.11
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.10 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.10
// XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.9 // XFAIL: use_system_cxx_lib && x86_64-apple-macosx10.9
// Libcxx when built for z/OS doesn't contain the aligned allocation funcitons,
// nor does the dynamic library shipped with z/OS.
// XFAIL: s390-zos
// REQUIRES: -faligned-allocation // REQUIRES: -faligned-allocation
// ADDITIONAL_COMPILE_FLAGS: -faligned-allocation // ADDITIONAL_COMPILE_FLAGS: -faligned-allocation
#include <new> #include <new>
#include <typeinfo> #include <typeinfo>
#include <string> #include <string>
#include <cassert> #include <cassert>
▲ Show 20 Lines • Show All 57 Lines • Show Last 20 Lines