[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.
Herald added a subscriber: ychen.

This is very exciting! I didn't look closely at the actual instrumentation code, as rnk knows that better and had some good comments.



================
Comment at: clang/include/clang/Driver/Options.td:500
 def b : JoinedOrSeparate<["-"], "b">, Flags<[Unsupported]>;
+def cfguard_no_checks : Flag<["-"], "cfguard-no-checks">, Flags<[CC1Option]>,
+  HelpText<"Emit Windows Control Flow Guard tables only (no checks).">;
----------------
rnk wrote:
> @hans, WDYT about the -cc1 interface? I think this is fine.
The -cc1 interface looks good to me, but since these are cc1 options only, I think they should be in CC1Options.td, not Options.td (I realize this is a pre-existing issue with -cfguard).


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:493
+  }
   if (CodeGenOpts.ControlFlowGuard) {
+    // Function ID tables and checks for Control Flow Guard (cfguard=2).
----------------
Maybe check for this first, and then the ControlFlowGuardNoChecks one in an else-if, to make this one take precedence?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5971
+
+    if (Instrument && !NoChecks)
+      //Emit CFG instrumentation and the table of address-taken functions.
----------------
It seems unfortunate that the Instrument and NoChecks variables allow four different states. whereas I think the logic should really only allow for three: 1) do nothing, 2) tables only, 3) tables and checks. Maybe we could have an enum instead?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5972
+    if (Instrument && !NoChecks)
+      //Emit CFG instrumentation and the table of address-taken functions.
       CmdArgs.push_back("-cfguard");
----------------
rnk wrote:
> Comment formatting needs to be fixed, you can use clang-format for this.
Also I'd suggest braces since even though there's just one statement, there are multiple lines here and in the else block.


================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:420
+  // Flags that can simply be passed through.
+  Args.AddAllArgs(CmdArgs, options::OPT__SLASH_guard);
 
----------------
Thanks for remembering /fallback. Please add a /guard:cf option to the first test in clang/test/Driver/cl-fallback.c


================
Comment at: llvm/docs/LangRef.rst:441
+    This calling convention is used for the Control Flow Guard check function,
+	which can be inserted before indirect calls to check that the call target 
+	is a valid function address. The check function has no return value, but
----------------
Indentation looks a little off here.

"function, which can be inserted before indirect calls" -- maybe instead "function, calls to which can be inserted ..."


================
Comment at: llvm/docs/LangRef.rst:447
+
+    - On X86_32 the target address is passed in ECX.
+    - On ARM the target address is passed in R0.
----------------
I don't think X86_32 is common nomenclature.. maybe just X86 is enough.


================
Comment at: llvm/docs/LangRef.rst:456
+    (architecture-specific) register. All other function arguments are 
+	passed as usual.
+
----------------
Indentation off here too.


================
Comment at: llvm/docs/LangRef.rst:458
+
+    - On X86-64 the target address is passed in RAX.
 "``cc <n>``" - Numbered convention
----------------
Maybe explicitly call out in the text that this calling convention is only used on x86_64, and is used instead of cfguard_checkcc (iiuc)?


================
Comment at: llvm/include/llvm/IR/CallingConv.h:79
+    /// Special calling convention on Windows for calling the Control
+    /// Guard Check ICall funtion. The function take exactly one argument
+    /// (address of the target function) passed in the first argument register,
----------------
nit: s/take/takes/


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:381
 
+  // Emit tables for any value of cfguard flag (i.e. cfguard=1 or cfguard=2)
   if (mdconst::extract_or_null<ConstantInt>(
----------------
ultra nit: period at the end, here and for other comments


================
Comment at: llvm/lib/Target/X86/X86FixupCFGuard.cpp:13
+/// for such cases and replaces the pair of instructions with a single
+/// call/invoke. For example:
+///
----------------
Naive question: Why do we generate code as in the examples in the first place, and can't some general optimization pass do this folding? From the examples it looks like straight-forward constant propagation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65761/new/

https://reviews.llvm.org/D65761