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

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.

[• Quuxplusone]

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.

[• 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

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]

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

31

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

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.

[• 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

29

Why are you removing this?

[• Quuxplusone]

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]

src/experimental/memory_resource.cpp

29

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
 
 //memory_resource::~memory_resource() {}
 
 // new_delete_resource()
 
 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 *do_allocate(size_t size, size_t align) override {
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+        if (__is_overaligned_for_new(align))
+            __throw_bad_alloc();
+#endif
+        return _VSTD::__libcpp_allocate(size, align);
+    }
 
-    virtual void do_deallocate(void* __p, size_t __n, size_t __align) {
-      _VSTD::__libcpp_deallocate(__p, __n, __align); /* FIXME */
+    void do_deallocate(void *p, size_t n, size_t align) override {
+      _VSTD::__libcpp_deallocate(p, n, align);
     }
 
-    virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-        { return &__other == this; }
+    bool do_is_equal(memory_resource const & other) const _NOEXCEPT override
+        { return &other == this; }
+
+public:
+    ~__new_delete_memory_resource_imp() override = default;
 };
 
 // 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;

www/cxx2a_status.html

-->
   <tr><td><a href="https://wg21.link/LWG2998">2998</a></td><td>Requirements on function objects passed to {forward_,}list-specific algorithms</td><td>Albuquerque</td><td><i>Nothing to do</i></td></tr>
   <tr><td><a href="https://wg21.link/LWG3001">3001</a></td><td>weak_ptr::element_type needs remove_extent_t</td><td>Albuquerque</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3024">3024</a></td><td>variant's copies must be deleted instead of disabled via SFINAE</td><td>Albuquerque</td><td></td></tr>
 
    <tr><td></td><td></td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2164">2164</a></td><td>What are the semantics of <tt>vector.emplace(vector.begin(), vector.back())</tt>?</td><td>Jacksonville</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2243">2243</a></td><td><tt>istream::putback</tt> problem</td><td>Jacksonville</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2816">2816</a></td><td><tt>resize_file</tt> has impossible postcondition</td><td>Jacksonville</td><td><i>Nothing to do</i></td></tr>
-  <tr><td><a href="https://wg21.link/LWG2843">2843</a></td><td>Unclear behavior of <tt>std::pmr::memory_resource::do_allocate()</tt></td><td>Jacksonville</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG2843">2843</a></td><td>Unclear behavior of <tt>std::pmr::memory_resource::do_allocate()</tt></td><td>Jacksonville</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2849">2849</a></td><td>Why does <tt>!is_regular_file(from)</tt> cause <tt>copy_file</tt> to report a "file already exists" error?</td><td>Jacksonville</td><td><i>Nothing to do</i></td></tr>
   <tr><td><a href="https://wg21.link/LWG2851">2851</a></td><td><tt>std::filesystem</tt> enum classes are now underspecified</td><td>Jacksonville</td><td><i>Nothing to do</i></td></tr>
   <tr><td><a href="https://wg21.link/LWG2946">2946</a></td><td>LWG 2758's resolution missed further corrections</td><td>Jacksonville</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2969">2969</a></td><td><tt>polymorphic_allocator::construct()</tt> shouldn't pass <tt>resource()</tt></td><td>Jacksonville</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2975">2975</a></td><td>Missing case for <tt>pair</tt> construction in scoped and polymorphic allocators</td><td>Jacksonville</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2989">2989</a></td><td><tt>path</tt>'s stream insertion operator lets you insert everything under the sun</td><td>Jacksonville</td><td>Completed</td></tr>
   <tr><td><a href="https://wg21.link/LWG3000">3000</a></td><td><tt>monotonic_memory_resource::do_is_equal</tt> uses <tt>dynamic_cast</tt> unnecessarily</td><td>Jacksonville</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3002">3002</a></td><td>[networking.ts] <tt>basic_socket_acceptor::is_open()</tt> isn't <tt>noexcept</tt></td><td>Jacksonville</td><td></td></tr>