Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[libc++][test] XFAIL sized deallocation tests for AIX, z/OS, and MinGW #98960

Merged
merged 2 commits into from
Jul 18, 2024
Merged

[libc++][test] XFAIL sized deallocation tests for AIX, z/OS, and MinGW #98960

merged 2 commits into from
Jul 18, 2024

Conversation

xingxue-ibm
Copy link
Contributor

The sized deallocation test cases fail on AIX, z/OS, and MinGW because they default to -fno-sized-deallocation. This patch XFAILs these test cases for the affected targets. Once they change the default, we will get unexpectedly pass.

@xingxue-ibm xingxue-ibm added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 15, 2024
@xingxue-ibm xingxue-ibm self-assigned this Jul 15, 2024
@xingxue-ibm xingxue-ibm requested a review from a team as a code owner July 15, 2024 20:44
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 15, 2024

@llvm/pr-subscribers-libcxx

Author: Xing Xue (xingxue-ibm)

Changes

The sized deallocation test cases fail on AIX, z/OS, and MinGW because they default to -fno-sized-deallocation. This patch XFAILs these test cases for the affected targets. Once they change the default, we will get unexpectedly pass.

Full diff: https://github.com/llvm/llvm-project/pull/98960.diff

2 Files Affected:

  • (modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp (+3)
  • (modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp (+3)
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp
index 01467ca9911e1..15df87d279867 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp
@@ -15,6 +15,9 @@
 // XFAIL: apple-clang
 // XFAIL: using-built-library-before-llvm-11
 
+// AIX, z/OS, and MinGW default to -fno-sized-deallocation.
+// XFAIL: target={{.+}}-aix{{.*}}, target={{.+}}-zos{{.*}}, target={{.+}}-mingw32{{.*}}
+
 #include <new>
 #include <cstddef>
 #include <cstdlib>
diff --git a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp
index fd7f2dbabdacc..d3c736fcbf135 100644
--- a/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp
+++ b/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp
@@ -15,6 +15,9 @@
 // XFAIL: apple-clang
 // XFAIL: using-built-library-before-llvm-11
 
+// AIX, z/OS, and MinGW default to -fno-sized-deallocation.
+// XFAIL: target={{.+}}-aix{{.*}}, target={{.+}}-zos{{.*}}, target={{.+}}-mingw32{{.*}}
+
 #include <new>
 #include <cstddef>
 #include <cstdlib>

@xingxue-ibm xingxue-ibm mentioned this pull request Jul 15, 2024
Merged
@xingxue-ibm xingxue-ibm requested a review from zibi2 July 15, 2024 20:47
daltenty
daltenty approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@daltenty daltenty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

wangpc-pp
wangpc-pp approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!
(Do we have buildbots for these configurations?)

ldionne
ldionne reviewed Jul 16, 2024
View reviewed changes
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming CI passes. I'd be surprised if this passes on MinGW though since we were already testing this on MinGW and it didn't fail? I don't understand why. Either way, I'm happy if the CI is green, since this matches my expectation of how things should look.

@mstorsjo
Copy link
Member

LGTM. Thanks! (Do we have buildbots for these configurations?)

We don’t have full Buildbot for mingw - but we do have it in the precommit CI using an existing release of Clang.

LGTM assuming CI passes. I'd be surprised if this passes on MinGW though since we were already testing this on MinGW and it didn't fail? I don't understand why.

The test is UNSUPPORTED: clang-18, which covers the setup in CI - the issue only appears if testing with a nightly snapshot.

mstorsjo
mstorsjo requested changes Jul 16, 2024
View reviewed changes
...d/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp Outdated
@@ -15,6 +15,9 @@
// XFAIL: apple-clang
// XFAIL: using-built-library-before-llvm-11

// AIX, z/OS, and MinGW default to -fno-sized-deallocation.
// XFAIL: target={{.+}}-aix{{.*}}, target={{.+}}-zos{{.*}}, target={{.+}}-mingw32{{.*}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patterns for mingw here should be {{.*}}-windows-gnu - the triples get normalized into that form. Please grep for that and see which exact spelling we use elsewhere (can’t easily check myself ATM).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or {{.+}} for the arch obviously is fine too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Targets used for MinGW are as follows. I will change to use {{.+}}-windows-gnu as suggested. Thanks, @mstorsjo!

target=x86_64-w64-windows-gnu
target=i686-w64-windows-gnu

mstorsjo
mstorsjo approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@xingxue-ibm
Copy link
Contributor Author

LGTM. Thanks! (Do we have buildbots for these configurations?)

We don’t have full Buildbot for mingw - but we do have it in the precommit CI using an existing release of Clang.

LGTM assuming CI passes. I'd be surprised if this passes on MinGW though since we were already testing this on MinGW and it didn't fail? I don't understand why.

The test is UNSUPPORTED: clang-18, which covers the setup in CI - the issue only appears if testing with a nightly snapshot.

The AIX precommit CI uses an existing Clang release as well. I don't think there is precommit CI for z/OS.

@xingxue-ibm xingxue-ibm merged commit b7a6cca into llvm:main Jul 18, 2024
54 checks passed
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Jul 22, 2024
@xingxue-ibm @Harini0924
[libc++][test] XFAIL sized deallocation tests for AIX, z/OS, and MinGW (
343536e 
llvm#98960)

The sized deallocation test cases fail on AIX, z/OS, and MinGW because
they default to `-fno-sized-deallocation`. This patch XFAILs these test
cases for the affected targets. Once they change the default, we will
get `unexpectedly pass`.
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
@xingxue-ibm @sgundapa
[libc++][test] XFAIL sized deallocation tests for AIX, z/OS, and MinGW (
542a0b8 
llvm#98960)

The sized deallocation test cases fail on AIX, z/OS, and MinGW because
they default to `-fno-sized-deallocation`. This patch XFAILs these test
cases for the affected targets. Once they change the default, we will
get `unexpectedly pass`.
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@xingxue-ibm @yuxuanchen1997
[libc++][test] XFAIL sized deallocation tests for AIX, z/OS, and MinGW (
9f3fe58 
#98960)

Summary:
The sized deallocation test cases fail on AIX, z/OS, and MinGW because
they default to `-fno-sized-deallocation`. This patch XFAILs these test
cases for the affected targets. Once they change the default, we will
get `unexpectedly pass`.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstorsjo mstorsjo mstorsjo approved these changes

@ldionne ldionne ldionne approved these changes

@mordante mordante mordante approved these changes

@daltenty daltenty daltenty approved these changes

@zibi2 zibi2 zibi2 approved these changes

@wangpc-pp wangpc-pp wangpc-pp approved these changes

Assignees

@xingxue-ibm xingxue-ibm

Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@xingxue-ibm @llvmbot @mstorsjo @ldionne @mordante @daltenty @zibi2 @wangpc-pp