[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5956
+given. In addition, the order in which arguments should be applied must also
+be given.
+
----------------
We should mention that the attribute cannot be applied to a member function (and if you decide it shouldn't apply to a static member function either, I'd call that out explicitly as well).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5975
+
+For variadic functions, the variadic arguments must simply come in the same
+order as they would to the builtin function, after all normal arguments. For
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5987
+Then the call `mysscanf("abc def", "%4s %4s", buf1, buf2)` will be diagnosed as
+if it were the call `mysscanf("abc def", "%4s %4s", buf1, buf2)`.
+}];
----------------



================
Comment at: clang/lib/AST/Decl.cpp:2021
       redeclarable_base(C) {
+
   static_assert(sizeof(VarDeclBitfields) <= sizeof(unsigned),
----------------
Spurious whitespace change can be reverted.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1006-1009
+  const auto *DeclFD = dyn_cast<FunctionDecl>(D);
+  if (!DeclFD)
+    // User will have already been warned.
+    return;
----------------
Sorry, I should have suggested this earlier -- might as well assert if `D` is something we should have already validated is a `FunctionDecl` before calling this helper.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1011-1014
+  if (isa<CXXMethodDecl>(DeclFD)) {
+    S.Diag(AL.getLoc(), diag::err_attribute_no_member_function) << AL;
+    return;
+  }
----------------
Hmm, this is getting into real nit territory, but, shouldn't it be fine to use a static member function?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1021-1022
+        return Union.get<Expr *>()->getBeginLoc();
+      else
+        return Union.get<IdentifierLoc *>()->Loc;
+    }();
----------------
Sorry, missed this one earlier as well (no `else` after a `return` per our usual coding guidelines).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1059-1061
+    if (!checkUInt32Argument(S, AL, IndexExpr, Index, I + 1, false)) {
+      return;
+    }
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1065
+      S.Diag(AL.getLoc(), diag::err_attribute_bounds_for_function)
+          << AL << Index << DeclFD->getName() << DeclFD->getNumParams();
+      return;
----------------



================
Comment at: clang/test/Sema/attr-diagnose-as-builtin.c:104
+struct S {
+  __attribute__((diagnose_as_builtin(__builtin_memset))) void f(); // expected-error{{'diagnose_as_builtin' attribute only applies to non-member functions}}
+};
----------------
It'd be good to add another test case for static member functions just to show whether we do or do not support them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024