This is an archive of the discontinued LLVM Phabricator instance.

[libc++][PMR] Move the pmr::memory_resource destructor into the dylib
ClosedPublic

Authored by philnik on Oct 18 2022, 12:44 PM.

Details

Summary

This avoids emitting the VTable of pmr::memory_resource in every TU.

Diff Detail

Event Timeline

philnik created this revision.Oct 18 2022, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 12:44 PM
philnik requested review of this revision.Oct 18 2022, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2022, 12:44 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 20 2022, 8:52 AM

I am surprised that we don't need to change the ABI list, can you look into why that's not needed?

I would assume it was already exported by the dylib because it wasn't marked with HIDE_FROM_ABI, but can you confirm that?

This revision is now accepted and ready to land.Oct 20 2022, 8:52 AM
philnik updated this revision to Diff 471777.Oct 29 2022, 1:07 PM

Rebased; updated ABI list

I am surprised that we don't need to change the ABI list, can you look into why that's not needed?

I would assume it was already exported by the dylib because it wasn't marked with HIDE_FROM_ABI, but can you confirm that?

The ABI lists actually need updating, the CI just failed before they were checked.

philnik updated this revision to Diff 471778.Oct 29 2022, 1:10 PM

Update changelog

philnik updated this revision to Diff 471836.Oct 30 2022, 8:00 AM

Rebased; Updated all abilists

EricWF requested changes to this revision.Oct 30 2022, 10:07 AM
EricWF added a subscriber: EricWF.

Doesn't this need additional availability markup for OS X?

This revision now requires changes to proceed.Oct 30 2022, 10:07 AM

Doesn't this need additional availability markup for OS X?

It does, but there are some bugs which doesn't allow us to add them currently. See D135813.

Also, this changes the initialization/atexit behavior of derived memory resource types.

Specifically, because the destructor is no longer defaulted, it's no longer trivial. So the compiler now needs to emit atexit destructors for global memory resources.

I'm not sure this change actually does what we want. I need a minute to test some things.

So I'm not convinced that we actually emit this vtable. At least not outside of the users control.

Can you provide me with example code that would cause the vtable for this type to be omitted inline?
(without providing a derived class with an inline constructor)

Also, this changes the initialization/atexit behavior of derived memory resource types.

Specifically, because the destructor is no longer defaulted, it's no longer trivial. So the compiler now needs to emit atexit destructors for global memory resources.

I'm not sure this change actually does what we want. I need a minute to test some things.

Are you sure about that? The class is already non-trivial because it declares virtual functions, and the only difference I can find here is that it doesn't call the destructor of S.

philnik updated this revision to Diff 471868.Oct 30 2022, 1:56 PM

XFAIL tests and update no exceptions abilist

This revision was not accepted when it landed; it landed in state Needs Review.Oct 31 2022, 4:37 PM
This revision was automatically updated to reflect the committed changes.