[PATCH] D31796: [cfi] Emit __cfi_check stub in the frontend

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/CodeGen/CGExpr.cpp:2786
 
+void CodeGenFunction::EmitCfiCheckStub() {
+  auto &Ctx = CGM.getModule().getContext();
----------------
Please add a comment explaining why we are creating this function.


================
Comment at: lib/CodeGen/CGExpr.cpp:2788
+  auto &Ctx = CGM.getModule().getContext();
+  llvm::Function *F = llvm::Function::Create(
+      llvm::FunctionType::get(VoidTy, {Int64Ty, Int8PtrTy, Int8PtrTy}, false),
----------------
What will happen in the CrossDSOCFI pass if it is given a module with this function definition? It looks like it will append its basic blocks to the end of the definition. Because the entry block you are creating here returns immediately, all of the blocks it adds will be unreachable.

Probably the easiest thing to do is to change CrossDSOCFI to replace any `__cfi_check` definition it sees with its own definition.


================
Comment at: lib/CodeGen/CGExpr.cpp:2790
+      llvm::FunctionType::get(VoidTy, {Int64Ty, Int8PtrTy, Int8PtrTy}, false),
+      llvm::GlobalValue::WeakAnyLinkage, "__cfi_check", &CGM.getModule());
+  llvm::BasicBlock *BB = llvm::BasicBlock::Create(Ctx, "entry", F);
----------------
Although this linkage may prevent inlining for now, we may in the future change the LTO implementation to promote weak functions to strong if the linker knows that a particular definition of a symbol is the final definition for the symbol. That will mean that this dummy definition may be inlined into callers.

This is a largely theoretical problem because we never create a direct call to the `__cfi_check` function. But if we wanted to fix this I can imagine creating an intrinsic that the CrossDSOCFI pass expands to the definition of `__cfi_check`.


================
Comment at: lib/CodeGen/CGExpr.cpp:2876
   FinishFunction();
+
   // The only reference to this function will be created during LTO link.
----------------
remove this line


Repository:
  rL LLVM

https://reviews.llvm.org/D31796