[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]

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