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

[• Quuxplusone]

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

[• Quuxplusone]

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

[• Quuxplusone]

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

[• Quuxplusone]

@EricWF ping!

[EricWF]

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

[EricWF]

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

src/experimental/memory_resource.cpp

29

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

48

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

[• Quuxplusone]

src/experimental/memory_resource.cpp

29

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

48

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.

[• Quuxplusone]

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]

src/experimental/memory_resource.cpp

48

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]

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.

[• Quuxplusone]

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

[• Quuxplusone]

src/experimental/memory_resource.cpp

48

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

[• Quuxplusone]

@EricWF ping?

[• Quuxplusone]

Rebased on master. Added @ldionne as reviewer.

[• Quuxplusone]

Rebased on master. @EricWF @ldionne ping please?

[EricWF]

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

29

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

62

These whitespace/naming changes are unrelated.

63

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

76

These changes are all unrelated.

[• Quuxplusone]

Address review comments.

[• Quuxplusone]

@EricWF ping!

[• Quuxplusone]

@EricWF ping!

[• Quuxplusone]

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

[EricWF]

LGTM other than removing the destructor.

src/experimental/memory_resource.cpp

30

Why are you removing this?

[• Quuxplusone]

src/experimental/memory_resource.cpp

30

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]

src/experimental/memory_resource.cpp

30

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

[zoecarver]

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

[• Quuxplusone]

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

[• Quuxplusone]

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

[EricWF]

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

[• Quuxplusone]

In D47344#1423466, @EricWF wrote:

@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]

Committed as rr355763

src/experimental/memory_resource.cpp

 //===------------------------ memory_resource.cpp -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "experimental/memory_resource"
+#include "memory"
 
 #ifndef _LIBCPP_HAS_NO_ATOMIC_HEADER
 #include "atomic"
 #elif !defined(_LIBCPP_HAS_NO_THREADS)
 #include "mutex"
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_LFTS_PMR
 
 // memory_resource
 
 //memory_resource::~memory_resource() {}
 
 // new_delete_resource()
 
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+static bool is_aligned_to(void *ptr, size_t align)
+{
+    void *p2 = ptr;
+    size_t space = 1;
+    void *result = _VSTD::align(align, 1, p2, space);
+    return (result == ptr);
+}
+#endif
+
 class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
     : public memory_resource
 {
 public:
     ~__new_delete_memory_resource_imp() = default;
 
 protected:
     virtual void* do_allocate(size_t __size, size_t __align)
-        { return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+    {
+        void *result = _VSTD::__libcpp_allocate(__size, __align);
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+        if (__size != 0 && !is_aligned_to(result, __align)) {
+            __throw_bad_alloc();
+        }
+#endif
+        return result;
+    }
 
     virtual void do_deallocate(void * __p, size_t, size_t __align)
-        { _VSTD::__libcpp_deallocate(__p, __align); /* FIXME */ }
+        { _VSTD::__libcpp_deallocate(__p, __align); }
 
     virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
         { return &__other == this; }
 };
 
 // null_memory_resource()
 
 class _LIBCPP_TYPE_VIS __null_memory_resource_imp
     : public memory_resource
 {
 public:
     ~__null_memory_resource_imp() = default;
 
 protected:
     virtual void* do_allocate(size_t, size_t) {
         __throw_bad_alloc();
     }
     virtual void do_deallocate(void *, size_t, size_t) {}
     virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
     { return &__other == this; }
 };
 
 namespace {
 
 union ResourceInitHelper {
   struct {
     __new_delete_memory_resource_imp new_delete_res;
     __null_memory_resource_imp       null_res;