RFR: 8269537: memset() is called after operator new [v5]
Leo Korinth
lkorinth at openjdk.java.net
Tue Apr 19 16:49:12 UTC 2022
> 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
-------------
Changes: https://git.openjdk.java.net/jdk/pull/5387/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=04
Stats: 174 lines in 8 files changed: 48 ins; 93 del; 33 mod
Patch: https://git.openjdk.java.net/jdk/pull/5387.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5387/head:pull/5387
PR: https://git.openjdk.java.net/jdk/pull/5387
More information about the serviceability-dev
mailing list