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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
