Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 :)
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3834 | purely subjective nit: diagnose_as feels a bit too generic. if you agree, does diagnose_as_builtin sound potentially better? | |
clang/lib/Sema/SemaChecking.cpp | ||
656 | nit: const | |
658 | nit: const auto * | |
660–665 | 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. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
1013 | nit: const? | |
clang/test/Sema/warn-fortify-source.c | ||
184 ↗ | (On Diff #380482) | we generally try to extensively test error cases around new attributes. i'd recommend the following cases:
there're probably more that might be nice, but those seem like a good foundation :) |
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3834 | 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. | |
clang/include/clang/Basic/AttrDocs.td | ||
5954–5955 | 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. | |
clang/lib/Sema/SemaChecking.cpp | ||
660–663 | ||
710–715 | ||
724–731 | ||
clang/lib/Sema/SemaDeclAttr.cpp | ||
1018–1019 | 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. | |
1026–1030 | 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? | |
clang/test/Sema/warn-fortify-source.c | ||
184 ↗ | (On Diff #380482) | Additional tests: diagnose_as(some_templated_function, 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); } |
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3834 | (Jumping in a bit late) I second the diagnose_as_builtin name. But then we should check that the attribute is only set on inline builtin declaration (as in Decl::isInlineBuiltinDeclaration) and state so in the documentation. |
Renamed to diagnose_as_builtin (since two people suggested that name). Let me
know if a name mentioning fortify is preferred.
Validation in handleDiagnoseAsBuiltinAttr:
- first argument is a builtin function
- number of arguments matches number of parameters of that function
- subsequent arguments are integers
- indices are in bounds for the parameters of the declared function
- types match for declared function and builtin function
Added diagnostics for the above errors.
Added some consts.
Moved tests to new file attr-diagnose-as-builtin.c.
Added many test cases.
Fixed diagnostic around checkUInt32Argument.
Clarified documentation.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3834 | I think Decl::isInlineBuiltinDeclaration may be the wrong thing to check; it fails for some builtin functions. Instead I'm checking against getBuiltinID. Let me know if you feel this is incorrect. |
Would @george.burgess.iv and/or @aaron.ballman mind taking another look at the latest revision?
LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :)
Thanks again!
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
2961 | tiny nit: for consistency with the other options here | |
clang/test/Sema/attr-diagnose-as-builtin.c | ||
63 | nit: can we also add a non-templated overload check in here? if the diag isn't beautiful, that's fine IMO. just having a test-case to show the expected behavior would be nice |
clang/test/Sema/attr-diagnose-as-builtin.c | ||
---|---|---|
63 | Sorry, not sure what is being requested. By a "non-templated overload check" do you mean something different from memcpy2 above? |
s/requires/takes in diagnostic message.
Test case with a non-template overloaded function.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
5973 | 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. | |
clang/lib/Sema/SemaChecking.cpp | ||
673 | We typically don't use top-level const on value types for local variables/parameters (only on pointers or references). (same comment applies elsewhere) | |
692 | Please spell out the type. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
1006 | I don't believe D can ever be null. | |
1014–1018 | Removes curly braces and an else after a return. | |
1040 | 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. | |
1055–1057 | ||
1061 | ||
1065–1066 | Please spell out the type as it's not spelled out in the initialization. | |
1068–1069 | 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. |
Error on member function. Test case for this.
Test case for attribute applied to a non-function declaration.
Handle variadic functions. Document this. Test case for variadic functions.
Remove const from some local values.
Spell out some types.
Change superflous dyn_cast_or_null to dyn_cast.
Remove some curly braces.
Pass in a NamedDecl directly rather than its name to a diagnostic.
Add a test case for types not being identical.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
5956 | We should mention that the attribute cannot be applied to a member function (and if you decide it shouldn't apply to a static member function either, I'd call that out explicitly as well). | |
5975 | ||
5987 | ||
clang/lib/AST/Decl.cpp | ||
2021 ↗ | (On Diff #389355) | Spurious whitespace change can be reverted. |
clang/lib/Sema/SemaDeclAttr.cpp | ||
1006–1009 | Sorry, I should have suggested this earlier -- might as well assert if D is something we should have already validated is a FunctionDecl before calling this helper. | |
1011–1014 | Hmm, this is getting into real nit territory, but, shouldn't it be fine to use a static member function? | |
1021–1022 | Sorry, missed this one earlier as well (no else after a return per our usual coding guidelines). | |
1059–1061 | ||
1065 | ||
clang/test/Sema/attr-diagnose-as-builtin.c | ||
105 | It'd be good to add another test case for static member functions just to show whether we do or do not support them. |
Revert spurious whitespace change.
assert(D)
No else after return.
Allow attribute to be applied to static member functions and test this.
Document that it can't be applied to a non-static member function.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3838 | Why does this need to be late parsed? I don't think the arguments to the attribute are ones that name class members or anything like that. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
1006–1009 | Sorry, my suggestion wasn't as clear as it could have been -- cast<> does the assertion for you, so you can switch dyn_cast to cast and remove the explicit assert here. |
@aaron.ballman Look to you like this is ready to land?
EDIT: oops sorry, missed your latest comments, will address them shortly.
Thanks for catching those! FWIW, I think we're close, but precommit CI is failing: https://reviews.llvm.org/harbormaster/unit/view/1534779/ In fact, LGTM aside from that bit.
purely subjective nit: diagnose_as feels a bit too generic. if you agree, does diagnose_as_builtin sound potentially better?