8271202: C1: assert(false) failed: live_in set of first block must be empty

[TheRealMDoerr]

I have written a checker which detects usage of the illegal phi function. In case of the reproducer provided in the JBS bug ("Reduced.java"), it finds the following and bails out:

invalidating local 8 because of type mismatch (new_value is NULL)
Bailing out because StoreIndexed (id 98) uses illegal phi (id 68)

I haven't checked why that node uses the illegal phi. That still seems to be a bug. Maybe there's a better solution to the underlying problem, but I hope my checker is useful to analyze bugs and to make C1 more resilient.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8271202: C1: assert(false) failed: live_in set of first block must be empty

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6683/head:pull/6683
$ git checkout pull/6683

Update a local copy of the PR:
$ git checkout pull/6683
$ git pull https://git.openjdk.java.net/jdk pull/6683/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6683

View PR using the GUI difftool:
$ git pr show -t 6683

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6683.diff

[bridgekeeper]

👋 Welcome back mdoerr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@TheRealMDoerr The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (3a013b3a)
• 00: Full (7770cefa)

[veresov]

That's quite an expensive search. Wouldn't it be cheaper to bail out where an invalid phi is used? As in LIRGenerator::move_to_phi? And save on the extra traversal of the whole thing?

[TheRealMDoerr]

Thanks for looking at it! I don't like my lengthy code so much, either. However, I haven't found a better place where we can check this. I don't think LIRGenerator::move_to_phi would be such a place because the direction is wrong. Defining node is the phi and the using one a StoreIndexed in this case. Wouldn't LIRGenerator::move_to_phi require it the other way around?
Note: Performance shouldn't be a concern as the search is only needed in very uncommon situations.

[neliasso]

How uncommon is it?

[TheRealMDoerr]

How uncommon is it?

It usually never happens with standard configuration. In some cases when JVMTI capability can_access_local_variables is enabled (e.g. when using -agentlib:jdwp=transport=dt_socket,address=localhost:8000,server=y,suspend=n). C1 runs into it when compiling jvm98 Compressor::compress with JVMTI agent attached for example.
In addition, it happens when using -XX:+DeoptimizeALot which also extends live ranges, but that's only a debug option.

I don't think this PR is ready for integration, but it finds out interesting things which should not happen IMHO.

[veresov]

I took a deeper look and I added a comment to bug report.

The root cause seems to be because of the irreducible loops (and therefore an unusual block traversal order when inserting phis) the phi invalidation logic in try_merge() doesn't invalidate phis that have invalid locals as inputs. I've attached a drawing:
8271202.pdf. Notice that i54 = phi (i43, 96) is not invalidated even though 96 is illegal. Transitively, i43, it should be illegal too. I would propose that we add a check for that and bailout in move_phi().

Suggested fix:

diff --git a/src/hotspot/share/c1/c1_LIRGenerator.cpp b/src/hotspot/share/c1/c1_LIRGenerator.cpp
index c064558b458..b386b541f89 100644
--- a/src/hotspot/share/c1/c1_LIRGenerator.cpp
+++ b/src/hotspot/share/c1/c1_LIRGenerator.cpp
@@ -963,6 +963,14 @@ void LIRGenerator::move_to_phi(PhiResolver* resolver, Value cur_val, Value sux_v
   Phi* phi = sux_val->as_Phi();
   // cur_val can be null without phi being null in conjunction with inlining
   if (phi != NULL && cur_val != NULL && cur_val != phi && !phi->is_illegal()) {
+    if (phi->is_local()) {
+      for (int i = 0; i < phi->operand_count(); i++) {
+        Value op = phi->operand_at(i);
+        if (op != NULL && op->type()->is_illegal()) {
+          bailout("illegal phi operand");
+        }
+      }
+    }
     Phi* cur_phi = cur_val->as_Phi();
     if (cur_phi != NULL && cur_phi->is_illegal()) {
       // Phi and local would need to get invalidated

[veresov]

Do you want me to take over this issue?

[y1yang0]

I think we should at least fix/find such illegal use in state merging rather than LIR generation, that's far beyond where problem occurs

[y1yang0]

should we add something like "FIXME" comment to indicate this is a workaround and we need further investigation to solve it?

[veresov]

I think we should at least fix/find such illegal use in state merging rather than LIR generation, that's far beyond where problem occurs

To find it you basically need another pass. I don't want to introduce another pass just to find an extremely rare situation. Piggybacking on the LIR generation pass is the most cache-friendly place I can think of. In fact, there is already a related bailout in move_to_phi().

I also don't think that this is necessarily even fixable. We artificially stretch the local liveness and that with conjunction with irreducible loops can create unmergable states. I think bailing out is appropriate. It is a very rare case.

[veresov]

Alright, we're tight on time with P3 for 18. I'm going to put out a corresponding PR.

[TheRealMDoerr]

Issue is fixed by openjdk/jdk18@54b800d. I wonder if we still have a potential problem when an illegal phi gets used. That can still happen (not in the provided test() method). E.g. If (id 73) uses illegal phi (id 68) in jvm98 Compressor::compress when using -XX:+DeoptimizeALot or JVMTI agent.