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.
Details
- Reviewers
rnk rsmith rjmccall erichkeane aaron.ballman ldionne Mordante MaskRay - Group Reviewers
Restricted Project - Commits
- rG2916b125f686: [clang] Enable sized deallocation by default in C++14 onwards
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
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 | ||
---|---|---|
6908 | Can this be Args.AddLastArg(CmdArgs, ...);? |
- Changes to Args.AddLastArg(CmdArgs, ...)
- Adds a note to clang/docs/ReleaseNotes.rst
- Fixs clangd test failure.
- Regenerate clang\test\AST\ast-dump-stmt-json.cpp and clang\test\AST\ast-dump-expr-json.cpp.
- Format code.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp | ||
---|---|---|
16–17 |
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
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 | ||
---|---|---|
6906–6907 |
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.)
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.
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?
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.
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 | ||
---|---|---|
14–15 | The comment you removed is still relevant and should be kept for these compilers, since it applies to them. | |
16–18 | 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. | |
18 | 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}} |
Beside the concern raised by platform maintainers: the cc1 default switch part should be made separately from the patch.
This makes revert easy and leaves fewer churn to the test suite.
If -fno-sized-deallocation is a better cc1 default (but not Driver's), I can make such a change.
- Disable sized deallocation for Apple targets.
- Update tests and don't use -fno-sized-deallocation any more.
- With one exception: clang/test/SemaCXX/builtin-operator-new-delete.cpp, which will generate two notes without line infos if using default settings.
Thanks! I ran tests with no error occurred on my local machine and I really want to know how to test it!
I added similar information yesterday, but I somehow didn't properly submit it.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp | ||
---|---|---|
18 | This // XFAIL: clang-12, clang-13 is still needed but should be // XFAIL: clang-13, clang-14. (Since the LLVM 14 release we only support these two versions.) For testing it's the easiest to remove line 147 of libcxx/utils/ci/buildkite-pipeline.yml That way all builds run. The Bootstrapping build builds clang and uses that clang to test libc++. That way we can validate which builds fail and which succeed. Maybe some more builds need to be (temporary) disabled. Once we verify that works we need to:
After the change has landed it will take some time before the change is propagated to the CI. Once it's propagated the followup patch can be landed. I'm willing to create the followup patch and land it at the proper time. |
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp | ||
---|---|---|
18 | Thanks! I will have a try later. :) |
clang/lib/Frontend/CompilerInvocation.cpp | ||
---|---|---|
4078–4082 ↗ | (On Diff #423555) |
(BTW I strongly support this patch, I just think we should do it properly on all platforms from the start :-)
Ideally, I think, we would set this up to work something like ObjCRuntime, where we're making queries to a common place that contains all the information necessary to decide what runtime features are available. In particular, we shouldn't treat Apple platforms as forever unique in providing a stable runtime interface with availability gating.
Now, we don't necessarily need the same complexity that ObjCRuntime supports, where the user can tell us to use a different runtime and runtime version from the default for the platform. On the other hand, maybe we want that, because it's a good way to deal with the compatibility problem that we have on non-Apple platforms. Users could tell us that they're targeting e.g. libsupc++ v4.8, and we could tell them in response that sized allocation isn't supported. And if we get them to tell us that, rather than "I have sized allocation support" specifically, it sets us up well to solve similar problems in the future.
I couldn't agree with you more, but I have no idea how to implement it. :-(
You mean that we may provide a option -fc++-runtime likes -fobjc-runtime, or extend -stdlib to specify version number in the form of -stdlib=libsupc++-v4.8?
Hmm. Allowing a version on -stdlib is intuitively appealing, but I'm not sure it actually gives us the information we need. As I recall, -stdlib selects the high-level stdlib and not the low-level one, and those are related in code but not necessarily at runtime; for example, you can (or at least could, historically) use libstdc++ on macOS, but the underlying low-level stdlib is going to be libc++abi, not libsupc++. And the low-level runtime is the one that actually provides global operator new functions. Is there a way to bridge that gap?
I was thinking about doing what we do for aligned allocation here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Darwin.cpp#L2652-L2673
I think this should work:
bool Darwin::isSizedDeallocationUnavailable() const { llvm::Triple::OSType OS; if (isTargetMacCatalyst()) return TargetVersion < sizedDeallocMinVersion(llvm::Triple::MacOSX); switch (TargetPlatform) { case MacOS: OS = llvm::Triple::MacOSX; break; case IPhoneOS: OS = llvm::Triple::IOS; break; case TvOS: OS = llvm::Triple::TvOS; break; case WatchOS: OS = llvm::Triple::WatchOS; break; } return TargetVersion < sizedDeallocMinVersion(OS); } // This could go in clang/include/clang/Basic/AlignedAllocation.h with a suitable rename, or another header inline llvm::VersionTuple sizedDeallocMinVersion(llvm::Triple::OSType OS) { switch (OS) { default: break; case llvm::Triple::Darwin: case llvm::Triple::MacOSX: // Earliest supporting version is 10.12. return llvm::VersionTuple(10U, 12U); case llvm::Triple::IOS: case llvm::Triple::TvOS: // Earliest supporting version is 10.0.0. return llvm::VersionTuple(10U); case llvm::Triple::WatchOS: // Earliest supporting version is 3.0.0. return llvm::VersionTuple(3U); case llvm::Triple::ZOS: return llvm::VersionTuple(); // All z/OS versions have no support. } llvm_unreachable("Unexpected OS"); }
Alternatively, @rjmccall 's approach might be better, but I'm not familiar with what's done for ObjCRuntime so I can't comment. I'm personally neutral on what approach is taken.
Hmm, that's really a tough nut. We have high-level standard libraries like libc++ and libstdc++, and low-level runtimes like libc++abi, libsupc++ and libcxxrt. There could be a lot of situations and we haven't think about MSVC yet.
I don't know if adding an option -cxxabilib=libsupc++-v4.8 or something like this would make sense, but I think we should make another revision to do it. I will apply @ldionne 's solution first.
Currently, no.
If someone is interesting in this, please feel free to commandeer. :-)
Changing the default improves GCC compatibility (Enabled by default under -std=c++14 and above.) and I think is the right direction.
@wangpc You can use your new username to commandeer this revision?
To other reviewers, what's still blocked now that the Apple target issue appears to be addressed?
Note, we could also keep Apple behavior unchanged in this patch and let folks more familiar with it to handle the change.
We probably need to make a decision whether this should be the release/17.x branch or not.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/sized_delete_array14.pass.cpp | ||
---|---|---|
17 | Maybe as a pragmatic solution // TODO(mordante) fix this test after updating clang in Docker // UNSUPPORTED: clang-17 The same for the other failing test. Then I'll fix this once the libc++ CI can be updated with this fix included. | |
libcxx/utils/ci/buildkite-pipeline.yml | ||
148 ↗ | (On Diff #425443) | Please undo the changes to this file. |
I noticed some of the CI jobs are still failing with the patch, I didn't look into them.
I don't think they are related to this patch, so I rebased again. If still failed, I will try to fix them later.
I see changes in the libc++ ABI list output
Symbol added: _ZdlPvmSt11align_val_t {'name': '_ZdlPvmSt11align_val_t', 'type': 'FUNC', 'is_defined': False} Symbol added: _ZdlPvm {'name': '_ZdlPvm', 'type': 'FUNC', 'is_defined': False} SYMBOL REMOVED: _ZdlPvSt11align_val_t {'is_defined': False, 'name': '_ZdlPvSt11align_val_t', 'type': 'FUNC'} Summary Added: 2 Removed: 1 Changed: 0
There seems to be small change in the symbol name. What does the m in the added symbol mean?
_ZdlPvmSt11align_val_t - added _ZdlPvSt11align_val_t - remove
There is also a new symbol _ZdlPvm added.
clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp | ||
---|---|---|
29–31 | Here I see changes in the symbol name. I see issues in the libc++ ABI list too. |
m means unsigned long in mangled name (5.1.5.2 Builtin types), which is the size of sized deallocation.
$ c++filt _ZdlPvmSt11align_val_t _ZdlPvm operator delete(void*, unsigned long, std::align_val_t) operator delete(void*, unsigned long)
I am not familiar with libcxx, can you please help me to fix these tests? I hope we can catch up with the release of LLVM 17.
Looking at the ABI list we currently have
_ZdlPv -> operator delete(void*) // this one is kept _ZdlPvSt11align_val_t -> operator delete(void*, std::align_val_t) // this one is removed
You add the overloads
operator delete(void*, unsigned long, std::align_val_t) operator delete(void*, unsigned long)
Looking in the current WP http://eel.is/c++draft/replacement.functions#lib:new,operator the removed overload is still listed there.
Do you know why operator delete(void*) and operator delete(void*, unsigned long) are both available?
clang/include/clang/Driver/Options.td | ||
---|---|---|
3145 | Can you update the test test/Lexer/cxx-features.cpp and remove -fsized-deallocation in C++14 and newer? This should no longer be required. |
I don't know the details about C++ spec, but I think they should be available for runtimes with/without sized deallocations.
I see one libc++ failure, I have not looked into the other failures.
libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp | ||
---|---|---|
29 | The bootstrap build fails. Currently main is clang-18. Since you intend to backport the patch it needs to be disabled on clang-17 too. |
I think it's too late to put this into Clang 17. This is a significant amount of change to add to the release branch; I'd prefer if it stayed in Clang 18 to give it some extra time to bake.
I see the libc++ build failed. It seems like timeouts uploading artifacts, so not related to your patch. Can you rebase and give it another run on the CI? (I didn't look at the other build failures.)
What are the next steps here? Have concerns been addressed? The CI failures seem unrelated (libcxx tests is an AIX bot out of disk, clang CI is an interpreter test that seems unrelated, but please confirm).
We definitely need the libc++ reviewers to approve before we move forward. IMO. the clang bits look to be in reasonable shape in terms of code quality.
I'm happy with the libc++ changes, please wait a few days for @ldionne to react.
I leave the approval of the clang part to @aaron.ballman
libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp | ||
---|---|---|
29 | Based on the timing I'm no longer in favor of backporting this patch. For the future please mark comments as done when addressed. That makes reviewing easier. |
The removal of the markers in the libc++ tests breaks these test. FYI the tests are using a Clang build of apt.llvm.org and are not using this patch for testing.
What name and email address would you like us to use for patch attribution? @ldionne do you have any remaining concerns (I think all the ones you laid out have already been addressed, so if I don't hear back, I'll assume anything remaining can be handled post-commit)?
Thanks! I have commit access.
@ldionne do you have any remaining concerns (I think all the ones you laid out have already been addressed, so if I don't hear back, I'll assume anything remaining can be handled post-commit)?
I will leave it for some days and commit it before Friday if it's OK. :-)
Oh great! Yeah, I think committing later in the week if we don't hear back is perfectly reasonable.
Sorry for missing a couple of direct pings. I have no concerns with the patch, perhaps I should not have marked my review as blocking in the first place. Thanks for doing this work, it's about time we updated this default!
Thanks all! I will land this patch later.
If there are some failures (especially libcxx part @Mordante :-) ), please help me to fix them. Thanks in advance!
FYI this resulted in some pretty wild code size swings, in particular between -10% and -15% for tramp3d-v4 (http://llvm-compile-time-tracker.com/compare.php?from=6cde64a94986165547ae5237ac7dd4bddfc9f2a7&to=2916b125f686115deab2ba573dcaff3847566ab9&stat=size-text). Not sure whether that's an expected result of this change or not.
Looks like this breaks tests on windows: http://45.33.8.238/win/83485/step_7.txt
Please take a look and revert for now if it takes a while to fix.
(Also, if the patch doesn't already do it, it probably shouldn't change defaults in clang-cl mode?)
This caused some linking errors with the GPU libc test suite, see https://lab.llvm.org/staging/#/builders/247/builds/5659.
clang++: error: ld.lld command failed with exit code 1 (use -v to see invocation) [331/473] Linking CXX executable libc/test/src/__support/libc.test.src.__support.uint_test.__hermetic__.__build__ FAILED: libc/test/src/__support/libc.test.src.__support.uint_test.__hermetic__.__build__ : && /home/ompworker/bbot/openmp-offload-libc-amdgpu-runtime/llvm.build/./bin/clang++ --target=x86_64-unknown-linux-gnu -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -Wl,--color-diagnostics -nostdlib -static libc/startup/gpu/amdgpu/CMakeFiles/libc.startup.gpu.amdgpu.crt1.dir/start.cpp.o libc/test/src/__support/CMakeFiles/libc.test.src.__support.uint_test.__hermetic__.__build__.dir/uint_test.cpp.o -o libc/test/src/__support/libc.test.src.__support.uint_test.__hermetic__.__build__ libc/test/UnitTest/libLibcTest.hermetic.a libc/test/UnitTest/libLibcHermeticTestSupport.hermetic.a libc/test/src/__support/liblibc.test.src.__support.uint_test.__hermetic__.libc.a -mcpu=gfx906 --target=amdgcn-amd-amdhsa -flto -Wl,-mllvm,-amdgpu-lower-global-ctor-dtor=0 && : ld.lld: error: undefined symbol: operator delete(void*, unsigned long) >>> referenced by lto.tmp:(LlvmLibcUIntClassTest_ConstructorFromUInt128Tests::~LlvmLibcUIntClassTest_ConstructorFromUInt128Tests()) >>> referenced by lto.tmp:(LlvmLibcUIntClassTest_ConstructorFromUInt128Tests::~LlvmLibcUIntClassTest_ConstructorFromUInt128Tests()) >>> referenced by lto.tmp:(LlvmLibcUIntClassTest_BasicArithmeticInt128Tests::~LlvmLibcUIntClassTest_BasicArithmeticInt128Tests()) >>> referenced 41 more times >>> did you mean: operator delete(void*) >>> defined in: lto.tmp clang++: error: ld.lld command failed with exit code 1 (use -v to see invocation)
The two failed cases are about Interpreter, which is not related to clang-cl mode I think? Is it because the windows environment doesn't support sized deallocation?
I don't know how to fix it, can someone help me?
It seems that the linker can't find sized deallocator (no support in the environment or AMDGPU libraries?).
I'm surprised. From the tests diff in this patch, it should increase code size I think.
We should have some implementations here I thought https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/new.cpp, maybe @sivachandra can elucidate on that. I actually don't know what the expected behavior is here, since we don't really have any of this support on GPU targets. I'm wondering why we're currently fine with compiling delete but not with the sized version, since their definitions should both be present in that file.
I've no idea what's happening but it seems like linking compiler-rt/lib/memprof/tests/MemProfUnitTests fails with this commit:
[17/17] Linking CXX executable compiler-rt/lib/memprof/tests/MemProfUnitTests FAILED: compiler-rt/lib/memprof/tests/MemProfUnitTests : && /repo/uabelho/dev-main/llvm/build-all-builtins/./bin/clang++ --target=x86_64-unknown-linux-gnu [...] compiler-rt/lib/memprof/tests/MemProfUnitTests -lstdc++ -ldl && : clang++: error: unable to execute command: Segmentation fault (core dumped) clang++: error: linker command failed due to signal (use -v to see invocation) ninja: build stopped: subcommand failed.
Good afternoon from the UK!
It looks as though this change has caused the following buildbot to start failing:
https://lab.llvm.org/buildbot/#/builders/216/builds/26407
are you able to take a look?
Thanks in advance,
Tom W
Because the issues have been ongoing for a few hours now, I think it'd make sense to revert these changes while trying to determine what the appropriate fix is. @wangpc would you mind doing the revert?
In D112921#4624674, @aaron.ballman wrote:
Because the issues have been ongoing for a few hours now, I think it'd make sense to revert these changes while trying to determine what the appropriate fix is. @wangpc would you mind doing the revert?Yeah I think we should. Can you revert it for me? I am away from my computer now.
Sure can -- I've reverted in a02f9a7756e5e4c76b422f9948d5dc3f56a1d139 and am reopening the review. Thanks!
clang/include/clang/Basic/SizedDeallocation.h | ||
---|---|---|
24 | If this is only used by clangDriver, I am not sure we want to expose it as a public header under include/. |
Does this need to be in Basic/? It's only used by clang/lib/Driver/ToolChains/Darwin.cpp