Skip to content

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