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

[Vladimir Ivanov]

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

I'm fine with enabling the checks only for 9 for now and handle 7 & 8 
class files separately.

>> 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,
>>
>
> Yes, that name sounds better and I've updated the code accordingly.

A leftover:

src/share/vm/runtime/fieldDescriptor.hpp:

+   void set_has_invalid_final_access(const bool value) {

>> ---
>> 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.
>
> The problematic updates are tracked *per-field* (and not *per-method* or
> * per-class*).
>
> Initially, I had a version that tracked problematic instructions on a
> per-class basis. With that version, once a problematic instruction was
> detected, the whole class was marked as containing problematic
> instructions. As a result, constant folding was disabled for accesses to
> *all* final fields of that class.
>
> Tracking problematic updates on a per-field basis seems to be less
> intrusive. That way we disable folding *only for fields to which
> problematic accesses are made* (and leave constant folding enabled for
> accesses to all other fields). So we can fold some accesses even for
> "misbehaving" classes. I hope that is fine.
>
> Regarding doing checks in verify_field_instructions() [2]: It turns out
> that we have two verifiers, one that is called for
>
> major_version >= STACKMAP_ATTRIBUTE_MAJOR_VERSION
>
> and a second one that is called for
>
> major_version < STACKMAP_ATTRIBUTE_MAJOR_VERSION.
>
> ClassVerifier::verify_field_instructions() is called only for class
> files >= STACKMAP_ATTRIBUTE_MAJOR_VERSION so we need to call
> detect_initialized_final_field_updates() after verification.
>
> But I think it is a good idea to move the check to the linkage phase (as
> you've suggested above).

While browsing the code, I got another idea: what about piggybacking on 
Rewriter pass (see Rewriter::scan_method)? It already sets some 
per-method properties (e.g., Method::has_monitor_bytecodes()).

I'd like to avoid additional pass over the bytecode, because it can 
cause slow downs during class loading (just a precaution - I haven't 
measured the impact).

>> ---
>> src/share/vm/interpreter/linkResolver.cpp:
>>
>>         THROW(vmSymbols::java_lang_IllegalAccessError());
>>
>> +         THROW(vmSymbols::java_lang_IllegalAccessError());
>>
>> Please, add an error message for diagnostic purposes.
>
> OK, I did that.

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

+   if (is_put && fd.access_flags().is_final()) {
+     ResourceMark rm(THREAD);
+     char msg[200];

There's much more convenient way now - stringStream. It takes care of 
memory management in resource area for you and provides convenient 
utility functions:

   ResourceMark rm(THREAD);
   stringStream ss;
   ss.printf("...", ...);
   THROW_MSG(vmSymbols::java_lang_IllegalAccessError(), ss.as_string());

---
+       if ((byte == Bytecodes::_putstatic && fd.is_static() && 
!m()->is_static_initializer()) ||
+           ((byte == Bytecodes::_putfield || byte == 
Bytecodes::_nofast_putfield) && !fd.is_static() && 
!m->is_object_initializer())) {

Maybe refactor it to make more readable?

   bool is_static_final_update = ...;
   bool is_instance_final_update = ...;
   if (is_static_final_update || is_instance_final_update) {
   ...

Otherwise, looks good.

Best regards,
Vladimir Ivanov