[PATCH] D112579: Allow non-variadic functions to be attributed with `__attribute__((format))`

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

> Agreed. This should be checking the instantiations, so by that point, the variadic template is really more like a fixed parameter list anyway.

FWIW, in my own mental model, there's a big semantic difference between (varargs functions, variadic templates) on the one hand and (non-template functions) on the other. In my experience, there's //nothing// you can do with a varargs ellipsis except forward it along as a `va_list`; and it's //uncommon// to do anything with a variadic parameter pack except forward it along via `std::forward`; but messing with fixed arguments is quite common. Technically, you can mess with a parameter pack too:

  void apple(const char *fmt, int x, int y) __attribute__((format(printf, 1, 2))) {
      printf(fmt, double(x), double(y));
  }
  void banana(const char *fmt, auto... args) __attribute__((format(printf, 1, 2))) {
      printf(fmt, double(args)...);
  }
  int main() {
      apple("%g %g", 17, 42);  // well-defined; shall we warn anyway? (My gut feeling is that this is relatively common)
      banana("%g %g", 17, 42);  // well-defined; shall we warn anyway? (My gut feeling is that this is extremely rare)
  }

This morning I am leaning in favor of this PR as written. If a programmer wants `apple`/`banana` to be callable like that, without any diagnostics, then their appropriate course of action is simply not to apply the `__attribute__((format(printf, 1, 2)))` annotation.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4051
+def warn_gcc_requires_variadic_function : Warning<
+  "GCC requires functions with %0 attribute to be variadic">,
+  InGroup<GccCompat>;
----------------
I'd say `with the %0 attribute` (add "the")


================
Comment at: clang/lib/AST/FormatString.cpp:325-329
+  // When using the format attribute with variadic templates in C++, you can
+  // receive an array that will necessarily decay to a pointer when passed to
+  // the final format consumer. Apply decay before type comparison.
+  if (C.getAsArrayType(argTy))
+    argTy = C.getArrayDecayedType(argTy);
----------------
Also, function references will decay to function pointers.  I have no idea if you need to do anything special here to get the "right behavior" for function references.  But please add a (compile-only?) test case for the function-pointer codepath, just to prove it doesn't crash or anything. 


================
Comment at: clang/test/Sema/attr-format.cpp:5-7
+template<typename... Args>
+void format(const char *fmt, Args &&... args) // expected-warning{{GCC requires functions with 'format' attribute to be variadic}}
+    __attribute__((format(printf, 1, 2)));
----------------
Can we also do an example of a //member function// variadic template?
```
struct S {
  template<class... Args>
  void format(const char *fmt, Args&&... args)
    __attribute__((format(printf, 2, 3)));
};
```
Also, I believe that this entire file should be removed from `Sema/` and combined into `SemaCXX/attr-format.cpp`.

I also notice that we have literally zero test coverage for cases where the format string is not the first argument to the function; but that can-and-should be addressed in a separate PR.


================
Comment at: clang/test/Sema/attr-format.cpp:14
+  format("bare string");
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, &do_format);
+  format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
----------------
Basically, add `, do_format` here.
(Aside: I'm surprised that Clang quietly lets you print a function pointer with `%p`. It'll work on sane architectures, but //in general// C and C++ don't require that function pointers even be the same size as `void*` — technically this should be UB or at least impl-defined behavior.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112579/new/

https://reviews.llvm.org/D112579