RFR: 8243996: Remove hardcoded field offsets from HotSpot

[Erik Österlund]

Hi Coleen,

Thanks for the review.

On 2020-05-13 20:07, coleen.phillimore at oracle.com wrote:
>
>
> 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'm trying to move the pre-existing various offset style inconsistencies 
of JavaClasses
(50-50 use of leading '_', sometimes using accessors, sometimes being 
public, sometimes
asserting offsets are initialized, etc) into a new RFR. I think there is 
plenty of weirdness there
and don't think fixing it belongs to this change.

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

Thanks!

/Erik

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