Page MenuHomePhabricator

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

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

Details

Summary

https://github.com/cplusplus/draft/commit/6216651aada9bc2f9cefe90edbde4ea9e32251ab

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

Event Timeline

Quuxplusone created this revision.May 24 2018, 2:02 PM

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.

src/experimental/memory_resource.cpp
27

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

31

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
src/experimental/memory_resource.cpp
27

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. :)

31

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
src/experimental/memory_resource.cpp
31

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.
src/experimental/memory_resource.cpp
31

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.

src/experimental/memory_resource.cpp
27

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

43

These whitespace/naming changes are unrelated.

45

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

61

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.

src/experimental/memory_resource.cpp
29

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.
src/experimental/memory_resource.cpp
29

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
src/experimental/memory_resource.cpp
29

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