8251047: GC stackwalking doesn't work when intrinsics are enabled #279
Conversation
|
👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into |
|
@JornVernee This change now passes all automated pre-integration checks, type
There are currently no new commits on the ➡️ To integrate this PR with the above commit message to the |
|
Not sure it's the best way to fix the problem (need to look into the code more closely), but the fix looks good for now. |
|
Thanks for the reviews. When running the Upcalls benchmark I ran into another crash that looks related, so I'm looking into that before integrating. |
JornVernee
force-pushed
the
RegisterMap_Assert
branch
from
67d7c0a to
b2ff352
Compare
The fix looks good. Still, IMO the best solution would be to adjust CallNative so it always kills RBP in case state transition is present. |
…e RegisterMap when jumping from the entry frame to the last java frame during a GC stack walk.
JornVernee
force-pushed
the
RegisterMap_Assert
branch
from
b2ff352 to
e1eb38f
Compare
|
@iwanowww Thanks for the review.
I'm already doing this, see the changes in lcm.cpp. RBP seems to need special handling beyond killing it in the IR. Looking at some of the runtime stub code, these are also explicitly saving RBP and creating an oop map for it. I have to look at this in more detail to figure out if we can refactor the current native call support to work more like that of the runtime stubs. (though that adds a lot of indirection, so maybe it's worth it to try and remove the special handling of RBP in the compiler instead, but that probably also touches more code and implicit assumptions). I added a comment in the last revision that clarifies this, as well as only killing callee saved regs in case we are actually doing a thread state transition. |
|
/integrate |
openjdk
bot
added
integrated
|
@JornVernee |
This is more-or-less a straight port of the x86 code to AArch64. GraphKit::make_native_call() calls SharedRuntime::make_native_invoker() to generate a blob that jumps to the native function entry point. This simply switches the thread state from Java to native and handles the safepoint poll on return from native code. AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame pointer) may hold a live oop over the MachCallNative node. Normally this would be saved by RegisterSaver::save_live_registers() but the native invoker blob is not a "proper" stub routine so doesn't have an oop map. I copied the x86 solution to this where the frame pointer register is saved to the stack and a pointer to that is stored in the frame anchor. This is then read back and added to the register map when walking the stack. I saw in the PR comments [1] that this might be a temporary fix, but I'm not sure if there's any plan to change that now? Another potential fix might be to change the C2 register definition of R29 and R29_H to be save-on-call as below. This works for the TestStackWalk.java test case but I don't know whether it has other unintended side-effects. reg_def R29 ( SOC, NS, Op_RegI, 29, r29->as_VMReg() ); // fp reg_def R29_H ( SOC, NS, Op_RegI, 29, r29->as_VMReg()->next()); JMH results from jdk/incubator/foreign/CallOverhead.java to show it's working: -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false: Benchmark Mode Cnt Score Error Units CallOverhead.jni_blank avgt 30 51.450 ? 0.363 ns/op CallOverhead.jni_identity avgt 30 54.145 ? 0.627 ns/op CallOverhead.panama_args10 avgt 30 1914.431 ? 73.771 ns/op CallOverhead.panama_args5 avgt 30 1394.274 ? 49.369 ns/op CallOverhead.panama_blank avgt 30 872.878 ? 20.716 ns/op CallOverhead.panama_blank_trivial avgt 30 873.852 ? 21.350 ns/op CallOverhead.panama_identity avgt 30 1058.729 ? 20.229 ns/op CallOverhead.panama_identity_memory_address avgt 30 1041.648 ? 22.930 ns/op CallOverhead.panama_identity_struct avgt 30 3212.483 ? 151.627 ns/op CallOverhead.panama_identity_trivial avgt 30 1056.398 ? 18.779 ns/op -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true: Benchmark Mode Cnt Score Error Units CallOverhead.jni_blank avgt 30 51.519 ? 0.345 ns/op CallOverhead.jni_identity avgt 30 54.689 ? 0.687 ns/op CallOverhead.panama_args10 avgt 30 42.856 ? 0.760 ns/op CallOverhead.panama_args5 avgt 30 42.192 ? 0.712 ns/op CallOverhead.panama_blank avgt 30 41.934 ? 0.349 ns/op CallOverhead.panama_blank_trivial avgt 30 2.806 ? 0.545 ns/op CallOverhead.panama_identity avgt 30 44.043 ? 0.398 ns/op CallOverhead.panama_identity_memory_address avgt 30 45.021 ? 0.409 ns/op CallOverhead.panama_identity_struct avgt 30 3206.829 ? 175.750 ns/op CallOverhead.panama_identity_trivial avgt 30 7.849 ? 1.651 ns/op [1] openjdk/panama-foreign#279 (comment)
Hi,
This patch fixes GC stack walking for optimized native calls.
For calls in compiled frames, the callee argument oops are processed using special case code in
CompiledMethod::preserve_callee_argument_oops. This code relies on the original bytecode to find the signature of the callee, but for optimized native calls this doesn't make sense, since the original call is replaced, and some of the arguments (notably some that are oops) are discarded. In the case of an optimized native call, we can skip all callee argument oop processing, since we can't pass any oops to native calls any ways.Since the current code was already looking at debug info to find the signature of the callee, I piggy-backed on that to add a flag for optimized native calls. If the flag is set, we return early, skipping any processing.
(I left in the assert I used to debug this, since it gives the failure a better error message, should we run into this again in the future)
Thanks,
Jorn
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/panama-foreign pull/279/head:pull/279$ git checkout pull/279