On Fri, 31 May 2024 00:22:37 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>>> 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`
>>
>> Something's been bothering me about this explanation and I think I've put my
>> finger on it. As you show, the same CP entry can be referenced by both
>> `getstatic` & `getfield` bytecodes though only one will successfully
>> resolve. Walking the bytecodes doesn't actually tell us anything - the
>> resolution status should be different for instance vs static fields which
>> means we're should always be safe to attempt the resolution of fields as
>> instance fields provided we ignore errors.
>>
>>> 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.
>>
>> The Method parameter is necessary for puts to final fields - either
>> `<clinit>` for static finals or an `<init>` method for instance finals. In
>> either case, the we don't actually resolve the field for puts so it doesn't
>> matter if we pass the "correct" method or not during pre resolution as it
>> will never successfully complete. I think we'd be OK to send any method we
>> want to that call when doing preresolution provided we ignore the errors
>
> If you look at the version in the Leyden repo, there are many different types
> of references that are handled in `ClassPrelinker::maybe_resolve_fmi_ref`
>
> https://github.com/openjdk/leyden/blob/4faa72029abb86b55cb33b00acf9f3a18ade4b77/src/hotspot/share/cds/classPrelinker.cpp#L307
>
> My goal is to defer all the safety checks to
> `InterpreterRuntime::resolve_xxx` so that we don't need to think about what
> is safe to pre-resolve, and what is not. Some of the checks are very complex
> (see linkResolver.cpp as well) and may change as the language evolve.
The current algorithm says:
for each bytecode in each method:
switch(bytecode) {
case getfield:
case outfield:
InterpreterRuntime::resolve_get_put(bc, raw_index, mh, cp, false
/*initialize_holder*/, CHECK);
break;
....
}
What I'm proposing is:
for each ResolvedFieldEntry
bool success = InterpreterRuntime::resolve_get_put(getfield, raw_index,
nullptr /* mh */, cp, false /*initialize_holder*/, CHECK);
if (success) {
// also resolve for put
InterpreterRuntime::resolve_get_put(putfield, raw_index, nullptr /* mh */,
cp, false /*initialize_holder*/, CHECK);
}
The method is not critical as the current algorithm attempts resolution with
multiple methods. The resolution logic already has to handle this for normal
execution and "knows" not to resolve entries (like puts of field fields)
regardless of the method as they need to do additional runtime checks.
The same will apply to invoke bytecodes later.... it feels safer to do only
what the bytecodes in some method have asked for but the runtime already has to
be robust against different kinds of gets/puts or invokes targeting the same cp
entries.
If you really want to only resolve the exact cases (ie: gets not puts, etc)
that were resolved in the training run, then we need to write out as part of
the classicist more explicitly what needs to be resolved:
ie:
@cp_resolved_gets 4, 7 8
@cp_resolved_puts 7 8 10
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1622343820
