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

[Vicente Romero]

Hi Bernard,

Thanks for your feedback,

On 5/11/19 10:11 AM, B. Blaser wrote:
> Hi Vicente,
>
> I worked on postfix unary operators long time ago and I believe your
> fix looks reasonable.
>
> However, the need of relaxing the assertion as you suggested is still
> tickling me a bit because 'visitLetExpr()' seems to correctly set the
> start position:
> http://hg.openjdk.java.net/jdk/jdk/file/fcf83b204c27/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java#l2313
> which should be sufficient for this particular let-expression.
>
> Does the problem come from the conditional expression?

I believe that the need for relaxing the current assertion:

Assert.check(code.isStatementStart());

comes from the definition of Code::isStatementStart which is checking 
against field: Code::letExprStackPos which in this case is set when Gen 
starts handling the let expression, Gen::visitLetExpr, but in this case 
the let expressions contain subexpressions that alter the stack pos, 
method invocations etc. To me it is too strict to expect at 
Gen::visitVarDef that the stack position will be the same as it was when 
Gen started generating the let expression. And I think this was the 
reason why this condition was more relaxed in the code previous to the 
switch expression patch.

Thanks,
Vicente


> Bernard
>
>
> On Thu, 9 May 2019 at 00:26, Vicente Romero <vicente.romero at oracle.com> 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