On Thu, 30 May 2024 04:15:24 GMT, Dan Heidinga <heidi...@openjdk.org> wrote:
>> `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.
>
>> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1621541729
