[WinCFG] Handle constant casts carefully in .gfids emission

[rnk]

Summary

The general Function::hasAddressTaken has two issues that make it
inappropriate for our purposes:

1. it is sensitive to dead constant users (PR43858 / crbug.com/1019970), leading to different codegen when debu info is enabled
2. it considers direct calls via a function cast to be address escapes

The first is fixable, but the second is not, because IPO clients rely on
this behavior. They assume this function means that all call sites are
analyzable for IPO purposes.

So, implement our own analysis, which gets closer to finding functions
that may be indirect call targets.

[efriedma]

I mentioned this on the bug, but do we care if a reference to a function is optimized away at the MIR level?

llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp

71

Does stripPointerCasts do the right thing here? It can look through multiple levels of casts.

[rnk]

I mentioned this on the bug, but do we care if a reference to a function is optimized away at the MIR level?

I don't think it's worth the work of tracking these things at the MIR level. The new implementation will come with its own set of bugs and quirks, some of which I mentioned on the bug: large code model, target specificity, separate codepath for emission as a global initializer.

We promise that adding -g doesn't change the code we generate, but we don't promise that adding -guard:cf,nochecks won't change the generated code.

llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp

71

Thanks, this should compare to Fn. I couldn't think of another simple way to identify pointer casts.

[rnk]

• fix cast detection

[efriedma]

LGTM

llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp

55

Even if we did decide CFG should protect indirectbr, the target would be the block in question, not the function. So this FIXME isn't really in the right place.

llvm/lib/CodeGen/AsmPrinter/WinCFGuard.cpp

 
 #include "WinCFGuard.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCStreamer.h"
 
 #include <vector>
 
 using namespace llvm;
 
 WinCFGuard::WinCFGuard(AsmPrinter *A) : AsmPrinterHandler(), Asm(A) {}
 
 WinCFGuard::~WinCFGuard() {}
 
 void WinCFGuard::endFunction(const MachineFunction *MF) {
 
   // Skip functions without any longjmp targets.
   if (MF->getLongjmpTargets().empty())
     return;
 
   // Copy the function's longjmp targets to a module-level list.
   LongjmpTargets.insert(LongjmpTargets.end(), MF->getLongjmpTargets().begin(),
                         MF->getLongjmpTargets().end());
 }
 
+/// Returns true if this function's address is escaped in a way that might make
+/// it an indirect call target. Function::hasAddressTaken gives different
+/// results when a function is called directly with a function prototype
+/// mismatch, which requires a cast.
+static bool isPossibleIndirectCallTarget(const Function *F) {
+  SmallVector<const Value *, 4> Users{F};
+  while (!Users.empty()) {
+    const Value *FnOrCast = Users.pop_back_val();
+    for (const Use &U : FnOrCast->uses()) {
+      const User *FnUser = U.getUser();
+      // FIXME: Should CFG protect indirectbr as well?
+      if (isa<BlockAddress>(FnUser))
+        continue;
+      if (const auto *Call = dyn_cast<CallBase>(FnUser)) {
+        if (!Call->isCallee(&U))
+          return true;
+      } else if (isa<Instruction>(FnUser)) {
+        // Consider any other instruction to be an escape. This has some weird
+        // consequences like no-op intrinsics being an escape or a store *to* a
+        // function address being an escape.
+        return true;
+      } else if (const auto *C = dyn_cast<Constant>(FnUser)) {
+        // If this is a constant pointer cast of the function, don't consider
+        // this escape. Analyze the uses of the cast as well. This ensures that
+        // direct calls with mismatched prototypes don't end up in the CFG
+        // table. Consider other constants, such as vtable initializers, to
+        // escape the function.
+        if (C->stripPointerCasts() == F)
+          Users.push_back(FnUser);
+        else
+          return true;
+      }
+    }
+  }
+  return false;
+}
+
 void WinCFGuard::endModule() {
   const Module *M = Asm->MMI->getModule();
   std::vector<const Function *> Functions;
   for (const Function &F : *M)
-    if (F.hasAddressTaken())
+    if (isPossibleIndirectCallTarget(&F))
       Functions.push_back(&F);
   if (Functions.empty() && LongjmpTargets.empty())
     return;
   auto &OS = *Asm->OutStreamer;
   OS.SwitchSection(Asm->OutContext.getObjectFileInfo()->getGFIDsSection());
   for (const Function *F : Functions)
     OS.EmitCOFFSymbolIndex(Asm->getSymbol(F));
 
   // Emit the symbol index of each longjmp target.
   OS.SwitchSection(Asm->OutContext.getObjectFileInfo()->getGLJMPSection());
   for (const MCSymbol *S : LongjmpTargets) {
     OS.EmitCOFFSymbolIndex(S);
   }
 }

llvm/test/CodeGen/WinCFGuard/cfguard-cast.ll

+; RUN: llc < %s -mtriple=x86_64-pc-windows-msvc | FileCheck %s
+
+; Check how constant function pointer casts are handled.
+
+declare void @unprototyped(...)
+
+define i32 @call_unprototyped() {
+  call void bitcast (void (...)* @unprototyped to void ()*)()
+  ret i32 0
+}
+
+; CHECK-LABEL: call_unprototyped:
+; CHECK: callq unprototyped
+; CHECK: xorl %eax, %eax
+; CHECK: retq
+
+declare void @escaped_cast()
+
+define i32 @escape_it_with_cast(i8** %p) {
+  store i8* bitcast (void ()* @escaped_cast to i8*), i8** %p
+  ret i32 0
+}
+
+declare void @dead_constant()
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 2, !"cfguard", i32 1}
+
+!dead_constant_root = !{!1}
+!1 = !DITemplateValueParameter(name: "dead_constant", value: i8* bitcast (void ()* @dead_constant to i8*))
+
+; CHECK-LABEL: .section .gfids$y,"dr"
+; CHECK-NEXT:  .symidx escaped_cast
+; CHECK-NOT:   .symidx
+