-
Notifications
You must be signed in to change notification settings - Fork 207
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
8278834: Error "Cannot read field "sym" because "this.lvar[od]" is null" when compiling #1141
Conversation
👋 Welcome back Glavo! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
@Glavo 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:
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 18 new commits pushed to the
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 (@phohensee) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
Webrevs
|
@Glavo, please add a note describing the reason for the backport, testing, and risk. |
javac crashed when we compiled the following code using JDK 17: import java.util.*;
public class Main {
public static void main(String[] args) {
Integer b = 1;
new Main(b) {
private final List<String> list = this.o instanceof Integer i ? new ArrayList<String>(i) : new ArrayList<String>();
}.run();
}
protected final Object o;
public Main(Object o) {
this.o = o;
}
public void run() {
}
} javac output:
This backport is to fix this problem. I think there are no known risks. What tests should I run for javac? |
Thank you for the info. The langtools tier1 tests would be the right ones to use. These are run as part of pre-submit testing, which should be configured on your repo (click on "Details" on the left of Pre-submit test status". Also, did the backport fix the crash? I assume it did, but that needs to be stated. I've tagged the JBS issue on your behalf. |
I ran and passed the pre-submit test. |
Excellent, thank you. Now we wait for maintainer approval. Once we have that, I can sponsor. |
Hi Glavo, Also, this is a 17.0.8 change. We are still working on 17.0.7. Usually, we keep these in-sync. Also, the fix is brand-new in head. Is there a good reason you need this in 17.0.7? I would prefer to wait for 17.0.8. See also the project homepage for the timetables. |
/reviewer 1 |
/reviewers 1 |
Fixes to test can always be backported as they don't have an effect on the deliverable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a javac expert, but this looks like a fix for a case where expected state (currentMethodSym) could be silently trashed, resulting in a later NPE. The code in tip is more complex, but retains the "restoration of state" idea in this code. Also, the patch includes a test (which passes) derived from the one attached to the original JBS issue.
@Glavo 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! |
Re-tagged the JBS issue. |
@Glavo, can you please merge head so that thye pre submit tests run? |
Okay. I've been busy with some other things lately, so didn't work on this PR. I'll run the tests later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-review. Backport has been approved, I can sponsor as soon as I see /integrate.
I ran the tier1 and tier2 tests. Tier1 all passed. Two tests in tier2 failed, but none of them seemed to be related to this PR. |
/integrate |
/sponsor |
Going to push as commit 47e4dba.
Your commit was automatically rebased without conflicts. |
@phohensee @Glavo Pushed as commit 47e4dba. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1141/head:pull/1141
$ git checkout pull/1141
Update a local copy of the PR:
$ git checkout pull/1141
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1141/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1141
View PR using the GUI difftool:
$ git pr show -t 1141
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1141.diff