Skip to content
This repository has been archived by the owner. It is now read-only.

8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop #59

Closed
wants to merge 3 commits into from
Closed

8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop #59

wants to merge 3 commits into from

Conversation

@lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Jun 15, 2021

Hi all,

Currently, the class TransPatterns sometimes doesn't transform the pattern variables and pattern symbols to the normal variables and symbols, especially the places where the pattern variables are used.
The following phases, such as LambdaToMethod, Lower and Gen, may crash or generate some wrong results.

The known issues are JDK-8267610 and JDK-8268748.

JDK-8267610 is an issue that pattern symbol causes the compiler to crash.

During transforming the InstanceOfTree (JCInstanceOf) , the BindingSymbol instead of the VarSymbol is used to make the new JCIdent and JCBinary trees. At the phase LambdaToMethod, the compiler can't capture this variable so that the lambda method has uncorrect parameters. So at the phase Gen, the compiler crashes because of NPE.

JDK-8268748 is an issue that pattern symbol causes that the compiler generates wrong bytecodes.

When transforming the BindingPatternTree (JCBindingPattern), the BindingSymbol is also handled uncorrectly and used to make the new JCIdent and JCAssign trees. At the phase Gen, the compiler find the wrong variables, so that the wrong bytecodes are generated.

These two issues are similar and influence each other. So I solve them at one patch.
The lines 208-212 are to solve JDK-8267610 with the test LambdaCannotCapturePatternVariables.
The lines 233-239 are to solve JDK-8268748 with the test NestedPatternVariablesBytecode.

If lines 208-212 are not included, the test NestedPatternVariablesBytecode can't pass.
If lines 233-239 are not included, the test LambdaCannotCapturePatternVariables can't pass.
So I put them together.

Thanks for taking the time to review.

Best Regards,
-- Guoxiong.


Progress

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

Issues

  • JDK-8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop
  • JDK-8268748: Javac generates uncorrect bytecodes when using nested pattern variables

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 59

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 15, 2021

👋 Welcome back gli! 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 Jun 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

@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 Jun 15, 2021
@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 15, 2021

/issue add 8268748

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

@lgxbslgx
Adding additional issue to issue list: 8268748: Javac generates error opcodes when using nest pattern variables.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 15, 2021

Webrevs

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 15, 2021

/issue remove 8268748
/issue add 8268748

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

@lgxbslgx
Removing additional issue from issue list: 8268748.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

@lgxbslgx
Adding additional issue to issue list: 8268748: Javac generates uncorrect bytecodes when using nested pattern variables.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 22, 2021

Ping for review.

Loading

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jun 23, 2021

Thanks for looking into this and sorry for trouble. Looking at the implementation, would it make sense to set the currentValue only after the expression is translated? E.g. something like:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
index 4c077968ff5..c57294bf4ab 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
@@ -189,12 +189,14 @@ public class TransPatterns extends TreeTranslator {
             //=>
             //(let T' N$temp = E; N$temp instanceof typeof($pattern) && <desugared $pattern>)
             //note the pattern desugaring performs binding variable assignments
-            Symbol exprSym = TreeInfo.symbol(tree.expr);
             Type tempType = tree.expr.type.hasTag(BOT) ?
                     syms.objectType
                     : tree.expr.type;
             VarSymbol prevCurrentValue = currentValue;
             try {
+                JCExpression translatedExpr = translate(tree.expr);
+                Symbol exprSym = TreeInfo.symbol(translatedExpr);
+
                 if (exprSym != null &&
                     exprSym.kind == Kind.VAR &&
                     exprSym.owner.kind.matches(Kinds.KindSelector.VAL_MTH)) {
@@ -206,7 +208,6 @@ public class TransPatterns extends TreeTranslator {
                             currentMethodSym);
                 }
 
-                JCExpression translatedExpr = translate(tree.expr);
                 Type principalType = principalType((JCPattern) tree.pattern);
                 result = makeBinary(Tag.AND,
                                     makeTypeTest(make.Ident(currentValue), make.Type(principalType)),

I think the currentValue is needed only when the pattern is being processed, not when the expression is being processed, unless I am mistaken.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 23, 2021

@lahodaj thanks for your comment.

I have tried your way before submitting this PR. But the compiler didn't work as expected.
It is because the following translations need the right currentValue to complete the translation.
If we don't set the currentValue before these translations, the result may be wrong.

                JCExpression translatedExpr = translate(tree.expr);
                Type principalType = principalType((JCPattern) tree.pattern);

So I revised the code like this patch which seems to use unnecessary variable actualVar.
Actually, this redundant step is necessary to avoid the regression.

Loading

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jun 23, 2021

Do you have details on what goes wrong when the currentValue is set after translating the expression? Looking into the code, I am not sure what that should break, and I ran existing tests and none seem to fail. So I wonder where it fails. Thanks!

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 23, 2021

In short:

I re-read the code and my current patch seems wrong. I would like to adopt your advice and your patch. Thanks for the clarification.


In detailed:

I can't find the original change locally now. I try my best to recall the process. The following steps lead me to the wrong direction.

First, I might use the following patch. As you can see, I didn't get the new Symbol after translate(tree.expr) by using the statement Symbol exprSym = TreeInfo.symbol(translatedExpr); so that some tests didn't pass.

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
index 16c2fdbf2a5..9be88ac6de1 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
@@ -192,6 +192,8 @@ public class TransPatterns extends TreeTranslator {
                     : tree.expr.type;
             VarSymbol prevCurrentValue = currentValue;
             try {
+                JCExpression translatedExpr = translate(tree.expr);
+
                 if (exprSym != null &&
                     exprSym.kind == Kind.VAR &&
                     exprSym.owner.kind.matches(Kinds.KindSelector.VAL_MTH)) {
@@ -203,7 +205,6 @@ public class TransPatterns extends TreeTranslator {
                             currentMethodSym);
                 }
 
-                JCExpression translatedExpr = translate(tree.expr);
                 Type principalType = principalType((JCPattern) tree.pattern);
                 result = makeBinary(Tag.AND,
                                     makeTypeTest(make.Ident(currentValue), make.Type(principalType)),

Then, I look into the failed tests and found that the method visitBindingPattern need to be adjusted, too. And I tried the following patch. It caused some tests failed, too.

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
index 16c2fdbf2a5..8c8a1935e38 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
@@ -186,12 +186,14 @@ public class TransPatterns extends TreeTranslator {
             //=>
             //(let T' N$temp = E; N$temp instanceof typeof($pattern) && <desugared $pattern>)
             //note the pattern desugaring performs binding variable assignments
-            Symbol exprSym = TreeInfo.symbol(tree.expr);
             Type tempType = tree.expr.type.hasTag(BOT) ?
                     syms.objectType
                     : tree.expr.type;
             VarSymbol prevCurrentValue = currentValue;
             try {
+                JCExpression translatedExpr = translate(tree.expr);
+                Symbol exprSym = TreeInfo.symbol(translatedExpr);
+
                 if (exprSym != null &&
                     exprSym.kind == Kind.VAR &&
                     exprSym.owner.kind.matches(Kinds.KindSelector.VAL_MTH)) {
@@ -203,7 +205,6 @@ public class TransPatterns extends TreeTranslator {
                             currentMethodSym);
                 }
 
-                JCExpression translatedExpr = translate(tree.expr);
                 Type principalType = principalType((JCPattern) tree.pattern);
                 result = makeBinary(Tag.AND,
                                     makeTypeTest(make.Ident(currentValue), make.Type(principalType)),
@@ -227,10 +228,13 @@ public class TransPatterns extends TreeTranslator {
         BindingSymbol binding = (BindingSymbol) tree.var.sym;
         Type castTargetType = principalType(tree);
         VarSymbol bindingVar = bindingContext.bindingDeclared(binding);
+        VarSymbol actualVar = (currentValue.flags() & Flags.MATCH_BINDING) != 0
+                ? bindingContext.getBindingFor((BindingSymbol) currentValue)
+                : currentValue;
 
         if (bindingVar != null) {
             JCAssign fakeInit = (JCAssign)make.at(TreeInfo.getStartPos(tree)).Assign(
-                    make.Ident(bindingVar), convert(make.Ident(currentValue), castTargetType)).setType(bindingVar.erasure(types));
+                    make.Ident(bindingVar), convert(make.Ident(actualVar), castTargetType)).setType(bindingVar.erasure(types));
             LetExpr nestedLE = make.LetExpr(List.of(make.Exec(fakeInit)),
                                             make.Literal(true));
             nestedLE.needsCond = true;

At this time, I mistakenly confirmed that both visitBindingPattern and visitTypeTest need to be revised because they are all about the BindingPattern. So finally, I try the code like this patch, which uses the actualVal after translation instead of currentValue to make the new tree. Actually, the visitBindingPattern don't need to be revised if visitTypeTest already set the currentValue correctly.

My patch seems good to solve the bug, but actually the currentValue in the method visitTypeTest should be set after translate(tree.expr) by using the statement Symbol exprSym = TreeInfo.symbol(translatedExpr); so that the following code, such as visitBindingPattern, could use this actual right currentValue.

After these analysis, I think my current patch may cause some new regresion because the currentValue is not set correctly. And all the tests of tools/javac passed locally by using your patch just now. So I would like to adopt your advice and your patch. Thanks for your clarification. I learn more in this bug.

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 23, 2021

@lahodaj I updated the code by adopting your advice. Considering that I can't accurately recall the potential regression situation, I suggest it should be more than one reviewer to approve this patch before integrating.

Loading

Copy link
Contributor

@lahodaj lahodaj left a comment

To me, looks good. @vicente-romero-oracle, could you please take a look? Thanks!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 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:

8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop
8268748: Javac generates uncorrect bytecodes when using nested pattern variables

Reviewed-by: jlahoda, 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 6 new commits pushed to the master branch:

  • 424cc50: 8269307: ProblemList java/awt/KeyboardFocusmanager/TypeAhead/ButtonActionKeyTest/ButtonActionKeyTest.java on win-x64
  • 63bcd33: 8269246: Scoped ByteBuffer vector access
  • 3fb28d3: 8269218: GaloisCounterMode.overlapDetection misses the JDK-8263436 fix again
  • d3d3b22: 8269265: ProblemList serviceability/sa/TestJmapCoreMetaspace.java with ZGC
  • 0c3fc27: 8268482: compiler/intrinsics/VectorizedMismatchTest.java failed with failed: length in range
  • a30141d: 8269179: Crash in TestMacroLogicVector::testSubWordBoolean: assert(_base >= VectorMask && _base <= VectorZ) failed: Not a Vector

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.

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 (@lahodaj, @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 Jun 24, 2021

List<String> actualCode = getCodeInstructions(code_attribute);
List<String> expectedCode = Arrays.asList(
"aload_1", "instanceof", "ifeq", "aload_1", "checkcast", "astore_2", "aload_2", "instanceof",
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Jun 24, 2021

Choose a reason for hiding this comment

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

not sure I like this code pattern check, if we change the generated code for whatever reason we will need to revisit this test. Is there other way of testing the same? like executing a code and checking its result(s) or throwing an exception if something unexpected happened?

Loading

Copy link
Member Author

@lgxbslgx lgxbslgx Jun 24, 2021

Choose a reason for hiding this comment

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

@vicente-romero-oracle Thanks for your review.

In this test, if the bug is not fixed which means the aload_2 would be aload_1, the code runs well too.
The aload_1 or aload_2 don't effect the result so that we can't check it by executing it.
There are some similar situations in the past when we want to verify the class files.
These tests would also fail if we change the logic of the code generation.

Another way is that only verify if the aload_2 is at the right place instead of verify all the instruments.
But it also need to be revisited and revised if the logic of the code generation is changed in the future.
It seems a unavoidable issue about verifing the class files.

Loading

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Jun 24, 2021

Choose a reason for hiding this comment

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

@vicente-romero-oracle Thanks for your review.

In this test, if the bug is not fixed which means the aload_2 would be aload_1, the code runs well too.
The aload_1 or aload_2 don't effect the result so that we can't check it by executing it.
There are some similar situations in the past when we want to verify the class files.
These tests would also fail if we change the logic of the code generation.

Another way is that only verify if the aload_2 is at the right place instead of verify all the instruments.
But it also need to be revisited and revised if the logic of the code generation is changed in the future.
It seems a unavoidable issue about verifing the class files.

I see, thanks for the clarification

Loading

@lgxbslgx
Copy link
Member Author

@lgxbslgx lgxbslgx commented Jun 24, 2021

Thanks @lahodaj @vicente-romero-oracle for the review.

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

Going to push as commit 7ab1285.
Since your change was applied there have been 9 commits pushed to the master branch:

  • cfa6a99: 8269316: ProblemList vmTestbase/vm/mlvm/mixed/stress/regression/b6969574/INDIFY_Test.java on Linux-X64 -Xcomp
  • 22d8675: 8269315: ProblemList javax/swing/JFileChooser/FileSystemView/SystemIconTest.java on Win-X64
  • 443a79a: 8269314: ProblemList serviceability/dcmd/gc/RunFinalizationTest.java on Win-X64 and linux-aarch64
  • 424cc50: 8269307: ProblemList java/awt/KeyboardFocusmanager/TypeAhead/ButtonActionKeyTest/ButtonActionKeyTest.java on win-x64
  • 63bcd33: 8269246: Scoped ByteBuffer vector access
  • 3fb28d3: 8269218: GaloisCounterMode.overlapDetection misses the JDK-8263436 fix again
  • d3d3b22: 8269265: ProblemList serviceability/sa/TestJmapCoreMetaspace.java with ZGC
  • 0c3fc27: 8268482: compiler/intrinsics/VectorizedMismatchTest.java failed with failed: length in range
  • a30141d: 8269179: Crash in TestMacroLogicVector::testSubWordBoolean: assert(_base >= VectorMask && _base <= VectorZ) failed: Not a Vector

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jun 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

@lgxbslgx Pushed as commit 7ab1285.

💡 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-8267610 branch Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants