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