This is an archive of the discontinued LLVM Phabricator instance.

C++14: Disable sized deallocation by default due to ABI breakage
ClosedPublic

Authored by rnk on Mar 19 2015, 2:47 PM.

Details

Summary

There are no widely deployed standard libraries providing sized
deallocation functions, so we have to punt and ask the user if they want
us to use sized deallocation. In the future, when such libraries are
deployed, we can teach the driver to detect them and enable this
feature.

N3536 claimed that a weak thunk from sized to unsized deallocation could
be emitted to avoid breaking backwards compatibility with standard
libraries not providing sized deallocation. However, this approach and
other variations don't work in practice.

With the weak function approach, the thunk has to have default
visibility in order to ensure that it is overridden by other DSOs
providing sized deallocation. Weak, default visibility symbols are
particularly expensive on MachO, so John McCall was considering
disabling this feature by default on Darwin. It also changes behavior
ELF linking behavior, causing certain otherwise unreferenced object
files from an archive to be pulled into the link.

Our second approach was to use an extern_weak function declaration and
do an inline conditional branch at the deletion call site. This doesn't
work because extern_weak only works on MachO if you have some archive
providing the default value of the extern_weak symbol. Arranging to
provide such an archive has the same challenges as providing the symbol
in the standard library. Not to mention that extern_weak doesn't really
work on COFF.

Diff Detail

Event Timeline

rnk updated this revision to Diff 22306.Mar 19 2015, 2:47 PM
rnk retitled this revision from to C++14: Disable sized deallocation by default due to ABI breakage.
rnk updated this object.
rnk added reviewers: rsmith, rjmccall.
rnk added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Mar 19 2015, 3:09 PM

I have one general concern: we used to provide sized deallocation outside of C++14 mode via -Xclang -fiszed-deallocation. With this patch, that no longer works, because -fsized-deallocation doesn't cause the sized deallocation functions to get implicitly declared any more.

Given that the implicit declarations are not really useful unless a sized deallocation function is known to be available, and that it seems overkill to have two flags for this feature, perhaps we should keep the implicit declarations and the usage thereof tied together under the same flag?

Please also update www/cxx_status.html and the release notes.

rnk added a comment.Mar 19 2015, 3:31 PM

I have one general concern: we used to provide sized deallocation outside of C++14 mode via -Xclang -fiszed-deallocation. With this patch, that no longer works, because -fsized-deallocation doesn't cause the sized deallocation functions to get implicitly declared any more.

Given that the implicit declarations are not really useful unless a sized deallocation function is known to be available, and that it seems overkill to have two flags for this feature, perhaps we should keep the implicit declarations and the usage thereof tied together under the same flag?

Makes sense. Doing that actually requires less change. :)

rnk updated this revision to Diff 22325.Mar 19 2015, 4:32 PM
rnk edited edge metadata.
  • Update cxx_status.html
  • Only provide sized operator delete with -fsized-deallocation
rsmith accepted this revision.Mar 19 2015, 4:53 PM
rsmith edited edge metadata.

LGTM, please update the release notes to mention this.

This revision is now accepted and ready to land.Mar 19 2015, 4:53 PM
This revision was automatically updated to reflect the committed changes.

Hi, I am wondering could -fsized-deallocation this be enabled by default nowadays in 2021?

rnk added a comment.Oct 29 2021, 11:20 AM

Hi, I am wondering could -fsized-deallocation this be enabled by default nowadays in 2021?

Yes, I think so. This should be a matter of adjusting the default in the driver. We should not bring back the code that defines the sized deallocation thunk, so a straight revert of this patch doesn't make sense.

I don't have time to work on this at the moment, but I would approve a patch.

Hi, I am wondering could -fsized-deallocation this be enabled by default nowadays in 2021?

Were you planning to work on this?

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

rnk added a comment.Nov 11 2021, 11:59 AM

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

It is already a flag, -fsized-deallocation. On some level, whatever default we choose is just a guess about the C++ library support level. Clang tries to encode all kinds of Linux distro-specific knowledge, and it's often wrong anyway. After all that logic, there will be some default. I think at this point the feature should default to being enabled.

In D8467#3125471, @rnk wrote:

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

It is already a flag, -fsized-deallocation. On some level, whatever default we choose is just a guess about the C++ library support level. Clang tries to encode all kinds of Linux distro-specific knowledge, and it's often wrong anyway. After all that logic, there will be some default. I think at this point the feature should default to being enabled.

Okay. I think that's fine for most platforms. Apple would like this to only be enabled conditionally based on deployment target.

Hi, I am wondering could -fsized-deallocation this be enabled by default nowadays in 2021?

Were you planning to work on this?

It's been working on at https://reviews.llvm.org/D112921

test/CodeGenCXX/cxx1y-sized-deallocation.cpp