RFR: 8243996: Remove hardcoded field offsets from HotSpot

[coleen.phillimore at oracle.com]

On 5/13/20 12:00 PM, John Rose wrote:
> 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.

Erik and I came up with these names earlier today as they were more 
descriptive than some original names.  "interpreter_init_stubs" and 
"interpreter_init_code" seem okay also.
>
> 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.

This seems like a good suggestion for these classes.  For the most part, 
javaClasses foo_offset are private.

I was going to reply to this thread "Looks good to me" and thanks for 
adding the comments.

Coleen
>
> — 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