On Mon, 3 Jun 2024 19:13:54 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> This makes sense. I will try to prototype it in the Leyden repo and then >> update this PR. > > I tried skipping the `methodHandle` parameter to > `InterpreterRuntime::resolve_get_put` but it's more complicated than I > thought. > > 1. The `fieldDescriptor::has_initialized_final_update()` will return true IFF > the class has `putfield` bytecodes to a final field outside of `<clinit>` > methods. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/rewriter.cpp#L463) > 2. When `InterpreterRuntime::resolve_get_put` is called for a `putfield`, it > adds `putfield` to the `ResolvedFieldEntry` only if > `fieldDescriptor::has_initialized_final_update()` is false. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/interpreterRuntime.cpp#L703) > 3. `InterpreterRuntime::resolve_get_put`calls > `LinkResolver::resolve_field()`, which always checks if the `methodHandle` is > `<init>` or not, without consulting > `fieldDescriptor::has_initialized_final_update()`. See > [here](https://github.com/openjdk/jdk/blob/9686e804a2b058955ff88149c54a0a7896c0a2eb/src/hotspot/share/interpreter/linkResolver.cpp#L1040) > > (2) is an optimization -- if a method sets final fields only inside > `<clinit>` methods, we should fully resolve the `putfield` bytecodes. > Otherwise every such `putfield` will result in a VM call. > > (3) is for correctness -- make sure that only `<init>` can modify final > fields. > > I am pretty sure (2) and (3) are equivalent. I.e., we should check against > the method in (3) only if `fieldDescriptor::has_initialized_final_update()` > is true. However, (3) is security related code, so I don't want to change it > inside an optimization PR like this one. Without fixing that, I cannot call > `InterpreterRuntime::resolve_get_put` with a null `methodHandle`, as it will > hit the assert. > > This goes back to my original point -- I'd rather do something stupid but > correct (call the existing APIs and live with the existing behavior), rather > than trying to analyze the resolution code and see if we can skip certain > checks.
I get what you're saying... and the `fieldDescriptor::has_initialized_final_update()` is a nice optimization that we don't want to mess up. Getting the straightforward code in makes sense as we can optimize it later given it runs at CDS archive build which isn't performance critical. Down the line, we can add new entry points into the LinkResolver for use by CDS that refuse to resolve final fields and avoid this kind of issue but that's a future problem. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19355#discussion_r1629702253
