[15] RFR(S): 8244719: CTW: C2 compilation fails with "assert(!VerifyHashTableKeys || _hash_lock == 0) failed: remove node from hash table before modifying it"

[Christian Hagedorn]

Thank you Tobias for your review!

I updated my webrev in place with your comments.

Best regards
Christian

On 10.06.20 14:13, Tobias Hartmann wrote:
> Hi Christian,
> 
> Looks good to me too.
> 
> In TestExceptionBlockWithPredecessorsMain:40/41, the brackets should be around the "i % 2 == 0"
> expression and in parse1.cpp:1297 there is an excess whitespace. No new webrev required.
> 
> Best regards,
> Tobias
> 
> On 05.06.20 17:58, Christian Hagedorn wrote:
>> Hi
>>
>> Please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8244719
>> http://cr.openjdk.java.net/~chagedorn/8244719/webrev.00/
>>
>> The assertion failure at [5] can be traced back to a wrong assumption made in
>> Parse::Block::init_graph(). It explicitly states in a comment there that we never call
>> next_path_num() along exception paths [1]. But it turns out that this is only true for bytecode
>> generated by Javac which does not seem to produce bytecode where an exception handler is reached by
>> an explicit jump or "fall through" bytecode. An exception handler is only reached with an athrow.
>>
>> However, it is possible to break that assumption with some custom bytecode. The jasm testcase
>> generates such a valid bytecode sequence where an exception handler is reached by jumps from another
>> exception handler:
>>
>>        69: astore_1
>>        70: aload_1
>>        71: aload_0
>>        72: getfield      #5                  // Field loopCounter:I
>>        75: bipush        10
>>        77: if_icmpge     93 // Explicit jump to exception handler, non-Javac
>>        90: goto          93 // Explicit jump to exception handler, non-Javac
>>        93: astore_1
>>        94: return
>>      Exception table:
>>         from    to  target type
>>             0    66    69   Class java/lang/RuntimeException
>>             0    66    93   Class java/lang/Throwable
>>
>> This means that the first time Parse::merge_exception() is called for the exception handler block at
>> bci 93, pnum is set to 3 since there are 2 predecessors (2 jumps to it). In the very first call to
>> merge_common(), is_merged() is still false and we record a state. All following calls to
>> merge_common() for this exception block will take the else case [2]. Once we are processing the
>> blocks for the exception handler at bci 69, we call merge() (and therefore next_path_num()) in
>> do_one_block() [3] twice with target_bci = 93 (2 jumps to bci 93). The last time with pnum = 1 for
>> bci 90: goto and we transform the phi with gvn and set the hash_lock for it to 1 at [4].
>>
>> Now comes a second bytecode modification trick where we first hit a trap while parsing a block in
>> do_all_blocks(). Therefore, all successor blocks on that path are not merged and skipped in the
>> first iteration of the loop in do_all_blocks() (at this point these blocks seem to be dead). But
>> later we can have a jump back to such a seemingly dead block again. Those are then processed in the
>> second iteration of the loop in do_all_blocks(). If one of these blocks now additionally throw an
>> exception, we can hit this assertion failure. An example could look as follows:
>>
>> Example:
>> // First iteration in do_all_blocks()
>> Parse B1;
>> Parse B2; // Hit trap. Stop parsing on that path, skip on B3 and B4 which immediately follow B2 and
>> have no other predecessors
>> Skip B3; // Was not merged. Assumed to be dead at this point
>> Skip B4; // Was not merged. Assumed to be dead at this point
>> Parse B5; // Discover jump to B3 -> merge B3. Will be processed but only in the next iteration since
>> rpo of B2 is smaller than the one of B5
>> Parse E1; // Parse exception handler 1 at bci 69
>> Parse E2; // Parse exception handler 2 at bci 93, apply gvn for phi
>>
>> // Next iteration in do_all_blocks()
>> Parse B3; // Is now merged and ready to be parsed. Has exception to E2: call merge_exception() ->
>> merge_common() with E2 as target and pnum > 1. We hit the assertion at [5] since we already applied
>> a transformation for a phi in the last iteration and therefore have a non-zero hash_lock.
>>
>> As a solution to this problem, I suggest to fix the wrong assumption by changing
>> Parse::Block::init_graph() to also count predecessors for exception blocks. This ensures that [4] is
>> really the last merge for a phi.
>>
>> I did some additional performance testing with standard benchmarks and did not find any regressions.
>>
>> Thank you!
>>
>> Best regards,
>> Christian
>>
>>
>> [1] http://hg.openjdk.java.net/jdk/jdk/file/71ec718a0bd0/src/hotspot/share/opto/parse1.cpp#l1314
>> [2] http://hg.openjdk.java.net/jdk/jdk/file/71ec718a0bd0/src/hotspot/share/opto/parse1.cpp#l1678
>> [3] http://hg.openjdk.java.net/jdk/jdk/file/71ec718a0bd0/src/hotspot/share/opto/parse1.cpp#l1508
>> [4] http://hg.openjdk.java.net/jdk/jdk/file/71ec718a0bd0/src/hotspot/share/opto/parse1.cpp#l1773
>> [5] http://hg.openjdk.java.net/jdk/jdk/file/71ec718a0bd0/src/hotspot/share/opto/parse1.cpp#l1764