[WinEH] Validate funclet tokens in colorEHFunclets()

[sgraenitz]

Summary

When coloring EH funclets during WinEHPrepare, we should check that none of the involved funclet bundle operands refers to an invalid/out-of-scope funclet pad. This would lead to silent binary truncations later on.
For the time being users can demote the error into a warning by passing the -no-funclet-bundle-errors flag.

[sgraenitz]

This came up in D134866. No upstream test case triggers the error. Clang reports the error/warning like this:

warning: linking module 'wineh-funclet-bundle-validation.ll': Invalid funclet token in BasicBlock catchret.dest (outside funclet)  %1 = catchpad within %0 [ptr null, i32 0, ptr null] [-Wlinker-warnings]

[rnk]

I think it would be more reasonable to emit a diagnostic at the point where we mark the successor as implausible. This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

llvm/lib/Analysis/EHPersonalities.cpp

111 ↗

(On Diff #465409)

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

[sgraenitz]

Move out of analysis pass and update for specific failure situation: verify that no funclet tokens exist outside of actual funclets (opt-in with -verify-funclet-tokens)

[sgraenitz]

In D135270#3837735, @rnk wrote:

This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

Fair point. I moved it into the first caller in WinEHPrepare.

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

This is done already as a debug check in verifyPreparedFunclets() at the end of prepareExplicitEH().

It's not happening in our repro though! All basic blocks are assigned exactly one color. The catchret.dest block is marked implausible, because it refers to another color's funclet pad. As far as I can tell, this isn't verified anywhere. I adjusted the patch for this specific situation.

[rnk]

In D135270#3843045, @sgraenitz wrote:

In D135270#3837735, @rnk wrote:

This is an analysis function, which is called during instrumentation passes, and I don't think we should emit errors from an analysis helper.

Fair point. I moved it into the first caller in WinEHPrepare.

What I had in mind was to essentially emit an error when the ColorVector has a size of more than one. That represents a situation where we would start cloning basic blocks in WinEHPrepare.

This is done already as a debug check in verifyPreparedFunclets() at the end of prepareExplicitEH().

It's not happening in our repro though! All basic blocks are assigned exactly one color. The catchret.dest block is marked implausible, because it refers to another color's funclet pad. As far as I can tell, this isn't verified anywhere. I adjusted the patch for this specific situation.

Removing implausible terminators happens before that check, and it's supposed to make it so that the check passes and all blocks have a unique color. I guess what I'm suggesting is:

• let's just delete, or flag off, this call to remove implausible terminators
• always call verifyPreparedFunclets
• replace the report_fatal_error calls with error diagnostics

[sgraenitz]

D138123 proposes to check funclet tokens in the IR Verifier

llvm/lib/CodeGen/WinEHPrepare.cpp

static cl::opt<bool> DisableCleanups(
     "disable-cleanups", cl::Hidden,
     cl::desc("Do not remove implausible terminators or other similar cleanups"),
     cl::init(false));
 
 static cl::opt<bool> DemoteCatchSwitchPHIOnlyOpt(
     "demote-catchswitch-only", cl::Hidden,
     cl::desc("Demote catchswitch BBs only (for wasm EH)"), cl::init(false));
 
+static cl::opt<bool> VerifyFuncletTokens(
+    "verify-funclet-tokens", cl::Hidden,
+    cl::desc("During WinEH preparations check for funclet tokens outside of "
+             "actual funclets"),
+    cl::init(false));
+
 namespace {
 
 class WinEHPrepare : public FunctionPass {
 public:
   static char ID; // Pass identification, replacement for typeid.
   WinEHPrepare(bool DemoteCatchSwitchPHIOnly = false)
       : FunctionPass(ID), DemoteCatchSwitchPHIOnly(DemoteCatchSwitchPHIOnly) {}
 
private:
   bool prepareExplicitEH(Function &F);
   void colorFunclets(Function &F);
 
   void demotePHIsOnFunclets(Function &F, bool DemoteCatchSwitchPHIOnly);
   void cloneCommonBlocks(Function &F);
   void removeImplausibleInstructions(Function &F);
   void cleanupPreparedFunclets(Function &F);
   void verifyPreparedFunclets(Function &F);
+  void verifyNoFuncletTokens(const std::vector<BasicBlock *> &Blocks);
 
   bool DemoteCatchSwitchPHIOnly;
 
   // All fields are reset by runOnFunction.
   EHPersonality Personality = EHPersonality::Unknown;
 
   const DataLayout *DL = nullptr;
   DenseMap<BasicBlock *, ColorVector> BlockColors;
void WinEHPrepare::colorFunclets(Function &F) {
   BlockColors = colorEHFunclets(F);
 
   // Invert the map from BB to colors to color to BBs.
   for (BasicBlock &BB : F) {
     ColorVector &Colors = BlockColors[&BB];
     for (BasicBlock *Color : Colors)
       FuncletBlocks[Color].push_back(&BB);
   }
+
+  if (VerifyFuncletTokens)
+    for (auto &Funclet : FuncletBlocks) {
+      auto *FuncletPad = dyn_cast<FuncletPadInst>(Funclet.first->getFirstNonPHI());
+      if (!FuncletPad)
+        verifyNoFuncletTokens(Funclet.second);
+    }
 }
 
 void WinEHPrepare::demotePHIsOnFunclets(Function &F,
                                         bool DemoteCatchSwitchPHIOnly) {
   // Strip PHI nodes off of EH pads.
   SmallVector<PHINode *, 16> PHINodes;
   for (BasicBlock &BB : make_early_inc_range(F)) {
     if (!BB.isEHPad())
for (BasicBlock &BB : F) {
     if (NumColors > 1)
       report_fatal_error("Multicolor BB!");
     assert((DisableDemotion || !(BB.isEHPad() && isa<PHINode>(BB.begin()))) &&
            "EH Pad still has a PHI!");
   }
 }
 #endif
 
+void WinEHPrepare::verifyNoFuncletTokens(const std::vector<BasicBlock *> &Blocks) {
+  for (BasicBlock *BB : Blocks) {
+    for (Instruction &I : *BB) {
+      if (auto *CB = dyn_cast<CallBase>(&I)) {
+        if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet)) {
+          std::string Buffer;
+          raw_string_ostream OS(Buffer);
+          OS << "Invalid funclet token in BasicBlock " << BB->getName();
+          Value *FuncletBundleOperand = BU->Inputs.front();
+          OS << " outside funclet:" << *FuncletBundleOperand << "\n";
+          report_fatal_error(OS.str().c_str());
+        }
+      }
+    }
+  }
+}
+
 bool WinEHPrepare::prepareExplicitEH(Function &F) {
   // Remove unreachable blocks.  It is not valuable to assign them a color and
   // their existence can trick us into thinking values are alive when they are
   // not.
   removeUnreachableBlocks(F);
 
   // Determine which blocks are reachable from which funclet entries.
   colorFunclets(F);

llvm/test/CodeGen/WinEH/wineh-funclet-bundle-validation.ll

+; RUN: not llc < %s -mtriple=x86_64-windows-msvc -verify-funclet-tokens -o - 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+
+declare void @opaque()
+
+declare i32 @__CxxFrameHandler3(...)
+
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+
+define void @invalid_funclet_token() personality ptr @__CxxFrameHandler3 {
+entry:
+  invoke void @opaque()
+          to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %entry
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:                                      ; preds = %entry
+  br label %eh.cont
+
+eh.cont:                                          ; preds = %catchret.dest, %invoke.cont
+  ret void
+
+catch:                                            ; preds = %catch.dispatch
+  %1 = catchpad within %0 [ptr null, i32 0, ptr null]
+  catchret from %1 to label %catchret.dest
+
+; CHECK-ERROR: LLVM ERROR: Invalid funclet token in BasicBlock catchret.dest outside funclet
+catchret.dest:                                    ; preds = %catch
+  call void @llvm.objc.storeStrong(ptr null, ptr null) [ "funclet"(token %1) ]
+  br label %eh.cont
+}
+
+attributes #0 = { nounwind }