[9] RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods

[Zoltán Majó]

Hi,


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!

Updated webrev:
http://cr.openjdk.java.net/~zmajo/8157181/webrev.05/

Testing: JPRT in progress.

Best regards,


Zoltan

>
> Thank you and best regards,
>
>
> Zoltan
>
>>
>> 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:
>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910 
>>>
>>>
>>>
>>> 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 
>>> ciField::will_link()).
>>>
>>> I'd prefer to take the second option (changing field resolution), but
>>> there might be some disadvantages related to option I might be 
>>> overseeing.
>>
>>
>>
>>
>>>
>>> Thank you!
>>>
>>> Best regards,
>>>
>>>
>>> Zoltan
>>>
>>>
>>>>
>>>>> More details about the failure are here [3].
>>>>>
>>>>> With the patch applied, the program always completes as it is 
>>>>> expected
>>>>> 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:
>>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/
>>>> Bailing out the whole compilation in C2 is even less appropriate. It
>>>> should generate an uncommon trap for such accesses instead (see
>>>> Parse::do_field_access).
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> [1]
>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595 
>>>>
>>>>
>>>
>