Author: Félix Cloutier Date: 2022-12-06T13:08:18-08:00 New Revision: cd95d7998c1dd30c6353aeca2686697287cb0443
URL: https://github.com/llvm/llvm-project/commit/cd95d7998c1dd30c6353aeca2686697287cb0443 DIFF: https://github.com/llvm/llvm-project/commit/cd95d7998c1dd30c6353aeca2686697287cb0443.diff LOG: [Clang][Sema] Fix attribute((format)) bug on non-variadic functions The [initial implementation][1] of __attribute__((format)) on non-variadic functions accidentally only accepted one data argument. This worked: ```c __attribute__((format(printf, 1, 2))) void f(const char *, int); ``` but this didn't: ```c __attribute__((format(printf, 1, 2))) void f(const char *, int, int); ``` This is due to an oversight in changing the way diagnostics are emitted for `attribute((format))`, and to a coincidence in the handling of the variadic case. Test cases only covered the case that worked by coincidence. Before the previous change, using `__attribute__((format))` on a non-variadic function at all was an error and clang bailed out. After that change, it only generates a GCC compatibility warning. However, as execution falls through, it hits a second diagnostic when the first data argument is neither 0 nor the last parameter of the function. This change updates that check to allow any parameter after the format string to be the first data argument when the function is non-variadic. When the function is variadic, it still needs to be the index of the `...` "parameter". Attribute documentation is updated to reflect the change and new tests are added to verify that it works with _two_ data parameters. [1]: https://reviews.llvm.org/D112579 Radar-Id: rdar://102069446 Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D137603 Added: clang/test/FixIt/attr-format.c Modified: clang/docs/ReleaseNotes.rst clang/include/clang/Basic/AttrDocs.td clang/lib/Sema/SemaDeclAttr.cpp clang/test/Sema/attr-format.c Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index adce36bb92363..e3a395481b8c3 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -319,6 +319,8 @@ Bug Fixes `Issue 58302 <https://github.com/llvm/llvm-project/issues/58302>`_ `Issue 58753 <https://github.com/llvm/llvm-project/issues/58753>`_ `Issue 59100 <https://github.com/llvm/llvm-project/issues/59100>`_ +- Fix issue using __attribute__((format)) on non-variadic functions that expect + more than one formatted argument. Improvements to Clang's diagnostics ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td index 40e1dd9cca5c1..a5ea3915a94d0 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -3191,6 +3191,18 @@ the function signature. For example: fmt(fmt, "hello", 123); // warning: format string is not a string literal } +When using the format attribute on a variadic function, the first data parameter +_must_ be the index of the ellipsis in the parameter list. Clang will generate +a diagnostic otherwise, as it wouldn't be possible to forward that argument list +to `printf`-family functions. For instance, this is an error: + +.. code-block:: c + + __attribute__((__format__(__printf__, 1, 2))) + void fmt(const char *s, int b, ...); + // ^ error: format attribute parameter 3 is out of bounds + // (must be __printf__, 1, 3) + Using the ``format`` attribute on a non-variadic function emits a GCC compatibility diagnostic. }]; diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 4032e96f0f6b5..dcddf4c910738 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -3891,27 +3891,38 @@ static void handleFormatAttr(Sema &S, Decl *D, const ParsedAttr &AL) { if (!checkUInt32Argument(S, AL, FirstArgExpr, FirstArg, 3)) return; - // check if the function is variadic if the 3rd argument non-zero + // FirstArg == 0 is is always valid. if (FirstArg != 0) { - if (isFunctionOrMethodVariadic(D)) - ++NumArgs; // +1 for ... - else - S.Diag(D->getLocation(), diag::warn_gcc_requires_variadic_function) << AL; - } - - // strftime requires FirstArg to be 0 because it doesn't read from any - // variable the input is just the current time + the format string. - if (Kind == StrftimeFormat) { - if (FirstArg != 0) { + if (Kind == StrftimeFormat) { + // If the kind is strftime, FirstArg must be 0 because strftime does not + // use any variadic arguments. S.Diag(AL.getLoc(), diag::err_format_strftime_third_parameter) - << FirstArgExpr->getSourceRange(); - return; + << FirstArgExpr->getSourceRange() + << FixItHint::CreateReplacement(FirstArgExpr->getSourceRange(), "0"); + return; + } else if (isFunctionOrMethodVariadic(D)) { + // Else, if the function is variadic, then FirstArg must be 0 or the + // "position" of the ... parameter. It's unusual to use 0 with variadic + // functions, so the fixit proposes the latter. + if (FirstArg != NumArgs + 1) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds) + << AL << 3 << FirstArgExpr->getSourceRange() + << FixItHint::CreateReplacement(FirstArgExpr->getSourceRange(), + std::to_string(NumArgs + 1)); + return; + } + } else { + // Inescapable GCC compatibility diagnostic. + S.Diag(D->getLocation(), diag::warn_gcc_requires_variadic_function) << AL; + if (FirstArg <= Idx) { + // Else, the function is not variadic, and FirstArg must be 0 or any + // parameter after the format parameter. We don't offer a fixit because + // there are too many possible good values. + S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds) + << AL << 3 << FirstArgExpr->getSourceRange(); + return; + } } - // if 0 it disables parameter checking (to use with e.g. va_list) - } else if (FirstArg != 0 && FirstArg != NumArgs) { - S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds) - << AL << 3 << FirstArgExpr->getSourceRange(); - return; } FormatAttr *NewAttr = S.mergeFormatAttr(D, AL, II, Idx, FirstArg); diff --git a/clang/test/FixIt/attr-format.c b/clang/test/FixIt/attr-format.c new file mode 100644 index 0000000000000..b2c1f75dad506 --- /dev/null +++ b/clang/test/FixIt/attr-format.c @@ -0,0 +1,9 @@ +// RUN: not %clang_cc1 %s -fsyntax-only -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s + +// CHECK: fix-it:"{{.*}}attr-format.c":{4:36-4:37}:"0" +__attribute__((format(strftime, 1, 1))) +void my_strftime(const char *fmt); + +// CHECK: fix-it:"{{.*}}attr-format.c":{8:34-8:36}:"2" +__attribute__((format(printf, 1, 10))) +void my_strftime(const char *fmt, ...); diff --git a/clang/test/Sema/attr-format.c b/clang/test/Sema/attr-format.c index 32f0bc6fe9539..d891828a77a4c 100644 --- a/clang/test/Sema/attr-format.c +++ b/clang/test/Sema/attr-format.c @@ -7,10 +7,14 @@ void b(const char *a, ...) __attribute__((format(printf, 1, 1))); // expected void c(const char *a, ...) __attribute__((format(printf, 0, 2))); // expected-error {{'format' attribute parameter 2 is out of bounds}} void d(const char *a, int c) __attribute__((format(printf, 1, 2))); // expected-warning {{GCC requires a function with the 'format' attribute to be variadic}} void e(char *str, int c, ...) __attribute__((format(printf, 2, 3))); // expected-error {{format argument not a string type}} +void f(int a, const char *b, ...) __attribute__((format(printf, 2, 1))); // expected-error {{'format' attribute parameter 3 is out of bounds}} +void g(int a, const char *b, ...) __attribute__((format(printf, 2, 2))); // expected-error {{'format' attribute parameter 3 is out of bounds}} +void h(int a, const char *b, ...) __attribute__((format(printf, 2, 3))); // no-error +void i(const char *a, int b, ...) __attribute__((format(printf, 1, 2))); // expected-error {{'format' attribute parameter 3 is out of bounds}} typedef const char *xpto; -void f(xpto c, va_list list) __attribute__((format(printf, 1, 0))); // no-error -void g(xpto c) __attribute__((format(printf, 1, 0))); // no-error +void j(xpto c, va_list list) __attribute__((format(printf, 1, 0))); // no-error +void k(xpto c) __attribute__((format(printf, 1, 0))); // no-error void y(char *str) __attribute__((format(strftime, 1, 0))); // no-error void z(char *str, int c, ...) __attribute__((format(strftime, 1, 2))); // expected-error {{strftime format attribute requires 3rd parameter to be 0}} @@ -94,3 +98,9 @@ __attribute__((format(printf, 1, 2))) void forward_fixed(const char *fmt, int i) forward_fixed(fmt, i); a(fmt, i); } + +__attribute__((format(printf, 1, 2))) void forward_fixed_2(const char *fmt, int i, int j) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}} + forward_fixed_2(fmt, i, j); + a(fmt, i); +} + _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
