This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove extension to support allocator<const T>
ClosedPublic

Authored by ldionne on Mar 4 2022, 6:57 AM.

Details

Summary

This extension is a portability trap for users, since no other standard
library supports it. Furthermore, the Standard explicitly allows
implementations to reject std::allocator<cv T>, so allowing it is
really going against the current.

This was discovered in D120684: this extension required const_casting
in __construct_range_forward, a fishy bit of code that can be removed
if we don't support the extension anymore.

This is a re-application of dbc647643577, which was reverted in 9138666f5
because it broke std::shared_ptr<T const>. Tests have now been added and
we've made sure that std::shared_ptr<T const> wouldn't be broken in this
version.

Diff Detail

Event Timeline

ldionne created this revision.Mar 4 2022, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 6:57 AM
ldionne requested review of this revision.Mar 4 2022, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 6:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/docs/ReleaseNotes.rst
69–72

I think this buries the lede a little bit. I'd say more like

- libc++ no longer supports containers of ``const``-qualified element type,
  such as ``vector<const T>`` and ``list<const T>``. This used to be supported
  as an extension. Likewise, ``std::allocator<const T>`` is no longer supported.
  If you were using ``vector<const T>``, replace it with ``vector<T>`` instead.

I actually wonder whether this change matters to list<const T>, since it's a node-based container. Should you add libcxx/test/libcxx/.../const_element_type.verify.cpp tests that check what happens with const-qualified element types? If list<const T> and set<const T> still work in practice, should we continue kinda-sorta-supporting them, and document here that vector and deque are the only two containers that have been de-supported?

ldionne updated this revision to Diff 413068.Mar 4 2022, 10:38 AM
ldionne marked an inline comment as done.

Address comments and fix CI.

This is the sort of thing to alert libc++-vendors about, right?

libcxx/include/__memory/allocator.h
73–74

Nit: Maybe swap these lines to alphabetize them? (also to put the shorter and more common cv-qualifier first)

libcxx/include/__memory/shared_ptr.h
291–293

Pre-existing: Why do we rebind the allocator at all? construct is supposed to work on any allocator in the same family, isn't it? (IIRC, it deduces the type-to-construct from the pointer type of __get_elem(), not from the type of __tmp.) Is this a QoI thing, or just "historical reasons"?

ldionne marked an inline comment as done.Mar 7 2022, 5:59 AM
ldionne added a subscriber: Restricted Project.
ldionne added inline comments.
libcxx/docs/ReleaseNotes.rst
69–72

[Sorry, it seems I didn't publish this reply the first time around]

I had to remove one instance of list<const T> in the tests because it broke, so I guess it means it doesn't work anymore either. Which makes sense, since std::allocator<const T> is not supposed to work, period. So regardless of the fact that std::list will rebind the allocator to the node type, you're still technically mentioning std::allocator<const T>, and so it's not valid.

I will reword the release note.

libcxx/include/__memory/shared_ptr.h
291–293

I'm not sure I understand. allocator_traits<>::construct(alloc, ptr, args...) will call alloc.construct(ptr, args...), and that requires ptr to be of type alloc.value_type [sic], doesn't it?

Quuxplusone added inline comments.Mar 7 2022, 7:58 AM
libcxx/include/__memory/shared_ptr.h
291–293

No, ptr is of type T*, where T is deduced independently:
https://en.cppreference.com/w/cpp/memory/allocator_traits/construct
allocate/deallocate are value_type-dependent, but construct/destroy are heterogeneous. Yes this makes no sense, but it's how it has always been. (I assume there was an original bad rationale related to node-based containers.)

ldionne added inline comments.Mar 7 2022, 9:42 AM
libcxx/include/__memory/shared_ptr.h
291–293

Hmm, I guess you're right. However, prior to C++11, std::allocator did use pointer instead of T*: https://en.cppreference.com/w/cpp/memory/allocator/construct

I suspect that is why it's like that. I'd be tempted to leave it as-is, at least in this patch. I can try to change it in a separate patch if you're curious.

ldionne accepted this revision as: Restricted Project.Mar 7 2022, 10:53 AM
This revision is now accepted and ready to land.Mar 7 2022, 10:53 AM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Mar 7 2022, 2:24 PM

After this change, gtest version that's used in LLVM is failing to build with:

In file included from /b/s/w/ir/x/w/llvm-llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:56:
In file included from /b/s/w/ir/x/w/staging/llvm_build/./bin/../include/c++/v1/memory:810:
/b/s/w/ir/x/w/staging/llvm_build/./bin/../include/c++/v1/__memory/allocator.h:72:5: error: static_assert failed due to requirement '!is_const<const testing::MatcherInterface<const std::string &>>::value' "std::allocator does not support const types"
    static_assert(!is_const<_Tp>::value, "std::allocator does not support const types");
    ^             ~~~~~~~~~~~~~~~~~~~~~

That broke compiler-rt runtimes whose tests use gtest: memprof, orc.

You can see the full log here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8820315548532839729/+/u/clang/build/stdout

After this change, gtest version that's used in LLVM is failing to build with:

In file included from /b/s/w/ir/x/w/llvm-llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:56:
In file included from /b/s/w/ir/x/w/staging/llvm_build/./bin/../include/c++/v1/memory:810:
/b/s/w/ir/x/w/staging/llvm_build/./bin/../include/c++/v1/__memory/allocator.h:72:5: error: static_assert failed due to requirement '!is_const<const testing::MatcherInterface<const std::string &>>::value' "std::allocator does not support const types"
    static_assert(!is_const<_Tp>::value, "std::allocator does not support const types");
    ^             ~~~~~~~~~~~~~~~~~~~~~

That broke compiler-rt runtimes whose tests use gtest: memprof, orc.

You can see the full log here: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8820315548532839729/+/u/clang/build/stdout

Thanks for the heads up. I'll need to add more tests for std::shared_ptr<T const>, it seems.

ldionne reopened this revision.Mar 8 2022, 6:09 AM
This revision is now accepted and ready to land.Mar 8 2022, 6:09 AM
ldionne updated this revision to Diff 413785.Mar 8 2022, 6:09 AM
ldionne edited the summary of this revision. (Show Details)

Add tests for std::shared_ptr and fix issues.

Quuxplusone accepted this revision.Mar 8 2022, 11:15 AM
Quuxplusone added inline comments.
libcxx/include/__memory/shared_ptr.h
285

AFAICT, we never used to instantiate TheUsersAllocator<_Tp> in pre-C++20 modes; this diff makes us instantiate that type. Pathological user-defined allocators might explode at that, so we might want to avoid the instantiation as a QoI issue. However, given that

  • we already instantiate that type in C++20, so this is just front-loading the pain (if any),
  • hopefully you'll quickly do a followup PR to eliminate the instantiation of _TpAlloc entirely,

I think this is fine.

291–293

However, prior to C++11, std::allocator did use pointer instead of T*

Ah. Well, that's definitely a good enough reason in C++03 codepaths, then... but this codepath is under _LIBCPP_STD_VER > 17! So yeah, I think it would be nice to change (and don't object to making that a separate PR if that's what you want).

I'll try landing this again now, since CI is green and I've added tests + fixed the issues reported by @phosek.

libcxx/include/__memory/shared_ptr.h
291–293

Alright, will do.

This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Mar 9 2022, 10:07 AM

This is the sort of thing to alert libc++-vendors about, right?

It seems this change is already causing problems for us. Google aims to uptake new versions of libc++ often. To support that, we are constantly testing new LLVM and libc++ versions ~daily. With this change, we can't continue testing new LLVM and libc++ versions. I estimate that it could take us a week to migrate our code, but that's a pretty soft estimate, I am not a domain expert. I would like to avoid a week long gap in our testing. Would you consider delaying this, setting up an opt-out configuration macro, or some other mitigation strategy to allow vendors to handle this transition gracefully?

ldionne added a subscriber: EricWF.Mar 9 2022, 1:58 PM

This is the sort of thing to alert libc++-vendors about, right?

It seems this change is already causing problems for us. Google aims to uptake new versions of libc++ often. To support that, we are constantly testing new LLVM and libc++ versions ~daily. With this change, we can't continue testing new LLVM and libc++ versions. I estimate that it could take us a week to migrate our code, but that's a pretty soft estimate, I am not a domain expert. I would like to avoid a week long gap in our testing. Would you consider delaying this, setting up an opt-out configuration macro, or some other mitigation strategy to allow vendors to handle this transition gracefully?

Yes. I've been in touch with @EricWF about this. I did not expect this to cause so much trouble. I think we definitely want to make this change in the mid/long term, however we should also make breaking changes in a friendly way whenever possible. I will revert this for now and I'll propose a softer approach (probably with a escape hatch) soon.

MaskRay added a subscriber: MaskRay.EditedMar 9 2022, 3:12 PM

Thanks for giving a transition period:) We are actively working on the migration.

The good news is that std::allocator<const is extremely rare. The breakage mostly comes from container usage like: [[ https://codesearch.debian.net/search?q=std%3A%3Avector%3Cconst%5B+%5D%2B%5Cw%2B%28%3C%5B%5E%3E*%5D%2B%3E%5B+%5D*%29%3F%3E&literal=0 | std::vector<const[ ]+\w+(<[^>*]+>[ ]*)?> ]], which is rare as well because https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48101 libstdc++ since 8.0 has static_assert with diagnostics like error: static assertion failed: std::vector must have a non-const, non-volatile value_type (deque/forward_list/list/multiset/set are similar)

libcxx/include/memory