On Wed, 29 May 2024 12:53:57 GMT, Dan Heidinga <heidi...@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
