JDK-8235438 : [JVMCI] StackTraceElement::decode should use the original Method
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 11,14
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-12-05
  • Updated: 2020-03-31
  • Resolved: 2019-12-06
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11 JDK 14
11.0.7-oracleFixed 14 b27Fixed
Description
The JDK14 version of StackTraceElement::decode is based on the JDK8 code which contains mixed usages of method->constants() and method->method_holder()->constants() assuming they point to the same thing.  In the case of anonymous methods this isn't true.  Usually this isn't a problem but if CDS is enabled the version flag of method->method_holder()->constants() is non-zero but version of method->constants() is 0 which causes the code to switch constants pools and it reads garbage. JDK-8140685 refactored this code to remove this logic and the JVMCI version of this code should be converted to use the same scheme.
Comments
OpenJDK 11 updates backport will follow after larger Graal integration, see https://github.com/oracle/graal/issues/2196
11-03-2020
URL: https://hg.openjdk.java.net/jdk/jdk/rev/8539243dc929 User: kvn Date: 2019-12-06 22:47:23 +0000
06-12-2019
Some refactoring to address review comments: https://cr.openjdk.java.net/~kvn/8235438/webrev.01/
06-12-2019
I will send RFR for JDK-8216977 backport to 11u-oracle and see what will happen.
06-12-2019
[~kvn] that looks good to me. I'll tweak our code to be more in sync with yours. The ShowHiddenFrames stuff is confusing. I think maybe we should include the changes from JDK-8216977 in our 11.
06-12-2019
https://cr.openjdk.java.net/~kvn/8235438/webrev.00/
05-12-2019
I am changing parameter declaration in decode() and decode_file_and_line() methods from `methodHandle method` to `const methodHandle& method`. That is what passed from JVMCIEnv::new_StackTraceElement() and java_lang_StackTraceElement::fill_in().
05-12-2019
ShowHiddenFrames code change was backported into OpenJDK 11u: JDK-8231049
05-12-2019
Actually I think the JVMCI version should ignore the ShowHiddenFrames flag. I guess that doesn't affect your port since that's gone but I think I need to remove turn that into a parameter in the JVMCI 11 version.
05-12-2019
JDK 14 version does not have ShowHiddenFrames code. Have to adjust changes which are based on 11u. Code was removed by JDK-8216977. Also there was change by JDK-8233913.
05-12-2019
Yes it should be the same as that changeset. It looks bigger than it is. It's a pretty straightforward refactor of that chunk of code in the middle. The biggest diffs come from the deletion of the JVMCI code.
05-12-2019
[~never] Is it the same as next ?: https://github.com/graalvm/labs-openjdk-11/commit/2309e6deee6c189870b02d9d7df5d432f1589580 Your code in the comment is modified too much by JBS.
05-12-2019
Suggested fix: commit 66353bb1b702ff31103a1acf280569b26de195ed Author: Tom Rodriguez <tom.rodriguez@oracle.com> Date: Mon Dec 2 20:32:32 2019 -0800 StackTraceElement::decode should use the original Method diff --git a/src/hotspot/share/classfile/javaClasses.cpp b/src/hotspot/share/classfile/javaClasses.cpp index cbcb004df0..ce4b00fe03 100644 --- a/src/hotspot/share/classfile/javaClasses.cpp +++ b/src/hotspot/share/classfile/javaClasses.cpp @@ -2614,70 +2614,58 @@ void java_lang_StackTraceElement::fill_in(Handle element, java_lang_StackTraceElement::set_fileName(element(), NULL); java_lang_StackTraceElement::set_lineNumber(element(), -1); } else { - // Fill in source file name and line number. - Symbol* source = Backtrace::get_source_file_name(holder, version); - oop source_file = java_lang_Class::source_file(java_class()); - if (source != NULL) { - // Class was not redefined. We can trust its cache if set, - // else we have to initialize it. - if (source_file == NULL) { - source_file = StringTable::intern(source, CHECK); - java_lang_Class::set_source_file(java_class(), source_file); - } - } else { - // Class was redefined. Dump the cache if it was set. - if (source_file != NULL) { - source_file = NULL; - java_lang_Class::set_source_file(java_class(), source_file); - } - if (ShowHiddenFrames) { - source = vmSymbols::unknown_class_name(); - source_file = StringTable::intern(source, CHECK); - } - } - java_lang_StackTraceElement::set_fileName(element(), source_file); + Symbol* source; + oop source_file; + int line_number; + decode_file_and_line(java_class, holder, version, method, bci, source, source_file, line_number, CHECK); - int line_number = Backtrace::get_line_number(method, bci); + java_lang_StackTraceElement::set_fileName(element(), source_file); java_lang_StackTraceElement::set_lineNumber(element(), line_number); } } -#if INCLUDE_JVMCI -void java_lang_StackTraceElement::decode(Handle mirror, methodHandle method, int bci, Symbol*& methodname, Symbol*& filename, int& line_number) { - int method_id = method->orig_method_idnum(); - int cpref = method->name_index(); - decode(mirror, method_id, method->constants()->version(), bci, cpref, methodname, filename, line_number); +void java_lang_StackTraceElement::decode_file_and_line(Handle java_class, InstanceKlass* holder, int version, + methodHandle method, int bci, + Symbol*& source, oop& source_file, int& line_number, TRAPS) { + // Fill in source file name and line number. + source = Backtrace::get_source_file_name(holder, version); + source_file = java_lang_Class::source_file(java_class()); + if (source != NULL) { + // Class was not redefined. We can trust its cache if set, + // else we have to initialize it. + if (source_file == NULL) { + source_file = StringTable::intern(source, CHECK); + java_lang_Class::set_source_file(java_class(), source_file); + } + } else { + // Class was redefined. Dump the cache if it was set. + if (source_file != NULL) { + source_file = NULL; + java_lang_Class::set_source_file(java_class(), source_file); + } + if (ShowHiddenFrames) { + source = vmSymbols::unknown_class_name(); + source_file = StringTable::intern(source, CHECK); + } + } + line_number = Backtrace::get_line_number(method, bci); } -void java_lang_StackTraceElement::decode(Handle mirror, int method_id, int version, int bci, int cpref, Symbol*& methodname, Symbol*& filename, int& line_number) { - // Fill in class name - InstanceKlass* holder = InstanceKlass::cast(java_lang_Class::as_Klass(mirror())); - Method* method = holder->method_with_orig_idnum(method_id, version); +#if INCLUDE_JVMCI +void java_lang_StackTraceElement::decode(methodHandle method, int bci, Symbol*& methodname, Symbol*& filename, int& line_number, TRAPS) { + ResourceMark rm(THREAD); + HandleMark hm(THREAD); - // The method can be NULL if the requested class version is gone - Symbol* sym = (method != NULL) ? method->name() : holder->constants()->symbol_at(cpref); + filename = NULL; + line_number = -1; - // Fill in method name - methodname = sym; + oop source_file; + int version = method->constants()->version(); + InstanceKlass* holder = method->method_holder(); + Handle java_class(THREAD, holder->java_mirror()); + decode_file_and_line(java_class, holder, version, method, bci, filename, source_file, line_number, CHECK); - if (!version_matches(method, version)) { - // If the method was redefined, accurate line number information isn't available - filename = NULL; - line_number = -1; - } else { - // Fill in source file name and line number. - // Use a specific ik version as a holder since the mirror might - // refer to a version that is now obsolete and no longer accessible - // via the previous versions list. - holder = holder->get_klass_version(version); - assert(holder != NULL, "sanity check"); - Symbol* source = holder->source_file_name(); - if (ShowHiddenFrames && source == NULL) { - source = vmSymbols::unknown_class_name(); - } - filename = source; - line_number = Backtrace::get_line_number(method, bci); - } + methodname = method->name(); } #endif // INCLUDE_JVMCI diff --git a/src/hotspot/share/classfile/javaClasses.hpp b/src/hotspot/share/classfile/javaClasses.hpp index 08f14226ab..f0bbfa5ae8 100644 --- a/src/hotspot/share/classfile/javaClasses.hpp +++ b/src/hotspot/share/classfile/javaClasses.hpp @@ -1374,6 +1374,10 @@ class java_lang_StackTraceElement: AllStatic { static void set_lineNumber(oop element, int value); static void set_declaringClassObject(oop element, oop value); + static void decode_file_and_line(Handle java_mirror, InstanceKlass* holder, int version, + methodHandle method, int bci, + Symbol*& source, oop& source_file, int& line_number, TRAPS); + public: // Create an instance of StackTraceElement static oop create(const methodHandle& method, int bci, TRAPS); @@ -1385,8 +1389,7 @@ class java_lang_StackTraceElement: AllStatic { static void serialize(SerializeClosure* f) NOT_CDS_RETURN; #if INCLUDE_JVMCI - static void decode(Handle mirror, int method, int version, int bci, int cpref, Symbol*& methodName, Symbol*& fileName, int& lineNumber); - static void decode(Handle mirror, methodHandle method, int bci, Symbol*& methodName, Symbol*& fileName, int& lineNumber); + static void decode(methodHandle method, int bci, Symbol*& methodName, Symbol*& fileName, int& lineNumber, TRAPS); #endif // Debugging diff --git a/src/hotspot/share/jvmci/jvmciEnv.cpp b/src/hotspot/share/jvmci/jvmciEnv.cpp index 181af70d97..1b5608f25b 100644 --- a/src/hotspot/share/jvmci/jvmciEnv.cpp +++ b/src/hotspot/share/jvmci/jvmciEnv.cpp @@ -894,8 +894,7 @@ JVMCIObject JVMCIEnv::new_StackTraceElement(const methodHandle& method, int bci, Symbol* method_name_sym; Symbol* file_name_sym; int line_number; - Handle mirror (THREAD, method->method_holder()->java_mirror()); - java_lang_StackTraceElement::decode(mirror, method, bci, method_name_sym, file_name_sym, line_number); + java_lang_StackTraceElement::decode(method, bci, method_name_sym, file_name_sym, line_number, CHECK_(JVMCIObject())); InstanceKlass* holder = method->method_holder(); const char* declaring_class_str = holder->external_name();
05-12-2019