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

[Vladimir Ivanov]

Zoltan,

> To summarize, webrev.06
> - *detects misbehaving fields in the verifier* for classes with version
> < JAVA_7_VERSION and *disables constant folding for those fields only*;
> - *enforces JVMS-7 conformance* for classes with version >=
> JAVA_7_VERSION and *leaves constant folding as it is* (no performance
> impact).
>
> As I mentioned to David Holmes, we can use JAVA_8_VERSION or
> JAVA_9_VERSION instead of JAVA_7_VERSION.

Agree.

> I'm currently running pre-PIT RBT to see if there are any problems with
> our testbase. Performance runs are also in progress.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~zmajo/8157181/webrev.06/

I really like how it shapes out!

Some comments follow:

Unfortunately, it doesn't (and, moreover, can't) cover class 
redefinition: if a new version of a method contains final field update, 
all the generated code which embed previous value should be invalidated.

The only way to handle such case now is to add nmethod dependencies when 
static final field loads are constant folded.

Please, file a separate bug (P4) for that and let's decide how to 
proceed with it later.

---
src/share/vm/classfile/verifier.cpp:

+ void Verifier::detect_invalid_final_field_accesses(instanceKlassHandle 
klass) {

The name is misleading: such accesses are legal according to JVMS-6 (and 
earlier). Maybe something like detect_initialized_final_field_updates?

Same for:
+   bool has_invalid_final_access() const    { return 
access_flags().has_field_invalid_final_access(); }
+   void set_has_invalid_final_access(const bool value) {
+   JVM_ACC_FIELD_INVALID_FINAL_ACCESS  = 0x00000100,


---
Why do you care about _nofast_putfield? The bytecode hasn't been 
rewritten yet.

+   if (klass()->major_version() < 51 && !klass->is_rewritten()) {
...
+         if (opcode == Bytecodes::_putstatic || opcode == 
Bytecodes::_putfield || opcode == Bytecodes::_nofast_putfield) {

---
Also, I don't think in its current shape it is part of verification, but 
instanceKlass linkage phase instead:

bool InstanceKlass::link_class_impl(
     instanceKlassHandle this_k, bool throw_verifyerror, TRAPS) {
...
     if (!this_k->is_linked()) {
       if (!this_k->is_rewritten()) {
         {
           bool verify_ok = verify_code(this_k, throw_verifyerror, THREAD);
           if (!verify_ok) {
             return false;
           }
         }
...
	<< HERE >>
...
         // also sets rewritten
         this_k->rewrite_class(CHECK_false);

---
I don't see much benefit in tracking the problematic updates per-methods 
and not per-class. So, you could do the checks in 
ClassVerifier::verify_field_instructions [1] and mark the whole class 
(ClassVerifier::_klass) as containing problematic instructions.

---
src/share/vm/interpreter/linkResolver.cpp:

         THROW(vmSymbols::java_lang_IllegalAccessError());

+         THROW(vmSymbols::java_lang_IllegalAccessError());

Please, add an error message for diagnostic purposes.

---
src/share/vm/ci/ciField.cpp:

-    if (_known_to_link_with_put == accessing_klass) {
+    if (_known_to_link_with_put_klass == accessing_klass && 
_known_to_link_with_put_method) {

Don't you need to check _known_to_link_with_put_method with 
accessing_method?

Why don't you use accessing_method instead of accessing_klass and 
migrate _known_to_link_with_put/get to ciMethod*?

   bool will_link(ciMethod* accessing_method, Bytecodes::Code bc);

I looked at all will_link usages and accessing_method is always available.


Otherwise, looks good!

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/84ff58dfd5e0/src/share/vm/classfile/verifier.cpp#l2226