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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! The idea LGTM, and sounds like a solid way for us to do better about diagnosing FORTIFY'ed calls in Clang. I have a handful of mostly nits/questions for you :)



================
Comment at: clang/include/clang/Basic/Attr.td:3822
+def DiagnoseAs : InheritableAttr {
+  let Spellings = [Clang<"diagnose_as">];
+  let Args = [ExprArgument<"Function">,
----------------
purely subjective nit: `diagnose_as` feels a bit too generic. if you agree, does `diagnose_as_builtin` sound potentially better?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:598
+  bool UseDAIAttr = false;
+  FunctionDecl *UseDecl = FD;
+
----------------
nit: `const`


================
Comment at: clang/lib/Sema/SemaChecking.cpp:600
+
+  auto DAIAttr = FD->getAttr<DiagnoseAsAttr>();
+  if (DAIAttr) {
----------------
nit: `const auto *`


================
Comment at: clang/lib/Sema/SemaChecking.cpp:602-607
+    DeclRefExpr *F = dyn_cast_or_null<DeclRefExpr>(DAIAttr->getFunction());
+    if (!F)
+      return;
+    FunctionDecl *AttrDecl = dyn_cast_or_null<FunctionDecl>(F->getFoundDecl());
+    if (!AttrDecl)
+      return;
----------------
It seems that this is serving as implicit validation of this attribute's members (e.g., if we can't figure out the Function that this DAIAttr is pointing to, we exit gracefully, etc.)

Would it be better to instead do this validation in `handleDiagnoseAsAttr`, and store the results (which we can then presume are valid here) in the `DiagnoseAsAttr`?

In that case, it might be easiest to simply store the `FunctionDecl` that's being referenced, rather than a DRE to it.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1013
+
+    Expr *IndexExpr = AL.getArgAsExpr(I);
+    uint32_t Index;
----------------
nit: `const`?


================
Comment at: clang/test/Sema/warn-fortify-source.c:184
 
+void *test_memcpy(const void *src, size_t c, void *dst) __attribute__((diagnose_as(__builtin_memcpy, 3, 1, 2))) {
+  return __builtin_memcpy(dst, src, c);
----------------
we generally try to extensively test error cases around new attributes. i'd recommend the following cases:

- `diagnose_as(__function_that_doesnt_exist)`
- `diagnose_as(function_i_predeclared_but_which_isnt_a_builtin)`
- `diagnose_as(__builtin_memcpy, 1, 2, 3, 4)` (too many args) 
- `diagnose_as(__builtin_memcpy, 1, 2)` (too few args)
- `diagnose_as(__builtin_memcpy, "abc", 2, 3)` 
- `void test_memcpy(double, double, double) __attribute__((diagnose_as(__builtin_memcpy, 1, 2, 3));` (mismatched param types)
-  `diagnose_as(__some_overloaded_function_name)`

there're probably more that might be nice, but those seem like a good foundation :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024