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

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 17 16:40:07 PDT 2019


rnk added a comment.

I think this looks pretty good, thanks! I really only noticed style nits. I think with some small fixes, we should go ahead and merge it. If you want, I can commit it on your behalf, but I know there are other folks at Microsoft with commit access to LLVM, if you'd prefer. When you upload the next diff, make sure to use the merge point with origin/master as the base, so that the patch will apply cleanly.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5959-5960
       CmdArgs.push_back("-cfguard");
-    else if (NoChecks)
-      //Emit only the table of address-taken functions.
+    }
+    else if (GuardArgs.equals_lower("cf,nochecks")) {
+      // Emit only the table of address-taken functions.
----------------
Style nit: LLVM puts the close bracket on the same line as else pretty consistently. clang-format will make it so.


================
Comment at: llvm/lib/CodeGen/CFGuardLongjmp.cpp:101
+  // targets.
+  for (MachineInstr *Setjmp : SetjmpCalls) {
+    MachineBasicBlock *OldMBB = Setjmp->getParent();
----------------
ajpaverd wrote:
> 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? 
I guess I was thinking that createTempSymbol would work, but I see you are taking steps to make these labels public, so that one is not appropriate. If the name matters, then yes, I think getOrCreateSymbol is the right API.


================
Comment at: llvm/lib/Transforms/CFGuard/CFGuard.cpp:220
+    GuardDispatch =
+        B.CreateCall(GuardDispatchType, GuardDispatchLoad, GuardDispatchArgs);
+  } else if (InvokeInst::classof(CB)) {
----------------
rnk wrote:
> You'd want to set the tail call kind here to forward `tail` or `musttail`. This matters for things like member pointer function thunks. This C++ makes a musttail thunk, for example:
>   struct A { virtual void f(); };
>   auto mp = &A::f;
> Which gives this IR:
>   define linkonce_odr void @"??_9A@@$BA at AA"(%struct.A* %this, ...) #0 comdat align 2 {
>   entry:
>     %0 = bitcast %struct.A* %this to void (%struct.A*, ...)***
>     %vtable = load void (%struct.A*, ...)**, void (%struct.A*, ...)*** %0, align 8, !tbaa !3
>     %1 = load void (%struct.A*, ...)*, void (%struct.A*, ...)** %vtable, align 8
>     musttail call void (%struct.A*, ...) %1(%struct.A* %this, ...)
>     ret void
>   }
> This does an indirect virtual call through the 0th slot, and if we lose musttail, it won't perfectly forward arguments.
> 
Thanks, I see the test case for this.


================
Comment at: llvm/lib/Transforms/CFGuard/CFGuard.cpp:254
+  // register (i.e. RAX on 64-bit X86 targets)
+  GuardDispatch->setCallingConv(CallingConv::CFGuard_Dispatch);
+
----------------
ajpaverd wrote:
> 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?
Yep. I'm not sure what the status of x86 global isel is, honestly. I guess I'll find out more at the dev meeting.


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


================
Comment at: llvm/test/CodeGen/X86/cfguard-checks.ll:127
+
+define double @func_cf_doubles() {
+entry:
----------------
I think it would be interesting to have another test that exercises x86_vectorcallcc, since that's something the bundle approach enables. I guess this is good C++ to start from:
```
struct HVA {
  double x, y, z, w;
};
void foo(void (__vectorcall *fp)(HVA), HVA *o) {
  fp(*o);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65761





More information about the llvm-commits mailing list