Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267652: c2 loop unrolling by 8 results in reading memory past array #488

Closed
wants to merge 1 commit into from

Conversation

VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Oct 6, 2021

The backport can't be applied clean to jdk11 ( due to the miss of 8223347: Integration of Vector API (Incubator) ) and needed big rework.

The main idea behind the backport is to add new check to functions with VecS or VecD argument ( those who can read less than 16 bytes from memory) and not touch VecX/VecY/VecZ types.

With LoopMaxUnroll=8
The problematic place before the patch :

0x000000011dd55603: vmovq 0x10(%r10,%rsi,1),%xmm0 <---loading 8 bytes from memory
0x000000011dd5560a: vpxor 0x10(%r11,%rsi,1),%xmm0,%xmm0 <--- loading 16 bytes from memory, it's not right
0x000000011dd55611: vmovq %xmm0,0x10(%r13,%rsi,1) ;*bastore {reexecute=0 rethrow=0 return_oop=0}
; - repro::xor_array@18 (line 12)

and after the patch:

0x000000011f9a4d95: vmovq 0x10(%r11,%rsi,1),%xmm0
0x000000011f9a4d9c: vmovq 0x10(%r10,%rsi,1),%xmm1
0x000000011f9a4da3: vpxor %xmm1,%xmm0,%xmm0
0x000000011f9a4da7: vmovq %xmm0,0x10(%r13,%rsi,1) ;*bastore {reexecute=0 rethrow=0 return_oop=0}
; - repro::xor_array@18 (line 12)

With LoopMaxUnroll=16 it's works the old way ( as expected), loading 16 bytes from memory with vmovdqu and vpxor

0x000000011e2b8903: vmovdqu 0x10(%r10,%rsi,1),%xmm0
0x000000011e2b890a: vpxor 0x10(%r11,%rsi,1),%xmm0,%xmm0
0x000000011e2b8911: vmovdqu %xmm0,0x10(%r13,%rsi,1) ;*bastore {reexecute=0 rethrow=0 return_oop=0}

Testing is pending.

Also fixed a typo at https://github.com/openjdk/jdk11u-dev/pull/488/files#diff-d6a3624f0f0af65a98a47378a5c146eed5016ca09b4de1acd0a3acc823242e82L9069


Progress

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

Issue

  • JDK-8267652: c2 loop unrolling by 8 results in reading memory past array

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/488/head:pull/488
$ git checkout pull/488

Update a local copy of the PR:
$ git checkout pull/488
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/488/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 488

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/488.diff

@VladimirKempik VladimirKempik changed the title 8267652: c2 loop unrolling by 8 results in reading memory past array Backport 0c99b192588b04aceaa27cda9cb42b45f95adffe Oct 6, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 6, 2021

👋 Welcome back vkempik! 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 openjdk bot changed the title Backport 0c99b192588b04aceaa27cda9cb42b45f95adffe 8223347: Integration of Vector API (Incubator) Oct 6, 2021
@openjdk
Copy link

openjdk bot commented Oct 6, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Oct 6, 2021
@VladimirKempik
Copy link
Author

VladimirKempik commented Oct 6, 2021

@neliasso could you please take a look here, as you are working on a similar backport.

@mlbridge
Copy link

mlbridge bot commented Oct 6, 2021

Webrevs

@VladimirKempik VladimirKempik changed the title 8223347: Integration of Vector API (Incubator) Backport dc12cb78b81f56e9d4b282cf7cad5faa9a9886bf Oct 6, 2021
@openjdk openjdk bot changed the title Backport dc12cb78b81f56e9d4b282cf7cad5faa9a9886bf 8267652: c2 loop unrolling by 8 results in reading memory past array Oct 6, 2021
@openjdk
Copy link

openjdk bot commented Oct 6, 2021

This backport pull request has now been updated with issue from the original commit.

@VladimirKempik
Copy link
Author

Full tier regression testing performed. Results are good.

@TheRealMDoerr
Copy link
Contributor

Thanks for backporting it! Looks like you have addressed all required nodes. So, I think it's good. We'll test the fix.

@openjdk
Copy link

openjdk bot commented Oct 14, 2021

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

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

8267652: c2 loop unrolling by 8 results in reading memory past array

Reviewed-by: mdoerr

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

  • 968c21e: 8272574: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 90d1bcb: 8261071: AArch64: Refactor interpreter native wrappers
  • 15931e5: 8230228: [TESTBUG] Several runtime/ErrorHandling tests may fail on some platforms
  • 15384ad: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails
  • 4557503: 8269656: The test test/langtools/tools/javac/versions/Versions.java has duplicate test cycles
  • 844c504: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
  • 4940e2e: 8137101: [TEST_BUG] javax/swing/plaf/basic/BasicHTML/4251579/bug4251579.java failure due to timing
  • 0a71d8e: 8221305: java/awt/FontMetrics/MaxAdvanceIsMax.java fails on MacOS + Solaris
  • d336b24: 8253015: Aarch64: Move linux code out from generic CPU feature detection
  • 2a028c4: 8180568: Refactor javax/crypto shell tests to plain java tests
  • ... and 25 more: https://git.openjdk.java.net/jdk11u-dev/compare/b1bab7ffe1565ab51ba8512d288b20f4163efc0c...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 14, 2021
@TheRealMDoerr
Copy link
Contributor

Test results are good. Hopefully, nobody will backport other nodes which require the addition.

@VladimirKempik
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 18, 2021

Going to push as commit 2c4ab76.
Since your change was applied there have been 44 commits pushed to the master branch:

  • fcb396f: 8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI)
  • 99ff268: 8239334: Tab Size does not work correctly in JTextArea with setLineWrap on
  • e2f8301: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem
  • 94893a5: 8274840: Update OS detection code to recognize Windows 11
  • 596677e: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits
  • b3926db: 8233638: [TESTBUG] Swing test ScreenMenuBarInputTwice.java fails on macos
  • 4bdcad5: 8233643: [TESTBUG] JMenu test bug4515762.java fails on macos
  • dfb97b0: 8233642: [TESTBUG] JMenuBar test bug 4750590.java fails on macos
  • 167c9f0: 8169953: JComboBox/8057893: ComboBoxEdited event is not fired! on Windows
  • 968c21e: 8272574: C2: assert(false) failed: Bad graph detected in build_loop_late
  • ... and 34 more: https://git.openjdk.java.net/jdk11u-dev/compare/b1bab7ffe1565ab51ba8512d288b20f4163efc0c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 18, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 18, 2021
@openjdk
Copy link

openjdk bot commented Oct 18, 2021

@VladimirKempik Pushed as commit 2c4ab76.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
2 participants