RFR: 8259937: guarantee(loc != NULL) failed: missing saved register with native invoker [v2]

[Jorn Vernee]

On Fri, 12 Feb 2021 09:37:02 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> We spotted this issue with Shenandoah and I managed to write a simple
>> test case that reproduces it reliably with Shenandoah but the issue is
>> independent of the GC.
>> 
>> The loop in the test case calls a native invoker with an oop live in
>> rbp. rbp is saved in the native invoker stub's frame. A safepoint is
>> triggered from the safepoint check in the native invoker. The stack
>> walking code sees that rbp contains an oop but can't find where that
>> oop is stored. That's because stack walking updates the caller's frame
>> with the location of rbp in the callee on calls to
>> frame::sender(). But the current code sets the last java frame to be
>> the compiled frame where rbp is live. So there's no call to
>> frame::sender() to update the location rbp. The fix I propose is that
>> the frame of the native invoker be visible by stack walking. On a
>> safepoint, stack walking starts from the native invoker thread, then
>> calls frame::sender() to move to the compiled frame. That causes rbp
>> to be properly recorded with its location in the native invoker frame.
>> 
>> Same problem affects both x86 and aarch64. I've tested this patch with:
>> 
>> make run-test TEST="java/foreign" TEST_VM_OPTS="-Xcomp" JTREG="TIMEOUT_FACTOR=10"
>> 
>> on both platforms.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   broken build

LGTM, but I've left some suggestions in line.

Thanks for cleaning up the frame layout code. Having a fixed frame is much better than repeatedly modifying the stack pointer.

I've also done some downstream stress testing with jextract, and everything works as expected.

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 3514:

> 3512: 
> 3513:   OopMapSet* oop_maps() const {
> 3514:     return _oop_maps;

The `saved_rbp_address_offset` field was added to JavaFrameAnchor just to serve this particular use case. It should be cleaned up now that it is no longer used.

If you don't want to take care of that, we can file a followup.

There is code in `frame_*.cpp`, `thread_*.hpp`, and `javaFrameAnchor_*.hpp` that deals with this field (not on all platforms/archs though). See https://github.com/openjdk/jdk/pull/634/files & https://github.com/openjdk/jdk/pull/1711/files for the original diffs.

test/hotspot/jtreg/gc/shenandoah/compiler/TestLinkToNativeRBP.java line 50:

> 48: public class TestLinkToNativeRBP {
> 49:     final static CLinker abi = CLinker.getInstance();
> 50:     static final LibraryLookup lookup = LibraryLookup.ofDefault();

The default library can be unreliable (but we've kept it in lieu of something better, which is still in the pipeline). We've had problems with it in the past since it acts differently in different environments, so we try to avoid it in tests.

It would be more robust to add a small test library that defines a dummy function and then depend on that instead.

If you've not done this before; you can just add a `lib<Name>.c` file to the same directory as the main test file and the build system will compile it for you. You can then load it with `LibraryLookup.ofLibrary("<Name>")` in the test. See the [test/jdk/java/foreign/stackwalk](https://github.com/openjdk/jdk/tree/master/test/jdk/java/foreign/stackwalk) directory for an example (one noteworthy thing is that the function has to be explicitly exported in order to work on Windows. See the example).

-------------

Marked as reviewed by jvernee (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2528