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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:1718-1719
 
+  void DiagnoseIgnoredNoSpeculativeLoadHardeningAttribute(const NamedDecl *,
+                                                          const Stmt *);
+
----------------
You should give these parameters identifiers to help people reading the source code understand what should be passed for each argument.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13080-13081
+void Sema::DiagnoseIgnoredNoSpeculativeLoadHardeningAttribute(
+    const NamedDecl *nd, const Stmt *body) {
+  std::deque<const Stmt *> nodes;
+  if (body) {
----------------
The identifiers don't match the usual naming conventions (here and elsewhere).


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13082
+  std::deque<const Stmt *> nodes;
+  if (body) {
+    nodes.push_back(body);
----------------
I would turn this into an early return rather than indent the entire function body one level for it.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13087-13089
+      for (const Stmt *child : node->children()) {
+        nodes.push_back(child);
+      }
----------------
`llvm::copy(Node->children(), std::back_inserter(Nodes));`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13090
+      }
+      if (auto *dre = dyn_cast<DeclRefExpr>(node)) {
+        if (auto *callee = dyn_cast<FunctionDecl>(dre->getDecl())) {
----------------
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?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13096
+                 diag::warn_no_speculative_load_hardening_may_be_ignored)
+                << nd->getName() << callee->getName();
+            Diag(callee->getLocation(), diag::note_callee_decl)
----------------
You should pass in `ND` and `Callee` directly rather than calling `getName()` on them; the diagnostic engine will handle properly quoting the name that's output as well.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13129
+    if (auto *nd = dyn_cast<NamedDecl>(dcl)) {
+      Sema::DiagnoseIgnoredNoSpeculativeLoadHardeningAttribute(nd, Body);
+    }
----------------
Why are you qualifying the call?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13294
     MD->setBody(Body);
+
     if (!MD->isInvalidDecl()) {
----------------
Spurious change?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54909