This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] ignore builtin varargs from pro-type-vararg-check
ClosedPublic

Authored by njames93 on May 31 2020, 3:44 AM.

Diff Detail

Event Timeline

njames93 created this revision.May 31 2020, 3:44 AM
aaron.ballman added inline comments.May 31 2020, 6:39 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
21

How would we know which builtins should or should not trigger this? Can we add that information to this comment (or, better yet, just fix the fixme up front)?

njames93 marked an inline comment as done.May 31 2020, 7:40 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
21

Good point, its more a case that I don't know all the variadic builtins clang supports. Those ones I included are just the ones you find in the gcc documentation. Shall I just remove the Fixme completely, If there is another one that's been missed we can address that later

njames93 updated this revision to Diff 267497.May 31 2020, 7:44 AM

Remove unnecessary fixme.

aaron.ballman added inline comments.May 31 2020, 9:14 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
21

Taking a look at Builtins.def shows quite a few more that likely should be added to the list. I think we should probably have the initial commit covering anything that's a C standard library function/macro that ends with "." in its builtin type signature where the C API isn't variadic. For instance, __builtin_isfinite, __builtin_isinf, etc. I'm fine if we don't vet every builtin we support (that's a large amount of work), but we should be able to cover the most common cases where there's a specification to compare against.

njames93 marked an inline comment as done.May 31 2020, 9:54 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
21
BUILTIN(__builtin_isgreater     , "i.", "Fnct")
BUILTIN(__builtin_isgreaterequal, "i.", "Fnct")
BUILTIN(__builtin_isless        , "i.", "Fnct")
BUILTIN(__builtin_islessequal   , "i.", "Fnct")
BUILTIN(__builtin_islessgreater , "i.", "Fnct")
BUILTIN(__builtin_isunordered   , "i.", "Fnct")
BUILTIN(__builtin_fpclassify, "iiiiii.", "Fnct")
BUILTIN(__builtin_isfinite,   "i.", "Fnct")
BUILTIN(__builtin_isinf,      "i.", "Fnct")
BUILTIN(__builtin_isinf_sign, "i.", "Fnct")
BUILTIN(__builtin_isnan,      "i.", "Fnct")
BUILTIN(__builtin_isnormal,   "i.", "Fnct")
BUILTIN(__builtin_signbit, "i.", "Fnct")
BUILTIN(__builtin_constant_p, "i.", "nctu")
BUILTIN(__builtin_classify_type, "i.", "nctu")
BUILTIN(__builtin_va_start, "vA.", "nt")
BUILTIN(__builtin_stdarg_start, "vA.", "nt")
BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")
BUILTIN(__builtin_printf, "icC*.", "Fp:0:")
BUILTIN(__builtin_snprintf, "ic*zcC*.", "nFp:2:")
BUILTIN(__builtin___snprintf_chk, "ic*zizcC*.", "Fp:4:")
BUILTIN(__builtin___sprintf_chk, "ic*izcC*.", "Fp:3:")
BUILTIN(__builtin___fprintf_chk, "iP*icC*.", "Fp:2:")
BUILTIN(__builtin___printf_chk, "iicC*.", "Fp:1:")
BUILTIN(__builtin_prefetch, "vvC*.", "nc")
BUILTIN(__builtin_shufflevector, "v."   , "nct")
BUILTIN(__builtin_convertvector, "v."   , "nct")
BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
BUILTIN(__builtin_annotation, "v.", "tn")
BUILTIN(__builtin_add_overflow, "b.", "nt")
BUILTIN(__builtin_sub_overflow, "b.", "nt")
BUILTIN(__builtin_mul_overflow, "b.", "nt")
BUILTIN(__builtin_preserve_access_index, "v.", "t")
BUILTIN(__builtin_nontemporal_store, "v.", "t")
BUILTIN(__builtin_nontemporal_load, "v.", "t")
BUILTIN(__builtin_os_log_format_buffer_size, "zcC*.", "p:0:nut")
BUILTIN(__builtin_os_log_format, "v*v*cC*.", "p:0:nt")
BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")

That's all the variadics inside Builtin.def that are called __builtin....
Would you suggest including all those apart from the va, print or format related ones

aaron.ballman added inline comments.Jun 2 2020, 3:41 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
21

I would include all of those that are not defined as being variadic in the C, C++, or POSIX standards. So things like __builtin_isgreater make sense to include, but __builtin_isinf_sign is a bit less clear to me. So a large part of that list seems like stuff we'd want to include, but not all of it.

njames93 updated this revision to Diff 268116.Jun 3 2020, 3:09 AM

Included more builtins.

aaron.ballman added inline comments.Jun 3 2020, 4:34 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
39

I think we may want to keep this one as it's the builtin for the va_start macro, which is not variadic.

40

Should remove commented-out code from the list.

41

If it's documented as being variadic, I think we want to remove it from the list so we warn on its use, right?

From here on down look like things that are not part of any standard, also, so we may want to remove them based on that, too.

49

Same here.

63

Except for this one -- I think we may want to keep this one because it's the MS version of va_start, which is not variadic.

njames93 updated this revision to Diff 268149.Jun 3 2020, 4:58 AM
njames93 marked 3 inline comments as done.

Add va_start.

njames93 added inline comments.Jun 3 2020, 5:10 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
40

Once we are happy I will remove it, more a temporary step.

41

This function should have this signature if default arguments were allowed.

extern void* __builtin_assume_aligned(const void* exp, size_t align, size_t misaligned = 0);
49

Likewise this one should have this signature

extern void ____builtin_prefetch(const void* addr, bool rw = false, unsigned locality = 3);
aaron.ballman added inline comments.Jun 3 2020, 5:35 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
40

SGTM!

41

Ah, I see now. Makes sense to keep it, but maybe update the comment as "documented as variadic to support default parameters".

If you've vetted the other builtins below to see that they're not actually variadic, I'm fine leaving them in the list as, btw.

njames93 marked an inline comment as done.Jun 3 2020, 8:09 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
39

Allowing va_start makes sense, but its breaking the current test cases. According to the documentation that is expected behaviour, So shall I remove __builtin_va_start again?

aaron.ballman added inline comments.Jun 3 2020, 8:15 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
39

I think that test case is wrong and should be changed, tbh. The point to the rule is to not call variadic functions. There's a different rule about not defining a variadic interface, so that warning should be do not use va_start/va_arg to define c-style vararg functions; use variadic templates instead, not this one.

njames93 updated this revision to Diff 268256.Jun 3 2020, 10:40 AM
  • Detect va_list type VarDecls to warn on
njames93 updated this revision to Diff 268378.Jun 4 2020, 12:41 AM

Hopefully fixed windows buildn

aaron.ballman accepted this revision.Jun 4 2020, 8:41 AM

LGTM, thank you for the fixes!

This revision is now accepted and ready to land.Jun 4 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.