RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v3]

[Ioi Lam]

On Wed, 29 May 2024 12:53:57 GMT, Dan Heidinga <heidinga at openjdk.org> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>> 
>>  - Fixed typo in previous commit
>>  - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
>>  - @matias9927 comments - moved remove_resolved_field_entries_if_non_deterministic() to cpCache
>>  - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
>>  - 8293980: Resolve CONSTANT_FieldRef at CDS dump time
>
> src/hotspot/share/cds/classListParser.cpp line 848:
> 
>> 846:   if (preresolve_fmi) {
>> 847:     ClassPrelinker::preresolve_field_and_method_cp_entries(THREAD, ik, &preresolve_list);
>> 848:   }
> 
> Can you clarify the approach here?
> 
> As I read the code, `ClassPrelinker::preresolve_class_cp_entries` will walk the whole constant pool looking for unresolved class entries that match and then resolve them.  `ClassPrelinker::preresolve_field_and_method_cp_entries` walks all methods bytecode by bytecode to resolve them.
> 
> Doesn't the `preresolve_list` already tell us which CP entries need to be resolved and the cp tag tell us the type of resolution to do?  Can we not do this in a single pass over the cp rather than walking method bytecodes?
> 
> Is the reason for this approach to avoid always resolving FieldMethodRefs for both get and put and only do them if there's a corresponding bytecode?

`preresolve_list` has the original CP indices (E.g., `putfield #123` as stored in the classfile), but in HotSpot, after bytecode rewriting, the u2 following the bytecode is changed to an index into the `cpcache()->_resolved_field_entries` array, so it becomes something like `putfield #45`. So we need to know how to convert the `123` index to the `45` index.

We could walk `_resolved_field_entries` to find the `ResolvedFieldEntry` whose `_cpool_index` is `123`. However, before the `ResolvedFieldEntry` is resolved, we don't know which bytecode is used to resolve it, so we don't know  whether it's for a static field or non-static field. Since we want to filter out the static fields in the PR, we need to:

- walk the bytecodes to find only getfield/putfield bytecodes
- these bytecodes will give us an index to the `_resolved_field_entries` array
- from there, we discover the original CP index
- then we see if this index is set to true in `preresolve_list`

There's also a compatibility issue -- it's possible to have static and non-static field access using the same CP index:


class Hack {
static int myField;
int foo(boolean flag) {
    try {
        if (flag) {
            // throw IncompatibleClassChangeError
            return /* pseudo code*/ getfield this.myField;
        } else {
            // OK
            return /* pseudo code*/ getstatic Hack.myField;
        } 
    } catch (Throwable) {
        return 5678;
    }
}


So we must call `InterpreterRuntime::resolve_get_put()` which performs all the checks for access rights, static-vs-non-static, etc. This call requires a Method parameter, so we must walk all the Methods to find an appropriate one.

Perhaps it's possible to refactor the `InterpreterRuntime` code to avoid passing in a Method, but I am hesitant to do that with code that deals with access right checks.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1619144592