[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8269537: memset() is called after operator new [v5]
From: Leo Korinth <lkorinth () openjdk ! java ! net>
Date: 2022-04-19 16:53:38
Message-ID: VxOzhcr-7I7NMQowGaPacYK-dXXchTC6gZhfFQDHkYY=.46eaeb8e-7526-404d-95f8-862eddbf4888 () github ! com
[Download RAW message or body]
On Tue, 19 Apr 2022 16:49:12 GMT, Leo Korinth <lkorinth@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic