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

8265984: Concurrent GC: Some tests fail "assert(is_frame_safe(f)) failed: Frame must be safe" #3814

Closed
wants to merge 1 commit into from

Conversation

@pliden
Copy link
Contributor

@pliden pliden commented Apr 30, 2021

JDK-8265702 introduced a change in OptoRuntime::handle_exception_C() where it no longer processed frames when asking for the caller. This change turned out to cause intermittent test failures. We shouldn't have to process frames here, since we're not touching any oops, but it seems that processing frames here avoids some other issue. To avoid noise in out testing I suggest we revert to processing frames here for now (and add the required ThreadWXEnable for macos/aarch64), while we continue to investigate why this is needed.

Testing: Passed tier1-7 on linux/x86 and macos/aarch64


Progress

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

Issue

  • JDK-8265984: Concurrent GC: Some tests fail "assert(is_frame_safe(f)) failed: Frame must be safe"

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3814/head:pull/3814
$ git checkout pull/3814

Update a local copy of the PR:
$ git checkout pull/3814
$ git pull https://git.openjdk.java.net/jdk pull/3814/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3814

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3814.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 30, 2021

👋 Welcome back pliden! 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.

@pliden
Copy link
Contributor Author

@pliden pliden commented Apr 30, 2021

/cc hotspot-gc

@openjdk openjdk bot added the hotspot-gc label Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@pliden
The hotspot-gc label was successfully added.

@pliden pliden force-pushed the 8265984_assert_is_frame_safe branch from 32d916d to 75d67a7 Apr 30, 2021
@pliden pliden marked this pull request as ready for review Apr 30, 2021
@openjdk openjdk bot added the rfr label Apr 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Webrevs

@@ -1377,7 +1377,7 @@ address OptoRuntime::handle_exception_C(JavaThread* current) {
// deoptimized frame

if (nm != NULL) {
RegisterMap map(current, false /* update_map */, false /* process_frames */);
RegisterMap map(current, false);
Copy link
Contributor

@zhengyu123 zhengyu123 Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you add MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, current)); here, as you suggested in CR?
Seems the code change is inconsistent with the comments above.

Copy link
Contributor Author

@pliden pliden May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted back to exactly what we used to do here prior to JDK-8265702, to avoid affecting anything but ZGC. I'm placing the ThreadWXEnable in the ZGC nmethod barrier since we want these as close to the patching code as possible, and we can end up in there through multiple paths.

Copy link
Contributor

@zhengyu123 zhengyu123 May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks.

fisk
fisk approved these changes May 3, 2021
Copy link
Contributor

@fisk fisk left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented May 3, 2021

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

8265984: Concurrent GC: Some tests fail "assert(is_frame_safe(f)) failed: Frame must be safe"

Reviewed-by: eosterlund, stefank, zgu

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

  • 80941f4: 8234446: Post-CMS workgroup hierarchy cleanup
  • ac760c7: 8266295: Remove unused _concurrent_iteration_safe_limit
  • b42d496: 8266388: C2: Improve constant ShiftCntV on x86
  • 05cfac9: 8266412: Remove redundant TemplateInterpreter entries
  • c5dc657: 8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]"
  • 8fa50eb: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction
  • 7e30130: 8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags
  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/07ecd421d1538ae53ffbea65eccfa938e19a9f0f...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 label May 3, 2021
stefank
stefank approved these changes May 3, 2021
@pliden
Copy link
Contributor Author

@pliden pliden commented May 3, 2021

Thanks all for reviewing!

/integrate

@openjdk openjdk bot closed this May 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 3, 2021
@pliden pliden deleted the 8265984_assert_is_frame_safe branch May 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 3, 2021

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

  • 1d9ea3a: 8266083: Shenandoah: Consolidate dedup/no dedup oop closures
  • 80941f4: 8234446: Post-CMS workgroup hierarchy cleanup
  • ac760c7: 8266295: Remove unused _concurrent_iteration_safe_limit
  • b42d496: 8266388: C2: Improve constant ShiftCntV on x86
  • 05cfac9: 8266412: Remove redundant TemplateInterpreter entries
  • c5dc657: 8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]"
  • 8fa50eb: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction
  • 7e30130: 8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags
  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • ... and 11 more: https://git.openjdk.java.net/jdk/compare/07ecd421d1538ae53ffbea65eccfa938e19a9f0f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 194bcec.

💡 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
4 participants