Hi Mandy,
It looks pretty good to me.
At least, I do not see any obvious issues.
Just some minor comments below.
webrev.03/hotspot/src/share/vm/classfile/javaClasses.cpp
2128 Symbol* java_lang_StackFrameInfo::get_file_name(Handle
stackFrame, InstanceKlass* holder) {
2129 if (MemberNameInStackFrame) {
2130 return holder->source_file_name();
2131 } else {
2132 int bci = stackFrame->int_field(_bci_offset);
2133 short version = stackFrame->short_field(_version_offset);
2134
2135 return Backtrace::get_source_file_name(holder, version);
2136 }
2137 }
The 'int bci' is not used above.
2139 void java_lang_StackFrameInfo::set_method_and_bci(Handle
stackFrame, const methodHandle& method, int bci) {
2140 if (MemberNameInStackFrame) {
2141 oop mname = stackFrame->obj_field(_memberName_offset);
2142 InstanceKlass* ik = method->method_holder();
2143 CallInfo info(method(), ik);
2144 MethodHandles::init_method_MemberName(mname, info);
2145 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2146 // method may be redefined; store the version
2147 int version = method->constants()->version();
2148 assert((jushort)version == version, "version should be short");
2149 java_lang_StackFrameInfo::set_version(stackFrame(),
(short)version);
2150 } else {
2151 int mid = method->orig_method_idnum();
2152 int version = method->constants()->version();
2153 int cpref = method->name_index();
2154 assert((jushort)mid == mid, "mid should be short");
2155 assert((jushort)version == version, "version should be short");
2156 assert((jushort)cpref == cpref, "cpref should be short");
2157
2158 java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
2159 java_lang_StackFrameInfo::set_version(stackFrame(),
(short)version);
2160 java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
2161 java_lang_StackFrameInfo::set_bci(stackFrame(), bci);
2162 }
2163 }
Minor: The calls to set_version() and set_bci() are the same for both
alternatives
and can be done just once before the if-else statement as
below.
With such refactoring it is clear what the common part is.
void java_lang_StackFrameInfo::set_method_and_bci(Handle stackFrame,
const methodHandle& method, int bci) {
java_lang_StackFrameInfo::set_bci(stackFrame(), bci); // method may
be redefined; store the version
int version = method->constants()->version();
assert((jushort)version == version, "version should be short");
java_lang_StackFrameInfo::set_version(stackFrame(), (short)version);
if (MemberNameInStackFrame) {
oop mname = stackFrame->obj_field(_memberName_offset);
InstanceKlass* ik = method->method_holder();
CallInfo info(method(), ik);
MethodHandles::init_method_MemberName(mname, info);
} else {
int mid = method->orig_method_idnum();
int cpref = method->name_index();
assert((jushort)mid == mid, "mid should be short");
assert((jushort)cpref == cpref, "cpref should be short");
java_lang_StackFrameInfo::set_mid(stackFrame(), (short)mid);
java_lang_StackFrameInfo::set_cpref(stackFrame(), (short)cpref);
}
}
webrev.03/hotspot/src/share/vm/prims/jvm.cpp
572 int limit = start_index+frame_count; 597 int limit =
start_index+frame_count;
Minor: Need spaces around the '+'.
webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp
62 // Parameters:
63 // thread. Current Java thread.
64 // magic. Magic value used for each stack walking
65 // classes_array User-supplied buffers. The 0th element is
reserved
. . .
86 // Parameters:
87 // mode. Restrict which frames to be decoded.
88 // decode_limit. Maximum of frames to be decoded.
89 // start_index. Start index to the user-supplied buffers.
90 // classes_array. Buffer to store classes in, starting at
start_index.
91 // frames_array. Buffer to store StackFrame in, starting at
start_index.
92 // NULL if not used.
93 // end_index. End index to the user-supplied buffers with
unpacked frames.
94 //
. . .
274 // Parameters:
275 // stackStream. StackStream object
276 // mode. Stack walking mode.
277 // skip_frames. Number of frames to be skipped.
278 // frame_count. Number of frames to be traversed.
279 // start_index. Start index to the user-supplied buffers.
280 // classes_array. Buffer to store classes in, starting at
start_index.
281 // frames_array. Buffer to store StackFrame in, starting at
start_index.
. . .
414 // Parameters:
415 // stackStream. StackStream object
416 // mode. Stack walking mode.
417 // magic. Must be valid value to continue the stack walk
418 // frame_count. Number of frames to be decoded.
419 // start_index. Start index to the user-supplied buffers.
420 // classes_array. Buffer to store classes in, starting at
start_index.
421 // frames_array. Buffer to store StackFrame in, starting at
start_index.
Minor: Dots after the parameter names looks strange.
Probably better to remove them or replace with colons.
Also, the line 65 is inconsistent with others.
114 if (!ShowHiddenFrames && StackWalk::skip_hidden_frames(mode)) {
115 if (method->is_hidden()) {
116 if (TraceStackWalk) {
117 tty->print(" hidden method: ");
method->print_short_name();
118 tty->print("\n");
119 }
120 continue;
121 }
122 }
Minor: Indent at the line 115 must be +2.
338 if (!skip_to_fillInStackTrace) {
339 if (ik == SystemDictionary::Throwable_klass() &&
340 method->name() ==
vmSymbols::fillInStackTrace_name()) {
341 // this frame will be skipped
342 skip_to_fillInStackTrace = true;
343 }
344 } else if
(!(ik->is_subclass_of(SystemDictionary::Throwable_klass()) &&
345 method->name() ==
vmSymbols::object_initializer_name())) {
346 // there are none or we've seen them all - either way
stop checking
347 skip_throwableInit_check = true;
348 break;
349 }
Minor: Indent must be +2 at 341 and 347.
360 for (int n=0; n < skip_frames && !vfst.at_end(); vfst.next(),
n++) {
451 int count = frame_count+start_index;
Minor: Need spaces around the '=' and '+'.
392 // Link the thread and vframe stream into the callee-visible
object:
397 // Do this before anything else happens, to disable any
lingering stream objects:
400 // Throw pending exception if we must:
Minor: Better to place dots instead of colons at the end.
Thanks,
Serguei
