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

Remi Forax forax at univ-mlv.fr
Thu Jun 2 16:30:35 UTC 2016



----- 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.  

> 
> 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
> >>>>
> >>>>
> >>>
> >
> 
> 


More information about the hotspot-compiler-dev mailing list