Page MenuHomePhabricator

LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

Authored by Quuxplusone on May 24 2018, 2:02 PM.



new_delete_resource().allocate(n, a) has basically two permissible results:

  • Return an appropriately sized and aligned block.
  • Throw bad_alloc.

Before this patch, libc++'s new_delete_resource would do a third and impermissible thing, which was to return an appropriately sized but inappropriately under-aligned block. This is now fixed.

(This came up while I was stress-testing unsynchronized_pool_resource on my MacBook. If we can't trust the default resource to return appropriately aligned blocks, pretty much everything breaks. For similar reasons, I would strongly support just patching __libcpp_allocate directly, but I don't care to die on that hill, so I made this patch as a <memory_resource>-specific workaround.)

Diff Detail

rCXX libc++

Event Timeline

Oops. If we do get an underaligned result, let's not leak it!

@EricWF ping; is this a reasonable solution to LWG 2843 on platforms without aligned new and delete?

EricWF accepted this revision.Jul 3 2018, 7:18 PM

LGTM. I'll add a test when I commit this.

This revision is now accepted and ready to land.Jul 3 2018, 7:18 PM
EricWF requested changes to this revision.Jul 3 2018, 7:28 PM

Actually, I spotted a couple of issues. Requesting changes.


Wait, can't this be written reinterpret_cast<uintptr_t>(ptr) % align == 0?


Also, I'm not sure about the size != 0 check. The returned pointer may be non-null and incorrectly aligned.

This revision now requires changes to proceed.Jul 3 2018, 7:28 PM
Quuxplusone added inline comments.Jul 3 2018, 7:49 PM

Yes on sane platforms, but that's technically implementation-defined behavior: the low bits of the uintptr_t may not correspond to the "alignment" of the original pointer (for example, on some hypothetical tagged-pointer architecture?). I don't know if libc++ cares about those platforms, but it seems like std::align was added specifically to solve this problem in a portable way, so I thought it would be best to use it.

If you reply and say "yeah but please change it anyway," I'll change it anyway. :)


This was covered in my commit message/summary: "If n == 0, return an unspecified result."
However, I am chagrined to state that I have no idea where I got that idea! I can't find support for that anywhere in the current Standard (although I'm not sure if I missed it).

We must choose here between "allocating zero bytes sometimes returns an underaligned pointer" and "allocating zero bytes sometimes throws bad_alloc." Neither one seems like good QoI. But as I no longer see why I thought "underaligned pointer" was permitted, I will change it to "sometimes throws bad_alloc" and re-upload.

Updated to no longer check "size != 0".
Also rolled in some drive-by cosmetic refactoring that I'd done later in my branch: these functions aren't in a public header and don't need to be uglified.

Quuxplusone added inline comments.Jul 3 2018, 7:58 PM

Hm! This and your other comment play into each other unfortunately well. When size == 0, the pointer we get back from __libcpp_allocate actually points to zero bytes, which means that when I call std::align on it, I invoke undefined behavior.

My new theory is "I don't care about that undefined behavior." I'd still rather take undefined-behavior-in-a-rare-case over implementation-defined-behavior-in-all-cases, and I'm too lazy to implement implementation-defined-behavior-only-in-a-rare-case unless you tell me to. :P

Quuxplusone edited the summary of this revision. (Show Details)

Remove some incorrect noexcept from <experimental/memory_resource>.

I noticed this because the calls to __clang_call_terminate showed up on Godbolt. But the pessimization is still there even without these noexcept! Basically the flow is this:

std::pmr::vector::~vector (noexcept(true))
- polymorphic_allocator::destroy (noexcept(X) but inlineable)
  - ~T (noexcept(true))
- polymorphic_allocator::deallocate (noexcept(X) but inlineable)
  - memory_resource::deallocate (noexcept(false) and not inlineable)

where "X" is "true" before this patch and "false" afterward.

Even for monotonic_buffer_resource, we can't conclude that deallocation will never throw, because deallocation *might* involve the upstream resource, which we don't know anything about.

Fix the "zero-byte allocation might be misaligned" issue by just never trying to allocate zero bytes — try to allocate 1 byte instead.

Quuxplusone marked 5 inline comments as done.Jul 21 2018, 11:47 AM
Quuxplusone added inline comments.

Now fixed. (The fix is always in the last place you look!)

Quuxplusone marked 2 inline comments as done.Jul 21 2018, 11:47 AM
Quuxplusone edited the summary of this revision. (Show Details)
Quuxplusone added a reviewer: ldionne.

Rebased on master. Added @ldionne as reviewer.

Rebased on master. @EricWF @ldionne ping please?

EricWF requested changes to this revision.Dec 9 2018, 7:01 PM

I don't think this patch is correct or desirable any longer.

I think what we should do is throw an exception right away if __is_overaligned_for_new(align), and not even try to allocate. The behavior is quite surprising anyway. If new *happens* to return correctly aligned memory, then the call spuriously succeeds seemingly at random.


Please change it anyway. This isn't what std::align was meant to do. Plus std::align suffers from the same problems.


Oh boy... God knows if this call is correct since the value it's passing for align doesn't match the pointers alignment.


These whitespace/naming changes are unrelated.


These changes are all unrelated.

This revision now requires changes to proceed.Dec 9 2018, 7:01 PM
Quuxplusone edited the summary of this revision. (Show Details)

Address review comments.

Quuxplusone marked 4 inline comments as done.Dec 10 2018, 7:38 AM

Rebased on master. @EricWF ping! It's been quite a few months...

EricWF accepted this revision.Mar 6 2019, 1:21 PM

LGTM other than removing the destructor.


Why are you removing this?

This revision is now accepted and ready to land.Mar 6 2019, 1:21 PM
Quuxplusone marked an inline comment as done.Mar 6 2019, 1:30 PM
Quuxplusone added inline comments.

Just removing unnecessary cruft. Same reason I added override (and removed virtual) on the other methods, and removed protected:, and removed the __ sigilling on function parameters in this .cpp file.

I had done a lot more random cleanup originally, and then you said "unrelated, please revert" so I did, but I assume I just kept all the cleanups in this one class since I was touching this range of lines no matter what.

EricWF added inline comments.Mar 6 2019, 1:47 PM

I strongly prefer seeing explicit declarations of non-trivial or virtual destructors.

@Quuxplusone if this fixes 2843 can you update it in www/cxx2a_status.html?

  • Replace the unnecessary destructor in class __new_delete_resource_imp.
  • Add LWG2843 to the list of completed issues (thanks @zoecarver!)

Oops, really replace the destructor this time. (Hadn't git commited.)

Quuxplusone marked 2 inline comments as done.Mar 6 2019, 3:08 PM
EricWF added a comment.Mar 8 2019, 3:30 PM

@Quuxplusone Since the LLVM license has changed since you created this, I need you to confirm that you accept LLVM's new license.

@Quuxplusone Since the LLVM license has changed since you created this, I need you to confirm that you accept LLVM's new license.

Yes, I accept LLVM's new license.

EricWF closed this revision.Mar 8 2019, 4:37 PM

Committed as rr355763