8260502: [s390] NativeMovRegMem::verify() fails because it's too strict

[TheRealMDoerr]

"fatal error: this is not a `NativeMovRegMem' site" was observed while testing the debug build without CompressedOops.
NativeMovRegMem::verify() tries to verify instructions beyond the load_const at a place at which we don't have valid instructions. See bug for details.
I suggest to remove the wrong part of the verification.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8260502: [s390] NativeMovRegMem::verify() fails because it's too strict

Reviewers

• Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 54fa039
• Richard Reingruber (@reinrich - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2258/head:pull/2258
$ git checkout pull/2258

[bridgekeeper]

👋 Welcome back mdoerr! 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]

@TheRealMDoerr The following label will be automatically applied to this pull request:

• hotspot-compiler

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

[mlbridge]

Webrevs

• 01: Full - Incremental (f178b20)
• 00: Full (54fa039)

[RealLucy]

Looks good to me.
Thanks for finding and fixing.

[openjdk]

@TheRealMDoerr 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:

8260502: [s390] NativeMovRegMem::verify() fails because it's too strict

Reviewed-by: lucy, rrich

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 19 new commits pushed to the master branch:

• a97aedf: 8256215: Shenandoah: re-organize saving/restoring machine state in assembler code
• 316d52c: 8260497: Shenandoah: Improve SATB flushing
• 11a70d1: 8260426: awt debug_mem.c DMem_AllocateBlock might leak memory
• 396a496: 8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp
• 62ffe75: Merge
• e28e111: 8260370: C2: LoopLimit node is not eliminated
• 7030d2e: 8260200: G1: Remove unnecessary update in FreeRegionList::remove_starting_at
• c7661ae: 8260561: [doc] HexFormat has incorrect @SInCE tag
• c52212b: Merge
• 408772c: 8259025: Record compact constructor using Objects.requireNonNull
• ... and 9 more: https://git.openjdk.java.net/jdk/compare/e696baabfbe71948cec623dcdd50ae84ca99d4c5...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.

[reinrich]

The fix looks good to me.

The comment before the declaration of NativeMovRegMem seems to be wrong. I'd suggest to change it like this:

s/optional, load only/optional, store only/

// The instruction sequence looks like this:
//   iihf        %r1,$bits1              ; load offset for mem access
//   iilf        %r1,$bits2
//   [compress oop]                      ; optional, store only
//   load/store  %r2,0(%r1,%r2)          ; memory access

I noticed that on x86 encoding/decoding the T_OBJECT field value in reg2mem/mem2reg is done before/after the code that is to be patched. Changing the s390/PPC versions to do the same may have solved JDK-8233081 as well. This wouldn't have increased the complexity of the already extremely complex C1 patching mechanics. But it is like it is.

[TheRealMDoerr]

Thanks for the reviews! I've fixed the comment as suggested. Thanks.

[reinrich]

LGTM, Thanks.

[TheRealMDoerr]

/integrate

[openjdk]

@TheRealMDoerr Since your change was applied there have been 21 commits pushed to the master branch:

• 8fe1323: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed
• ecde52e: 8260506: VersionHelper cleanup
• a97aedf: 8256215: Shenandoah: re-organize saving/restoring machine state in assembler code
• 316d52c: 8260497: Shenandoah: Improve SATB flushing
• 11a70d1: 8260426: awt debug_mem.c DMem_AllocateBlock might leak memory
• 396a496: 8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp
• 62ffe75: Merge
• e28e111: 8260370: C2: LoopLimit node is not eliminated
• 7030d2e: 8260200: G1: Remove unnecessary update in FreeRegionList::remove_starting_at
• c7661ae: 8260561: [doc] HexFormat has incorrect @SInCE tag
• ... and 11 more: https://git.openjdk.java.net/jdk/compare/e696baabfbe71948cec623dcdd50ae84ca99d4c5...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8752257.

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