[PATCH] D54909: [clang][slh] add Clang attr no_speculative_load_hardening

Mon Dec 17 13:58:27 PST 2018

zbrid marked 8 inline comments as done.
zbrid added inline comments.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:3667
+
+  Warning: This attribute may not prevent Speculative Load Hardening from being
+  enabled for a function which inlines a function that has the
----------------
kristof.beyls wrote:
> aaron.ballman wrote:
> > chandlerc wrote:
> > > kristof.beyls wrote:
> > > > Hi Zola,
> > > > 
> > > > I'm afraid I don't have time for an in-depth look in the next few days,
> > > > but I wonder if you have an answer to the follow higher-level question.
> > > > I wonder why not define no_speculative_load_hardening as giving a
> > > > guarantee that speculative load hardening doesn't happen on the code.
> > > > I'm not sure if there are users of this attribute that need the
> > > > guarantee that speculative load hardening doesn't happen, but there may?
> > > > I guess that to implement such behaviour, it would require also introducing a
> > > > no_speculative_load_hardening attribute at the LLVM-IR level?
> > > > Or somehow (I don't know if that's possible) make the existing
> > > > speculative_load_hardening attribute at LLVM-IR level have 3 potential
> > > > states: "no hardening", "must harden" and "don't care".
> > > > I guess the current proposed behaviour is because that's easier to implement,
> > > > or is there another reason?
> > > > What do you think?
> > > > 
> > > > Thanks,
> > > > 
> > > > Kristof
> > > FWIW, I had suggested this design because I think it is simple, and addresses the only use case I have in mind.
> > > 
> > > That use case is essentially to remove SLH from functions that are performance sensitive and manually mitigated (if at risk at all). The intent is that people can use this based on profiling to recover some performance hit from SLH where it is safe to do so. As a consequence, the goal was not what you describe.
> > > 
> > > My thought process goes like this:
> > > 
> > > - Some routines may explicitly opt into SLH because they are at unusual risk. There is an attribute to do this. This attribute always wins: the code with the attribute is *never* allowed to be emitted w/o the hardening.
> > > - Other routines may opt *out* of SLH if it is enabled on the command line, much like routines may opt out of sanitizer instrumentation.
> > > - Everything else picks up whichever default is selected at the command line.
> > > 
> > > Now, we have to deal with what happens if the inliner tries to inline a function marked with an explicit attribute into code marked with this `no_speculative_load_hardening` attribute. I don't expect this to ever realistically occur, but we need to handle it if it does. Sadly, we can't just produce an error -- the backend may be the only thing that sees this. We could add an LLVM attribute as you suggest and block inlining in this case, but that seems very intrusive and is only designed to handle an edge case that we don't anticipate happens much if ever in practice. The final option which seems the most reasonable is what this implements: the positive attribute always wins. IMO, this is a reasonable handling of the edge case, and keeps things as simple as possible.
> > I think the current design is the right approach, but I'd appreciate seeing a code example in the documentation that more clearly shows the expectation. Something like:
> > ```
> > __attribute__((speculative_load_hardening)) int foo(int i) { return i; }
> > __attribute__((no_speculative_load_hardening)) int bar(int i) {
> >   // Note: bar() may still have speculative load hardening if foo() is
> >   // inlined into bar(). Mark foo() with __attribute__((noinline)) to
> >   // avoid this situation.
> >   return foo(i);
> > }
> > ```
> > Feel free to devise a less terrible example, too. ;-)
> > 
> > One question I have is: do we think it's worth diagnosing this slightly different case, given that the user has signaled explicitly that they want `foo()` inlined, but the call is has speculative load hardening disabled:
> > ```
> > __attribute__((speculative_load_hardening)) inline int foo(int i) { return i; }
> > __attribute__((no_speculative_load_hardening)) int bar(int i) {
> >   return foo(i); // Warning?
> > }
> > ```
> Thanks for explaining the detailed rationale.
> 
> Referring to: "Now, we have to deal with what happens if the inliner tries to inline a function marked with an explicit attribute into code marked with this no_speculative_load_hardening attribute." , which is the case of "SLH-function ---inline into---> noSLH-function".
> I'm assuming you don't call out the reverse "noSLH-function ---inline into---> SLH-function" situation as you can mark the noSLH-function with a noinline attribute if you really want to prevent SLH to be applied to the code in the noSLH-function.
> 
> Overall, I agree that it doesn't seem worthwhile to introduce the extra complexity needed for that remaining "SLH-function ---inline into----> noSLH-function" theoretical edge case.
> Or in other words, you've convinced me the design tradeoffs in this patch are right.
@aaron.ballman I will try to implement a warning like you proposed for that particular case. Still looking into it., but wanted to ack.

================
Comment at: clang/test/SemaCXX/attr-speculative-load-hardening.cpp:19

+void f4() __attribute__((no_speculative_load_hardening, speculative_load_hardening)); // expected-error {{attributes are not compatible}}
+// expected-note at -1 {{conflicting attribute is here}}
----------------
> Should this situation be diagnosed?
> 
> __attribute__((no_speculative_load_hardening))
> void func();
> 
> __attribute__((speculative_load_hardening))
> void func();
> Which attribute should "win"?
> 
> If you want to diagnose, you can use checkAttrMutualExclusion() to handle this in SemaDeclAttr.cpp.
>

@aaron.ballman 
That's a great idea. So far I've added the use of checkAttrMutualExclusion which catches the cases in this test and the three added below. I'm working on a change that will catch the case in your example. I think I'll have to make a change in Clang to check for mutual exclusion when the attributes are merged rather than where they are currently checked. I don't have all the details yet, but I think I'll send a separate patch for that once I figure out what's going on.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54909/new/

https://reviews.llvm.org/D54909