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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
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:
> > > > 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.
> cf-protection flag has several options: Branch, Return, Full and None.
> I think that if other architectures have different flavors of cf-protection then we could had these new flavors.
> Could you please give me an example of a more descriptive notation to the attribute in the IR?
> I think that if other architectures have different flavors of cf-protection then we could had these new flavors.
Again, that's assuming that the other architectures would even use attributes. Even if other architectures gain attributes for control flow protection then we can just name them after whatever the protection is called in that architecture.
> Could you please give me an example of a more descriptive notation to the attribute in the IR?
`shstk-compatible` and `ibt-compatible` seem like perfectly fine names to me.


Repository:
  rL LLVM

https://reviews.llvm.org/D40482