Page MenuHomePhabricator

[libcxx] Make allocator<T>:allocate throw bad_array_new_length
ClosedPublic

Authored by miyuki on Sep 30 2021, 9:09 AM.

Details

Summary

Currently the member functions std::allocator<T>::allocate,
std::experimental::pmr::polymorphic_allocator::allocate and
std::resource_adaptor<T>::do_allocate throw an exception of type
std::length_error when the requested size exceeds the maximum size.

According to the C++ standard ([allocator.members]/4,
[mem.poly.allocator.mem]/1), std::allocator<T>::allocate and
std::pmr::polymorphic_allocator::allocate must throw a
std::bad_array_new_length exception in this case.

The patch fixes the issue with std::allocator<T>::allocate and changes
the type the exception thrown by
std::experimental::pmr::resource_adaptor<T>::do_allocate to
std::bad_array_new_length as well for consistency.

The patch resolves LWG 3237, LWG 3038 and LWG 3190.

Diff Detail

Event Timeline

miyuki requested review of this revision.Sep 30 2021, 9:09 AM
miyuki created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 9:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I like this, but @ldionne needs to look.

libcxx/docs/Status/Cxx20Issues.csv
165

Here and above: s/""/"14.0"/?

libcxx/include/experimental/memory_resource
187

For the record: Personally I don't think it matters whether we change experimental::pmr::polymorphic_allocator or not. I have adopted this patch into my next update of D89057 (std::pmr::polymorphic_allocator).

ldionne requested changes to this revision.Oct 1 2021, 9:30 AM

I'm fine with the change in itself, however if we introduce a new function in the dylib, we'll need to be clever about rolling it out. If someone tries to back-deploy to an older OS where that function isn't in the dylib, they'll get an error when their binary is loaded cause it'll rely on the non-existent symbol. This can be worked around by looking at the deployment target and only relying on the symbol if we're on a target that has the symbol, but that's more complicated. Instead, I would suggest just making this an inline function in the header, similarly to what we do for __throw_bad_any_cast and some others.

libcxx/include/new
151

For consistency?

This revision now requires changes to proceed.Oct 1 2021, 9:30 AM

The reason why I'm fine with this is that I expect few people will get broken by this. It's thrown on failure to allocate, which is a corner case (I mean, it's more of a corner case than something like out_of_range), and also I don't think a lot of people are expecting that you'd catch a failure to allocate with std::length_error.

Quuxplusone added inline comments.Oct 1 2021, 10:28 AM
libcxx/test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/allocate.pass.cpp
68

The reason why I'm fine with this is that I expect few people will get broken by this. It's thrown on failure to allocate, which is a corner case (I mean, it's more of a corner case than something like out_of_range), and also I don't think a lot of people are expecting that you'd catch a failure to allocate with std::length_error.

Btw, notice that bad_array_new_length inherits from (and thus can-be-caught-as) bad_alloc, which is great. The fact that length_error cannot be caught as bad_alloc explains why, on the left side of this diff, we had to catch the least-common-ancestor class exception.

Also btw, @miyuki, how about fixing the comment on line 64 while you're here?

miyuki added inline comments.Oct 4 2021, 3:20 AM
libcxx/include/new
151

The function __throw_bad_array_new_length is implemented in libstdc++, so it doesn't get defined when using libsupc++ as the ABI library. There is no __throw_bad_array_new_length function in libstdc++, but I thought that this might change in the future, so I named the function differently to avoid possible name clash.

But now I noticed that there is a __cxa_throw_bad_array_new_length function which is part of the Itanium ABI and is implemented in both libc++abi and libsupc++ (introduced in CXXABI_1.3.8, GCC 4.9). Should I use it instead?

miyuki updated this revision to Diff 376852.Oct 4 2021, 3:30 AM

Addressed Arthur's comments.

miyuki edited the summary of this revision. (Show Details)Oct 4 2021, 3:30 AM
miyuki marked 2 inline comments as done.
miyuki added inline comments.Oct 4 2021, 4:14 AM
libcxx/include/new
151

I meant to say "The function __throw_bad_alloc is implemented in libstdc++"

ldionne added inline comments.Oct 4 2021, 10:21 AM
libcxx/include/new
151

The issue I see with using __cxa_throw_bad_array_new_length is the same as with defining __libcpp_throw_bad_array_new_length in the dylib -- a program deployed against an OS where the libc++.dylib (well, libc++abi.dylib really) doesn't ship that symbol would fail to run.

Quuxplusone added inline comments.Oct 4 2021, 10:29 AM
libcxx/include/new
151

__cxa_throw_bad_array_new_length is part of the Itanium C++ ABI, so it should be available on most systems... but yeah, perhaps it wouldn't be there on Windows. And also, looking at git grep __cxa_ libcxx/, we can see that libc++ generally doesn't contain calls to __cxa_ functions, so there's no reason to start now.
I think an inline __throw_bad_array_new_length [no _libcpp_] defined in the header sounds like the most conventional, and thus best, answer.

ldionne requested changes to this revision.Oct 4 2021, 11:29 AM

Also, can we please add a release note for this?

Requesting changes so it shows up correctly in the queue.

This revision now requires changes to proceed.Oct 4 2021, 11:29 AM
miyuki updated this revision to Diff 377139.Oct 5 2021, 3:21 AM

Addressed Louis' comments.

miyuki marked 3 inline comments as done.Oct 5 2021, 3:22 AM
miyuki updated this revision to Diff 377793.Oct 7 2021, 3:23 AM

Rebased

miyuki updated this revision to Diff 377804.Oct 7 2021, 4:41 AM

Updated Linux ABI list

Quuxplusone accepted this revision as: Quuxplusone.Oct 11 2021, 11:30 AM

This LGTM; @ldionne ping?
Also, @miyuki, please re-upload the diff so that we get a clean buildkite run. Let's make sure this passes CI.

libcxx/docs/ReleaseNotes.rst
59–60
ldionne accepted this revision.Oct 12 2021, 8:04 AM

Thanks!

This revision is now accepted and ready to land.Oct 12 2021, 8:04 AM
miyuki updated this revision to Diff 380415.Oct 18 2021, 8:33 AM
  1. Rebased
  2. Fixed grammar in ReleaseNotes
This revision was landed with ongoing or failed builds.Oct 18 2021, 11:13 AM
This revision was automatically updated to reflect the committed changes.