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

8257740: Compiler crash when compiling type annotation on multicatch inside lambda #1648

Closed
wants to merge 2 commits into from

Conversation

@lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Dec 6, 2020

Hi all,

When translating a lambda expression to method, the method LambdaToMethod.LambdaAnalyzerPreprocessor.LambdaTranslationContext.translate(final Symbol sym, LambdaSymbolKind skind) creates new VarSymbol(s) according to the old VarSymbol(s). But this translate method doesn't set the field data of the new VarSymbol. Maybe it is because the field data is unused in later phase.

Unfortunately, com.sun.tools.javac.jvm.Code.fillExceptionParameterPositions and com.sun.tools.javac.jvm.Code.fillLocalVarPosition would use the field data if the data is ElementKind.EXCEPTION_PARAMETER.

It causes the exception_index won't be set in fillExceptionParameterPositions. Later, if the exception_index is used, the compiler will crash with error java.lang.AssertionError: exception_index is not set.

This patch fixes it and adds corresponding test cases.
In my patch, I only set the field data if the data is ElementKind.EXCEPTION_PARAMETER. Because I don't see another situation that the field data would be used.

Thank your for taking the time to review.

Best Regards.


Progress

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

Issue

  • JDK-8257740: Compiler crash when compiling type annotation on multicatch inside lambda

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 6, 2020

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

Loading

@openjdk openjdk bot added the rfr label Dec 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 6, 2020

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

  • 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.

Loading

@openjdk openjdk bot added the compiler label Dec 6, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 6, 2020

Webrevs

Loading

Runnable r = () -> {
try {
System.out.println();
} catch (@T8257740_2_Anno Exception e) { // multi-catch
Copy link
Member

@jbf jbf Dec 10, 2020

Choose a reason for hiding this comment

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

This looks like a uni-catch?

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Dec 10, 2020

Choose a reason for hiding this comment

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

Sorry for the mistake. I will fix it later.

Loading

/*
* @test
* @bug 8257740
* @summary Compiler crash when compiling type annotation on uni-catch inside lambda
Copy link
Member

@jbf jbf Dec 10, 2020

Choose a reason for hiding this comment

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

Did you manage to make it crash on a uni-catch as well?

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Dec 10, 2020

Choose a reason for hiding this comment

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

I test the uni-catch locally by using this test case. The compiler crashes and the error message is same as multi-catch. But I don't know why the reporter only reproduce it by using multi-catch. My local environment is linux&x86_64.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Dec 10, 2020

I fix the multi-catch test case. Thank you for taking the time to review.

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 7, 2021

@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 7, 2021

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

8257740: Compiler crash when compiling type annotation on multicatch inside lambda

Reviewed-by: vromero

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

  • d8ad630: 8258772: Some runtime/cds tests fail with +LogCompilation or +StressX
  • 4ce83f2: 8039278: console.sh failed Automatically with exit code 1
  • 2e99e28: 8213126: java/awt/Window/MainKeyWindow/TestMainKeyWindow.java time-out on mac10.13
  • 8530ef0: 8259375: JvmtiExport::jni_Get/SetField_probe calls do not need ResetNoHandleMark
  • f91f92d: 8259317: Remove JVM option BreakAtWarning
  • 3f9f86f: 8258484: AIX build fails in Harfbuzz with XLC 16.01.0000.0006
  • 1c33847: 8259067: bootclasspath append takes out object lock
  • 0e6de4e: 8259339: AllocateUninitializedArray C2 intrinsic fails with void.class input
  • 81c0624: 8259354: Fix race condition in AbstractEventStream.nextThreadName
  • 227f99d: 8233555: [TESTBUG] JRadioButton tests failing on MacoS
  • ... and 334 more: https://git.openjdk.java.net/jdk/compare/d3ac1bf16c2e6ffc1f5b1759631750cb7eac2e17...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.

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 (@vicente-romero-oracle) 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).

Loading

@openjdk openjdk bot added the ready label Jan 7, 2021
@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Jan 7, 2021

looks good to me

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jan 7, 2021

@vicente-romero-oracle Thank you for your review. Could I get your help to sponsor this patch? Thanks again.

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jan 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 7, 2021

@lgxbslgx
Your change (at version 6c65234) is now ready to be sponsored by a Committer.

Loading

// Because method com.sun.tools.javac.jvm.Code.fillExceptionParameterPositions and
// com.sun.tools.javac.jvm.Code.fillLocalVarPosition would use it.
// See JDK-8257740 for more information.
if (((VarSymbol) sym).isExceptionParameter()) {
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Jan 7, 2021

Choose a reason for hiding this comment

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

another option could be to set the data to all VarSymbols instead of adding a special case for exception parameters. I'm OK with this solution, just that it could be that setting the data value for all VarSymbols could advert similar issues in the future

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jan 7, 2021

Because VarSymbol.data is private, currently I need to do it as below.

// way 1
if (((VarSymbol) sym).isExceptionParameter()) {
    ((VarSymbol) ret).setData(ElementKind.EXCEPTION_PARAMETER);
}
if (((VarSymbol) sym).isResourceVariable()) {
    ((VarSymbol) ret).setData(ElementKind.RESOURCE_VARIABLE);
}

If we revise VarSymbol.data to public, we can do it as below.

// way 2
// class VarSymbol
public Object data;

// Usage
 ((VarSymbol) ret).data = ((VarSymbol) sym).data;

Or we can provide a method to get VarSymbol.data, the corresponding code is shown below.

// way 3
// class VarSymbol
public Object getData() {
    return this.data;
}

// Usage
((VarSymbol) ret).setData(((VarSymbol) sym).getData());

In my opinion, way 3 is better. Which way do you think is the best way?

Loading

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Jan 8, 2021

Because VarSymbol.data is private, currently I need to do it as below.

// way 1
if (((VarSymbol) sym).isExceptionParameter()) {
    ((VarSymbol) ret).setData(ElementKind.EXCEPTION_PARAMETER);
}
if (((VarSymbol) sym).isResourceVariable()) {
    ((VarSymbol) ret).setData(ElementKind.RESOURCE_VARIABLE);
}

If we revise VarSymbol.data to public, we can do it as below.

// way 2
// class VarSymbol
public Object data;

// Usage
 ((VarSymbol) ret).data = ((VarSymbol) sym).data;

Or we can provide a method to get VarSymbol.data, the corresponding code is shown below.

// way 3
// class VarSymbol
public Object getData() {
    return this.data;
}

// Usage
((VarSymbol) ret).setData(((VarSymbol) sym).getData());

In my opinion, way 3 is better. Which way do you think is the best way?

NVM, what you already did is the best way for now, we prefer to keep the data field private, sorry for the noise and thanks again for the good job

Loading

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Jan 8, 2021

/sponsor

Loading

@openjdk openjdk bot closed this Jan 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 8, 2021

@vicente-romero-oracle @lgxbslgx Since your change was applied there have been 375 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 697bf7a.

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

Loading

@lgxbslgx lgxbslgx deleted the JDK-8257740 branch Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants