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