On Tue, 7 Sep 2021 12:25:54 GMT, Leo Korinth <lkori...@openjdk.org> wrote:
> The basic problem is that we are relying on undefined behaviour, as
> documented in the code:
>
> // This whole business of passing information from ResourceObj::operator new
> // to the ResourceObj constructor via fields in the "object" is technically
> UB.
> // But it seems to work within the limitations of HotSpot usage (such as no
> // multiple inheritance) with the compilers and compiler options we're using.
> // And it gives some possibly useful checking for misuse of ResourceObj.
>
>
> I am removing the undefined behaviour by passing the type of allocation
> through a thread local variable.
>
> This solution has some advantages:
> 1) it is not UB
> 2) it is simpler and easier to understand
> 3) it uses less memory (I could make it use even less if I made the enum
> `allocation_type` a u8)
> 4) in the *very* unlikely situation that stack memory (or embedded) already
> equals the data calculated from the address of the object, the code will also
> work.
>
> When doing the change, I also updated `allocated_on_stack()` to the new name
> `allocated_on_stack_or_embedded()` which is much harder to misinterpret.
>
> I also disallow to "fake" the memory type by explicitly calling
> `ResourceObj::set_allocation_type`.
>
> This forced me to change two places that is faking the allocation type of an
> embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking of
> the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray` can
> allocate any type of object. My guess is that `GrowableArray` has changed
> behaviour, or maybe that it was hard to understand because the old naming of
> `allocated_on_stack()`.
>
> I have also tried to update the comments. In doing that I not only changed
> the comments for this change, but also for the *incorrect* advice to always
> delete object you allocate with new.
>
> Testing on debug build tier1-3
> Testing on release build tier1
Changes requested by iklam (Reviewer).
src/hotspot/share/memory/allocation.hpp line 439:
> 437: void* operator new(size_t size, const std::nothrow_t&
> nothrow_constant) throw() {
> 438: address res = (address)resource_allocate_bytes(size,
> AllocFailStrategy::RETURN_NULL);
> 439: DEBUG_ONLY(if (res != NULL) _thread_last_allocated =
> RESOURCE_AREA;)
Maybe we should also guard against the possibility of nested allocations, which
may trash `_thread_last_allocated`?
#define PUSH_RESOURCE_OBJ_ALLOC_TYPE(t) \
assert(_thread_last_allocated == STACK_OR_EMBEDDED, "must not be nested"); \
DEBUG_ONLY(_thread_last_allocated = t); \
...
if (res != NULL) {
PUSH_RESOURCE_OBJ_ALLOC_TYPE(RESOURCE_AREA);
}
Similarly, the `ResourceObj` constructor should use a corresponding
`POP_RESOURCE_OBJ_ALLOC_TYPE` macro.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5387
