RFR: 8269537: memset() is called after operator new [v3]

[Kim Barrett]

On Tue, 5 Oct 2021 20:39:30 GMT, Leo Korinth <lkorinth at 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
>
> Leo Korinth has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use a thread local buffer so that the compiler might reorder operator new.

Changes requested by kbarrett (Reviewer).

src/hotspot/share/asm/codeBuffer.cpp line 143:

> 141:   NOT_PRODUCT(clear_strings());
> 142: 
> 143:   assert(_default_oop_recorder.allocated_on_stack_or_embedded(), "should be embedded object");

It might have been better to do this name change separately, to reduce distraction.

src/hotspot/share/asm/codeBuffer.hpp line 369:

> 367: // addresses in a sibling section.
> 368: 
> 369: class CodeBuffer: public ResourceObj DEBUG_ONLY(COMMA private Scrubber) {

Deriving from ResourceObj rather than the previous fakery seems good.  I think the multiple-inheritance derivation from Scrubber could be removed, instead adding ~CodeBuffer to do the zapping.

src/hotspot/share/asm/codeBuffer.hpp line 376:

> 374:   // CodeBuffers must be allocated on the stack except for a single
> 375:   // special case during expansion which is handled internally.  This
> 376:   // is done to guarantee proper cleanup of resources.

This comment seems dangling now that the associated operator definitions have been removed.

src/hotspot/share/classfile/stackMapFrame.hpp line 66:

> 64: 
> 65:   StackMapFrame(const StackMapFrame& cp) :
> 66:       ResourceObj(),

Good find.

src/hotspot/share/gc/g1/g1Allocator.hpp line 243:

> 241:     _g1h(g1h),
> 242:     _allocation_region(NULL),
> 243:     _allocated_regions(2, mtGC),

Good riddance.

src/hotspot/share/memory/allocation.cpp line 154:

> 152: 
> 153: #ifdef ASSERT
> 154: thread_local ResourceObj::RecentAllocations ResourceObj::_recent_allocations;

Don't use `thread_local`.  See the style guide.

src/hotspot/share/memory/allocation.cpp line 234:

> 232: }
> 233: #endif // ASSERT
> 234: 

Keep the blank line after the `#endif`

src/hotspot/share/memory/allocation.hpp line 398:

> 396: class ResourceObj ALLOCATION_SUPER_CLASS_SPEC {
> 397:  public:
> 398:   enum allocation_type : uint8_t { STACK_OR_EMBEDDED, RESOURCE_AREA, C_HEAP, ARENA };

Consider adding an "empty" allocation-type, that indicates an entry in the RecentAllocations is unused.

src/hotspot/share/memory/allocation.hpp line 414:

> 412:   // allocation is done in a recursive step. In that case an assert will trigger.
> 413:   class RecentAllocations {
> 414:     static const unsigned BufferSize = 5;

"5" seems like an odd choice.

src/hotspot/share/memory/allocation.hpp line 416:

> 414:     static const unsigned BufferSize = 5;
> 415:     uintptr_t _begin[BufferSize];
> 416:     uintptr_t _past_end[BufferSize];

I think these should be `const void*` rather than `uintptr_t`.

src/hotspot/share/memory/memRegion.hpp line 110:

> 108: // A ResourceObj version of MemRegionClosure
> 109: 
> 110: class MemRegionClosureRO: public ResourceObj {

I think this class could be deleted entirely, with the only derived class (`DirtyCardToOopClosure`) instead deriving from ResourceObj.

src/hotspot/share/prims/jvmtiDeferredUpdates.hpp line 132:

> 130:   JvmtiDeferredUpdates() :
> 131:     _relock_count_after_wait(0),
> 132:     _deferred_locals_updates(1, mtCompiler) { }

Good riddance.

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

PR: https://git.openjdk.java.net/jdk/pull/5387