Review Request: JDK-8157464: StackWalker.getCallerClass() is not
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Sep 14 08:18:21 UTC 2016
Hi Mandy,
A couple of minor comments.
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157464/webrev.02/hotspot/src/share/vm/prims/stackwalk.cpp.frames.html
135 // fill in StackFrameInfo and initialize MemberName
136 if (live_frame_info(mode)) {
137 assert (use_frames_array(mode), "Bad mode for get live frame");
138 Handle stackFrame(frames_array->obj_at(index));
139 fill_live_stackframe(stackFrame, method, bci, stream.java_frame(), CHECK_0);
140 } else if (need_method_info(mode)) {
141 assert (use_frames_array(mode), "Bad mode for get stack frame");
142 Handle stackFrame(frames_array->obj_at(index));
143 fill_stackframe(stackFrame, method, bci);
144 } else {
145 assert (get_caller_class(mode) == true, "Bad mode for get caller
class");
146 if (index == start_index && method->caller_sensitive()) {
147 ResourceMark rm(THREAD);
148 THROW_MSG_0(vmSymbols::java_lang_UnsupportedOperationException(),
149 err_msg("StackWalker::getCallerClass called from @CallerSensitive %s
method",
150 method->name_and_sig_as_C_string()));
151 }
152
153 frames_array->obj_at_put(index, method->method_holder()->java_mirror());
154 }
The else statement at the L144 is not clear.
Would it be correct to rearrange the condition like this ? :
} else if (get_caller_class(mode)) {
frames_array->obj_at_put(index,
method->method_holder()->java_mirror());
if (index == start_index && method->caller_sensitive()) {
ResourceMark rm(THREAD);
THROW_MSG_0(vmSymbols::java_lang_UnsupportedOperationException(),
err_msg("StackWalker::getCallerClass called from
@CallerSensitive %s method",
method->name_and_sig_as_C_string()));
}
} else {
assert(false, "Bad stack walk mode");
}
Also, it seems that the condition at L146 needs a short comment to
clarify that
the (index == start_index) part is for optimization (is it correct?).
If this optimization is not that important then it could be removed
for clarity.
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157464/webrev.02/jdk/test/java/lang/StackWalker/CallerSensitiveMethod/csm/jdk/test/CallerSensitiveTest.java.html
49 boolean sm = false;
50 if (args.length > 0 && args[0].equals("sm")) {
Better to add a comment that "sm" means Security Manager but not
"Sensitive Method" :).
At least, one line of two empty lines L78, L79 is useless.
Thanks,
Serguei
On 9/13/16 15:23, Mandy Chung wrote:
> Hi Coleen,
>
> Thanks for the review.
>
>
>> On Sep 13, 2016, at 2:32 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 9/13/16 2:54 PM, Daniel Fuchs wrote:
>>> Hi Mandy,
>>>
>>> This looks good to me.
>>> But I wonder about these 5 lines - isn't this introducing a change
>>> of behavior if the caller is an anonymous class?
>>>
>>> 149 InstanceKlass* ik = method->method_holder();
>>> 150 if (ik->is_anonymous()) {
>>> 151 // use the host class as the caller class
>>> 152 ik = ik->host_klass();
>>> 153 }
>>>
>>> What is the reason for returning the host class instead?
>> Somehow I think you want to see all the lambda classes in the stack trace, or skip them but the host class might be confusing because it's something like java/lang/invoke/LambdaForm a lot of the time. At least you'd get java.lang.invoke.LambdaForm$BMH/1170794006 which has a little more information.
>>
> StackWalker::walk wants to see all lambda classes in the stack trrace (if SHOW_HIDDEN_FRAMES is set). StackWalker::getCallerClass does not since it wants to find the true caller. I have revised the patch to skip the hidden frames instead:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157464/webrev.02/
>
>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8157464/webrev.01/hotspot/src/share/vm/prims/stackwalk.cpp.udiff.html
>>
>> + THROW_MSG_0(vmSymbols::java_lang_UnsupportedOperationException(),
>>
>> I think you need ResourceMark rm(THREAD); above this line.
>>
> Fixed. Thanks for catching it.
>
>> I can't review the test because I'm not sure what it's doing. Does it create a module just for the test?
>>
> It creates a module called “csm” where CallerSensitiveTest class is in and then invoke CallerSensitiveTest with and without security manager. This test can run in an unnamed module but I chose it as an example to build a module.
>
> This test invokes StackWalker::getCallerClass via static reference, reflection, MethodHandle, lambda. Also verify that StackWalker::getCallerClass can't be called from @CallerSensitive method.
>
> Mandy
>
More information about the hotspot-runtime-dev
mailing list