[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 Vladimir for your review!

Best regards,
Christian

On 05.06.20 19:56, Vladimir Kozlov wrote:
> Good.
> 
> Thanks,
> Vladimir
> 
> On 6/5/20 8:58 AM, 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 
>>