[jdk17] RFR: 8267610: NPE at at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop

[Guoxiong Li]

On Tue, 15 Jun 2021 15:05:40 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> 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](https://bugs.openjdk.java.net/browse/JDK-8267610) and [JDK-8268748](https://bugs.openjdk.java.net/browse/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.

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.

-------------

PR: https://git.openjdk.java.net/jdk17/pull/59