This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2186R2 (Remove Garbage Collection)
ClosedPublic

Authored by philnik on Oct 30 2021, 5:44 AM.

Details

Diff Detail

Event Timeline

philnik requested review of this revision.Oct 30 2021, 5:44 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2021, 5:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Oct 30 2021, 11:25 AM

This approach seems acceptable to me because I don't imagine that anyone really cares about the pointer safety facilities. However, it seems to deliberately take the bad middle path between two better extremes:

  • If we believe the rationale in the paper (and my own prejudices), I suggest that we should simplify libc++ by aggressively removing the facilities from all C++ versions, removing the ifdefs, removing the tests, removing the detail header, and release-noting it. (And removing the abilist entries and memory.cpp stuff? Or is that a bridge too far?)
  • If we truly believe that there exists C++20 code in the wild that is using these facilities, then unconditionally dropping them from C++2b seems user-hostile. We should use the usual libc++ approach of allowing users to opt back in to removed features via _LIBCPP_ENABLE_CXX23_REMOVED_FEATURES/_LIBCPP_ENABLE_CXX23_REMOVED_POINTER_SAFETY.
libcxx/include/__memory/pointer_safety.h
0

Pre-existing: This comment should just be

#endif // !defined(_LIBCPP_CXX03_LANG) && _LIBCPP_STD_VER <= 20
0

These four functions are defined in src/memory.cpp, and thus in the abilists. Should we do anything special with them in either of those places? (I suspect "do nothing special" is correct, but I'm not sure.)

0

Please update the synopsis in <memory> with // removed in C++23 comments, too (or delete those lines entirely, in the event that we decide on the aggressive-removal option).

This revision now requires changes to proceed.Oct 30 2021, 11:25 AM
philnik updated this revision to Diff 383758.Nov 1 2021, 3:04 AM
philnik marked 2 inline comments as done.

I'm going with option 1 right now. If anybody thinks there are users I can revert the changes later.

  • Removed pointer_safety
ldionne requested changes to this revision.Nov 1 2021, 7:17 AM
ldionne added a subscriber: ldionne.

At first sight, I'm OK with this removal. I, too, can't imagine anyone relying on that code. However, I think we should still keep those symbols around because we have a hard policy of never removing symbols exported from the dylib (except in special circumstances where the symbol is also defined in any TU that uses the said function, like a function template that would end up being exported from the dylib by mistake).

Long story short, I would create a legacy_pointer_safety.cpp file and shove declare_reachable & friends in it. I would add a comment explaining that we don't ship that feature anymore, but we're still providing the symbols for strict backwards ABI compatibility.

This way, if someone has compiled code that (for some probably bad) reason relied on those symbols, their program will keep working when libc++ is updated from under their feet (e.g. when you update your OS on any platform that uses libc++). If someone used to rely on those and tries to compile their code against a new libc++, they will have to get rid of those references because the library won't provide the declarations anymore - but I think it's reasonable to request some minimal action if folks are recompiling their code. Finally, this solution is very low maintenance for us.

This revision now requires changes to proceed.Nov 1 2021, 7:17 AM
Quuxplusone requested changes to this revision.Nov 1 2021, 8:17 AM

LGTM mod Louis's comments! To fix the CI redness, you'll need to git rm the autogenerated header test file that is no longer being generated.
I think the new release note should go like this:

API Changes
-----------

The declarations of functions `declare_reachable`, `undeclare_reachable`, `declare_no_pointers`,
`undeclare_no_pointers`, and `get_pointer_safety` have been removed not only from C++2b but
from all modes. Their symbols are still provided by the dynamic library for the benefit of
existing compiled code. All of these functions have always behaved as no-ops.

(Technically get_pointer_safety is not a "no-op"; and undeclare_reachable is a template, not a function; but I don't think those technicalities are worth complicating the message.)

philnik updated this revision to Diff 383897.Nov 1 2021, 2:50 PM
  • Added legacy_pointer_safety.cpp for ABI compatiblilty

How do I add the symbols back into the library?

ldionne added inline comments.Nov 2 2021, 8:24 AM
libcxx/src/legacy_pointer_safety.cpp
1

Please don't add the name of the file. We're trying to move away from that.

12–13
17–20

I believe that should fix your ABI list issues.

philnik updated this revision to Diff 384162.Nov 2 2021, 10:44 AM
philnik marked 3 inline comments as done.

Applied comments

Quuxplusone added inline comments.
libcxx/src/legacy_pointer_safety.cpp
13

Remove bogus (if harmless) trailing dot in wg21.link./

philnik updated this revision to Diff 384543.Nov 3 2021, 11:40 AM
  • Removed .
philnik marked an inline comment as done.Nov 3 2021, 11:40 AM
ldionne accepted this revision.Nov 8 2021, 2:29 PM

Thanks.

Just for posterity, this doesn't create a precedent for backporting removals to all standard modes when a paper lands. This case is special -- our support for garbage collection has always been a joke and I think the library is better for not presenting this basically unimplemented API to users. At least we don't pretend that we implement something reasonable.

This revision is now accepted and ready to land.Nov 8 2021, 2:29 PM

I don't have commit permissions, so someone else has to commit it for me. "Nikolas Klauser" <nikolasklauser@berlin.de>

This revision was automatically updated to reflect the committed changes.

@philnik FYI There was a trivial merge conflict in the release notes.

Pawday added a subscriber: Pawday.Feb 25 2023, 3:36 AM

Why it doesn't rely on _LIBCPP_STD_VER?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2023, 3:36 AM

Why it doesn't rely on _LIBCPP_STD_VER?

As discussed in the paper, there is no known implementation of the strict pointer safety facilities, which means that every use of this is a no-op, and therefore probably wrong. Pretty much the only code you can find is inside tests or a standard library: https://sourcegraph.com/search?q=context:global+declare_reachable+file:%5C.cpp+-file:en%5C.cppreference%5C.com+-file:declare_reachable%5C.pass%5C.cpp+-file:memory%5C.cpp+-file:legacy_pointer_safety%5C.cpp&patternType=standard&sm=1&groupBy=repo

Why it doesn't rely on _LIBCPP_STD_VER?

As discussed in the paper, there is no known implementation of the strict pointer safety facilities, which means that every use of this is a no-op, and therefore probably wrong. Pretty much the only code you can find is inside tests or a standard library: https://sourcegraph.com/search?q=context:global+declare_reachable+file:%5C.cpp+-file:en%5C.cppreference%5C.com+-file:declare_reachable%5C.pass%5C.cpp+-file:memory%5C.cpp+-file:legacy_pointer_safety%5C.cpp&patternType=standard&sm=1&groupBy=repo

Does this mean if i set my compiler to -std=c++14 for example, and will include "memory" i would not get the full c++ 14 support (with std::declare_reachable for example)?

Why it doesn't rely on _LIBCPP_STD_VER?

As discussed in the paper, there is no known implementation of the strict pointer safety facilities, which means that every use of this is a no-op, and therefore probably wrong. Pretty much the only code you can find is inside tests or a standard library: https://sourcegraph.com/search?q=context:global+declare_reachable+file:%5C.cpp+-file:en%5C.cppreference%5C.com+-file:declare_reachable%5C.pass%5C.cpp+-file:memory%5C.cpp+-file:legacy_pointer_safety%5C.cpp&patternType=standard&sm=1&groupBy=repo

Does this mean if i set my compiler to -std=c++14 for example, and will include "memory" i would not get the full c++ 14 support (with std::declare_reachable for example)?

Technically yes, but I don't think anybody cares.

Why it doesn't rely on _LIBCPP_STD_VER?

As discussed in the paper, there is no known implementation of the strict pointer safety facilities, which means that every use of this is a no-op, and therefore probably wrong. Pretty much the only code you can find is inside tests or a standard library: https://sourcegraph.com/search?q=context:global+declare_reachable+file:%5C.cpp+-file:en%5C.cppreference%5C.com+-file:declare_reachable%5C.pass%5C.cpp+-file:memory%5C.cpp+-file:legacy_pointer_safety%5C.cpp&patternType=standard&sm=1&groupBy=repo

Does this mean if i set my compiler to -std=c++14 for example, and will include "memory" i would not get the full c++ 14 support (with std::declare_reachable for example)?

Yes, that is correct. However, I think that was widely seen as a mis-feature, hence why it was never implemented and then removed. Like @philnik said, libc++'s stance here is that we're better off making it a compilation error for people who try to use it than let it compile, but do nothing good. That technically makes us non-conforming in C++14 mode, however in practice it's better to be non-conforming than conforming (in that specific aspect).

libcxx/test/libcxx/diagnostics/detail.headers/memory/pointer_safety.module.verify.cpp