On Tue, 19 Apr 2022 16:49:12 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 > > Leo Korinth has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Merge branch 'master' into _8269537 > - Merge branch 'master' into _8269537 > - review updates > - Use a thread local buffer so that the compiler might reorder operator new. > - First update > > * Change backing type of ResourceObj::allocation_type to be u8. Also > remove no longer needed mask and explicit zero value of STACK_OR_EMBEDDED > value. > > * Now setting allocation type with set_type() with assert. > - 8269537: memset() is called after operator new Rebased. I think my approach is better than the current. I think we would be better off without tracking the allocation type at all though, so please consider that approach as well. ------------- PR: https://git.openjdk.java.net/jdk/pull/5387
