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

[Andrew Paverd via Phabricator via llvm-commits]

ajpaverd marked 9 inline comments as done.
ajpaverd added inline 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).">;
----------------
hans wrote:
> 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).
Thanks for all the helpful feedback @hans. I think I've addressed all your comments in the latest revision.


================
Comment at: llvm/lib/CodeGen/CFGuardLongjmp.cpp:101
+  // targets.
+  for (MachineInstr *Setjmp : SetjmpCalls) {
+    MachineBasicBlock *OldMBB = Setjmp->getParent();
----------------
rnk wrote:
> Rather than changing the machine CFG, I would suggest using MachineInstr::setPostInstrSymbol, which I've been planning to use for exactly this purpose. :) You can make labels with MCContext::createSymbol, and you'll want to come up with symbols that won't clash with C or C++ symbols. I see MSVC makes labels like `$LN4`, so maybe `$cfgsjN` or something like that. I think MCContext will add numbers for you.
Thanks for the suggestion! It looks like MCContext::createSymbol is private, so I'm generating a new symbol name and number for MCContext::getOrCreateSymbol. Does this look OK? 


================
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:
+///
----------------
rnk wrote:
> hans wrote:
> > 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.
> Actually, I used this test IR, LLVM seems to always fold the memory operand into the call:
> ```
> @fptr = external dso_local global void()*
> define i32 @foo() {
> 	%fp1 = load void()*, void()** @fptr
> 	call void %fp1()
> 	%fp2 = load void()*, void()** @fptr
> 	call void %fp2()
> 	ret i32 0
> }
> ```
> 
> Maybe it won't do it if there are more parameters, I'm not sure.
> 
> I ran llc with both isels for x64 and ia32, and it always folded the load into the call. Maybe it's best to make this a verification pass that emits an error via MCContext if there is an unfolded load of the CFG check function pointer?
I'm seeing this when compiling with optimizations disabled. When I run llc with `-fast-isel=false`, the following slightly modified IR does not fold the memory operand into the call:

```
@fptr = external dso_local global void()*
define i32 @foo() #0 {
  %fp1 = load void()*, void()** @fptr
  call void %fp1()
  %fp2 = load void()*, void()** @fptr
  call void %fp2()
  ret i32 0
}
attributes #0 = { noinline optnone }
```

It looks like this is caused by checks for `OptLevel == CodeGenOpt::None` in:

  - SelectionDAGISel::IsLegalToFold
  - X86DAGToDAGISel::IsProfitableToFold
  - X86DAGToDAGISel::PreprocessISelDAG (in this case OptLevel != CodeGenOpt::None)

I guess this is not high priority as it only happens at -O0. Should I look into enabling these specific optimizations when CFG is enabled, or just emit a warning when this happens?


================
Comment at: llvm/lib/Transforms/CFGuard/CFGuard.cpp:254
+  // register (i.e. RAX on 64-bit X86 targets)
+  GuardDispatch->setCallingConv(CallingConv::CFGuard_Dispatch);
+
----------------
rnk wrote:
> So, this isn't going to work if the original calling convention was something other than the default. For x64, the only commonly used non-standard convention would be vectorcall. Changing the convention here in the IR is going to make it hard to pass that along.
> 
> I think as you can see from how much code you have here already, replacing call instructions in general is really hard. These days there are also bundles, which I guess is something else missing from the above.
> 
> Here's a sketch of an alternative approach:
> - load __guard_dispatch_icall_fptr as normal
> - get the pre-existing function type of the callee
> - cast the loaded pointer to the original function pointer type
> - use `CB->setCalledOperand` to replace the call target
> - add the original call target as an "argument bundle" to the existing instruction
> - change SelectionDAGBuilder.cpp to recognize the new bundle kind and create a new ArgListEntry in the argument list
> - make and set a new bit in ArgListEntry, something like isCFGTarget
> - change CC_X86_Win64_C to put CFGTarget arguments in RAX, similar to how CCIfNest puts things in R10
> 
> This way, the original call instruction remains with exactly the same arrangement of args, attributes, callbr successors, tail call kind, etc. All you're doing is changing the callee, and passing the original callee as an extra argument on the side. Basically, this approach leverages bundles to avoid messing with the function type, which more or less requires replacing the call. :)
> 
> There are probably issues with this design that I haven't foreseen, but I think it's worth a try.
This design seems to work well. I guess at some point we would also have to teach GlobalISel to recognize these operand bundles, right?


================
Comment at: llvm/lib/Transforms/CFGuard/CFGuard.cpp:207
+  
+  // Create a copy of the call/invoke instruction and add the new bundle.
+  CallBase *NewCB;
----------------
Is this the correct way to add operand bundles to what is essentially an existing call/invoke instruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65761