switch statement with yield and try/catch produces EmptyStackException

[felix-schoepf-esd]

We wanted to upgrade to STS 4.20.0, which is based on Eclipse Platform 4.29.0 and includes Eclipse Java Development Tools 3.19.200. Unfortunately, after the update, we are running into the following error:

java.util.EmptyStackException
	at java.base/java.util.Stack.peek(Stack.java:103)
	at java.base/java.util.Stack.pop(Stack.java:85)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.popTypeBinding(CodeStream.java:7803)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.ireturn(CodeStream.java:5645)
	at org.eclipse.jdt.internal.compiler.codegen.StackMapFrameCodeStream.ireturn(StackMapFrameCodeStream.java:449)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.generateReturnBytecode(CodeStream.java:2593)
	at org.eclipse.jdt.internal.compiler.ast.ReturnStatement.generateReturnBytecode(ReturnStatement.java:257)
	at org.eclipse.jdt.internal.compiler.ast.ReturnStatement.generateCode(ReturnStatement.java:240)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:355)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:292)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:763)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:833)
	at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:413)
	at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:1324)
	at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:790)
	at org.eclipse.jdt.core.dom.ASTParser.internalCreateAST(ASTParser.java:1245)
	at org.eclipse.jdt.core.dom.ASTParser.createAST(ASTParser.java:868)
	at org.eclipse.jdt.core.manipulation.CoreASTProvider$1.run(CoreASTProvider.java:294)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.jdt.core.manipulation.CoreASTProvider.createAST(CoreASTProvider.java:286)
	at org.eclipse.jdt.core.manipulation.CoreASTProvider.getAST(CoreASTProvider.java:199)
	at org.eclipse.jdt.core.manipulation.SharedASTProviderCore.getAST(SharedASTProviderCore.java:138)
	at org.eclipse.jdt.internal.ui.viewsupport.SelectionListenerWithASTManager$PartListenerGroup.calculateASTandInform(SelectionListenerWithASTManager.java:166)
	at org.eclipse.jdt.internal.ui.viewsupport.SelectionListenerWithASTManager$PartListenerGroup$1.run(SelectionListenerWithASTManager.java:151)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

eclipse.buildId=4.20.0.202309081654
java.version=20.0.2
java.vendor=Eclipse Adoptium
BootLoader constants: OS=win32, ARCH=x86_64, WS=win32, NL=de_DE
Framework arguments:  -product org.springframework.boot.ide.branding.sts4 -product org.springframework.boot.ide.branding.sts4
Command-line arguments:  -os win32 -ws win32 -arch x86_64 -product org.springframework.boot.ide.branding.sts4 -data file:/D:/work/workspaces/sts4/sts-test/ -product org.springframework.boot.ide.branding.sts4

I tracked it down to an issue with a switch statement, a labeled expression, yield and try/catch. Similar to the following code example, which triggers the EmptyStackException as well:

	private static int switchWithYield() {
		String myStringNumber = "1";
		return switch (myStringNumber) {
		case "1" -> {
			try {
				yield Integer.parseInt(myStringNumber);
			} catch (NumberFormatException e) {
				throw new RuntimeException("Failed parsing number", e);
			}
		}
		default -> throw new IllegalArgumentException("Unexpected value: " + myStringNumber);
		};
	}

Compiler compliance level is 17.

The same code perfectly compiles with STS 4.19.0 based on Eclipse Platform 4.28.0 and and included Eclipse Java Development Tools 3.19.100.

[iloveeclipse]

Still on master.

Complete example:

public class X {

	public static void main(String[] args) {
		switchWithYield("1");
		switchWithYield("42");
	}

	private static int switchWithYield(String myStringNumber) {
		return switch (myStringNumber) {
		case "1" -> {
			try {
				yield Integer.parseInt(myStringNumber);
			} catch (NumberFormatException e) {
				throw new RuntimeException("Failed parsing number", e);
			}
		}
		default -> throw new IllegalArgumentException("Unexpected value: " + myStringNumber);
		};
	}
}

Stack trace on master:

java.util.EmptyStackException
	at java.base/java.util.Stack.peek(Stack.java:101)
	at java.base/java.util.Stack.pop(Stack.java:83)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.popTypeBinding(CodeStream.java:7803)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.ireturn(CodeStream.java:5645)
	at org.eclipse.jdt.internal.compiler.codegen.StackMapFrameCodeStream.ireturn(StackMapFrameCodeStream.java:449)
	at org.eclipse.jdt.internal.compiler.codegen.CodeStream.generateReturnBytecode(CodeStream.java:2593)
	at org.eclipse.jdt.internal.compiler.ast.ReturnStatement.generateReturnBytecode(ReturnStatement.java:257)
	at org.eclipse.jdt.internal.compiler.ast.ReturnStatement.generateCode(ReturnStatement.java:240)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:355)
	at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:292)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:763)
	at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:833)
	at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:413)
	at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:1075)
	at org.eclipse.jdt.internal.compiler.Compiler.resolve(Compiler.java:1111)
	at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:282)
	at org.eclipse.jdt.internal.core.CompilationUnitProblemFinder.process(CompilationUnitProblemFinder.java:348)
	at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:193)
	at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:94)
	at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:740)
	at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:806)
	at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1325)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:132)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:94)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:91)
	at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:158)
	at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.reconcile(CompositeReconcilingStrategy.java:94)
	at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.reconcile(JavaCompositeReconcilingStrategy.java:107)
	at org.eclipse.jface.text.reconciler.MonoReconciler.process(MonoReconciler.java:78)
	at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:207)

[jarthana]

Will take a look if no takers found post Java 21.
Copying @mpalat also

[MonsterDruide1]

I'm also running into this. Interestingly, even removing the concatenation on the bottom error in default will work around this issue.
This will throw an exception:

public static Object bugReport(String val) {
		return switch(val) {
		case "whatever" -> {
			try {
				yield null;
			} catch(NumberFormatException e) {
				throw new RuntimeException(e);
			}
		}
		default -> {
			String concat = "Concat "+val;
			throw new RuntimeException(val);
		}
		};
	}

But this won't: (removing the concat-line)

public static Object bugReport(String val) {
		return switch(val) {
		case "whatever" -> {
			try {
				yield null;
			} catch(NumberFormatException e) {
				throw new RuntimeException(e);
			}
		}
		default -> {
			throw new RuntimeException(val);
		}
		};
	}

[srikanth-sankaran]

@jarthana the mention of string concatenation makes me wonder if this may have something to do with the indified string catenation. Did we add a switch to retain the old behavior? If so it should be easy to verify if this is on the right track.

[jarthana]

@jarthana the mention of string concatenation makes me wonder if this may have something to do with the indified string catenation. Did we add a switch to retain the old behavior? If so it should be easy to verify if this is on the right track.

Yes, it's CompilerOptions.OPTION_UseStringConcatFactory. Let me check with this code.

Edit: Indeed, disabling the option makes the error go away. Will check what's going on.

[srikanth-sankaran]

@jarthana the mention of string concatenation makes me wonder if this may have something to do with the indified string catenation. Did we add a switch to retain the old behavior? If so it should be easy to verify if this is on the right track.

Yes, it's CompilerOptions.OPTION_UseStringConcatFactory. Let me check with this code.

Edit: Indeed, disabling the option makes the error go away. Will check what's going on.

Thanks Jay, I have reassigned this ticket to you.

[jarthana]

It appears that now String concatenation involves an invoke dynamic is interfering with the changes around the popInvokeTypeBinding() and its invocation from invokeDynamic(). I don't know well how the CodeStream#switchSaveTypeBindings or containsSwitchWithTry are supposed to work. But I quickly checked disabling the latter before calling invokeDynamic() from invokeDynamicForStringConcat() fixes this. This can be a permanent fix unless Manoj or Conrad, who originally authored this, can confirm.

The other option could be to introduce another invoke* method that does not call popInvokeTypeBinding(), but instead calls popTypeBinding() so, the clients can choose the right one.

[jarthana]

I have a draft patch. I don't really understand the isSwitchStackTrackingActive() checks inside the pop*() methods and don't want to mess with it much. What I seems to avoid all of it by simply switching off the flag when we generate the invoke dynamic for the switch concatenation. Feel free to review and comment. I will also wait for Manoj/Conrad to throw some light on isSwitchStackTrackingActive() and such.

[mpalat]

I have a draft patch. I don't really understand the isSwitchStackTrackingActive() checks inside the pop*() methods and will also wait for Manoj/Conrad to throw some light on isSwitchStackTrackingActive() and such.

This is used to figure out whether we need to do the tracking at all - this is required only if we have a try inside a switch - this is determined early in the compilation phase

[srikanth-sankaran]

We have another report here: #1727 - this is impacting multiple people, let us close this for M1.

[srikanth-sankaran]

Smallest reproducing test case:

public class X {
	String foo() {
		String x = "1";
		return switch (x) {
		case "1" -> {
			try {
				yield x;
			} finally  {
				
			}
		}
		default -> throw new RuntimeException("" + x); 
		};
	}
}

[sbrannen]

@srikanth-sankaran, have you seen the potentially related issue I raised in #1686?

[srikanth-sankaran]

It appears that now String concatenation involves an invoke dynamic is interfering with the changes around the popInvokeTypeBinding() and its invocation from invokeDynamic(). I don't know well how the CodeStream#switchSaveTypeBindings or containsSwitchWithTry are supposed to work. But I quickly checked disabling the latter before calling invokeDynamic() from invokeDynamicForStringConcat() fixes this. This can be a permanent fix unless Manoj or Conrad, who originally authored this, can confirm.

The other option could be to introduce another invoke* method that does not call popInvokeTypeBinding(), but instead calls popTypeBinding() so, the clients can choose the right one.

This is a red herring. I suspect the defect is in org.eclipse.jdt.internal.compiler.codegen.CodeStream.invokeDynamicForStringConcat(StringBuilder, List<TypeBinding>) in this call:

	this.invokeDynamic(invokeDynamicNumber,
			2, // <<<<----------------------------------------- Hardcoded argsSize
			1, // int
			ConstantPool.ConcatWithConstants,
			signature.toString().toCharArray(),
			TypeIds.T_JavaLangObject,
			getPopularBinding(ConstantPool.JavaLangStringConstantPoolName));

This hardcoded argsSize to be 2. I think it should be computed dynamically based on arguments of org.eclipse.jdt.internal.compiler.codegen.CodeStream.invokeDynamicForStringConcat(StringBuilder, List<TypeBinding>)

This results in one too many pops in the concerned test case.

[srikanth-sankaran]

@srikanth-sankaran, have you seen the potentially related issue I raised in #1686?

Other than both cases having switch expressions, the underlying causes are very different. The current bug is about implementation of string concatenation using invoke dynamic. #1686 doesn't use string concatenation at all.

[srikanth-sankaran]

basically the stack size computed may differ

@srikanth-sankaran : I see that in most cases maxstack value is increased in new classes by one or two. Looking at your change, I believe that should be expected, as it was hard-coded before and now can be higher. Classfile diffs are here.

I will start touching bundles later, have to finish some other work...

Sorry if I am chiming in late, but the stack size need not always increase. While in the remastered tests in https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1740/files they only increase, that is a coincidence.

Consider this test case:

public class X {
  public static void main(String[] args) {
	  new X().foo(new String[] { "o", "n", "e", "!" });
  }
  public void goo(String ... ss) {}
  public void foo(String[] args) {
	  int one = 1;
	  String s = "one=" + args[0] + args[1] + args[2] + args[3];
	  goo("one=" + args[0] + args[1] + args[2] + args[3], s, s, s);
  }
}

When compiled with 4.30, stacksize of foo is 13 while with the fix here it becomes 11

[srikanth-sankaran]

The key computation is this from org.eclipse.jdt.internal.compiler.codegen.CodeStream.invokeDynamic(int, int, int, char[], char[], boolean, TypeReference, TypeReference[], int, TypeBinding)

this.stackDepth += returnTypeSize - argsSize;  // line 1
if (this.stackDepth > this.stackMax) {
    this.stackMax = this.stackDepth;
}

where this.stackDepth is the depth of the operand stack now vs this.stackMax is the high water mark.

What line 1 doing is computing the operand stack depth as a result of the invoke dynamic operation. We are invoking the string concat bootstrap method after having loaded up the operands on the stack. The operation is going to pop argSize slots worth of operands, concatenate them and push the result on to the stack (returnTypeSize worth of slots on the stack)

Before the bug fix, argSize was getting passed a hardcoded incorrect constant value of 2. returnTypeSize is always 1 for string concat factory method since it would leave the concatenated string on the operand stack.

So before the bug fix

this.stackDepth += returnTypeSize - argsSize; 

was really boiling down to:

this.stackDepth += 1- 2; 

decrementing stackDepth by 1.

Now with the bug fix in place returnTypeSize continues to be 1 (one string result) but argument count can be 1, 2, 3, 4, 5, 6 ... depending upon the actual string concat operation (eg: for "one=" + args[0] + args[1] + args[2] + args[3] argSize is 4 as we concat at one stroke)

Accordingly the current stackDepth may increase relatively speaking when computed argSize is 1 (vs hardcoded 2) and will decrease relatively speaking when computed argSize is 3,4,5 etc (vs hardcoded 2) and will stay the same when computed value happens to be same hardcoded value

[srikanth-sankaran]

@srikanth-sankaran : I see that in most cases maxstack value is increased in new classes by one or two.
Looking at your change, I believe that should be expected, as it was hard-coded before and now can be higher.

computed argSize can also be lower. It can be lower, higher or the same.

[srikanth-sankaran]

Section 4.9.2 Structural Constraints of JVMS has this to say:

• At no point during execution can the operand stack grow to a depth greater than
that implied by the max_stack item.

So we should compute the right value and capture the right value - or else we risk a java.lang.ClassFormatError at runtime. (That said, JVMS may use this value computed by the compiler as a hint - I don't know)

[srikanth-sankaran]

For curiosity's sakes, I instrumented ECJ to store 1 instead of the computed 2 for main's stack size in:

public class X {
	public static void main(String[] args) {
		System.out.println("Hello World!"); 
	}
}

Running it results in:

Error: Unable to initialize main class X
Caused by: java.lang.VerifyError: Operand stack overflow
Exception Details:
  Location:
    X.main([Ljava/lang/String;)V @3: ldc
  Reason:
    Exceeded max stack size.
  Current Frame:
    bci: @3
    flags: { }
    locals: { '[Ljava/lang/String;' }
    stack: { 'java/io/PrintStream' }
  Bytecode:
    0000000: b200 1012 16b6 0018 b1