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

[Daniel D. Daugherty]

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"

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

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 so I understand the AARCH64 and X86 changes. I don't quite
understand the SPARC change... but I can be convinced otherwise.

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