From 0a338875941526d05ffdaeb6d32694843f3304a1 Mon Sep 17 00:00:00 2001 From: zmajo Date: Mon, 12 Dec 2016 12:53:38 +0100 Subject: [PATCH] 8157181: Compilers accept modification of final fields outside initializer methods Summary: Track initialized final field updates; disable constant folding if an update is detected. Reviewed-by: vlivanov, dnsimon, forax, never, kvn, coleenp --- src/share/vm/ci/ciField.cpp | 2 +- src/share/vm/ci/ciField.hpp | 21 +++---------- src/share/vm/ci/ciFlags.hpp | 33 +++++++++++--------- src/share/vm/interpreter/rewriter.cpp | 39 ++++++++++++++++++++++-- src/share/vm/oops/method.cpp | 5 ++- src/share/vm/oops/method.hpp | 3 ++ src/share/vm/runtime/fieldDescriptor.hpp | 6 ++++ src/share/vm/utilities/accessFlags.hpp | 22 ++++++++++--- 8 files changed, 92 insertions(+), 39 deletions(-) diff --git a/src/share/vm/ci/ciField.cpp b/src/share/vm/ci/ciField.cpp index 7dd09a98e..b448d7700 100644 --- a/src/share/vm/ci/ciField.cpp +++ b/src/share/vm/ci/ciField.cpp @@ -207,7 +207,7 @@ void ciField::initialize_from(fieldDescriptor* fd) { // Check to see if the field is constant. bool is_final = this->is_final(); bool is_stable = FoldStableValues && this->is_stable(); - if (_holder->is_initialized() && (is_final || is_stable)) { + if (_holder->is_initialized() && ((is_final && !has_initialized_final_update()) || is_stable)) { if (!this->is_static()) { // A field can be constant if it's a final static field or if // it's a final non-static field of a trusted class (classes in diff --git a/src/share/vm/ci/ciField.hpp b/src/share/vm/ci/ciField.hpp index 75263e3f2..3e46da517 100644 --- a/src/share/vm/ci/ciField.hpp +++ b/src/share/vm/ci/ciField.hpp @@ -124,22 +124,8 @@ public: return _holder->is_shared() && !is_static(); } - // Is this field a constant? - // - // Clarification: A field is considered constant if: - // 1. The field is both static and final - // 2. The canonical holder of the field has undergone - // static initialization. - // 3. If the field is an object or array, then the oop - // in question is allocated in perm space. - // 4. The field is not one of the special static/final - // non-constant fields. These are java.lang.System.in - // and java.lang.System.out. Abomination. - // - // A field is also considered constant if it is marked @Stable - // and is non-null (or non-zero, if a primitive). - // For non-static fields, the null/zero check must be - // arranged by the user, as constant_value().is_null_or_zero(). + // Is this field a constant? See ciField::initialize_from() for details + // about how a field is determined to be constant. bool is_constant() { return _is_constant; } // Get the constant value of this field. @@ -176,6 +162,9 @@ public: bool is_stable () { return flags().is_stable(); } bool is_volatile () { return flags().is_volatile(); } bool is_transient () { return flags().is_transient(); } + // The field is modified outside of instance initializer methods + // (or class/initializer methods if the field is static). + bool has_initialized_final_update() { return flags().has_initialized_final_update(); } bool is_call_site_target() { ciInstanceKlass* callsite_klass = CURRENT_ENV->CallSite_klass(); diff --git a/src/share/vm/ci/ciFlags.hpp b/src/share/vm/ci/ciFlags.hpp index 60d5632f4..ac4d5f18e 100644 --- a/src/share/vm/ci/ciFlags.hpp +++ b/src/share/vm/ci/ciFlags.hpp @@ -46,20 +46,25 @@ private: public: // Java access flags - bool is_public () const { return (_flags & JVM_ACC_PUBLIC ) != 0; } - bool is_private () const { return (_flags & JVM_ACC_PRIVATE ) != 0; } - bool is_protected () const { return (_flags & JVM_ACC_PROTECTED ) != 0; } - bool is_static () const { return (_flags & JVM_ACC_STATIC ) != 0; } - bool is_final () const { return (_flags & JVM_ACC_FINAL ) != 0; } - bool is_synchronized() const { return (_flags & JVM_ACC_SYNCHRONIZED) != 0; } - bool is_super () const { return (_flags & JVM_ACC_SUPER ) != 0; } - bool is_volatile () const { return (_flags & JVM_ACC_VOLATILE ) != 0; } - bool is_transient () const { return (_flags & JVM_ACC_TRANSIENT ) != 0; } - bool is_native () const { return (_flags & JVM_ACC_NATIVE ) != 0; } - bool is_interface () const { return (_flags & JVM_ACC_INTERFACE ) != 0; } - bool is_abstract () const { return (_flags & JVM_ACC_ABSTRACT ) != 0; } - bool is_strict () const { return (_flags & JVM_ACC_STRICT ) != 0; } - bool is_stable () const { return (_flags & JVM_ACC_FIELD_STABLE) != 0; } + bool is_public () const { return (_flags & JVM_ACC_PUBLIC ) != 0; } + bool is_private () const { return (_flags & JVM_ACC_PRIVATE ) != 0; } + bool is_protected () const { return (_flags & JVM_ACC_PROTECTED ) != 0; } + bool is_static () const { return (_flags & JVM_ACC_STATIC ) != 0; } + bool is_final () const { return (_flags & JVM_ACC_FINAL ) != 0; } + bool is_synchronized () const { return (_flags & JVM_ACC_SYNCHRONIZED ) != 0; } + bool is_super () const { return (_flags & JVM_ACC_SUPER ) != 0; } + bool is_volatile () const { return (_flags & JVM_ACC_VOLATILE ) != 0; } + bool is_transient () const { return (_flags & JVM_ACC_TRANSIENT ) != 0; } + bool is_native () const { return (_flags & JVM_ACC_NATIVE ) != 0; } + bool is_interface () const { return (_flags & JVM_ACC_INTERFACE ) != 0; } + bool is_abstract () const { return (_flags & JVM_ACC_ABSTRACT ) != 0; } + bool is_strict () const { return (_flags & JVM_ACC_STRICT ) != 0; } + bool is_stable () const { return (_flags & JVM_ACC_FIELD_STABLE ) != 0; } + // In case the current object represents a field, return true if + // the field is modified outside of instance initializer methods + // (or class/initializer methods if the field is static) and false + // otherwise. + bool has_initialized_final_update() const { return (_flags & JVM_ACC_FIELD_INITIALIZED_FINAL_UPDATE) != 0; }; // Conversion jint as_int() { return _flags; } diff --git a/src/share/vm/interpreter/rewriter.cpp b/src/share/vm/interpreter/rewriter.cpp index 2474ae758..a903b4580 100644 --- a/src/share/vm/interpreter/rewriter.cpp +++ b/src/share/vm/interpreter/rewriter.cpp @@ -396,10 +396,45 @@ void Rewriter::scan_method(Method* method, bool reverse, bool* invokespecial_err break; } + case Bytecodes::_putstatic : + case Bytecodes::_putfield : { + if (!reverse) { + // Check if any final field of the class given as parameter is modified + // outside of initializer methods of the class. Fields that are modified + // are marked with a flag. For marked fields, the compilers do not perform + // constant folding (as the field can be changed after initialization). + // + // The check is performed after verification and only if verification has + // succeeded. Therefore, the class is guaranteed to be well-formed. + InstanceKlass* klass = method->method_holder(); + u2 bc_index = Bytes::get_Java_u2(bcp + prefix_length + 1); + constantPoolHandle cp(method->constants()); + Symbol* field_name = cp->name_ref_at(bc_index); + Symbol* field_sig = cp->signature_ref_at(bc_index); + Symbol* ref_class_name = cp->klass_name_at(cp->klass_ref_index_at(bc_index)); + + if (klass->name() == ref_class_name) { + fieldDescriptor fd; + klass->find_field(field_name, field_sig, &fd); + if (fd.access_flags().is_final()) { + if (fd.access_flags().is_static()) { + assert(c == Bytecodes::_putstatic, "must be putstatic"); + if (!method->is_static_initializer()) { + fd.set_has_initialized_final_update(true); + } + } else { + assert(c == Bytecodes::_putfield, "must be putfield"); + if (!method->is_object_initializer()) { + fd.set_has_initialized_final_update(true); + } + } + } + } + } + } + // fall through case Bytecodes::_getstatic : // fall through - case Bytecodes::_putstatic : // fall through case Bytecodes::_getfield : // fall through - case Bytecodes::_putfield : // fall through case Bytecodes::_invokevirtual : // fall through case Bytecodes::_invokestatic : case Bytecodes::_invokeinterface: diff --git a/src/share/vm/oops/method.cpp b/src/share/vm/oops/method.cpp index b1db4dc00..b0d4950b1 100644 --- a/src/share/vm/oops/method.cpp +++ b/src/share/vm/oops/method.cpp @@ -590,7 +590,7 @@ bool Method::is_constant_getter() const { } bool Method::is_initializer() const { - return name() == vmSymbols::object_initializer_name() || is_static_initializer(); + return is_object_initializer() || is_static_initializer(); } bool Method::has_valid_initializer_flags() const { @@ -606,6 +606,9 @@ bool Method::is_static_initializer() const { has_valid_initializer_flags(); } +bool Method::is_object_initializer() const { + return name() == vmSymbols::object_initializer_name(); +} objArrayHandle Method::resolved_checked_exceptions_impl(Method* this_oop, TRAPS) { int length = this_oop->checked_exceptions_length(); diff --git a/src/share/vm/oops/method.hpp b/src/share/vm/oops/method.hpp index 974bced59..5eeeaa117 100644 --- a/src/share/vm/oops/method.hpp +++ b/src/share/vm/oops/method.hpp @@ -627,6 +627,9 @@ class Method : public Metadata { // valid static initializer flags. bool is_static_initializer() const; + // returns true if the method name is + bool is_object_initializer() const; + // compiled code support // NOTE: code() is inherently racy as deopt can be clearing code // simultaneously. Use with caution. diff --git a/src/share/vm/runtime/fieldDescriptor.hpp b/src/share/vm/runtime/fieldDescriptor.hpp index 9c3101b38..1810a16cd 100644 --- a/src/share/vm/runtime/fieldDescriptor.hpp +++ b/src/share/vm/runtime/fieldDescriptor.hpp @@ -106,6 +106,7 @@ class fieldDescriptor VALUE_OBJ_CLASS_SPEC { bool is_field_access_watched() const { return access_flags().is_field_access_watched(); } bool is_field_modification_watched() const { return access_flags().is_field_modification_watched(); } + bool has_initialized_final_update() const { return access_flags().has_field_initialized_final_update(); } bool has_generic_signature() const { return access_flags().field_has_generic_signature(); } void set_is_field_access_watched(const bool value) { @@ -118,6 +119,11 @@ class fieldDescriptor VALUE_OBJ_CLASS_SPEC { update_klass_field_access_flag(); } + void set_has_initialized_final_update(const bool value) { + _access_flags.set_has_field_initialized_final_update(value); + update_klass_field_access_flag(); + } + // Initialization void reinitialize(InstanceKlass* ik, int index); diff --git a/src/share/vm/utilities/accessFlags.hpp b/src/share/vm/utilities/accessFlags.hpp index a66bc9ec0..bc56262d1 100644 --- a/src/share/vm/utilities/accessFlags.hpp +++ b/src/share/vm/utilities/accessFlags.hpp @@ -76,11 +76,12 @@ enum { // These bits must not conflict with any other field-related access flags // (e.g., ACC_ENUM). // Note that the class-related ACC_ANNOTATION bit conflicts with these flags. - JVM_ACC_FIELD_ACCESS_WATCHED = 0x00002000, // field access is watched by JVMTI - JVM_ACC_FIELD_MODIFICATION_WATCHED = 0x00008000, // field modification is watched by JVMTI - JVM_ACC_FIELD_INTERNAL = 0x00000400, // internal field, same as JVM_ACC_ABSTRACT - JVM_ACC_FIELD_STABLE = 0x00000020, // @Stable field, same as JVM_ACC_SYNCHRONIZED - JVM_ACC_FIELD_HAS_GENERIC_SIGNATURE = 0x00000800, // field has generic signature + JVM_ACC_FIELD_ACCESS_WATCHED = 0x00002000, // field access is watched by JVMTI + JVM_ACC_FIELD_MODIFICATION_WATCHED = 0x00008000, // field modification is watched by JVMTI + JVM_ACC_FIELD_INTERNAL = 0x00000400, // internal field, same as JVM_ACC_ABSTRACT + JVM_ACC_FIELD_STABLE = 0x00000020, // @Stable field, same as JVM_ACC_SYNCHRONIZED and JVM_ACC_SUPER + JVM_ACC_FIELD_INITIALIZED_FINAL_UPDATE = 0x00000100, // (static) final field updated outside (class) initializer, same as JVM_ACC_NATIVE + JVM_ACC_FIELD_HAS_GENERIC_SIGNATURE = 0x00000800, // field has generic signature JVM_ACC_FIELD_INTERNAL_FLAGS = JVM_ACC_FIELD_ACCESS_WATCHED | JVM_ACC_FIELD_MODIFICATION_WATCHED | @@ -150,6 +151,8 @@ class AccessFlags VALUE_OBJ_CLASS_SPEC { bool is_field_access_watched() const { return (_flags & JVM_ACC_FIELD_ACCESS_WATCHED) != 0; } bool is_field_modification_watched() const { return (_flags & JVM_ACC_FIELD_MODIFICATION_WATCHED) != 0; } + bool has_field_initialized_final_update() const + { return (_flags & JVM_ACC_FIELD_INITIALIZED_FINAL_UPDATE) != 0; } bool on_stack() const { return (_flags & JVM_ACC_ON_STACK) != 0; } bool is_internal() const { return (_flags & JVM_ACC_FIELD_INTERNAL) != 0; } bool is_stable() const { return (_flags & JVM_ACC_FIELD_STABLE) != 0; } @@ -229,6 +232,15 @@ class AccessFlags VALUE_OBJ_CLASS_SPEC { atomic_clear_bits(JVM_ACC_FIELD_MODIFICATION_WATCHED); } } + + void set_has_field_initialized_final_update(const bool value) { + if (value) { + atomic_set_bits(JVM_ACC_FIELD_INITIALIZED_FINAL_UPDATE); + } else { + atomic_clear_bits(JVM_ACC_FIELD_INITIALIZED_FINAL_UPDATE); + } + } + void set_field_has_generic_signature() { atomic_set_bits(JVM_ACC_FIELD_HAS_GENERIC_SIGNATURE); -- GitLab