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

[Zola Bridges via Phabricator via llvm-commits]

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


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13090
+      }
+      if (auto *dre = dyn_cast<DeclRefExpr>(node)) {
+        if (auto *callee = dyn_cast<FunctionDecl>(dre->getDecl())) {
----------------
aaron.ballman wrote:
> Should be `const auto *` (same below) rather than inferring the `const` qualification.
> 
> Moreover, this seems incorrect -- you don't mean any declref to an attributed function, do you? I would have imagined that only call expressions matter. e.g., this should not diagnose:
> ```
> [[clang::speculative_load_hardening]] inline int bar(int x) { return x; }
> 
> [[clang::no_speculative_load_hardening]] int foo() {
>   if (bar) {  // No reason to warn about speculative load hardening because there's no call.
>   }
>   return sizeof(bar(0)); // Unevaluated expression probably doesn't need a warning either.
> }
> ```
> I'm a bit concerned about the performance cost of touching every child AST node in the function body. I wonder if it would be possible to check `CallExpr`s as they're handled instead? I'm not certain if there's a way to get from the `Expr` to the context in which the expression appears so that we can see if the expression is within a function with speculative load hardening disabled or not, so this may be a dead-end idea.
> 
> If it turns out that handling it at the `CallExpr` site isn't feasible, then I think we should probably revert this diagnostic and go with the previous version of the patch. We can take another swing at a diagnostic if we find that people actually hit this corner case often enough to warrant the extra expense of the check (or we can solve it another way, through clang-tidy or the static analyzer). WDYT?
I looked into handling this at the CallExpr site and it looks like the context needed for this diagnostic is available. I'll update the patch when I get that version working. That suggestion makes a lot of sense to me.

As for checking every DeclRefExpr, that was due to my misunderstanding of the AST. Given what you said, I think what I really want are the DeclRefExpr that are children of CallExpr. That'll be reflected in the updated patch.

I'll keep all these style comments in mind when making the update too.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13294
     MD->setBody(Body);
+
     if (!MD->isInvalidDecl()) {
----------------
aaron.ballman wrote:
> Spurious change?
Yes, I'll revert that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54909