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

[Oren Ben Simhon via Phabricator via llvm-commits]

oren_ben_simhon added inline comments.


================
Comment at: include/llvm/Target/TargetOptions.h:221
+    /// mechanisms. For example: in X86 - Indirect Branch Tracking.
+    unsigned InstrumentControlFlow : 1;
+
----------------
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?


================
Comment at: lib/Target/TargetMachine.cpp:76
   RESET_OPTION(NoTrappingFPMath, "no-trapping-math");
+  RESET_OPTION(InstrumentControlFlow, "instrument-control-flow");
 
----------------
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?



Repository:
  rL LLVM

https://reviews.llvm.org/D40482