8315954: getArgumentValues002.java fails on Graal #15705

dougxc · 2023-09-13T09:46:01Z

This PR adds HotSpotResolvedJavaMethod.getOopMapAt to get the oop map for a method at a given BCI. This is required to do correct clearing of oops at OSR entry points.

As part of this addition, I needed to be able to detect requests for oop maps at invalid BCIs. For this, I added InterpreterOopMap::has_valid_mask(). When an oop map computation is requested for an invalid BCI, this method returns false.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8315954: getArgumentValues002.java fails on Graal (Bug - P4)

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to c6c6c0d8
Frederic Parain (@fparain - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15705/head:pull/15705
$ git checkout pull/15705

Update a local copy of the PR:
$ git checkout pull/15705
$ git pull https://git.openjdk.org/jdk.git pull/15705/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15705

View PR using the GUI difftool:
$ git pr show -t 15705

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15705.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-13T09:46:49Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-13T09:47:45Z

@dougxc The following labels will be automatically applied to this pull request:

graal
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

dougxc · 2023-09-13T09:47:48Z

src/hotspot/share/interpreter/oopMapCache.cpp

 void InterpreterOopMap::initialize() {
  _method    = nullptr;
-  _mask_size = USHRT_MAX;  // This value should cause a failure quickly
+  _mask_size = INT_MAX;  // This value should cause a failure quickly


Unless I'm mistaken, USHRT_MAX is a legal (but unlikely) value (i.e. max_locals in a class file can be 65635) so I changed this to use INT_MAX instead.

It's actually not legal since _mask_size is always a multiple of bits_per_entry which is 2 so any odd value would work. I would be leery of touching this unless you really need to.

Ok, I'll revert it to USHRT_MAX.

mlbridge · 2023-09-13T09:57:56Z

Webrevs

tkrodriguez · 2023-09-13T16:57:52Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java

+    }
+
+    @Override
+    public long getLiveObjectLocalsAt(int bci, BitSet bigOopMap) {


This seems overly complicated to me. Why isn't it simply:

public BitSet getLiveObjectLocalsAt(int bci) { int locals = getMaxLocals(); int nwords = ((locals - 1) / 64) + 1; long liveness[] = new long[nwords]; compilerToVM().getLiveObjectLocalsAt(this, bci, liveness); return new BitSet(liveness); }

In the common case we can avoid the overhead of allocating a long array and initializing each of it elements with a JNI upcall.

It's premature optimization. This is called once per OSR compile and the call itself is doing a dataflow over the bytecodes which seems more expensive then a couple JNI calls. The array can be filled in using copy_bytes_to from HotSpot, which is a pair of JNI calls. You already have 2 JNI calls because of the unsafe allocation. I can maybe accept that returning a long from the JVM to avoid an allocation for the < 64 case isn't a terrible idea but I can't see any universe where we care about that.

This method should be returning a BitSet not a long. Having the caller do that fixup is super ugly.

Ok, I'm convinced.

tkrodriguez · 2023-09-13T16:58:50Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaMethod.java

+     * @throws UnsupportedOperationException if local variable liveness is not provided
+     *         by the current JVMCI runtime
+     */
+    default long getLiveObjectLocalsAt(int bci, BitSet bigOopMap) {


I think this should be in HotSpotResolvedJavaMethod since there's no apparent need for it in SVM.

… to HotSpotResolvedJavaMethod

dougxc · 2023-09-14T11:47:30Z

src/hotspot/share/interpreter/oopMapCache.cpp

  tmp->fill(method, bci);
-  entry->resource_copy(tmp);
+  if (tmp->has_valid_mask()) {
+    entry->resource_copy(tmp);


If tmp is invalid (e.g. oop map was requested for invalid BCI), then resource_copy crashes the VM in strange ways since it blindly trusts the mask size to be valid.

This is not the only place where resource_copy() is called, could you add an assert in resource_copy() itself to check that it is never called with an invalid bci/mask_size.
Thank you.

Ok, I've added that assertion.

tkrodriguez

This looks good now.

tkrodriguez · 2023-09-14T17:17:10Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethod.java

+    /**
+     * Computes which local variables contain live object values
+     * at the instruction denoted by {@code bci}. This is the "oop map" used
+     * by the garbage collector.


by the garbage collector for interpreter frames.

tkrodriguez · 2023-09-14T17:27:35Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

+  int nwords = ((nlocals - 1) / 64) + 1;
+  JVMCIPrimitiveArray oop_map = JVMCIENV->wrap(oop_map_handle);
+  int oop_map_len = JVMCIENV->get_length(oop_map);
+  if (nwords > oop_map_len) {


Should we sanity check against mask.number_of_entries()? One wrinkle here is that compute_one_oop_map also computes information about the stack so the mask it computes can be larger than just max_locals. For the purposes of OSR this doesn't matter as none of the JITs support OSR with a non-empty stack, so we would never call it for a bci with a non-empty stack. So should we disallow calling it with a non-empty stack or just properly handle it by passing in an array long enough to contain max_locals + max_stack?

We only look up the mask for locals and so ignore stack indexes in the mask altogether. I'm assuming therefore that mask.is_oop(i) can never hit any problems.
Note that this API should be safe when called for any valid BCI, not just those for an OSR entry point. Even if called for a BCI with a non-empty stack, the current implementation simply ignores that part of the mask.

Yes that's implied by the name of the method. It would make me happy if there was a comment pointing out that we're explicitly ignoring whether the stack is non-empty and contains oops.

Instead, I generalized getLiveObjectLocalsAt to getOopMapAt since the VM computation is for both locals and operand stack anyway.
When called for OSR entry points, the result will be the same since (currently) HotSpot requires the stack to be empty.

openjdk · 2023-09-15T13:55:55Z

@dougxc This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8315954: getArgumentValues002.java fails on Graal

Reviewed-by: never, fparain

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 137 new commits pushed to the master branch:

90bcdbd: 8316581: Improve performance of Symbol::print_value_on()
8350268: 8316453: [JVMCI] Using Xcomp on jargraal must eagerly initialize JVMCI
3809d69: 8316240: Open source several add/remove MenuBar manual tests
38bf119: 8310228: Improve error reporting for uncaught native exceptions on Windows
349723c: 8315739: Missing null check in os::vm_min_address
8cbe42b: 8316421: libjava should load shell32.dll eagerly
378bcd5: 8316595: Alpine build fails after JDK-8314021
b3d75fe: 8310874: Runthese30m crashes with klass should be in the placeholders during verification
6a4b665: 8316659: assert(LockingMode != LM_LIGHTWEIGHT || flag == CCR0) failed: bad condition register
913e43f: 8316582: Minor startup regression in 22-b15 due JDK-8310929
... and 127 more: https://git.openjdk.org/jdk/compare/f8df754b9a3f58ff5f26e63de70d02f3433a9384...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

tkrodriguez

New version looks good.

fparain

Runtime changes look good to me. Thank you for the additional assert.

dougxc · 2023-09-21T16:26:49Z

Thanks for the reviews.

/integrate

openjdk · 2023-09-21T16:28:48Z

Going to push as commit 542b300.
Since your change was applied there have been 138 commits pushed to the master branch:

0637900: 8316433: net.dll should delay load winhttp.dll
90bcdbd: 8316581: Improve performance of Symbol::print_value_on()
8350268: 8316453: [JVMCI] Using Xcomp on jargraal must eagerly initialize JVMCI
3809d69: 8316240: Open source several add/remove MenuBar manual tests
38bf119: 8310228: Improve error reporting for uncaught native exceptions on Windows
349723c: 8315739: Missing null check in os::vm_min_address
8cbe42b: 8316421: libjava should load shell32.dll eagerly
378bcd5: 8316595: Alpine build fails after JDK-8314021
b3d75fe: 8310874: Runthese30m crashes with klass should be in the placeholders during verification
6a4b665: 8316659: assert(LockingMode != LM_LIGHTWEIGHT || flag == CCR0) failed: bad condition register
... and 128 more: https://git.openjdk.org/jdk/compare/f8df754b9a3f58ff5f26e63de70d02f3433a9384...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-21T16:29:01Z

@dougxc Pushed as commit 542b300.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

added ResolvedJavaMethod.getLiveObjectLocalsAt

23b94e3

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 13, 2023

dougxc commented Sep 13, 2023

View reviewed changes

dougxc marked this pull request as ready for review September 13, 2023 09:52

openjdk bot added the rfr Pull request is ready for review label Sep 13, 2023

dougxc marked this pull request as draft September 13, 2023 15:26

openjdk bot removed the rfr Pull request is ready for review label Sep 13, 2023

tkrodriguez reviewed Sep 13, 2023

View reviewed changes

dougxc force-pushed the JDK-8315954 branch from 15a6f2b to e55662a Compare September 13, 2023 20:58

simplified getLiveObjectLocalsAt and moved it from ResolvedJavaMethod…

b35ba22

… to HotSpotResolvedJavaMethod

dougxc force-pushed the JDK-8315954 branch from e55662a to b35ba22 Compare September 13, 2023 21:01

need to zero oop_map_buf

0f01fe3

dougxc commented Sep 14, 2023

View reviewed changes

tkrodriguez approved these changes Sep 14, 2023

View reviewed changes

generalized getLiveObjectLocalsAt to getOopMapAt

c6c6c0d

dougxc marked this pull request as ready for review September 15, 2023 13:54

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2023

tkrodriguez approved these changes Sep 18, 2023

View reviewed changes

add assertion to InterpreterOopMap::resource_copy

3c903ec

fparain approved these changes Sep 21, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 21, 2023

openjdk bot closed this Sep 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2023

dougxc deleted the JDK-8315954 branch August 20, 2024 06:21

8315954: getArgumentValues002.java fails on Graal #15705

8315954: getArgumentValues002.java fails on Graal #15705

Uh oh!

Conversation

dougxc commented Sep 13, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 13, 2023

Uh oh!

openjdk bot commented Sep 13, 2023

Uh oh!

dougxc Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

dougxc commented Sep 21, 2023

Uh oh!

openjdk bot commented Sep 21, 2023

Uh oh!

openjdk bot commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dougxc commented Sep 13, 2023 •

edited by openjdk bot

Loading

dougxc Sep 13, 2023 •

edited

Loading

mlbridge bot commented Sep 13, 2023 •

edited

Loading

openjdk bot commented Sep 15, 2023 •

edited

Loading