[clang] cd95d79 - [Clang][Sema] Fix attribute((format)) bug on non-variadic functions
Félix Cloutier via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 6 13:09:00 PST 2022
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);
+}
+
More information about the cfe-commits
mailing list