8380059: [lworld] zero: java/foreign/enablenativeaccess/TestEnableNativeAccessJarManifest.java failed with SIGSEGV in InterpreterRuntime::write_flat_field

[johan-sjolen]

Hi,

The zero interpreter has fast paths for basic getters and setters, but these fail to take into account how flattened fields work. This causes flattened fields to be copied onto the stack directly, instead of wrapped in an oop (for getters), and flattened fields to be transformed into oops (for setters). Instead of adapting the fast paths, we bail to the slow path.

To convince you, this is getter_entry:

    switch (entry->tos_state()) {
      // SNIP!
      case atos: SET_STACK_OBJECT(object->obj_field(offset),    0); break;

and this is the basic bytecode dispatch (slow path):

switch (tos_type) {
  // SNIP!
case atos:
                oop val;
                if (entry->is_flat()) {
                  CALL_VM(InterpreterRuntime::read_flat_field(THREAD, obj, entry), handle_exception);
                  val = THREAD->vm_result_oop();
                  THREAD->set_vm_result_oop(nullptr);
                } else {
                  val = obj->obj_field(field_offset);
                }

I added a test in order to coax the getter/setter pattern optimization to run consistently and re-ran the previously disabled test along with the new one on linux-x64-zero, and they both pass. Running the new test with lworld -Xint linux-x64-zero crashes similarly to the reported bug.

Everything below this header is bonus info for the curious

We fail within init of TestSuite which has a flattened field:

 - private value 'skipFailedInvocationCounts' 'Ljava/lang/Boolean;' @12  Flat inline type field 'java/lang/Boolean':
   - private final value 'value' 'Z' @12  false (0x00)
   - [null_marker] @13 Field marked as non-null

and in SuiteRunner.java:183 we can see this:

    skipFailedInvocationCounts = suite.skipFailedInvocationCounts(); // (suite has type XmlSuite)

and XmlSuite has the following getter

  public Boolean skipFailedInvocationCounts() {
    return m_skipFailedInvocationCounts;
  }

This is probably triggering the buggy zero code.

Just some more discovery stuff:

Let's look at si_addr which is 0x0000000001000108. Alright, we're running with compressed klass pointers but without UseCompactObjectHeaders, so when doing the write_flat_field we finally reach return CompressedKlassPointers::decode_not_null(_compressed_klass); and _compressed_klass has offset 8. So, the actual "oop" is at 0x1000100. OK, I dunno, that could be a flattened Boolean I guess?

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

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

Issue

• JDK-8380059: [lworld] zero: java/foreign/enablenativeaccess/TestEnableNativeAccessJarManifest.java failed with SIGSEGV in InterpreterRuntime::write_flat_field (Bug - P4)

Reviewers

• Coleen Phillimore (@coleenp - Committer)
• Paul Hübner (@Arraying - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2382

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2382.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@johan-sjolen 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:

8380059: [lworld] zero: java/foreign/enablenativeaccess/TestEnableNativeAccessJarManifest.java failed with SIGSEGV in InterpreterRuntime::write_flat_field

Reviewed-by: coleenp, phubner

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 104 new commits pushed to the lworld branch:

• 7558cd7: 8382702: [lworld] Define VMObjectAlloc behavior for value objects
• 46aac16: 8383417: [lworld] Implement limitation to the size of instances due to flattening
• 7f562b3: 8383771: [lworld] jdk/javadoc/doccheck/checks/jdkCheckLinks.java fails after JDK-8383141
• ... and 101 more: https://git.openjdk.org/valhalla/compare/5131e8a6aeae465546b700ccee160b9abc98cf21...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp, @Arraying) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[Arraying]

• private value 'skipFailedInvocationCounts' 'Ljava/lang/Boolean;' @12 Flat inline type field 'java/lang/Boolean':

I'm not super familiar with the Zero internals or potential optimizations we have/could have baked in. However, we can't really yield that many field flattening benefits with just an interpreter. Perhaps we still enjoy a lower overall allocation rate when only buffering at points of field access? Would it make sense to revisit the flattening decisions when running Zero? As in, disable field flattening entirely and guard cases like these with assertions.

[johan-sjolen]

tier1-tier5 with enable preview looks good.

[johan-sjolen]

• private value 'skipFailedInvocationCounts' 'Ljava/lang/Boolean;' @12 Flat inline type field 'java/lang/Boolean':

I'm not super familiar with the Zero internals or potential optimizations we have/could have baked in. However, we can't really yield that many field flattening benefits with just an interpreter. Perhaps we still enjoy a lower overall allocation rate when only buffering at points of field access? Would it make sense to revisit the flattening decisions when running Zero? As in, disable field flattening entirely and guard cases like these with assertions.

We could always set UseFieldFlattening and UseArrayFlattening to false if we build with zero.

[mlbridge]

Webrevs

• 00: Full (352d15b9)

[coleenp]

This makes sense.

[Arraying]

The change looks good, thanks! Let's defer the discussion of if Zero should support flattening.

[johan-sjolen]

/integrate

Thank you for the reviews, let's see what the future holds for Zero + Valhalla.

[openjdk]

@johan-sjolen
Your change (at version 352d15b) is now ready to be sponsored by a Committer.

[Arraying]

/sponsor

[openjdk]

Going to push as commit 46dafa3.
Since your change was applied there have been 104 commits pushed to the lworld branch:

• 7558cd7: 8382702: [lworld] Define VMObjectAlloc behavior for value objects
• 46aac16: 8383417: [lworld] Implement limitation to the size of instances due to flattening
• 7f562b3: 8383771: [lworld] jdk/javadoc/doccheck/checks/jdkCheckLinks.java fails after JDK-8383141
• ... and 101 more: https://git.openjdk.org/valhalla/compare/5131e8a6aeae465546b700ccee160b9abc98cf21...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@Arraying @johan-sjolen Pushed as commit 46dafa3.

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