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

[Zoltán Majó]

Hi Vladimir,


thank you for the feedback! Please see comments inline.

On 06/09/2016 01:28 PM, Vladimir Ivanov wrote:
> 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.

OK.

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

Thank you, I'm glad to hear that!

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

OK, I've filed JDK-8159150 [1].

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

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

That is a mistake, I've removed the condition related to _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 agree and have updated the code.

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

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

Sure I do. Thank you for catching that mistake.

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

That is a good idea. I've removed the parameter accessing_klass and use 
only accessing_method.

For put bytecodes, the VM caches the accessing method. For get 
bytecodes, it is sufficient to cache the accessing klass; we derive the 
accessing klass from the accessing _method.

Please let me know if you think we should proceed differently.

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8157181/webrev.07/

JPRT is in progress.

>
>
> Otherwise, looks good!

Thanks a lot!

Best regards,


Zoltan

[1] https://bugs.openjdk.java.net/browse/JDK-8159150
[2] 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/84ff58dfd5e0/src/share/vm/classfile/verifier.cpp#l173
>
> Best regards,
> Vladimir Ivanov
>
> [1] 
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/84ff58dfd5e0/src/share/vm/classfile/verifier.cpp#l2226