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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
