[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/Attr.td:3822
+def DiagnoseAs : InheritableAttr {
+  let Spellings = [Clang<"diagnose_as">];
+  let Args = [ExprArgument<"Function">,
----------------
george.burgess.iv wrote:
> purely subjective nit: `diagnose_as` feels a bit too generic. if you agree, does `diagnose_as_builtin` sound potentially better?
Agreed on it being a bit generic -- it sounds like this is only useful for Fortify, so I wonder if I'm wrong about that or whether we should name it `fortify_diagnose_as_builtin` or `fortify_diagnostic`, etc.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5936-5937
+applied to the called function as if it were the function specified by the
+attribute. The function whose diagnostics to mimic as well as the correct order
+of arguments must be specified.
+
----------------
I think we should go into more detail about what the correct order of arguments actually means. The example doesn't make it particularly clear what's going on.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:602-605
+    DeclRefExpr *F = dyn_cast_or_null<DeclRefExpr>(DAIAttr->getFunction());
+    if (!F)
+      return;
+    FunctionDecl *AttrDecl = dyn_cast_or_null<FunctionDecl>(F->getFoundDecl());
----------------
george.burgess.iv wrote:
> 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/SemaChecking.cpp:653
 
-    const Expr *ObjArg = TheCall->getArg(Index);
+    auto IndexOptional = TranslateIndex(Index);
+    if (!IndexOptional)
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:668
   auto ComputeStrLenArgument = [&](unsigned Index) -> Optional<llvm::APSInt> {
-    Expr *ObjArg = TheCall->getArg(Index);
+    auto IndexOptional = TranslateIndex(Index);
+    if (!IndexOptional)
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1018-1019
+    if (!checkUInt32Argument(S, AL, IndexExpr, Index, UINT_MAX, false)) {
+      S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
+          << AL << I << IndexExpr->getSourceRange();
+      return;
----------------
This diagnostic looks incorrect to me -- the call to `checkUInt32Argument()` diagnoses if the expression is not an appropriately-sized integer. This diagnostic is about the value received being out of bounds.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1026-1030
+  if (!AL.isArgExpr(0)) {
+    S.Diag(AL.getLoc(), diag::err_attribute_argument_type)
+        << AL << AANT_ArgumentConstantExpr;
+    return;
+  }
----------------
This also seems a bit wrong -- we don't expect a constant expression, we expect a builtin function ID. You may need to use a new diagnostic here instead of reusing an existing one. Also, we should check that we got an actual builtin function as the expression, shouldn't we?


================
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);
----------------
george.burgess.iv wrote:
> 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 :)
Additional tests:

`diagnose_as(some_templated_function, 1, 2, 3)`
`diagnose_as(some_templated_function_instantiation<int>, 1, 2, 3)`
`diagnose_as("123", 1, 2, 3)`

Also interesting to test is redeclaration behavior:
```
void my_awesome_func(const char *);

void use1() {
  my_awesome_func(0);
}

[[clang::diagnose_as(__builtin_strlen, 1)]] void my_awesome_func(const char *str) {}

void use2() {
  my_awesome_func(0);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024