RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods
zoltan.majo at oracle.com
Thu Jun 2 15:20:37 UTC 2016
On 06/02/2016 11:29 AM, Zoltán Majó wrote:
> But there is a problem with JPRT: A JCK test, putstatic01801m, fails.
> The reason for the failure is that the the test generates a method
> that contains a putstatic to a static final field (i.e., the bytecodes
> generated by the test do not conform to the JVMS). (Even the test
> mentions that the Java source equivalent to the generated bytecodes is
> compilable only if the "final" keyword is removed from the declaration
> of the static field.)
> So (if we decide to push the current fix) the issue with the test
> needs to be fixed first. Do you know how we could proceed with that?
After a (private) discussion with Igor Ignatyev, I've filed a bug with
the JCK team to address the issue with the putstatic01801m test.
In the bug report, David Holmes suggest the following:
"If this is a long standing non-compliance with the JVMS then its impact
can not be considered high. The longer the VM and spec do not match the
more likely we adjust the spec to match the VM behaviour. When that is
not feasible then the usual approach is to enforce the correct rules as
of the current classfile version, otherwise we risk introducing
compatibility issues. Even if we make the correct behaviour the default,
we will usually provide a flag to enable the old behaviour for
compatibility purposes. Only if this is a security concern would we
force the change to the new behaviour. "
So, to follow David's suggestions, I did the following modifications to
the previous webrev:
- the check for the caller method in LinkResolver::resolve_field() is
performed only for classes with _major_version >= 52;
- I added a new diagnostic flag, CheckFinalFieldModifications to disable
the new checks (please feel free to suggest a better flag name).
That should solve the problem with the JCK test. However, if we follow
David's suggestions, we can't guarantee for all classes that final
fields won't change after initialization. So the compilers can generate
incorrect code when compiling accessing to fields of those classes.
Therefore, in the current webrev, I've disabled folding of accesses to
all non-stable fields.
We could relax that criteria, e.g., by disabling folding only for those
field that were declared in a class with _major_version < 52. But it
would be great if you could give me some feedback before I look into
that. Thank you very much in advance!
Testing: JPRT in progress.
> Thank you and best regards,
>> Best regards,
>> Vladimir Ivanov
>>> I agree with you, alternatively we can propose a more generic fix (fix
>>> the interpreter and the compilers as well). The fix would most likely
>>> affect field resolution in LinkResolver::resolve_field() around here:
>>> Changing resolve_field() to throw an IllegalAccessError for
>>> inappropriate field access will propagate the exception to the
>>> interpreter. Changing ciField::will_link() will most likely kill (some)
>>> compilations (if, e.g., fields are linked through
>>> I'd prefer to take the second option (changing field resolution), but
>>> there might be some disadvantages related to option I might be
>>> Thank you!
>>> Best regards,
>>>>> More details about the failure are here .
>>>>> With the patch applied, the program always completes as it is
>>>>> by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed (because we
>>>>> always interpret methods non-conforming with the VM spec).
>>>>> Here is the updated webrev:
>>>> Bailing out the whole compilation in C2 is even less appropriate. It
>>>> should generate an uncommon trap for such accesses instead (see
>>>> Best regards,
>>>> Vladimir Ivanov
More information about the hotspot-compiler-dev