[PATCH] D40482: [X86] Instrument Control Flow For Indirect Branch Tracking

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: include/llvm/Target/TargetOptions.h:221
+    /// mechanisms. For example: in X86 - Indirect Branch Tracking.
+    unsigned InstrumentControlFlow : 1;
+
----------------
oren_ben_simhon wrote:
> pcc wrote:
> > oren_ben_simhon wrote:
> > > pcc wrote:
> > > > This doesn't seem like it needs to be a TargetOption. Can you test for the function attribute directly in your pass?
> > > Sure, I will add a check in the pass for the function attribute. 
> > > Still, I would like the user to be able to give the llc the command option "instrument-control-flow" so that it will be enabled even if the attribute is not set in the function.
> > > Is there a better way to do that than TargetOption?
> > Why do you need this capability?
> To run the control flow pass in LLC for example for debug purposes.
> Do you suggest to add it only to my pass? but then the flag will not be global for all architectures. right?
Please add it to your pass. If it turns out that we need it on other architectures, we can always move it.


================
Comment at: lib/Target/TargetMachine.cpp:76
   RESET_OPTION(NoTrappingFPMath, "no-trapping-math");
+  RESET_OPTION(InstrumentControlFlow, "instrument-control-flow");
 
----------------
oren_ben_simhon wrote:
> pcc wrote:
> > oren_ben_simhon wrote:
> > > pcc wrote:
> > > > This name seems very generic and doesn't really describe what the attribute does. How about something like `cet-compatible`?
> > > I wouldn't like this attribute to be target independent.  cet-compatible refers to Intel specific way to protect against control flow attacks. Does "cf-protection" make more sense?
> > I assume that by "wouldn't" you meant "would".
> > 
> > I'm afraid that the name "cf-protection" doesn't really make sense to me either. There are a variety of ways to protect against control flow attacks, both purely software based (e.g. `-fsanitize=cfi`) and using hardware features such as CET, and specifically in the case of CET there also needs to be support from the rest of the operating system (so a user shouldn't expect this feature to provide any protection unless their hardware and operating system specifically supports it). So I think it would be less confusing if the attribute refers to the specific technology used.
> First, I am sorry, peter, for the delayed response. I was off for couple of days.
> You are correct, I meant would.
> 
> I believe that it would be good to stay in sync with GCC and ICC by using the same target-independent flag name of -cf-protection.
> 
> I believe it is good to make a target independent flags for SW based solution (like-fsantize=..) and HW based solution (like -cf-protection). Does this make sense or i am missing something?
> 
I don't think it makes sense to have a target-independent attribute for control flow protection, even just for hardware-based. The software-based solutions in Clang involve adding checks at the IR generation level, so it does not require attributes. The mechanism that you are introducing works at the backend level, so it requires an attribute in order to communicate with the backend. We can already see that there is variation between software and hardware, so I think it is premature to assume that all hardware-based control flow protections can be implemented using a single attribute.

And irrespective of what we end up calling the flag on the Clang side, we can at least give it a more descriptive name in IR.


Repository:
  rL LLVM

https://reviews.llvm.org/D40482