RFR: JDK-8222795: post inc operator inside compute function of HashMap results in Exception

[Jan Lahoda]

Hi Vicente,

This looks very similar to:
JDK-8222169
https://bugs.openjdk.java.net/browse/JDK-8222169

I think the main issue here is that the stack size gets recorded in 
visitLetExpr, but the recorded stack size is wrong: there is a jump 
pending (the jump when "v > 5" is false, that jumps over the "v--" into 
"v++"), and when the jump is eventually resolved (in Code.addLocalVar in 
this case, probably) these will change (reduce) the stack size, and the 
asserts will fail.

My plan for JDK-8222169 was to do "code.resolvePending();" at the 
beginning of Gen.visitLetExpr, which should ensure the recorded stack 
size is correct.

I apologize for introducing this problem.

Thanks,
     Jan

On 14. 05. 19 0:07, Vicente Romero wrote:
> ping,
> 
> Thanks,
> Vicente
> 
> On 5/8/19 6:25 PM, Vicente Romero wrote:
>> Please review fix for regression [1] at [2]. The regression was 
>> introduced by [3] which introduced the support for switch expressions. 
>> The compiler is failing with an assertion error for code:
>>
>> import java.util.HashMap;
>> import java.util.Map;
>>
>> public class LetExpressionAssertionTest {
>>     void m() {
>>         Map<String, Integer> m = new HashMap<>();
>>         m.put("a",2);
>>         m.compute("a", (k, v) -> (v > 5) ? v-- : v++);
>>     }
>> }
>>
>> Where `v--` and `v++` are converted to let expressions. Recall that in 
>> this case `v` is of type integer so the let expressions are similar to:
>>
>> `(let /*synthetic*/ final Integer $373182087 = v in (let v = 
>> Integer.valueOf((int)(v.intValue() - 1)); in $373182087))`
>>
>> notice the cast, the ::intValue invocation etc.
>>
>> This patch, [3], also made some changes related to let expressions. In 
>> particular it changed a common idiom in Gen:
>>
>> `Assert.check(code.state.stacksize == 0);` by:
>> Assert.check(code.isStatementStart());
>>
>> where Code::isStatementStart is:
>> public boolean isStatementStart() {
>>      return !alive ||state.stacksize ==letExprStackPos;
>> }
>> which is basically the same check as before, modulo the `alive` check. 
>> The problem is that at Gen::visitVarDef we had a more relaxed 
>> assertion check before [3]:
>>
>> Assert.check(letExprDepth != 0 || code.state.stacksize == 0); which 
>> was also substituted by:
>> Assert.check(code.isStatementStart());
>>
>> this implies a semantic change. In the old code `letExprDepth` was 
>> just a counter of "nestedness" of let expressions and the original 
>> condition in Gen::visitVarDef was mostly to check if the compiler was 
>> generating a let expression or not. Enforcing that 
>> code.state.stacksize has to be equal to the stack position of the let 
>> expression when we started generating the let expression, which is 
>> what the current assertion is checking is a too strong condition. 
>> Which can be false if, as in this case, the let expression has several 
>> method invocations inside, etc that will move the stack as get 
>> generated. This patch is proposing going back to a more relaxed 
>> assertion at Gen::visitVarDef,
>>
>> Thanks,
>> Vicente
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8222795
>> [2] http://cr.openjdk.java.net/~vromero/8222795/webrev.00/
>> [3] https://bugs.openjdk.java.net/browse/JDK-8206986
>