[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

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/Analysis/EHPersonalities.cpp

 //===- EHPersonalities.cpp - Compute EH-related information ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
+static cl::opt<bool> NoFuncletBundleErrors(
+    "no-funclet-bundle-errors", cl::Hidden,
+    cl::desc("Bail out if validation of funclet bundle operands fails during "
+             "EH funclet coloring"),
+    cl::init(false));
+
 /// See if the given exception handling personality function is one that we
 /// understand.  If so, return a description of it; otherwise return Unknown.
 EHPersonality llvm::classifyEHPersonality(const Value *Pers) {
   const GlobalValue *F =
       Pers ? dyn_cast<GlobalValue>(Pers->stripPointerCasts()) : nullptr;
   if (!F || !F->getValueType() || !F->getValueType()->isFunctionTy())
     return EHPersonality::Unknown;
   return StringSwitch<EHPersonality>(F->getName())

 bool llvm::canSimplifyInvokeNoUnwind(const Function *F) {
   EHPersonality Personality = classifyEHPersonality(F->getPersonalityFn());
   // We can't simplify any invokes to nounwind functions if the personality
   // function wants to catch asynch exceptions.  The nounwind attribute only
   // implies that the function does not throw synchronous exceptions.
   return !isAsynchronousEHPersonality(Personality);
 }
 
+class InvalidFuncletTokenDiagnostic : public DiagnosticInfo {
+  std::string Msg;
+
+public:
+  InvalidFuncletTokenDiagnostic(DiagnosticSeverity Severity,
+                                StringRef BasicBlock, FuncletPadInst *ParentPad,
+                                Value *ReferredPad)
+      : DiagnosticInfo(DK_Linker, Severity) {
+    raw_string_ostream OS(Msg);
+    OS << "Invalid funclet token in BasicBlock " << BasicBlock;
+    if (!ParentPad)
+      OS << " (outside funclet)";
+    OS << *ReferredPad;
+  }
+  void print(DiagnosticPrinter &DP) const override { DP << Msg; }
+};
+
 DenseMap<BasicBlock *, ColorVector> llvm::colorEHFunclets(Function &F) {
   SmallVector<std::pair<BasicBlock *, BasicBlock *>, 16> Worklist;
   BasicBlock *EntryBlock = &F.getEntryBlock();
   DenseMap<BasicBlock *, ColorVector> BlockColors;
 
   // Build up the color map, which maps each block to its set of 'colors'.
   // For any block B the "colors" of B are the set of funclets F (possibly
   // including a root "funclet" representing the main function) such that
   // F will need to directly contain B or a copy of B (where the term "directly
while (!Worklist.empty()) {
     }
     // Note that this is a member of the given color.
     ColorVector &Colors = BlockColors[Visiting];
     if (!is_contained(Colors, Color))
       Colors.push_back(Color);
     else
       continue;
 
+    // Make sure all instructions with funclet tokens refer to the current pad.
+    auto *FuncletPad = dyn_cast<FuncletPadInst>(Color->getFirstNonPHI());
+    for (Instruction &I : *Visiting) {
+      if (auto *CB = dyn_cast<CallBase>(&I)) {
+        if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet)) {
+          Value *FuncletBundleOperand = BU->Inputs.front();
+          if (FuncletBundleOperand != FuncletPad)
+            F.getContext().diagnose(InvalidFuncletTokenDiagnostic(
+                NoFuncletBundleErrors ? DS_Warning : DS_Error,
+                Visiting->getName(), FuncletPad, FuncletBundleOperand));
+        }
+      }
+    }
+
     DEBUG_WITH_TYPE("winehprepare-coloring",
                     dbgs() << "  Assigned color \'" << Color->getName()
                            << "\' to block \'" << Visiting->getName()
                            << "\'.\n");
 
     BasicBlock *SuccColor = Color;
     Instruction *Terminator = Visiting->getTerminator();
     if (auto *CatchRet = dyn_cast<CatchReturnInst>(Terminator)) {

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

+; RUN: llc < %s -mtriple=x86_64-windows-msvc -no-funclet-bundle-errors -o - 2>&1 | FileCheck %s --check-prefix=CHECK-WARNING
+
+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-WARNING: warning: 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 }