[Clang][Sema] Fix attribute((format)) bug on non-variadic functions

[fcloutier]

Summary

The initial implementation of __attribute__((format)) on non-variadic functions accidentally only accepted one data argument. This worked:

__attribute__((format(printf, 1, 2)))
void f(const char *, int);

but this didn't:

__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.

rdar://102069446

[aaron.ballman]

LGTM! This should probably have a release note as well.

clang/lib/Sema/SemaDeclAttr.cpp

3896–3908

Given that we know what value we want to see there, should we emit a FixIt in these cases to adjust the format arg to the right value? (Can be done in a follow-up if you feel like doing it, I don't insist.)

[fcloutier]

Addressed Aaron's comment about fixits, ran clang-format.

[aaron.ballman]

LGTM, thank you!

clang/docs/ReleaseNotes.rst

319

[fcloutier]

Fix new fixit test for Windows, where directory separators are backslashes instead of forward slashes.

clang/docs/ReleaseNotes.rst

- Fix bug with using enum that could lead to enumerators being treated as if
   `Issue 58067 <https://github.com/llvm/llvm-project/issues/58057>`_
   `Issue 59014 <https://github.com/llvm/llvm-project/issues/59014>`_
   `Issue 54746 <https://github.com/llvm/llvm-project/issues/54746>`_
 - Fix assert that triggers a crash during some types of list initialization that
   generate a CXXTemporaryObjectExpr instead of a InitListExpr. This 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/include/clang/Basic/AttrDocs.td

.. code-block:: c
 
   void bar(void) {
     fmt("%s %i", "hello", 123); // OK
     fmt("%i %g", "hello", 123); // warning: arguments don't match format
     extern const char *fmt;
     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.
   }];
 }
 
 def AlignValueDocs : Documentation {
   let Category = DocCatType;
   let Content = [{

clang/lib/Sema/SemaDeclAttr.cpp

static void handleFormatAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   // check the 3rd argument
   Expr *FirstArgExpr = AL.getArgAsExpr(2);
   uint32_t FirstArg;
   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 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();
+          << 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;
       }
-  // if 0 it disables parameter checking (to use with e.g. va_list)
-  } else if (FirstArg != 0 && FirstArg != NumArgs) {
+    } 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;
       }
+    }
+  }
 
   FormatAttr *NewAttr = S.mergeFormatAttr(D, AL, II, Idx, FirstArg);
   if (NewAttr)
     D->addAttr(NewAttr);
 }
 
 /// Handle __attribute__((callback(CalleeIdx, PayloadIdx0, ...))) attributes.
 static void handleCallbackAttr(Sema &S, Decl *D, const ParsedAttr &AL) {

clang/test/FixIt/attr-format.c

+// 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, ...);

clang/test/Sema/attr-format.c

 //RUN: %clang_cc1 -fsyntax-only -verify %s
 
 #include <stdarg.h>
 
 void a(const char *a, ...) __attribute__((format(printf, 1, 2)));    // no-error
 void b(const char *a, ...) __attribute__((format(printf, 1, 1)));    // expected-error {{'format' attribute parameter 3 is out of bounds}}
 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}}
 
 int (*f_ptr)(char*,...) __attribute__((format(printf, 1,2))); // no-error
 int (*f2_ptr)(double,...) __attribute__((format(printf, 1, 2))); // expected-error {{format argument not a string type}}
 
 struct _mystruct {
void call_nonvariadic(void) {
   d3("%d", 123);
   d3("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
 }
 
 __attribute__((format(printf, 1, 2))) void forward_fixed(const char *fmt, int i) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
   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);
+}
+