Skip to content

[libc++][test] Updates sized deallocation tests. #97833

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.

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

Merged

Conversation

mordante
Copy link
Member

@mordante mordante commented Jul 5, 2024

In #90373 size deallocation was enabled by default. Some test were disabled to propagate the clang changes to the libc++ CI. These changes have been propagated so the test filter can be updated.

In llvm#90373 size deallocation was enabled by default. Some test were
disabled to propagate the clang changes to the libc++ CI. These changes
have been propagated so the test filter can be updated.
@mordante mordante marked this pull request as ready for review July 5, 2024 18:20
@mordante mordante requested a review from a team as a code owner July 5, 2024 18:20
@mordante
Copy link
Member Author

mordante commented Jul 5, 2024

@wangpc-pp FYI this updates the libc++ sized deallocation tests.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 5, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

In #90373 size deallocation was enabled by default. Some test were disabled to propagate the clang changes to the libc++ CI. These changes have been propagated so the test filter can be updated.


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

3 Files Affected:

  • (modified) libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp (+2-2)
  • (modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp (+3-2)
  • (modified) libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp (+3-2)
diff --git a/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp b/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
index aa3ce210e3638d..ef04ccddf1835c 100644
--- a/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
+++ b/libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp
@@ -21,8 +21,8 @@
 // GCC doesn't support the aligned-allocation flags.
 // XFAIL: gcc
 
-// TODO(mordante) fix this test after updating clang in Docker
-// UNSUPPORTED: clang-15, clang-16, clang-17, clang-18, clang-19
+// These compiler versions do not have proper sized deallocation support.
+// UNSUPPORTED: clang-17, clang-18
 
 // RUN: %{build} -faligned-allocation -fsized-deallocation
 // RUN: %{run}
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 0241e7cefcac3d..dc8254680310cd 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
@@ -8,8 +8,9 @@
 
 // test sized operator delete[] replacement.
 
-// TODO(mordante) fix this test after updating clang in Docker
-// UNSUPPORTED: clang-15, clang-16, clang-17, clang-18, clang-19
+// These compiler versions do not have proper sized deallocation support.
+// UNSUPPORTED: clang-17, clang-18
+
 // UNSUPPORTED: sanitizer-new-delete, c++03, c++11
 // XFAIL: apple-clang
 // XFAIL: using-built-library-before-llvm-11
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 2ab691618ea46d..a03fc9f3e8266e 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
@@ -8,8 +8,9 @@
 
 // test sized operator delete replacement.
 
-// TODO(mordante) fix this test after updating clang in Docker
-// UNSUPPORTED: clang-15, clang-16, clang-17, clang-18, clang-19
+// These compiler versions do not have proper sized deallocation support.
+// UNSUPPORTED: clang-17, clang-18
+
 // UNSUPPORTED: sanitizer-new-delete, c++03, c++11
 // XFAIL: apple-clang
 // XFAIL: using-built-library-before-llvm-11

@mordante mordante merged commit 5aa8ef8 into llvm:main Jul 6, 2024
57 of 58 checks passed
@mordante mordante deleted the review/adjusts_tests_for_sized_deallocation branch July 6, 2024 09:56
@mstorsjo
Copy link
Member

This isn't quite right for all platforms. Yes, #90373 did enable sized deallocation on all platforms except z/OS (and Darwin, when targeting older versions). Since #97076 and #97232, we also disabled it for AIX and MinGW. So this PR breaks running the libcxx tests with a fresh clang-19 on these platforms.

I guess the right thing to do here is to add // UNSUPPORTED for those platforms, in these tests?

@wangpc-pp
Copy link
Contributor

I guess the right thing to do here is to add // UNSUPPORTED for those platforms, in these tests?

Yes, I think we should do this.

@xingxue-ibm
Copy link
Contributor

I guess the right thing to do here is to add // UNSUPPORTED for those platforms, in these tests?

Since these test the behavior of sized deallocation but incorrectly assume that -fsized-deallocation is the default for all platforms, another option is to explicitly specify -fsized-deallocation as follows.

// ADDITIONAL_COMPILE_FLAGS: -fsized-deallocation

@xingxue-ibm
Copy link
Contributor

xingxue-ibm commented Jul 15, 2024

The affected test cases are under the std sub-directory because the standard expects -fsized-deallocation to be the default. So it is better to XFAIL those test cases for targets where -fno-sized-deallocation is the default. With XFAIL we will get unexpectedly pass once the affected targets change the default. I will post a PR for that shortly.

@ldionne
Copy link
Member

ldionne commented Jul 15, 2024

The affected test cases are under the std sub-directory because the standard expects -fsized-deallocation to be the default. So it is better to XFAIL those test cases for targets where -fno-sized-deallocation is the default. With XFAIL we will get unexpectedly pass once the affected targets change the default. I will post a PR for that shortly.

Yes, this would be my preferred approach as well. I'd like us to move away from passing -fwhatever as much as possible. Of course we sometimes have to, but since Clang now does the right thing by default (apart from exceptions as discussed above), I would XFAIL the platforms on which it "doesn't behave correctly".

@xingxue-ibm
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants