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

[Vicente Romero]

Hi Jan,

I have closed my bug as a duplicate of JDK-8222169,

Thanks for the clarification,
Vicente

On 5/14/19 10:19 AM, Jan Lahoda wrote:
> 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
>>