8358801: javac produces class that does not pass verifier.

[lahodaj]

Consider code like:

public class Main {

    private boolean test(String s, int i) {
        if (s.subSequence(0, 1) instanceof Runnable r) {
            return true;
        }

        Integer dummy;
        switch (i) {
            case 0:
                String clashing = null;
                return true;
            default:
                return true;
        }
    }

    public static void main(String[] args) {
    }
}

javac will produce code that won't (rightfully) pass the verifier:

$ java Main.java
Exception in thread "main" java.lang.VerifyError: Inconsistent stackmap frames at branch target 49
Exception Details:
  Location:
    Main.test(Ljava/lang/String;I)Z @49: iconst_1
  Reason:
    Type top (current frame, locals[4]) is not assignable to 'java/lang/String' (stack map, locals[4])
  Current Frame:
    bci: @25
    flags: { }
    locals: { 'Main', 'java/lang/String', integer }
    stack: { integer }
  Stackmap Frame:
    bci: @49
    flags: { }
    locals: { 'Main', 'java/lang/String', integer, top, 'java/lang/String' }
    stack: { }
  Bytecode:
    0000000: 2b03 04b6 0007 3a04 1904 c100 0d99 000b
    0000010: 1904 c000 0d4e 04ac 1cab 0000 0000 0018
    0000020: 0000 0001 0000 0000 0000 0013 013a 0404
    0000030: ac04 ac
  Stackmap Table:
    same_frame(@24)
    same_frame(@44)
    append_frame(@49,Top,Object[#8])

        at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
        at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3035)
        at java.base/java.lang.Class.getMethodsRecursive(Class.java:3177)
        at java.base/java.lang.Class.findMethod(Class.java:2465)
        at java.base/java.lang.System$1.findMethod(System.java:1980)
        at java.base/jdk.internal.misc.MethodFinder.findMainMethod(MethodFinder.java:86)
        at jdk.compiler/com.sun.tools.javac.launcher.SourceLauncher.execute(SourceLauncher.java:194)
        at jdk.compiler/com.sun.tools.javac.launcher.SourceLauncher.run(SourceLauncher.java:138)
        at jdk.compiler/com.sun.tools.javac.launcher.SourceLauncher.main(SourceLauncher.java:76)

Now, the problem, as far as I can tell, is this: javac will desugar the pattern matching instanceof along the lines of:

if (... (var $temp = s.subSequence(0, 1) in ... && ...) ...) {
    return true;
}

($temp is register/local variable number 4)
specifically, note the && in the middle of the let expression, and that the expression is in the conditional position. What happens here is that at the position of the && will basically generate a jump to skip the if (i.e. a jump whose target is just behind the if, for the case where the ... left to it evaluates to false). The problem here is that at the source position of the jump, the variable $temp is defined/has value. And the set of variables which are defined/have value is stored at the moment of jump, and restored at the target place of the jump. I.e. in this case, javac will think variable number 4 has a value.

This, by itself, while it does not seem right, but does not lead to a breakage, as the variable is not in code.lvar, and hence javac will ignore it.

The problem is that inside the first case, variable number 4 is declared, and lvar is filled for it. In the first case, it is still not problematic, as the variable is defined/has value there as well. But, for the default case, the variable still has an lvar entry (as the variable is still declared, just should be unassigned), but the defined/has value flag stored earlier is restored again. So, javac thinks the variable is fully valid, and generates a bogus StackMapTable entry listing the variable, leading to the verifier failure.

I think the source of all trouble is the wrong "defined" flag. The solution in this PR is to make sure the variables declared by the let expression are removed from the defined variables stored in the conditional jump chains.

Note this only applies to jumps that go outside of the left expression (and hence outside of the scope of the variable), internal jumps inside the let expression, if there would be any, shouldn't be affected.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Warning

 ⚠️ Found trailing period in issue title for 8358801: javac produces class that does not pass verifier.
Found leading lowercase letter in issue title for 8358801: javac produces class that does not pass verifier.

Issue

• JDK-8358801: javac produces class that does not pass verifier. (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25849/head:pull/25849
$ git checkout pull/25849

Update a local copy of the PR:
$ git checkout pull/25849
$ git pull https://git.openjdk.org/jdk.git pull/25849/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25849

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25849.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jlahoda! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@lahodaj 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.

[mlbridge]

Webrevs

• 00: Full (fb10cc09)

[liach]

Should we move this truncation of variables to CondItem itself? I moved the truncation of continue/break to GenContext and found that helpful, maybe we can require an extra int arg for makeCondItem(int, Chain, Chain) for the limit?

[lahodaj]

To me, personally: while doing it here is not perfect (mostly because I would prefer if we didn't have to do this bookkeeping at all), it seems to me like a fairly local property - the let expr starts, and then does its own cleanup. It already calls code.endScopes(limit); above this change, so this is basically an extension to that. If we did it at makeCondItem time, we would need to pass the limit everywhere makeCondItem is called, making it non-local. Many parts of the code would now suddenly have to care whether the node is inside a let expression. That feels to me like increasing complexity.

[liach]

Sounds reasonable. I asked this because I could see undefineVariablesInChain being used elsewhere, such as GenContext::addCont/addExit. We should probably update those two sites, as I didn't consider a linked list chain being used in those sites.

[liach]

Or you can even make it an instance method on Chain - the limit can be considered an intrinsic property to where the chain leads to, except this information is not available when a chain is constructed (should it be?)