Review Request: JDK-8157464: StackWalker.getCallerClass() is not

[serguei.spitsyn at oracle.com]

On 9/14/16 01:18, serguei.spitsyn at oracle.com wrote:
>
> Hi Mandy,
>
Forgot to say that the fix looks good to me. :)

Thanks,
Serguei

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