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
