This is an archive of the discontinued LLVM Phabricator instance.

[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

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

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
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
20

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
20

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

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.

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

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
SeanP added a subscriber: SeanP.Jun 18 2021, 11:14 AM

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

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
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

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.