[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 8166679: JNI AsyncGetCallTrace replaces topmost frame name with <no Java callstack recor
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2016-10-21 22:22:27
Message-ID: 637e7454-ec9a-caa9-483c-cc7818a4ba89 () oracle ! com
[Download RAW message or body]

 > Ok. Let me know what you think now after a bit more explanation.

I'm good with it. Thumbs up!

Dan



On 10/21/16 1: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 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
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]