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