RFR: 8243996: Remove hardcoded field offsets from HotSpot

[John Rose]

I like this cleanup!

There are new tricky dependencies between the interpreter and other
runtime bits, and class loading; they are adequately documented in new
comments.

A few comments of my own:

The whitespace on the new line in vmSymbols.hpp needs to be adjusted to
resemble whitespace on neighboring lines.  Hotspot is a jungle of old code,
and our new code needs to blend into that jungle, not make spots of new
color (unless of course there is a purpose to some new color).

I don’t want to gripe needlessly about identifier choice, but… maybe I
do in this case:  “interpreter_init_code” and “interpreter_init_code_stub”
seem overly cryptic to me; they don’t help me remember what they do.
I suggest that“interpreter_init_stubs” and “interpreter_init_code” or
“interpreter_init_stub_code” and “interpreter_init_main_code” would
be easier on maintainers.  But if it’s just me, leave it as is.  We coders eat
cryptic identifiers for breakfast.

Can someone remind me (a) which platforms use the C++ interpreter,
and (b) how well they are tested?

I spot-checked for queries to the hard-coded offsets, and found enough
“guarantee(this_offset != 0…)” to make me feel secure.  If there’s a bug
related to this change, it’s quite possibly a zero offset being used on some
path in the JVM which is rarely executed.  (Such a bug could be introduced
in the future, right?)  Usually we avoid such bugs by encapsulating variable
state into C++ access functions which perform their own assertions, rather
than requiring users of the variable to make their own checks.

Did you look into making all those public variables private, and wrapping
access functions around them?  I think that would make for a more complete
cleanup.  It would touch more places in the interpreter, including platform
code, which I think would be a good thing.  It would (a) allow review now
of dependency checks in the interpreter, and (b) force downstream porters
to similarly review their use of hard-coded offsets.

— John

> On May 13, 2020, at 8:26 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi,
> 
> Let's remove hardcoded field offsets. Main trick to get this done is to split interpreter initialization
> into two parts: 1. reserve the interpreter memory, 2. generate the interpreter. This allows reserving
> it before loading methods (which generates i2c adapters that assert things are "in the interpreter",
> requiring knowledge about its address range), and then load the methods, and then generate the interpreter.
> This moving of interpreter code generation until after methods have been loaded allows offsets to be
> looked up, needed by code generation. That way, we no longer need to hardcode field offsets, which
> makes it less annoying to change the field layout code in the future.
> 
> Thanks to Coleen for pre-reviewing this.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8243996
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8243996/webrev.01/
> 
> Thanks,
> /Erik