Page MenuHomePhabricator

[clang] Enable sized deallocation by default in C++14 onwards
Needs RevisionPublic

Authored by pcwang-thead on Nov 1 2021, 4:12 AM.

Details

Reviewers
rnk
rsmith
rjmccall
erichkeane
aaron.ballman
ldionne
Group Reviewers
Restricted Project
Summary

Since C++14 has been released for about seven years and most standard
libraries have implemented sized deallocation functions, it's time to
make this feature default again.
For tests assumed that sized deallocation is disabled, just force them
to disable this feature by adding -fno-sized-deallocation.

Diff Detail

Unit TestsFailed

TimeTest
210 mslibcxx CI AArch64 > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_array::sized_delete_array14.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/include/c++/v1 -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/include/c++/v1 -I /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsized-deallocation -lc++experimental -nostdlib++ -L /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/lib -Wl,-rpath,/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/lib -lc++ -pthread -o /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/std/language.support/support.dynamic/new.delete/new.delete.array/Output/sized_delete_array14.pass.cpp.dir/t.tmp.exe
230 mslibcxx CI AArch64 > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_single::sized_delete14.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/include/c++/v1 -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/include/c++/v1 -I /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsized-deallocation -lc++experimental -nostdlib++ -L /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/lib -Wl,-rpath,/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/lib -lc++ -pthread -o /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-01/llvm-project/libcxx-ci/build/aarch64/std/language.support/support.dynamic/new.delete/new.delete.single/Output/sized_delete14.pass.cpp.dir/t.tmp.exe
220 mslibcxx CI AArch64 -fno-exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_array::sized_delete_array14.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsized-deallocation -lc++experimental -nostdlib++ -L /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -Wl,-rpath,/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/std/language.support/support.dynamic/new.delete/new.delete.array/Output/sized_delete_array14.pass.cpp.dir/t.tmp.exe
220 mslibcxx CI AArch64 -fno-exceptions > llvm-libc++-shared-cfg-in.std/language_support/support_dynamic/new_delete/new_delete_single::sized_delete14.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/sized_delete14.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -isystem /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support -include /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsized-deallocation -lc++experimental -nostdlib++ -L /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -Wl,-rpath,/home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildslave/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/std/language.support/support.dynamic/new.delete/new.delete.single/Output/sized_delete14.pass.cpp.dir/t.tmp.exe
80 mslibcxx CI Apple back-deployment macosx10.9 > libc++.std/language_support/support_dynamic/new_delete/new_delete_array::sized_delete_array14.pass.cpp
Script: -- : 'COMPILED WITH'; /Library/Developer/CommandLineTools/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -v --target=x86_64-apple-macosx10.9 -include /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.9/include/c++/v1 -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.9/libcxx/include/c++build -I/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_FORMAT -Wno-macro-redefined -D_LIBCPP_HAS_NO_INCOMPLETE_RANGES -fsized-deallocation -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.9/lib -Wl,-rpath,/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com-1/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.9/macos-roots/macOS/libc++/10.9 -L/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8…
View Full Test Results (22 Failed)

Event Timeline

pcwang-thead created this revision.Nov 1 2021, 4:12 AM
pcwang-thead requested review of this revision.Nov 1 2021, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 4:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix errors in clang-tools-extra\test\clang-tidy\checkers\misc-new-delete-overloads.cpp

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 5:12 AM
aaron.ballman added a subscriber: aaron.ballman.

Adding some reviewers for visibility.

Add -fno-sized-allocation to:

  • clang\test\AST\ast-dump-expr-json.cpp
  • clang\test\AST\ast-dump-stmt-json.cpp
  • clang\test\CodeGenCXX\dllimport.cpp
  • clang\test\SemaCXX\MicrosoftExtensions.cpp
  • clang-tools-extra/clangd/unittests/FindTargetTests.cpp
rnk added a comment.Nov 1 2021, 10:26 AM

Please add a note to clang/docs/ReleaseNotes.rst about the behavior change.

The clangd test failure seems related to this change, and the other ones could be as well.

clang/lib/Driver/ToolChains/Clang.cpp
6405

Can this be Args.AddLastArg(CmdArgs, ...);?

  • Changes to Args.AddLastArg(CmdArgs, ...)
  • Adds a note to clang/docs/ReleaseNotes.rst
  • Fixs clangd test failure.

Removes unnecessary -fno-sized-deallocation and some comments.

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 10:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
pcwang-thead marked an inline comment as done.
  • Regenerate clang\test\AST\ast-dump-stmt-json.cpp and clang\test\AST\ast-dump-expr-json.cpp.
  • Format code.
ldionne requested changes to this revision.Nov 2 2021, 5:56 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp
15–24
This revision now requires changes to proceed.Nov 2 2021, 5:56 AM

Makes required changes

rnk accepted this revision.Nov 2 2021, 12:32 PM

Looks good from my end.

pcwang-thead marked an inline comment as done.

Adds ADDITIONAL_COMPILE_FLAGS and guard macros to:

  • libcxx\test\std\language.support\support.dynamic\new.delete\new.delete.single\sized_delete14.pass.cpp
  • libcxx\test\std\language.support\support.dynamic\new.delete\new.delete.array\sized_delete_array14.pass.cpp

Is this going to be reviewed again or committed?

kwk added a subscriber: kwk.Nov 12 2021, 1:47 AM

You've disabled sized deallocation in some tests by providing -fno-sized-deallocation . I admit I haven't looked at those tests in detail but I would like to understand if those tests would otherwise fail. If they fail, is there work that needs to be done later to make them work again?

clang/lib/Driver/ToolChains/Clang.cpp
6403

@rjmccall mentioned here that

Apple would like this to only be enabled conditionally based on deployment target.

Is this considered at all?

Is this going to be reviewed again or committed?

This patch still requires approval by the libc++ group.
The last build failed for multiple libc++ CI jobs, these should be fixed.
(I haven't looked at the code.)

rjmccall requested changes to this revision.Nov 12 2021, 9:23 AM

Yeah, I think this either needs deployment restriction on Apple platforms or it needs the thunk / weak-linking approach from the original patch when the target OS isn't recent enough. @ldionne should know the right OS versions.

This revision now requires changes to proceed.Nov 12 2021, 9:23 AM
rnk added a comment.Nov 12 2021, 9:36 AM

Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467, which is why the code was removed.

The next steps are to sort out right defaults for Apple and understand the libc++ test failures. Would it be reasonable to take a shortcut here, leave the feature disabled for Apple targets, and defer those details to those that own the target?

Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467, which is why the code was removed.

The weak function that intercedes if the strong function isn't found statically was, yeah, a poorly thought out idea. Weak *imports* work reliably, at least on Darwin, but they do require SDK support.

The next steps are to sort out right defaults for Apple and understand the libc++ test failures. Would it be reasonable to take a shortcut here, leave the feature disabled for Apple targets, and defer those details to those that own the target?

Yes, I think that would be acceptable.

ldionne requested changes to this revision.Nov 12 2021, 11:50 AM

Regarding the back-deployment target for Apple, I believe the first OSes where libc++.dylib had sized deallocation are macOS 10.12, iOS 10.0, watchOS 3.0, and tvOS 10.0. We should be able to enable reliance on sized deallocation functions automatically whenever the deployment target is greater than or equal to these versions.

The comments I left in the test file apply to both test files.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp
12–13

The comment you removed is still relevant and should be kept for these compilers, since it applies to them.

15–16

This shouldn't be necessary anymore if it is enabled by default in Clang trunk, no? And in fact we definitely don't want to add these flags here, since we're trying to test the default Clang behavior. I think that's why CI is failing with a bunch of XPASSes.

17

We will also need to add the following annotation to express that the test is expected to fail when run on targets where sized deallocation wasn't available yet:

// Sized deallocation was added in macOS 10.12 and aligned OSes.
// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11}}
miyuki added a subscriber: miyuki.Dec 1 2021, 6:06 AM