[Verifier] Allow undef token argument to llvm.experimental.gc.result

[dbakunevich]

Summary

The verifier checks that the token of “experimental_gc_result” intrinsic is some kind of call. Therefore, checks that the token is not poison or undef.
See: https://github.com/llvm/llvm-project/issues/57871

[apilipenko]

llvm/lib/IR/Verifier.cpp

5237–5242

llvm/test/Verifier/gc_result_token.ll

23

Add a new line.

[nikic]

Can you please explain in more detail what the motivation for this change is? I've read your patch description, and I've read the linked issue, but I still don't understand why this change is necessary. The fact that a fuzzer can produce IR that does not verify is not surprising -- there is only an issue if IR that originally verifies stops verifying after the application of an IR optimization pass.

[dbakunevich]

As part of the optimization in the unreachable code, we remove tokens, thereby replacing them with undef/poison in intrinsics. But the verifier falls on the assertion, within of what it sees token poison in unreachable code, which in turn is incorrect.
The original code was in the form of a Java program that crashed on this assert. As part of the search for a bug, based on ir obtained from java, a mini test was written that reflects the problem. If you need, I can send Java code with this bug.

[apilipenko]

This structural constraint (gc.result being tied to a statepoint instruction) can be violated in dead code. We've recently fixed a similar issue for gc.relocates in D128904.

[nikic]

Thanks for the explanation. I do wonder how we end up in the situation where the gc.statepoint gets removed, but the gc.result is not also removed.

llvm/lib/IR/Verifier.cpp

5237

Just isa<UndefValue>() is sufficient, it also handles poison.

llvm/test/Verifier/gc_result_token.ll

9

Branch is unnecessary here -- you want label_2 to be unreachable in the CFG, not just dynamically dead.

[mkazantsev]

llvm/test/Verifier/gc_result_token.ll

9

No, this is fine. If a pass is unable to modify CFG, it still has a right to replace operands with undef under if(false).

[mkazantsev]

In D134427#3817919, @nikic wrote:

Thanks for the explanation. I do wonder how we end up in the situation where the gc.statepoint gets removed, but the gc.result is not also removed.

@nikic gc.statepoint doesn't need to be removed. It's enough that gc.result is sunk into a block under if(false), and then some opt understood this and replace its operand with poison.

Why it didn't remove the call at all is a good question, but it could have its reasons.

llvm/lib/IR/Verifier.cpp

case Intrinsic::experimental_gc_statepoint:
     Check(Call.getParent()->getParent()->hasGC(),
           "Enclosing function does not use GC.", Call);
 
     verifyStatepoint(Call);
     break;
   case Intrinsic::experimental_gc_result: {
     Check(Call.getParent()->getParent()->hasGC(),
           "Enclosing function does not use GC.", Call);
+
+    auto *Statepoint = Call.getArgOperand(0);
+    if (isa<UndefValue>(Statepoint))
+      break;
+
     // Are we tied to a statepoint properly?
-    const auto *StatepointCall = dyn_cast<CallBase>(Call.getArgOperand(0));
+    const auto *StatepointCall = dyn_cast<CallBase>(Statepoint);
     const Function *StatepointFn =
         StatepointCall ? StatepointCall->getCalledFunction() : nullptr;
     Check(StatepointFn && StatepointFn->isDeclaration() &&
               StatepointFn->getIntrinsicID() ==
                   Intrinsic::experimental_gc_statepoint,
           "gc.result operand #1 must be from a statepoint", Call,
           Call.getArgOperand(0));
 

llvm/test/Verifier/gc_result_token.ll

+; RUN: opt -S -passes=verify < %s | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo() gc "statepoint_example" personality ptr @P {
+; CHECK-NOT: gc.result operand #1 must be from a statepoint   
+entry:
+    br label %label_1
+
+label_1:
+    ; CHECK: ret void
+    ret void
+
+label_2:
+    ; CHECK: token poison
+    %call = call noundef i32 @llvm.experimental.gc.result.i32(token poison)
+    unreachable
+}   
+
+declare i32 @llvm.experimental.gc.result.i32(token)
+
+declare ptr @P()