RFR(S): 8166679: JNI AsyncGetCallTrace replaces topmost frame name with <no Java callstack recorded> starting with Java 9 b133

[Daniel D. Daugherty]

On 10/21/16 1:42 PM, Coleen Phillimore wrote:
>
> Chris,
>
> This change looks good.  Thank you for the analysis and fixing the 
> regression.
>
> On 10/21/16 3:13 PM, Chris Plummer wrote:
>> Hi Dan,
>>
>> Thanks for the review. Comments inline below:
>>
>> On 10/21/16 7:59 AM, Daniel D. Daugherty wrote:
>>> On 10/20/16 2:28 PM, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> Please review the following:
>>>>
>>>> http://cr.openjdk.java.net/~cjplummer/8166679/webrev.00/webrev.hotspot/ 
>>>>
>>>
>>> src/cpu/aarch64/vm/frame_aarch64.cpp
>>>     So we're in a "if (StubRoutines::returns_to_call_stub()" block
>>>     and the assumption was that a frame that returns to a call stub
>>>     must be an entry frame. Hence the use of is_entry_frame_valid().
>>>     However, your investigation revealed that you can be in an
>>>     interpreter frame that returns to a call stub here. That sounds
>>>     both familiar and right :-)
>>>
>>>     L209:       bool jcw_safe = (jcw < thread->stack_base()) && ( 
>>> jcw > (address)sender.fp());
>>>         nit: please remove extra blank here: "( jcw"
>> Ok.
>>>
>>>     I like the new JavaCallWrapper sanity check. I never thought of
>>>     that when I worked on AsyncGetCallTrace().
>>>
>>> src/cpu/sparc/vm/frame_sparc.cpp
>>>     old L281:     if (sender.is_entry_frame()) {
>>>     old L282:       return sender.is_entry_frame_valid(thread);
>>>     old L283:     }
>>>         I don't understand this one. Why isn't is_entry_frame_valid()
>>>         correct here? You are in a "if (sender.is_entry_frame())" 
>>> block.
>> I starred at this one a bit too, since the code is not quite the same 
>> as x86 and aarch64. I'm not 100% sure I got it right, so I opted to 
>> just change it to what used to be there, especially since 8159284 
>> never turned up on sparc. I did try to go down the path of making 
>> sure that 8166679 (this CR I'm fixing) does occur on Solaris-sparc, 
>> but getting Dev Studio installed on a Solaris-sparc machine was 
>> proving difficult. Maybe I should take another stab at that.
>>
>> As for the similarities and differences between the sparc code an 
>> x86, for x86 before my changes we had:
>>
>>       if (StubRoutines::returns_to_call_stub(sender_pc)) {
>>         ...
>>         frame sender(sender_sp, sender_unextended_sp, saved_fp, 
>> sender_pc);
>>         return sender.is_entry_frame_valid(thread);
>>       }
>>
>> And for sparc:
>>
>>       frame sender(_SENDER_SP, younger_sp, adjusted_stack);
>>       if (sender.is_entry_frame()) {
>>         return sender.is_entry_frame_valid(thread);
>>       }
>>
>> So for x86 we are only adding the sender.is_entry_frame_valid() check 
>> if the "current frame" returns to a stub, but for sparc we are doing 
>> the check if the "sender frame" is an entry frame. I don't know the 
>> reason for this difference. Aren't stubs entry frames? If yes, it 
>> seem that having the check done in this way would cause this CR on 
>> sparc just like it does on sparc.
>
> I looked at this too and decided the platforms were equivalent, only 
> coded differently.   On sparc we create a sender frame, and x86 we 
> look at sender_pc before creating a sender frame.  And is_entry_frame is:
>
> inline bool frame::is_entry_frame() const {
>   return StubRoutines::returns_to_call_stub(pc());
> }

That just makes this even more "interesting".

So what we're saying here is that for either form of the question:

     Is this an entry frame?

we cannot call is_entry_frame_valid() because that function will
sometimes return false when the "entry frame" is also an interpreter
frame...

Are we possibly fixing this in the wrong place? Dunno. It's Friday
afternoon and maybe I'm just too fried to think this one through...

Dan


>
> Thanks,
> Coleen
>
>>>
>>>     I can see wanting to add the JavaCallWrapper sanity check as
>>>     an additional check. If you do that:
>>>
>>>     L286       bool jcw_safe = (jcw <= thread->stack_base()) && ( 
>>> jcw > sender_fp);
>>>         nit: please remove extra blank here: "( jcw"
>> Ok.
>>>
>>> src/cpu/x86/vm/frame_x86.cpp
>>>     Again we're in a if (StubRoutines::returns_to_call_stub()" block
>>>     so I see why is_entry_frame_valid() is not the right call.
>>>
>>>     L208:       bool jcw_safe = (jcw < thread->stack_base()) && ( 
>>> jcw > (address)sender.fp());
>>>         nit: please remove extra blank here: "( jcw"
>> Ok.
>>>
>>>
>>> OK so I understand the AARCH64 and X86 changes. I don't quite
>>> understand the SPARC change... but I can be convinced otherwise.
>> Ok. Let me know what you think now after a bit more explanation. I 
>> can put some more effort into trying out the test case on sprarc if 
>> needed.
>>
>> thanks,
>>
>> Chris
>>>
>>> If you fix the nits, I don't need to see a new webrev.
>>>
>>> Dan
>>>
>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8166679
>>>>
>>>> The fix is to partially undo the changes for JDK-8159284. There are 
>>>> two places where the fix for JDK-8159284 added an extra check of 
>>>> the validity of the entry frame, but really only the first one is 
>>>> appropriate since for the second one we are not in an entry frame. 
>>>> More details can be found near the end of the bug comments.
>>>>
>>>> Note I did a straight patch of the old version of the code. It 
>>>> could probably use some formatting and comment cleanup. I decided 
>>>> not to clean it up to make it easy to compare the current code with 
>>>> the original. I'll clean it up if you feel it would be best to.
>>>>
>>>> Tested by running KitchenSink more times than I can count, since 
>>>> that's where JDK-8159284 turned up. However, that's not proving 
>>>> much since I could not reproduce JDK-8159284 even without its fix 
>>>> in place (it also couldn't be reproduced at the time JDK-8159284 
>>>> was was being investigated and fixed). For this reason I can't be 
>>>> 100% sure that JDK-8159284 is not being re-introduced with my changes.
>>>>
>>>> Also tested by running a very large set of tests trough RBT, close 
>>>> to what we do for PIT testing, minus product builds and a few tests 
>>>> that take a long time to run.
>>>>
>>>> Lastly, I also tested with the test case in the CR to make sure it 
>>>> now passes. Unforgettably it's not possible to add the test case as 
>>>> a jtreg test since it requires the installation of the Oracle 
>>>> Studio tools.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>
>>
>