Skip to content

Commit

Permalink
8302865: Illegal bytecode for break from if with instanceof pattern m…
Browse files Browse the repository at this point in the history 
…atching condition

Reviewed-by: vromero
  • Loading branch information
@lahodaj
lahodaj committed Jun 21, 2023
1 parent 67fbd87 commit a15db1a
Show file tree
Hide file tree
Showing 6 changed files with 195 additions and 68 deletions.
68 changes: 46 additions & 22 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1447,12 +1447,7 @@ public void visitBlock(JCBlock tree) {
public void visitDoLoop(JCDoWhileLoop tree) {
attribStat(tree.body, env.dup(tree));
attribExpr(tree.cond, env, syms.booleanType);
if (!breaksOutOf(tree, tree.body)) {
//include condition's body when false after the while, if cannot get out of the loop
MatchBindings condBindings = matchBindings;
condBindings.bindingsWhenFalse.forEach(env.info.scope::enter);
condBindings.bindingsWhenFalse.forEach(BindingSymbol::preserveBinding);
}
handleLoopConditionBindings(matchBindings, tree, tree.body);
result = null;
}

Expand All @@ -1466,19 +1461,10 @@ public void visitWhileLoop(JCWhileLoop tree) {
} finally {
whileEnv.info.scope.leave();
}
if (!breaksOutOf(tree, tree.body)) {
//include condition's bindings when false after the while, if cannot get out of the loop
condBindings.bindingsWhenFalse.forEach(env.info.scope::enter);
condBindings.bindingsWhenFalse.forEach(BindingSymbol::preserveBinding);
}
handleLoopConditionBindings(condBindings, tree, tree.body);
result = null;
}

private boolean breaksOutOf(JCTree loop, JCTree body) {
preFlow(body);
return flow.breaksOutOf(env, loop, body, make);
}

public void visitForLoop(JCForLoop tree) {
Env<AttrContext> loopEnv =
env.dup(env.tree, env.info.dup(env.info.scope.dup()));
Expand All @@ -1503,11 +1489,50 @@ public void visitForLoop(JCForLoop tree) {
finally {
loopEnv.info.scope.leave();
}
if (!breaksOutOf(tree, tree.body)) {
//include condition's body when false after the while, if cannot get out of the loop
condBindings.bindingsWhenFalse.forEach(env.info.scope::enter);
condBindings.bindingsWhenFalse.forEach(BindingSymbol::preserveBinding);
handleLoopConditionBindings(condBindings, tree, tree.body);
}

/**
* Include condition's bindings when false after the loop, if cannot get out of the loop
*/
private void handleLoopConditionBindings(MatchBindings condBindings,
JCStatement loop,
JCStatement loopBody) {
if (condBindings.bindingsWhenFalse.nonEmpty() &&
!breaksTo(env, loop, loopBody)) {
addBindings2Scope(loop, condBindings.bindingsWhenFalse);
}
}

private boolean breaksTo(Env<AttrContext> env, JCTree loop, JCTree body) {
preFlow(body);
return flow.breaksToTree(env, loop, body, make);
}

/**
* Add given bindings to the current scope, unless there's a break to
* an immediately enclosing labeled statement.
*/
private void addBindings2Scope(JCStatement introducingStatement,
List<BindingSymbol> bindings) {
if (bindings.isEmpty()) {
return ;
}

var searchEnv = env;
while (searchEnv.tree instanceof JCLabeledStatement labeled &&
labeled.body == introducingStatement) {
if (breaksTo(env, labeled, labeled.body)) {
//breaking to an immediately enclosing labeled statement
return ;
}
searchEnv = searchEnv.next;
introducingStatement = labeled;
}

//include condition's body when false after the while, if cannot get out of the loop
bindings.forEach(env.info.scope::enter);
bindings.forEach(BindingSymbol::preserveBinding);
}

public void visitForeachLoop(JCEnhancedForLoop tree) {
Expand Down Expand Up @@ -2242,8 +2267,7 @@ public void visitIf(JCIf tree) {
afterIfBindings = condBindings.bindingsWhenFalse;
}

afterIfBindings.forEach(env.info.scope::enter);
afterIfBindings.forEach(BindingSymbol::preserveBinding);
addBindings2Scope(tree, afterIfBindings);

result = null;
}
Expand Down
53 changes: 11 additions & 42 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,18 +287,18 @@ public boolean aliveAfter(Env<AttrContext> env, JCTree that, TreeMaker make) {
}
}

public boolean breaksOutOf(Env<AttrContext> env, JCTree loop, JCTree body, TreeMaker make) {
public boolean breaksToTree(Env<AttrContext> env, JCTree breakTo, JCTree body, TreeMaker make) {
//we need to disable diagnostics temporarily; the problem is that if
//"that" contains e.g. an unreachable statement, an error
//message will be reported and will cause compilation to skip the flow analysis
//step - if we suppress diagnostics, we won't stop at Attr for flow-analysis
//related errors, which will allow for more errors to be detected
Log.DiagnosticHandler diagHandler = new Log.DiscardDiagnosticHandler(log);
try {
SnippetBreakAnalyzer analyzer = new SnippetBreakAnalyzer();
SnippetBreakToAnalyzer analyzer = new SnippetBreakToAnalyzer(breakTo);

analyzer.analyzeTree(env, body, make);
return analyzer.breaksOut();
return analyzer.breaksTo();
} finally {
log.popDiagnosticHandler(diagHandler);
}
Expand Down Expand Up @@ -1909,52 +1909,21 @@ public boolean isAlive() {
}
}

class SnippetBreakAnalyzer extends AliveAnalyzer {
private final Set<JCTree> seenTrees = new HashSet<>();
private boolean breaksOut;
class SnippetBreakToAnalyzer extends AliveAnalyzer {
private final JCTree breakTo;
private boolean breaksTo;

public SnippetBreakAnalyzer() {
}

@Override
public void visitLabelled(JCTree.JCLabeledStatement tree) {
seenTrees.add(tree);
super.visitLabelled(tree);
}

@Override
public void visitWhileLoop(JCTree.JCWhileLoop tree) {
seenTrees.add(tree);
super.visitWhileLoop(tree);
}

@Override
public void visitForLoop(JCTree.JCForLoop tree) {
seenTrees.add(tree);
super.visitForLoop(tree);
}

@Override
public void visitForeachLoop(JCTree.JCEnhancedForLoop tree) {
seenTrees.add(tree);
super.visitForeachLoop(tree);
}

@Override
public void visitDoLoop(JCTree.JCDoWhileLoop tree) {
seenTrees.add(tree);
super.visitDoLoop(tree);
public SnippetBreakToAnalyzer(JCTree breakTo) {
this.breakTo = breakTo;
}

@Override
public void visitBreak(JCBreak tree) {
breaksOut |= (super.alive == Liveness.ALIVE &&
!seenTrees.contains(tree.target));
super.visitBreak(tree);
breaksTo |= breakTo == tree.target && super.alive == Liveness.ALIVE;
}

public boolean breaksOut() {
return breaksOut;
public boolean breaksTo() {
return breaksTo;
}
}

Expand Down
61 changes: 61 additions & 0 deletions test/langtools/tools/javac/patterns/BindingsTest1.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,67 @@ public void run() {
throw new AssertionError();
}

{
L: {
while (!(o1 instanceof String s)) {
break L;
}

s.length();
}
}

{
L: {
for (; !(o1 instanceof String s); ) {
break L;
}

s.length();
}
}

{
int j = 0;
L: while (j++ < 2)
if (!(o1 instanceof String s)) {
break L;
}
}

{
int j = 0;
L: for (; j++ < 2; )
if (!(o1 instanceof String s)) {
break L;
}
}

{
//"s" in the outter scope does not flow out of the if, but
//variables inside a lambda or anonymous or local class may:
L: if (!(o1 instanceof String s)) {
Runnable r = () -> {
NESTED: {
if (!(o1 instanceof String n)) {
break NESTED;
}

n.length();
}
};
break L;
}
}

switch (0) {
case 0:
if (!(o1 instanceof String s)) {
break;
}
s.length();
}

//binding in an anonymous class:
if (!(invokeOnce("") instanceof String s)) {
throw new AssertionError();
Expand Down
57 changes: 57 additions & 0 deletions test/langtools/tools/javac/patterns/BindingsTest2.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,5 +247,62 @@ public static void meth() {
s = "";
}
}
{
LBL1: LBL2: if (!(o1 instanceof String s)) {
break LBL1;
}

System.err.println(s);
}
{
LBL1: LBL2: if (!(o1 instanceof String s)) {
break LBL2;
}

System.err.println(s);
}
{
LBL1: LBL2: if (o1 instanceof String s) {
} else {
break LBL1;
}

System.err.println(s);
}
{
LBL1: LBL2: if (o1 instanceof String s) {
} else {
break LBL2;
}

System.err.println(s);
}
{
switch (0) {
case 0:
if (!(o1 instanceof String s)) {
break;
}
}
s.length();
}

{
int j = 0;
L: while (j++ < 2)
if (!(o1 instanceof String s)) {
break L;
}
s.length();
}

{
int j = 0;
L: for (; j++ < 2; )
if (!(o1 instanceof String s)) {
break L;
}
s.length();
}
}
}
13 changes: 9 additions & 4 deletions test/langtools/tools/javac/patterns/BindingsTest2.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,16 @@ BindingsTest2.java:179:13: compiler.err.cant.resolve.location: kindname.variable
BindingsTest2.java:196:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:204:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:212:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:221:17: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:231:17: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:241:17: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:247:17: compiler.err.cant.assign.val.to.var: final, s
BindingsTest2.java:255:32: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:262:32: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:270:32: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:278:32: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:287:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:296:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:305:13: compiler.err.cant.resolve.location: kindname.variable, s, , , (compiler.misc.location: kindname.class, BindingsTest2, null)
BindingsTest2.java:135:17: compiler.err.unreachable.stmt
BindingsTest2.java:160:17: compiler.err.unreachable.stmt
BindingsTest2.java:185:17: compiler.err.unreachable.stmt
52 errors
BindingsTest2.java:241:17: compiler.err.unreachable.stmt
57 errors
11 changes: 11 additions & 0 deletions test/langtools/tools/javac/patterns/BreakAndLoops.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ protected void doWork() throws Throwable {
shouldPass = true;
} else if (innerLoop.supportsAnonymousBreak && brk == Break.BREAK) {
shouldPass = true;
} else if (outterLabel == OutterLabel.LABEL && brk == Break.BREAK_LABEL && outterLoop != OutterLoop.NONE) {
shouldPass = switch(mainLoop) {
case WHILE, FOR -> true;
case DO_WHILE -> switch (innerLoop) {
case WHILE, FOR, FOR_EACH -> true;
//the statement following the do-while is unreachable:
case BLOCK, DO_WHILE, NONE -> {
yield false;
}
};
};
} else {
shouldPass = false;
}
Expand Down

1 comment on commit a15db1a

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.