[Bug libstdc++/103162] New: overconstrained std::pmr::memory_resource allocate/deallocate methods

[Bug libstdc++/103162] New: overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

            Bug ID: 103162
           Summary: overconstrained std::pmr::memory_resource
                    allocate/deallocate methods
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: florin.iucha at amd dot com
  Target Milestone: ---

std::pmr::memory_resource::allocate() is annotated with
__attribute__((__returns_nonnull__)) and
std::pmr::memory_resource::deallocate() is annotated with
__attribute__((__nonnull__)) .

Looking at https://en.cppreference.com/w/cpp/memory/memory_resource/allocate it
is not clear why the annotation is required.

I have implemented several specialized memory resources, which sometimes can be
used to manage pools in CPU space, sometimes from pools in other memory spaces,
where allocating from 0 is perfectly acceptable.

Checking https://eel.is/c++draft/mem.res.class, there's no mention of "null" in
the context of allocate/deallocate .

This attribute can be helpful to catch bugs in specialized memory resources in
general, but specifically in my case it is tripping the address sanitizer when
I'm returning null.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
"A derived class shall implement this function to return a pointer to allocated
storage (6.7.5.5.2) with a size of at least bytes, aligned to the specified
alignment."

And 6.7.5.5.2 says:

"An allocation function attempts to allocate the requested amount of storage.
If it is successful, it returns the address of the start of a block of storage
whose length in bytes is at least as large as the requested size. [...]
If the request succeeds, the value returned by a replaceable allocation
function is a non-null pointer value ([basic.compound]) p0 different from any
previously returned value p1, [...]"

It says it's non-null.

The attribute is not there to catch bugs in memory resources, it's there to
tell the compiler that the returned pointer is not null, and so it can optimize
accordingly.

In an environment where 0 is a valid address, does
-fno-delete-null-pointer-checks make any difference?

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

Florin Iucha <florin.iucha at amd dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #2 from Florin Iucha <florin.iucha at amd dot com> ---
Thank you Jonathan - adding the flag silences the sanitizer output, so I'll use
it henceforth.

-----------------

20.12.2.3 says nothing about not null, and it is the section about
std::pmr::memory_resource members. It indicates that it reports errors via
exceptions. It is still not clear to me which of the paragraphs in 6.7.5.5.2
truly apply here, but I can see how the reference from 20.12.2.3 could be
interpreted as implemented.

Are memory_resource members "allocation functions" or are the next layer up
(std::pmr::polymorphic_allocator) the "allocation functions" ?

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |FIXED

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yeah, it's a bit unclear. My reading is that neither is an allocation function,
it's just referring to [basic.stc.dynamic.allocation] for the definition of
"allocated storage". Although arguably the non-null requirement only applies to
allocated storage returned by "a replaceable allocation function".

I'll ask the C++ committee for clarification.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |INVALID

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oops, not sure how my browser changed the resolution to FIXED, reverting it to
INVALID.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-11-09

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think I've convinced myself that the non-null requirement doesn't apply here.
A custom memory_resource can do whatever it wants, so the attribute can't be
used here.

It can be used on the do_allocate overriders in the standard memory resources,
although that won't be very useful in practice unless the compiler is able to
devirtualize and inline the calls.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|NEW                         |RESOLVED

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The committee's view is that this function should not be able to return a null
pointer because you can't store objects there (so if that's not true for your
environment, use -fno-delete-null-pointer-checks to inform the compiler of your
special case).

I will create a library issue to fix the wording in the standard, to make the
semantics clear (without the misleading reference to the storage returned by
allocation functions).

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #7 from Florin Iucha <florin.iucha at amd dot com> ---
That is most unfortunate because it will force us to duplicate the library used
for resource management. Not only that, but if I were to implement the
pmr::memory_resource interface in terms of the other library which returns
uintptr_t/size_t (including the occasional 0), I would have to pay an
additional virtual method call just to wrap a static_cast / bit_cast.

There are uses for the memory_resources pattern, outside of its current
application in the standard library - for example allocating ranges in a file,
or in a shared memory segment. 0 is valid allocation, although obviously not
directly dereferenceable.

Also -fno-delete-null-pointer-checks is not helping me (I mistakenly thought it
silenced the sanitizer) because I am not directly dereferencing the result.
However the error I am getting is when I run the undefined behavior sanitizer
and it notices that the values returned from allocate() (retrieved from my
do_allocate) or passed back to deallocate() violate the attributes.

So I am left with three unsavory choices:
   1. patch the standard library to remove the checks;
   2. do not run undefined behavior sanitizer, or figure out some way to
distribute suppression files with the binaries;
   3. internally fork the libraries, with one flavor to be used with real
memory and real objects, the other with abstract "address ranges", and delegate
one implementation to the other, paying an extra virtual method call.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Florin Iucha from comment #7)
> There are uses for the memory_resources pattern, outside of its current
> application in the standard library - for example allocating ranges in a
> file, or in a shared memory segment. 0 is valid allocation, although
> obviously not directly dereferenceable.

Well then it's not a valid use of pmr::memory_resource. It needs to return a
pointer to dereferenceable memory (after casting it from void* to something
else of course).

If you have a scenario where you are doing some additional step to convert the
returned value into a dereferenceable pointer, can't you just adjust the
returned values to make that work?

e.g. set the most significant bit in the returned value, and clear it when
converting it to a pointer, or add a constant offset to every returned value
and then subtract that again when converting it to a pointer.

If you're abusing the pmr::memory_resource interface to return a uintptr_t
disguised a a void*, well that's abusing the API and not intended to be
supported (according to the author of the proposal that added it to C++).

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Florin Iucha from comment #2)
> It indicates that it reports errors via exceptions.

Yes, but that doesn't mean 0 is a valid return value. The non-nothrow forms of
operator new also use exceptions to report errors, but that doesn't mean that
they can return 0 as a valid pointer. pmr::memory_resource is intended to work
the same way: either return a valid pointer to dereferencable memory, or throw
an exception. There is no third alternative, of returning an invalid or null
pointer value.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #10 from Florin Iucha <florin.iucha at amd dot com> ---
(In reply to Jonathan Wakely from comment #8)
> > There are uses for the memory_resources pattern, outside of its current
> > application in the standard library - for example allocating ranges in a
> > file, or in a shared memory segment. 0 is valid allocation, although
> > obviously not directly dereferenceable.
> 
> Well then it's not a valid use of pmr::memory_resource. It needs to return a
> pointer to dereferenceable memory (after casting it from void* to something
> else of course).
>
... 
>
> If you're abusing the pmr::memory_resource interface to return a uintptr_t
> disguised a a void*, well that's abusing the API and not intended to be
> supported (according to the author of the proposal that added it to C++).

Thank you for taking the time to consider this - I will create the "abstract
integer range" allocator and use that for the other use cases.

It might be good though to update the documentation - perhaps even on
cppreference if you have access - to explicitly indicate these constraints as
following from the design intent.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm going to get the standard clarified instead (and then cppreference will
probably follow suit).

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #12 from Florin Iucha <florin.iucha at amd dot com> ---
(In reply to Jonathan Wakely from comment #9)
> (In reply to Florin Iucha from comment #2)
> > It indicates that it reports errors via exceptions.
> 
> Yes, but that doesn't mean 0 is a valid return value. The non-nothrow forms
> of operator new also use exceptions to report errors, but that doesn't mean
> that they can return 0 as a valid pointer. pmr::memory_resource is intended
> to work the same way: either return a valid pointer to dereferencable
> memory, or throw an exception. There is no third alternative, of returning
> an invalid or null pointer value.

Well, bitten by a co-variant of Hyrum's law - the documentation doesn't
explicitly say "0 is not a valid return", and I was relying on the
documentation and my reading of the spec, without having access to the mental
model of the authors. Thank you again for acting as a conduit and clarifying
the design intent.

[Bug libstdc++/103162] overconstrained std::pmr::memory_resource allocate/deallocate methods

[florin.iucha at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103162

--- Comment #13 from Florin Iucha <florin.iucha at amd dot com> ---
(In reply to Jonathan Wakely from comment #11)
> I'm going to get the standard clarified instead (and then cppreference will
> probably follow suit).

That will be great!