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

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
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.

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

Commit messages:
 - whitespaces
 - fix & test

Changes: https://git.openjdk.java.net/jdk/pull/2528/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259937
  Stats: 395 lines in 16 files changed: 264 ins; 53 del; 78 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov-2
On Thu, 11 Feb 2021 15:31:11 GMT, Roland Westrelin <[hidden email]> 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.

Seems reasonable.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
> 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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2528/files
  - new: https://git.openjdk.java.net/jdk/pull/2528/files/88eb13d0..5b9dfff7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=00-01

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dinn-2
In reply to this post by Vladimir Kozlov-2
On Thu, 11 Feb 2021 18:49:48 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   broken build
>
> Seems reasonable.

The code here looks ok. I'm slightly concerned about the consequences of adding a new stack frame visible to stack walking code. Does this have the potential to break serviceability code that reports and/or analyzes stack frames (whether that's code in OpenJDK or 3rd party code)?

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
On Fri, 12 Feb 2021 10:41:18 GMT, Andrew Dinn <[hidden email]> wrote:

> The code here looks ok. I'm slightly concerned about the consequences of adding a new stack frame visible to stack walking code. Does this have the potential to break serviceability code that reports and/or analyzes stack frames (whether that's code in OpenJDK or 3rd party code)?

Thanks for the review. The native invoker code is new in jdk 16 so it's unlikely tools already rely of some specific layout.

@iwanowww are you the author of the native invoker code? What do you think?

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Jorn Vernee-2
In reply to this post by Andrew Dinn-2
On Fri, 12 Feb 2021 10:41:18 GMT, Andrew Dinn <[hidden email]> wrote:

>> Seems reasonable.
>
> The code here looks ok. I'm slightly concerned about the consequences of adding a new stack frame visible to stack walking code. Does this have the potential to break serviceability code that reports and/or analyzes stack frames (whether that's code in OpenJDK or 3rd party code)?

@adinn I'm not aware of any such use-cases (whether in the JDK or elsewhere). They would only be affected if they were using Panama native calls, which were introduce pretty recently, and are also still in incubator state.

Inside the JDK the only place where this code is currently being used is in the jdk/java/foreign test stuite, as well as in the internal implementation of the Panama linker. If that test suite still passes I'm happy to call it safe.

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Jorn Vernee-2
In reply to this post by Roland Westrelin-4
On Fri, 12 Feb 2021 09:37:02 GMT, Roland Westrelin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

 - improved test
 - cleanup
 - Merge branch 'master' into JDK-8259937
 - test & debug
 - broken build
 - whitespaces
 - fix & test

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2528/files
  - new: https://git.openjdk.java.net/jdk/pull/2528/files/5b9dfff7..cef05b6f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=01-02

  Stats: 18656 lines in 546 files changed: 11988 ins; 3826 del; 2842 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
> 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:

  Revert "test & debug"
 
  This reverts commit cb9dd24c9fcccc6997e9fca874e2860f966b9576.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2528/files
  - new: https://git.openjdk.java.net/jdk/pull/2528/files/cef05b6f..9f80616f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=02-03

  Stats: 72 lines in 1 file changed: 0 ins; 72 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Jorn Vernee-2
On Tue, 23 Feb 2021 12:59:44 GMT, Jorn Vernee <[hidden email]> wrote:

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

@JornVernee thanks for the review and the comments. The new commits should address them (cb9dd24 was pushed by accident and reverted).

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Jorn Vernee-2
In reply to this post by Roland Westrelin-4
On Wed, 24 Feb 2021 16:12:59 GMT, Roland Westrelin <[hidden email]> 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:
>
>   Revert "test & debug"
>  
>   This reverts commit cb9dd24c9fcccc6997e9fca874e2860f966b9576.

Thanks for addressing the comments! Looks good.

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

Marked as reviewed by jvernee (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dinn-2
On Wed, 24 Feb 2021 20:57:10 GMT, Jorn Vernee <[hidden email]> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert "test & debug"
>>  
>>   This reverts commit cb9dd24c9fcccc6997e9fca874e2860f966b9576.
>
> Thanks for addressing the comments! Looks good.

@JornVernee I'm not clear that your response addresses my point. I'm concerned that a thread stack dump reported by serviceability code may contain an extra frame for the stub call. This could occur while the Java thread is still in native and it could also include the case wher the native call re-enters into Java i.e. the extra frame could appear at the top of the stack dump or interleaved between Java method frames.

I don't see how that problem is mitigated by your suggestion that this only relates to Panama API use. Code which consumes any such stack dump (incluing 3rd party code) that might be affected by the presence of this extra frame will not care (or even be aware) that the native callout is a Panama call.

Anyway, since no one from the serviceability team has noted this as a potential problem I'm ok to see the patch proceed.

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ivanov-2
In reply to this post by Roland Westrelin-4
On Wed, 24 Feb 2021 16:12:59 GMT, Roland Westrelin <[hidden email]> 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:
>
>   Revert "test & debug"
>  
>   This reverts commit cb9dd24c9fcccc6997e9fca874e2860f966b9576.

Marked as reviewed by vlivanov (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ivanov-2
In reply to this post by Andrew Dinn-2
On Thu, 25 Feb 2021 10:06:47 GMT, Andrew Dinn <[hidden email]> wrote:

>> Thanks for addressing the comments! Looks good.
>
> @JornVernee I'm not clear that your response addresses my point. I'm concerned that a thread stack dump reported by serviceability code may contain an extra frame for the stub call. This could occur while the Java thread is still in native and it could also include the case wher the native call re-enters into Java i.e. the extra frame could appear at the top of the stack dump or interleaved between Java method frames.
>
> I don't see how that problem is mitigated by your suggestion that this only relates to Panama API use. Code which consumes any such stack dump (incluing 3rd party code) that might be affected by the presence of this extra frame will not care (or even be aware) that the native callout is a Panama call.
>
> Anyway, since no one from the serviceability team has noted this as a potential problem I'm ok to see the patch proceed.

Overall, the fix looks good.

At some point, there was no frame for native invoker set up and native state transitions were put inline in generated code, but that was rewritten.

Regarding the refactorings: I find newly introduced `spill_register()`/`fill_register()` methods very confusing.
I'd prefer to see `spill_output_registers()`/`fill_output_registers()` instead and an assert in `NativeInvokerGenerator` constructor (akin to the one in `NativeInvokerGenerator::generate()` on x86_64):
  assert(_output_registers.length() <= 1
    || (_output_registers.length() == 2 && !_output_registers.at(1)->is_valid()), "no multi-reg returns");

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
> 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:

  Vladimir I's review

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2528/files
  - new: https://git.openjdk.java.net/jdk/pull/2528/files/9f80616f..99da4e3a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=03-04

  Stats: 26 lines in 2 files changed: 9 ins; 3 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Vladimir Ivanov-2
On Fri, 26 Feb 2021 16:07:32 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert "test & debug"
>>  
>>   This reverts commit cb9dd24c9fcccc6997e9fca874e2860f966b9576.
>
> Marked as reviewed by vlivanov (Reviewer).

> Overall, the fix looks good.
>
> At some point, there was no frame for native invoker set up and native state transitions were put inline in generated code, but that was rewritten.
>
> Regarding the refactorings: I find newly introduced `spill_register()`/`fill_register()` methods very confusing.
> I'd prefer to see `spill_output_registers()`/`fill_output_registers()` instead and an assert in `NativeInvokerGenerator` constructor (akin to the one in `NativeInvokerGenerator::generate()` on x86_64):
>
> ```
>   assert(_output_registers.length() <= 1
>     || (_output_registers.length() == 2 && !_output_registers.at(1)->is_valid()), "no multi-reg returns");
> ```

Thanks for the review. I pushed a new change. Does that look ok to you now?

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

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ivanov-2
In reply to this post by Roland Westrelin-4
On Mon, 1 Mar 2021 08:19:22 GMT, Roland Westrelin <[hidden email]> 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:
>
>   Vladimir I's review

It looks much better now. Thanks!

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

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

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

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
> 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:

  whitespaces

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2528/files
  - new: https://git.openjdk.java.net/jdk/pull/2528/files/99da4e3a..17a7c4ce

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2528&range=04-05

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2528.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2528/head:pull/2528

PR: https://git.openjdk.java.net/jdk/pull/2528
Reply | Threaded
Open this post in threaded view
|

Integrated: 8259937: guarantee(loc != NULL) failed: missing saved register with native invoker

Roland Westrelin-4
In reply to this post by Roland Westrelin-4
On Thu, 11 Feb 2021 15:31:11 GMT, Roland Westrelin <[hidden email]> 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.

This pull request has now been integrated.

Changeset: 6baecf39
Author:    Roland Westrelin <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/6baecf39
Stats:     487 lines in 28 files changed: 305 ins; 103 del; 79 mod

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

Reviewed-by: kvn, jvernee, vlivanov

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

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