[regression] java.lang.VerifyError: Operand stack overflow

[snjeza]

Related issues

• Caused by: java.lang.VerifyError: Operand stack overflow redhat-developer/vscode-java#3232
• Bug 527899 [9] Implement JEP 280: Indify String Concatenation #1139

Steps to reproduce

• create the following class

public class Test {
	public static void main(String[] args) {
		String value = "abc";
		value.equalsIgnoreCase("" + null);
		System.out.println(value.substring(1));
	}
}

• run it
  It throws

Error: Unable to initialize main class org.sample.Test
Caused by: java.lang.VerifyError: Operand stack overflow
Exception Details:
  Location:
    org/sample/Test.main([Ljava/lang/String;)V @17: iconst_1
  Reason:
    Exceeded max stack size.
  Current Frame:
    bci: @17
    flags: { }
    locals: { '[Ljava/lang/String;', 'java/lang/String' }
    stack: { 'java/io/PrintStream', 'java/lang/String' }
  Bytecode:
    0000000: 1210 4c2b ba00 1200 00b6 0016 57b2 001c
    0000010: 2b04 b600 22b6 0026 b1                 

A workaorund
set org.eclipse.jdt.core.compiler.codegen.useStringConcatFactory = disabled

The issue has been caused by be45d8f

It can't be reproduced in Eclipse <= I20230628-1800

[iloveeclipse]

@jarthana : could you please take a look?

[snjeza]

A potential patch:

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Expression.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Expression.java
index 7e091ed000..1b61a45530 100644
--- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Expression.java
+++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Expression.java
@@ -966,14 +966,9 @@ public void buildStringForConcatation(BlockScope blockScope, CodeStream codeStre
 				addArgumentToRecipe(blockScope, codeStream, recipe, this.resolvedType, argTypes);
 				break;
 			default :
-				if (this.resolvedType.id == TypeIds.T_null) {
-					// Optimize it, avoid aconst_null, simply append the String Literal
-					recipe.append((String) null);
-				} else {
-					generateCode(blockScope, codeStream, true);
-					codeStream.invokeStringValueOf(typeID);
-					addArgumentToRecipe(blockScope, codeStream, recipe, blockScope.getJavaLangString(), argTypes);
-				}
+				generateCode(blockScope, codeStream, true);
+				codeStream.invokeStringValueOf(typeID);
+				addArgumentToRecipe(blockScope, codeStream, recipe, blockScope.getJavaLangString(), argTypes);
 				break;
 		}
 	} else {

[iloveeclipse]

A potential patch:

1. Does it passes the tests?
2. Could you provide a regression test?

[snjeza]

@iloveeclipse I will try.

[snjeza]

Does it passes the tests?
Could you provide a regression test?

@iloveeclipse I have updated the regression tests.

[iloveeclipse]

I have updated the regression tests.

Thanks. But can you please add your own test (with the snippet from bug description) that shows the fix is working?

[snjeza]

But can you please add your own test (with the snippet from bug description) that shows the fix is working

Yes, I can.
@iloveeclipse I have added it - 9a33f67#diff-621ae34c3c2a3c4ab0ed4676ffdbba5af95c72e7437dad5b7448c9251c7754afR1128

[jarthana]

I think the reason why the fix works is it increments the stack length by way of valueOf, which makes enough room for the subsequent iconst_1 operation. Of course, the problem was we were short on stack length to begin with is the cause and hence the fix works. Perhaps we should investigate why the stack is insufficient.

If I make the following change, I don't see the problem:

//		System.out.println(value.substring(1));
		String str = value.substring(1);

[jarthana]

OK, I see that right after the concat statement, the stackDepth goes down by one. The reason is, we don't really add any stackDepth for the null operand as a result of this code but later the invokedynamic makes decrements the stackDepth by one (because arguments - returntypes = -1) :

				if (this.resolvedType.id == TypeIds.T_null) {
					// Optimize it, avoid aconst_null, simply append the String Literal
					recipe.append((String) null);
				}

We have two options:

1. We either remove the optimization and put back the aconstnull or
2. just plainly increment the stackDepth by 1 to account for the reduction we expect to happen later.

I am inclined to go with the first.

@snjeza Your patch works, but it generates lot of additional code for the null constant. What do you think of the options I just mentioned?

[snjeza]

2. just plainly increment the stackDepth by 1 to account for the reduction we expect to happen later.

@jarthana This works fine.
I have updated the PR - 77bab66

[jarthana]

2. just plainly increment the stackDepth by 1 to account for the reduction we expect to happen later.

@jarthana This works fine. I have updated the PR - 77bab66

@snjeza
Thinking again, the approach to just increment the stackDepth is probably kludge. And it's not enough just to increment it. We must also adjust the stackMax if required as can be seen in many other places that increment that count.

Here's an alternate fix : #1265

[snjeza]

@snjeza
Thinking again, the approach to just increment the stackDepth is probably kludge. And it's not enough just to increment it. We must also adjust the stackMax if required as can be seen in many other places that increment that count.
Here's an alternate fix : #1265

@jarthana #1265 fixes the issue, but ConstantTest.test009() should be fixed.