[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 [v4]
From:       Leo Korinth <lkorinth () openjdk ! java ! net>
Date:       2021-10-20 9:36:38
Message-ID: OLUjj1GGh7kLi5tWiI_keOCWU-VgxtjesyoWHDtifSs=.213f71a6-c4b5-4107-b64f-b59efbfe7309 () github ! com
[Download RAW message or body]

> 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:

  review updates

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5387/files
  - new: https://git.openjdk.java.net/jdk/pull/5387/files/c9d45906..640f2d83

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=02-03

  Stats: 11 lines in 4 files changed: 0 ins; 7 del; 4 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


[prev in list] [next in list] [prev in thread] [next in thread]