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

[Tobias Hartmann]

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