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

[pcwang-thead]

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.

[pcwang-thead]

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

[aaron.ballman]

Adding some reviewers for visibility.

[pcwang-thead]

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]

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

6558

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

[pcwang-thead]

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

[pcwang-thead]

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

[pcwang-thead]

• Regenerate clang\test\AST\ast-dump-stmt-json.cpp and clang\test\AST\ast-dump-expr-json.cpp.
• Format code.

[ldionne]

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

13–16

[pcwang-thead]

Makes required changes

[rnk]

Looks good from my end.

[pcwang-thead]

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

[zixuan-wu]

Is this going to be reviewed again or committed?

[kwk]

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

6556–6557

@rjmccall mentioned here that

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

Is this considered at all?

[Mordante]

In D112921#3126492, @zixuan-wu wrote:

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]

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.

[rnk]

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?

[rjmccall]

In D112921#3127767, @rnk wrote:

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]

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.

13–17

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.

15

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

[MaskRay]

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.

[pcwang-thead]

• 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.

[pcwang-thead]

In D112921#3453114, @MaskRay wrote:

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.

That will be helpful, thanks!

[Mordante]

I didn't look at the code, but I have some hints how we can test libc++.

[pcwang-thead]

In D112921#3462592, @Mordante wrote:

I didn't look at the code, but I have some hints how we can test libc++.

Thanks! I ran tests with no error occurred on my local machine and I really want to know how to test it!

[Mordante]

In D112921#3463887, @pcwang-thead wrote:

In D112921#3462592, @Mordante wrote:

I didn't look at the code, but I have some hints how we can test libc++.

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

17

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
https://github.com/llvm/llvm-project/blob/afcc6baac52fcc91d1636f6803f5c230e7018016/libcxx/utils/ci/buildkite-pipeline.yml#L147

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:

• undo the buildkite changes
• disable this test temporary (UNSUPPORTED: clang-15)
• create a followup patch to reenable the test

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.

[pcwang-thead]

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

17

Thanks! I will have a try later. :)

[ldionne]

clang/lib/Frontend/CompilerInvocation.cpp

4078–4082

Why not implement it correctly from the start? @arphaman @ahatanak What would be the right incantation to enable sized deallocation starting in macOS 10.12, iOS 10.0, watchOS 3.0, and tvOS 10.0?

[ldionne]

(BTW I strongly support this patch, I just think we should do it properly on all platforms from the start :-)

[rjmccall]

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.

[pcwang-thead]

In D112921#3473022, @ldionne wrote:

(BTW I strongly support this patch, I just think we should do it properly on all platforms from the start :-)

I couldn't agree with you more, but I have no idea how to implement it. :-(

In D112921#3473080, @rjmccall wrote:

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.

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?

[rjmccall]

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?

[ldionne]

In D112921#3473632, @pcwang-thead wrote:

In D112921#3473022, @ldionne wrote:

(BTW I strongly support this patch, I just think we should do it properly on all platforms from the start :-)

I couldn't agree with you more, but I have no idea how to implement it. :-(

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.

[pcwang-thead]

In D112921#3474964, @rjmccall wrote:

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?

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.

[pcwang-thead]

• Add handling of Apple targets.
• Update libc++ tests.

[Romain-Geissler-1A]

Hi,

Is there any update about this ?

[pcwang-thead]

In D112921#4003939, @Romain-Geissler-1A wrote:

Hi,

Is there any update about this ?

Currently, no.
If someone is interesting in this, please feel free to commandeer. :-)