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

Thu Jun 2 16:55:02 UTC 2016

Hi Zoltan

----- Mail original -----
> De: "Zoltán Majó" <zoltan.majo at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: hotspot-compiler-dev at openjdk.java.net
> 
> Hi Rémi,
> 
> 
> On 06/02/2016 06:30 PM, Remi Forax wrote:
> > [...]
> >
> > ----- Mail original -----
> >> De: "Zoltán Majó" <zoltan.majo at oracle.com>
> >> À: hotspot-compiler-dev at openjdk.java.net
> >> Envoyé: Jeudi 2 Juin 2016 17:20:37
> >> Objet: Re: [9] RFR (S): 8157181: Compilers accept modification of final
> >> 	fields outside initializer methods
> >>
> >> 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.
> > Please do not do that, it will be a huge regression in term of perf at
> > least for the language runtimes i maintain
> > (and i suppose for Nashorn, JRuby or Groovy), Here are the perf assertions
> > i use routinely,
> > static final method handle is considered as constant, final fields (even
> > not static) of VM anonymous class is considered as truly final.
> >
> 
> I fully understand your concerns. And thank you for sharing your asserts!
> 
> Let's see what others think about how to go about this problem. So far
> the following options were explored
> - bail out compilation of non-compliant methods;
> - enforce JVMS conformance for all classes and keep constant folding
> enabled;
> - enforce JVMS conformance only for classes with _major_version >=52 and
> completely disable constant folding.
> 
> I'm not sure which option is the best. Also, there might be other
> options as well.

what about bailing out compilation of non-compliant methods if major <= 52 or if a backward compat flag is set and enforcing the JVMS spec for major >= 53 ?

> 
> Thank you and best regards,
> 
> 
> Zoltan

Rémi

> 
> 
> 
> >> 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
> > Rémi
> >
> >>> 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
> >>>>>>
> >>>>>>
> >>
> 
> 