[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:5973
+``mymemset(n, c, s)`` will diagnose overflows as if it were the call
+``__builtin_memset(s, c, n)``;
+}];
----------------
One thing that's not quite clear from the docs is how to handle variadic arg builtins like `__builtin_printf` -- how do you specify "the variadic args go here"?

Another thing that's unclear is whether you can apply this attribute to a member function, and if so, how does that change the indexes given for the implicit `this` argument?

We should add test coverage for both of these situations.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:673
 
-  auto ComputeExplicitObjectSizeArgument =
+  const auto TranslateIndex = [&](unsigned Index) -> Optional<unsigned> {
+    if (UseDAIAttr) {
----------------
We typically don't use top-level `const` on value types for local variables/parameters (only on pointers or references). (same comment applies elsewhere)


================
Comment at: clang/lib/Sema/SemaChecking.cpp:692
       return llvm::None;
-    return Result.Val.getInt();
+    auto Integer = Result.Val.getInt();
+    Integer.setIsUnsigned(true);
----------------
Please spell out the type.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1006
+                                        const ParsedAttr &AL) {
+  const auto *DeclFD = dyn_cast_or_null<FunctionDecl>(D);
+  if (!DeclFD)
----------------
I don't believe `D` can ever be null.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1014-1018
+      if (Union.is<Expr *>()) {
+        return Union.get<Expr *>()->getBeginLoc();
+      } else {
+        return Union.get<IdentifierLoc *>()->Loc;
+      }
----------------
Removes curly braces and an `else` after a `return`.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1040
+    S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments_for)
+        << AL << AttrFD->getName() << AttrFD->getNumParams();
+    return;
----------------
Passing in a `NamedDecl` will be automatically quoted by the diagnostics engine, while passing in a `StringRef` will not. We prefer syntax to be quoted in diagnostics to reduce confusion.


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



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



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1065-1066
+
+    const auto T1 = AttrFD->getParamDecl(I - 1)->getType();
+    const auto T2 = DeclFD->getParamDecl(Index - 1)->getType();
+
----------------
Please spell out the type as it's not spelled out in the initialization.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1068-1069
+
+    if (T1.getCanonicalType().getUnqualifiedType() !=
+        T2.getCanonicalType().getUnqualifiedType()) {
+      S.Diag(IndexExpr->getBeginLoc(), diag::err_attribute_parameter_types)
----------------
This may be fine, but it may also be overly restrictive. Consider: 
```
void f(void *dest, char val, size_t n) __attribute__((diagnose_as_builtin(__builtin_memset, 1, 2, 3))) {}
```
I think this will get diagnosed because of the type mismatch between `char` and `int`, won't it? But notionally, the argument passed to `val` will be promoted to `int` anyway, so they're morally equivalent. This would be a good test case to add.

FWIW, I prefer erring on the side of being overly restrictive (we can relax restrictions more easily than we can add new ones), which is why I think this is fine for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024