8259275: JRuby crashes while resolving invokedynamic instruction #104

calvinccheung · 2021-01-11T17:55:24Z

Please review this proposed change which fixes 2 problems during CDS dump time:

in ClassListParser::resolve_indy(), exception could be thrown from bootstrap_specifier.resolve_bsm() but was not handled;
in ConstantPool::remove_unshareable_info(), tag was not setup correctly on error conditions.

Passed tiers 1 - 4 testing.

Progress

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

Issue

JDK-8259275: JRuby crashes while resolving invokedynamic instruction

Reviewers

Ioi Lam (@iklam - Reviewer)
Yumin Qi (@yminqi - Reviewer) ⚠️ Review applies to 6786de9
Lois Foltan (@lfoltan - Reviewer) ⚠️ Review applies to e139e96

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/104/head:pull/104
$ git checkout pull/104

calvinccheung · 2021-01-11T17:56:05Z

/label remove hotspot
/label add hotspot-runtime

bridgekeeper · 2021-01-11T17:56:23Z

👋 Welcome back ccheung! 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 · 2021-01-11T17:57:01Z

@calvinccheung The hotspot label was not set.

openjdk · 2021-01-11T17:58:13Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2021-01-11T18:01:18Z

Webrevs

lfoltan · 2021-01-11T20:16:44Z

src/hotspot/share/classfile/classListParser.cpp

+    }
+    oop exception = THREAD->pending_exception();
+    if (exception->is_a(SystemDictionary::Error_klass())) {
+      CLEAR_PENDING_EXCEPTION;


Just for my clarification, why do you clear the pending exception in the case of Error_klass()? Please consider adding a comment explaining this.

Hi Lois,

Thanks for your review. I've added some comment in ClassListParser::resolve_indy for the Error_klass case.

I would have expected only LinkageErrors to be ignored as they are the only "expected" errors aren't they?

LinkageError should work since BootstrapMethodError, NoSuchMethodError, etc. are subclasses.
I've changed the check to:
if (exception->is_a(SystemDictionary::LinkageError_klass())) {

After offline discussion with Ioi, we've decided to use log_warning(cds) to output the exception message since log_warning does not require setting a log level. Also, the exit(1) is not needed, we can continue with the CDS dumping after printing the message and clearing the exception.

iklam

Looks good to me.

openjdk · 2021-01-12T00:16:52Z

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

8259275: JRuby crashes while resolving invokedynamic instruction

Reviewed-by: iklam, minqi, lfoltan

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

17b4db3: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
5f9cd72: 8259645: Revert JDK-8245956 JavaCompiler still uses File API instead of Path API in a specific case
b03880e: 8259634: MemorySegment::asByteBuffer does not respect spatial bounds
8a81cf1: 8259298: broken link in Stream::toList spec
67e1b63: 8259380: Correct pretouch chunk size to cap with actual page size
28ff2de: 8259237: Demo selection changes with left/right arrow key. No need to press space for selection.
a7e5da2: 8258384: AArch64: SVE verify_ptrue fails on some tests
2cb271e: 8253996: Javac error on jdk16 build 18: invalid flag: -Xdoclint:-missing
d60a937: 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl
e05f36f: 8259043: More Zero architectures need linkage with libatomic

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

yminqi

Looks good!

calvinccheung · 2021-01-12T00:52:01Z

@iklam, @yminqi Thanks for your review.

dholmes-ora · 2021-01-12T02:25:28Z

src/hotspot/share/classfile/classListParser.cpp

+  ClassListParser::resolve_indy_impl(class_name_symbol, THREAD);
+  if (HAS_PENDING_EXCEPTION) {
+    ResourceMark rm(THREAD);
+    tty->print("resolve_indy for class %s has", class_name_symbol->as_C_string());


Shouldn't this be using appropriate unified logging?

Ok. I've changed it to LogTarget(Debug, cds, lambda).

dholmes-ora

I missed the fact some of this code was just being moved around when I took the first look, so my comments could be seen as something suitable for a follow up RFE.

This is not a full review.
Thanks,
David

dholmes-ora · 2021-01-12T08:17:53Z

src/hotspot/share/classfile/classListParser.cpp

+      // or NoSuchMethodError so that CDS dumping can continue.
+      CLEAR_PENDING_EXCEPTION;
+    } else {
+      exit(1);


We should never just use exit() - at a minimum vm_exit(). But we should not silently exit with no indication of what went wrong. I'm not sure of the call chain here but can't we just return and let higher layers do the same until we eventually hit a cleaner exit path that reports the exception?

Edit: I see now that this code was moved from another location not introduced with this fix, but this still seems wrong to me.

For now, I think the logging in ClassListParser::resolve_indy should just by tty->print instead of using UL. This way, if the exit(1) is hit, there will be some information about the reason. Also, not too many users would enable -Xlog:cds+lambda=debug.

lfoltan

Looks good, thank you for adding the comment!
Lois

1. use log_warning(cds) to output the exception message; 2. no need to exit on exception.

calvinccheung · 2021-01-13T05:49:41Z

/reviewer credit dholmes

openjdk · 2021-01-13T05:49:57Z

@calvinccheung Reviewer dholmes has already made an authenticated review of this PR, and does not need to be credited manually.

calvinccheung · 2021-01-13T05:51:14Z

/integrate

openjdk · 2021-01-13T05:52:06Z

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

1cf2378: 8259353: VectorReinterpretNode is incorrectly optimized out
17b4db3: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
5f9cd72: 8259645: Revert JDK-8245956 JavaCompiler still uses File API instead of Path API in a specific case
b03880e: 8259634: MemorySegment::asByteBuffer does not respect spatial bounds
8a81cf1: 8259298: broken link in Stream::toList spec
67e1b63: 8259380: Correct pretouch chunk size to cap with actual page size
28ff2de: 8259237: Demo selection changes with left/right arrow key. No need to press space for selection.
a7e5da2: 8258384: AArch64: SVE verify_ptrue fails on some tests
2cb271e: 8253996: Javac error on jdk16 build 18: invalid flag: -Xdoclint:-missing
d60a937: 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl
... and 1 more: https://git.openjdk.java.net/jdk16/compare/020ec8485251698d1187204ac13321f4726e45ea...master

Your commit was automatically rebased without conflicts.

Pushed as commit 15dd8f3.

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

8259275: JRuby crashes while resolving invokedynamic instruction

03d4b22

calvinccheung marked this pull request as ready for review January 11, 2021 17:56

openjdk bot added rfr Pull request is ready for review hotspot-runtime hotspot-runtime-dev@openjdk.java.net labels Jan 11, 2021

remove commented code

44977ca

lfoltan reviewed Jan 11, 2021

View reviewed changes

update comment in ClassListParser::resolve_indy()

6786de9

iklam approved these changes Jan 12, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 12, 2021

yminqi approved these changes Jan 12, 2021

View reviewed changes

dholmes-ora reviewed Jan 12, 2021

View reviewed changes

update per comments from David H.

e139e96

dholmes-ora reviewed Jan 12, 2021

View reviewed changes

lfoltan approved these changes Jan 12, 2021

View reviewed changes

more changes to ClassListParser::resolve_indy

506341e

1. use log_warning(cds) to output the exception message; 2. no need to exit on exception.

iklam approved these changes Jan 12, 2021

View reviewed changes

openjdk bot closed this Jan 13, 2021

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 Jan 13, 2021

calvinccheung deleted the 8259275-jruby-crash branch January 13, 2021 05:56

8259275: JRuby crashes while resolving invokedynamic instruction #104

8259275: JRuby crashes while resolving invokedynamic instruction #104

Uh oh!

Conversation

calvinccheung commented Jan 11, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

calvinccheung commented Jan 11, 2021

Uh oh!

bridgekeeper bot commented Jan 11, 2021

Uh oh!

openjdk bot commented Jan 11, 2021

Uh oh!

openjdk bot commented Jan 11, 2021

Uh oh!

mlbridge bot commented Jan 11, 2021 • 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!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yminqi left a comment

Choose a reason for hiding this comment

Uh oh!

calvinccheung commented Jan 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora 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!

lfoltan left a comment

Choose a reason for hiding this comment

Uh oh!

calvinccheung commented Jan 13, 2021

Uh oh!

openjdk bot commented Jan 13, 2021

Uh oh!

calvinccheung commented Jan 13, 2021

Uh oh!

openjdk bot commented Jan 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

calvinccheung commented Jan 11, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2021 •

edited

Loading

openjdk bot commented Jan 12, 2021 •

edited

Loading