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

[Vladimir Kozlov]

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