[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

In D148381#4646833 , @rapidsna wrote:

> `-fbounds-safety` doesn't allow this. In our internal adoption experience, we 
> haven't encountered such use cases yet. So, I think it's best to make the 
> model restrictive to avoid surprises. If we were to support it, I think it 
> should at least be limited to cases where the array subscript expression is 
> known to be in bounds at compile time, to avoid an OOB access when the 
> counted_by argument is evaluated.

Additionally: it is probably safe from an aliasing perspective (or at least not 
worse than using any other field) to use an array subscript in a count 
expression, provided the array's storage exists within the struct. However, we 
certainly wouldn't want people to go towards `array[variable]`, 
`pointer[anything]`, or (worse!) `FAM[anything]`, and constant array subscripts 
are confusingly adjacent to the boundary we need to close. If we're just 
entertaining the possibility without motivating use cases at this time, I'd 
advise to leave it be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Commit is 
https://github.com/llvm/llvm-project/commit/04e6178ae932c9a1d939dcfe3ef1189f4bbb21aa.


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

https://reviews.llvm.org/D158318

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier closed this revision.
fcloutier added a comment.

Apologies, I landed the change but forgot to update the commit message to 
include the "Differential Revision:" link. -_- I'm closing this change and I'll 
update the GitHub issue, which is linked.


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

https://reviews.llvm.org/D158318

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 553271.
fcloutier added a comment.

It seems that clang allows `char` specifiers to match `bool` in `scanf` today, 
without my change (https://godbolt.org/z/e8PrjY65h). I think that this is a 
mistake, but that's almost certainly up for debate and I'd like to avoid scope 
creep on this change.

The new format-strings-scanf.cpp file covers the correct and "barely wrong" 
cases that I could think of.


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

https://reviews.llvm.org/D158318

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/FormatString.cpp
  clang/test/Sema/attr-format.c
  clang/test/SemaCXX/attr-format.cpp
  clang/test/SemaCXX/format-strings-scanf.cpp

Index: clang/test/SemaCXX/format-strings-scanf.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/format-strings-scanf.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -Wformat-non-iso -fblocks -std=c++11 %s
+
+__attribute__((format(scanf, 1, 2)))
+int scanf(const char *, ...);
+
+template
+__attribute__((format(scanf, 1, 2)))
+int scan(const char *fmt, Args &&...args) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+return scanf(fmt, args...);
+}
+
+union bag {
+bool b;
+unsigned char uc;
+signed char sc;
+unsigned short us;
+signed short ss;
+unsigned int ui;
+signed int si;
+unsigned long ul;
+signed long sl;
+unsigned long long ull;
+signed long long sll;
+__fp16 f16;
+float ff;
+double fd;
+long double fl;
+};
+
+void test(void) {
+bag b;
+scan("%hhi %hhu %hhi %hhu", , , , );
+scan("%hi %hu", , );
+scan("%i %u", , );
+scan("%li %lu", , );
+scan("%lli %llu", , );
+scan("%f", );
+scan("%lf", );
+scan("%Lf", );
+
+// expected-warning@+4{{format specifies type 'short *' but the argument has type 'signed char *'}}
+// expected-warning@+3{{format specifies type 'unsigned short *' but the argument has type 'unsigned char *'}}
+// expected-warning@+2{{format specifies type 'short *' but the argument has type 'bool *'}}
+// expected-warning@+1{{format specifies type 'unsigned short *' but the argument has type 'bool *'}}
+scan("%hi %hu %hi %hu", , , , );
+
+// expected-warning@+3{{format specifies type 'long *' but the argument has type 'short *'}}
+// expected-warning@+2{{format specifies type 'char *' but the argument has type 'short *'}}
+// expected-warning@+1{{format specifies type 'int *' but the argument has type 'short *'}}
+scan("%hhi %i %li", , , );
+
+// expected-warning@+3{{format specifies type 'float *' but the argument has type '__fp16 *'}}
+// expected-warning@+2{{format specifies type 'float *' but the argument has type 'double *'}}
+// expected-warning@+1{{format specifies type 'float *' but the argument has type 'long double *'}}
+scan("%f %f %f", , , );
+
+// expected-warning@+3{{format specifies type 'double *' but the argument has type '__fp16 *'}}
+// expected-warning@+2{{format specifies type 'double *' but the argument has type 'float *'}}
+// expected-warning@+1{{format specifies type 'double *' but the argument has type 'long double *'}}
+scan("%lf %lf %lf", , , );
+
+// expected-warning@+3{{format specifies type 'long double *' but the argument has type '__fp16 *'}}
+// expected-warning@+2{{format specifies type 'long double *' but the argument has type 'float *'}}
+// expected-warning@+1{{format specifies type 'long double *' but the argument has type 'double *'}}
+scan("%Lf %Lf %Lf", , , );
+}
Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -72,12 +72,20 @@
   int x = 123;
   int  = x;
   const char *s = "world";
+  bool b = false;
   format("bare string");
   format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
   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}}
 
+  format("%c %c %hhd %hd %d\n", (char)'a', 'a', 'a', (short)123, (int)123);
+  format("%f %f %f\n", (__fp16)123.f, 123.f, 123.);
+  format("%Lf", (__fp16)123.f); // expected-warning{{format specifies type 'long double' but the argument has type '__fp16'}}
+  format("%Lf", 123.f); // expected-warning{{format specifies type 'long double' but the argument has type 'float'}}
+  format("%hhi %hhu %hi %hu %i %u", b, b, b, b, b, b);
+  format("%li", b); // expected-warning{{format specifies type 'long' but the argument has type 'bool'}}
+
   struct foo f;
   format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies 

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.



Comment at: clang/lib/AST/FormatString.cpp:480-481
+  break;
+case BuiltinType::Half:
+case BuiltinType::Float16:
+case BuiltinType::Float:

aaron.ballman wrote:
> Should these be checking for `T == C.FloatTy` to return 
> `NoMatchPromotionTypeConfusion`?
I don't think it's necessary. `T` is the format specifier's expected type, and 
no format specifier expects a `float` (due to floating-point types being 
promoted to `double` by default argument promotion), so there's never a case 
where `T` is `FloatTy`.


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

https://reviews.llvm.org/D158318

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 552579.
fcloutier marked an inline comment as done.
fcloutier added a comment.

Add release note, ensure `bool` as a formatted formal parameter is accepted.


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

https://reviews.llvm.org/D158318

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/FormatString.cpp
  clang/test/Sema/attr-format.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -72,12 +72,20 @@
   int x = 123;
   int  = x;
   const char *s = "world";
+  bool b = false;
   format("bare string");
   format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
   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}}
 
+  format("%c %c %hhd %hd %d\n", (char)'a', 'a', 'a', (short)123, (int)123);
+  format("%f %f %f\n", (__fp16)123.f, 123.f, 123.);
+  format("%Lf", (__fp16)123.f); // expected-warning{{format specifies type 'long double' but the argument has type '__fp16'}}
+  format("%Lf", 123.f); // expected-warning{{format specifies type 'long double' but the argument has type 'float'}}
+  format("%hhi %hhu %hi %hu %i %u", b, b, b, b, b, b);
+  format("%li", b); // expected-warning{{format specifies type 'long' but the argument has type 'bool'}}
+
   struct foo f;
   format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
 
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -94,13 +94,9 @@
   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);
+__attribute__((format(printf, 1, 2)))
+void forward_fixed(const char *fmt, _Bool b, char i, short j, int k, float l, double m) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  forward_fixed(fmt, b, i, j, k, l, m);
+  a(fmt, b, i, j, k, l, m);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -458,6 +458,10 @@
 switch (BT->getKind()) {
 default:
   break;
+case BuiltinType::Bool:
+  if (T == C.IntTy || T == C.UnsignedIntTy)
+return MatchPromotion;
+  break;
 case BuiltinType::Int:
 case BuiltinType::UInt:
   if (T == C.SignedCharTy || T == C.UnsignedCharTy ||
@@ -465,6 +469,24 @@
   T == C.WideCharTy)
 return MatchPromotion;
   break;
+case BuiltinType::Char_U:
+  if (T == C.UnsignedIntTy)
+return MatchPromotion;
+  if (T == C.UnsignedShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Char_S:
+  if (T == C.IntTy)
+return MatchPromotion;
+  if (T == C.ShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Half:
+case BuiltinType::Float16:
+case BuiltinType::Float:
+  if (T == C.DoubleTy)
+return MatchPromotion;
+  break;
 case BuiltinType::Short:
 case BuiltinType::UShort:
   if (T == C.SignedCharTy || T == C.UnsignedCharTy)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -130,6 +130,13 @@
 Attribute Changes in Clang
 --
 
+- When a non-variadic function is decorated with the ``format`` attribute,
+  Clang now checks that the format string would match the function's parameters'
+  types after default argument promotion. As a result, it's no longer an
+  automatic diagnostic to use parameters of types that the format style
+  supports but that are never the result of default argument promotion, such as
+  ``float``. (`#59824: 

[PATCH] D158318: [Sema] tolerate more promotion matches in format string checking

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added a reviewer: aaron.ballman.
Herald added a project: All.
fcloutier requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

It's been reported that when using __attribute__((format)) on non-variadic 
functions, certain values that normally get promoted when passed as variadic 
arguments now unconditionally emit a diagnostic:

  c
  void foo(const char *fmt, float f) __attribute__((format(printf, 1, 2)));
  void bar(void) {
foo("%g", 123.f);
//   ^ format specifies type 'double' but the argument has type 'float'
  }

This is normally not an issue because float values get promoted to doubles when 
passed as variadic arguments, but needless to say, variadic argument promotion 
does not apply to non-variadic arguments.

While this can be fixed by adjusting the prototype of `foo`, this is sometimes 
undesirable in C (for instance, if `foo` is ABI). In C++, using variadic 
templates, this might instead require call-site fixing, which is tedious and 
arguably needless work:

  c++
  template
  void foo(const char *fmt, Args &&...args) __attribute__((format(printf, 1, 
2)));
  void bar(void) {
foo("%g", 123.f);
//   ^ format specifies type 'double' but the argument has type 'float'
  }

To address this issue, we teach FormatString about a few promotions that have 
always been around but that have never been exercised in the direction that 
FormatString checks for:

- `char`, `unsigned char` -> `int`, `unsigned`
- `half`, `float16`, `float` -> `double`

This addresses issue https://github.com/llvm/llvm-project/issues/59824.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158318

Files:
  clang/lib/AST/FormatString.cpp
  clang/test/Sema/attr-format.c
  clang/test/SemaCXX/attr-format.cpp


Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -78,6 +78,9 @@
   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}}
 
+  format("%c %c %hhd %hd %d\n", (char)'a', 'a', 'a', (short)123, (int)123);
+  format("%f %f %f\n", (__fp16)123.f, 123.f, 123.);
+
   struct foo f;
   format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies 
type 'int' but the argument has type 'struct foo'}}
 
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -94,13 +94,9 @@
   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);
+__attribute__((format(printf, 1, 2)))
+void forward_fixed(const char *fmt, char i, short j, int k, float l, double m) 
{ // expected-warning{{GCC requires a function with the 'format' attribute to 
be variadic}}
+  forward_fixed(fmt, i, j, k, l, m);
+  a(fmt, i, j, k, l, m);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -465,6 +465,24 @@
   T == C.WideCharTy)
 return MatchPromotion;
   break;
+case BuiltinType::Char_U:
+  if (T == C.UnsignedIntTy)
+return MatchPromotion;
+  if (T == C.UnsignedShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Char_S:
+  if (T == C.IntTy)
+return MatchPromotion;
+  if (T == C.ShortTy)
+return NoMatchPromotionTypeConfusion;
+  break;
+case BuiltinType::Half:
+case BuiltinType::Float16:
+case BuiltinType::Float:
+  if (T == C.DoubleTy)
+return MatchPromotion;
+  break;
 case BuiltinType::Short:
 case BuiltinType::UShort:
   if (T == C.SignedCharTy || T == C.UnsignedCharTy)


Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -78,6 +78,9 @@
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
   format("bad format %s"); // expected-warning{{more '%' 

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

[Félix Cloutier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcd95d7998c1d: [Clang][Sema] Fix attribute((format)) bug on 
non-variadic functions (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137603

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/FixIt/attr-format.c
  clang/test/Sema/attr-format.c

Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -7,10 +7,14 @@
 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 @@
   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);
+}
+
Index: clang/test/FixIt/attr-format.c
===
--- /dev/null
+++ 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, ...);
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3891,27 +3891,38 @@
   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(), 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 479658.
fcloutier added a comment.

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


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

https://reviews.llvm.org/D137603

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/FixIt/attr-format.c
  clang/test/Sema/attr-format.c

Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -7,10 +7,14 @@
 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 @@
   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);
+}
+
Index: clang/test/FixIt/attr-format.c
===
--- /dev/null
+++ 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, ...);
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3890,27 +3890,38 @@
   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 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 479427.
fcloutier added a comment.

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


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

https://reviews.llvm.org/D137603

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/FixIt/attr-format.c
  clang/test/Sema/attr-format.c

Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -7,10 +7,14 @@
 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 @@
   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);
+}
+
Index: clang/test/FixIt/attr-format.c
===
--- /dev/null
+++ 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, ...);
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3890,27 +3890,38 @@
   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 

[PATCH] D137714: Do not merge traps in functions annotated optnone

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier requested changes to this revision.
fcloutier added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5321
 
+  SetLLVMFunctionAttributesForDefinition(D, Fn);
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);

hnrklssn wrote:
> delcypher wrote:
> > I'm a little worried about this ordering change here. Could this have some 
> > unintended consequences?
> Yeah I was also a bit worried about that when making the change, since the 
> functions are both quite broad and I'm not familiar with them from before. 
> However it doesn't break any test cases, so I'm not sure what the 
> consequences would be exactly.
> 
> For reference, also moving the call to setNonAliasAttributes so that it is 
> still before the call to SetLLVMFunctionAttributesForDefinition breaks a ton 
> of test cases so I'm somewhat hopeful that we have good test coverage for 
> this type of change.
Could you get it from `CurCodeDecl->hasAttr()` in CGExpr 
instead? Then you wouldn't have to change this.

Caveat: am not sure that `CurCodeDecl` is always set. For instance, do you have 
a `CurCodeDecl` when you generate a C++ static initializer? On the upside, if 
it's NULL, you can just bail out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: ahatanak, dcoughlin, aaron.ballman.
fcloutier added a project: clang.
Herald added a project: All.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137603

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c

Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -7,10 +7,14 @@
 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 @@
   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);
+}
+
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3890,27 +3890,33 @@
   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;
+} else if (isFunctionOrMethodVariadic(D)) {
+  // Else, if the function is variadic, then FirstArg must be 0 or the "position"
+  // of the ... parameter.
+  

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

[Félix Cloutier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG92edd74b37c7: Allow non-variadic functions to be attributed 
with `__attribute__((format))` (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/format-strings.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -1,7 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -Wformat-nonliteral -verify %s
+#include 
+
+int printf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+
 struct S {
-  static void f(const char*, ...) __attribute__((format(printf, 1, 2)));
-  static const char* f2(const char*) __attribute__((format_arg(1)));
+  static void f(const char *, ...) __attribute__((format(printf, 1, 2)));
+  static const char *f2(const char *) __attribute__((format_arg(1)));
 
   // GCC has a hidden 'this' argument in member functions which is why
   // the format argument is argument 2 here.
@@ -38,6 +42,47 @@
 
 // Make sure we interpret member operator calls as having an implicit
 // this argument.
-void test_operator_call(S s, const char* str) {
+void test_operator_call(S s, const char *str) {
   s("%s", str);
 }
+
+template 
+void format(const char *fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+template 
+Arg (Arg ) { return a; }
+
+struct foo {
+  int big[10];
+  foo();
+  ~foo();
+
+  template 
+  void format(const char *const fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  __attribute__((format(printf, 2, 3))) {
+printf(fmt, expand(args)...);
+  }
+};
+
+void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  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}}
+
+  struct foo f;
+  format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
+
+  f.format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
+  f.format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
+}
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -816,6 +816,7 @@
   __attribute__((__format__(__printf__, 2, 3))) {
 va_list ap;
 va_start(ap, fmt);
+vprintf(fmt, ap);
 vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
 va_end(ap);
   };
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -2,18 +2,18 @@
 
 #include 
 
-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-error {{format attribute requires variadic function}}
-void e(char *str, int c, ...) __attribute__((format(printf, 2,3))); // expected-error {{format argument not a string type}}
+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 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 442396.
fcloutier added a comment.

There was a merge conflict on the release notes, updating the differential to 
get a CI build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/format-strings.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -1,7 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -Wformat-nonliteral -verify %s
+#include 
+
+int printf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+
 struct S {
-  static void f(const char*, ...) __attribute__((format(printf, 1, 2)));
-  static const char* f2(const char*) __attribute__((format_arg(1)));
+  static void f(const char *, ...) __attribute__((format(printf, 1, 2)));
+  static const char *f2(const char *) __attribute__((format_arg(1)));
 
   // GCC has a hidden 'this' argument in member functions which is why
   // the format argument is argument 2 here.
@@ -38,6 +42,47 @@
 
 // Make sure we interpret member operator calls as having an implicit
 // this argument.
-void test_operator_call(S s, const char* str) {
+void test_operator_call(S s, const char *str) {
   s("%s", str);
 }
+
+template 
+void format(const char *fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+template 
+Arg (Arg ) { return a; }
+
+struct foo {
+  int big[10];
+  foo();
+  ~foo();
+
+  template 
+  void format(const char *const fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  __attribute__((format(printf, 2, 3))) {
+printf(fmt, expand(args)...);
+  }
+};
+
+void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  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}}
+
+  struct foo f;
+  format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
+
+  f.format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
+  f.format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
+}
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -816,6 +816,7 @@
   __attribute__((__format__(__printf__, 2, 3))) {
 va_list ap;
 va_start(ap, fmt);
+vprintf(fmt, ap);
 vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
 va_end(ap);
   };
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -2,18 +2,18 @@
 
 #include 
 
-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-error {{format attribute requires variadic function}}
-void e(char *str, int c, ...) __attribute__((format(printf, 2,3))); // expected-error {{format argument not a string type}}
+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) 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.



Comment at: clang/test/SemaCXX/attr-format.cpp:76
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but 
the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);

aaron.ballman wrote:
> This pointed out an interesting test case. What should the behavior be for:
> ```
> format("%p", 0);
> ```
> Because that sure feels like a more reasonable thing for someone to write 
> expecting it to be treated as a null pointer constant.
I think that the current behavior is the right one:

```
test.c:4:17: warning: format specifies type 'void *' but the argument has type 
'int' [-Wformat]
printf("%p\n", 0);
~~ ^
%d
```

The warning goes away if you use `(void *)0`, as expected. 
`__attribute__((format))` has no semantic meaning, so we can't (and shouldn't) 
infer that 0 is a pointer based on the usage of %p.



Comment at: clang/test/SemaCXX/attr-format.cpp:77-78
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but 
the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  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}}

aaron.ballman wrote:
> This likely isn't specific to your changes, but the `%p` in these examples 
> should be warning the user (a function or function pointer is not a pointer 
> to void or a pointer to a character type, so that call is UB).
This is already a -Wformat-pedantic warning, which IMO is the right warning 
group for it:

```
test.c:4:17: warning: format specifies type 'void *' but the argument has type 
'int (*)()' [-Wformat-pedantic]
printf("%p\n", main);
~~ ^~~~
1 warning generated.
```

The relevant bit is clang/lib/AST/FormatString.cpp:

```
case CPointerTy:
  if (argTy->isVoidPointerType()) {
return Match;
  } if (argTy->isPointerType() || argTy->isObjCObjectPointerType() ||
argTy->isBlockPointerType() || argTy->isNullPtrType()) {
return NoMatchPedantic;
  } else {
return NoMatch;
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 442373.
fcloutier marked 2 inline comments as done.
fcloutier added a comment.

Address documentation comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/format-strings.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -1,7 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -Wformat-nonliteral -verify %s
+#include 
+
+int printf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+
 struct S {
-  static void f(const char*, ...) __attribute__((format(printf, 1, 2)));
-  static const char* f2(const char*) __attribute__((format_arg(1)));
+  static void f(const char *, ...) __attribute__((format(printf, 1, 2)));
+  static const char *f2(const char *) __attribute__((format_arg(1)));
 
   // GCC has a hidden 'this' argument in member functions which is why
   // the format argument is argument 2 here.
@@ -38,6 +42,47 @@
 
 // Make sure we interpret member operator calls as having an implicit
 // this argument.
-void test_operator_call(S s, const char* str) {
+void test_operator_call(S s, const char *str) {
   s("%s", str);
 }
+
+template 
+void format(const char *fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+template 
+Arg (Arg ) { return a; }
+
+struct foo {
+  int big[10];
+  foo();
+  ~foo();
+
+  template 
+  void format(const char *const fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  __attribute__((format(printf, 2, 3))) {
+printf(fmt, expand(args)...);
+  }
+};
+
+void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  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}}
+
+  struct foo f;
+  format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
+
+  f.format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
+  f.format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
+}
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -816,6 +816,7 @@
   __attribute__((__format__(__printf__, 2, 3))) {
 va_list ap;
 va_start(ap, fmt);
+vprintf(fmt, ap);
 vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
 va_end(ap);
   };
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -2,18 +2,18 @@
 
 #include 
 
-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-error {{format attribute requires variadic function}}
-void e(char *str, int c, ...) __attribute__((format(printf, 2,3))); // expected-error {{format argument not a string type}}
+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) 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

I'm afraid that's also not possible: `D` is a `Decl`, so it doesn't have 
`getType()`. `Decl` is the tightest-fitting superclass of `BlockDecl`, 
`FunctionDecl` and `ObjCMethodDecl` (because `BlockDecl` is a direct subclass 
of it).

One option could be to cast the `Decl` to a `FunctionDecl` and then use 
`FDecl->isVariadic()`, similarly to how it goes for `BlockDecl` and 
`ObjCMethodDecl`. I'm not sure that it's equivalent, but if you believe it is 
and like it better, I can do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 441703.
fcloutier set the repository for this revision to rG LLVM Github Monorepo.
fcloutier added a comment.

Thanks, Aaron. I wasn't sure how to follow up given how long it had been since 
the review started. I understand that we're all busy (which explains the week 
delay on my part here as well).

I've addressed all of your comments except the one on this bit 
:

  if (const FunctionType *FnTy = D->getFunctionType())
IsVariadic = cast(FnTy)->isVariadic();

The proposed change isn't identical because `D->getFunctionType()` can return 
nullptr (for instance, if `D` is a `BlockDecl`). However, in the case `FnTy` 
isn't nullptr, then it is guaranteed to be a `FunctionProtoType` as the 
attribute is rejected on functions without a prototype.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/format-strings.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -1,7 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -Wformat-nonliteral -verify %s
+#include 
+
+int printf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+
 struct S {
-  static void f(const char*, ...) __attribute__((format(printf, 1, 2)));
-  static const char* f2(const char*) __attribute__((format_arg(1)));
+  static void f(const char *, ...) __attribute__((format(printf, 1, 2)));
+  static const char *f2(const char *) __attribute__((format_arg(1)));
 
   // GCC has a hidden 'this' argument in member functions which is why
   // the format argument is argument 2 here.
@@ -38,6 +42,47 @@
 
 // Make sure we interpret member operator calls as having an implicit
 // this argument.
-void test_operator_call(S s, const char* str) {
+void test_operator_call(S s, const char *str) {
   s("%s", str);
 }
+
+template 
+void format(const char *fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+template 
+Arg (Arg ) { return a; }
+
+struct foo {
+  int big[10];
+  foo();
+  ~foo();
+
+  template 
+  void format(const char *const fmt, Args &&...args) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  __attribute__((format(printf, 2, 3))) {
+printf(fmt, expand(args)...);
+  }
+};
+
+void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  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}}
+
+  struct foo f;
+  format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
+
+  f.format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
+  f.format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
+}
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -816,6 +816,7 @@
   __attribute__((__format__(__printf__, 2, 3))) {
 va_list ap;
 va_start(ap, fmt);
+vprintf(fmt, ap);
 vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
 va_end(ap);
   };
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -2,18 +2,18 @@
 
 #include 
 
-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 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Would it be better if I asked a colleague to finish the review?


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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Ping


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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 435302.
fcloutier added a comment.
Herald added a project: All.

Apologies the long delay: things happened and I was pulled away. I have some 
time to finish this change now. I recommend re-reading the discussion up to now 
since it's not _that_ long and it provides a lot of very useful context.

The new change addresses requests from the previous round. The most substantial 
changes are around how Clang detects that a format string is being forwarded to 
another format function. This is now expressed in terms of transitions from 
format argument passing styles, such that given the following 3 function 
archetypes:

  c
  void fixed(const char *, int) __attribute__((format(printf, 1, 2)));
  void variadic(const char *, ...) __attribute__((format(printf, 1, 2)));
  void valist(const char *, va_list) __attribute__((format(printf, 1, 0)));

there are no warnings for:

- a `variadic` function forwarding its format to a `valist` function
- a `valist` function forwarding its format to another `valist` function
- a `fixed` function forwarding its format to another `fixed` function (new)
- a `fixed` function forwarding its format to a `variadic` function (new)

In other words, for instance, `fixed` can call `variadic` in its implementation 
without a warning. Anything else, like forwarding the format of a `valist` 
function to a `fixed` function, is a diagnostic.

`fixed` to `fixed`/`variadic` transitions don't check that arguments have 
compatible types, but it conceivably could. This is a limitation of the current 
implementation. However, at this point, we don't think that this is a very 
worthwhile effort; this could change in the future if adoption of the `format` 
attribute on functions with a fixed signature ramps up.

I also added a number of tests to make sure that we still have reasonable 
warnings. One interesting edge case when using `__attribute__((format))` on 
functions with fixed parameters is that it's possible to come up with 
combinations that are impossible, for instance:

  c
  struct nontrivial { nontrivial(); ~nontrivial(); };
  void foo(const char *, nontrivial) __attribute__((format(printf, 1, 2)));

It's not a diagnostic to declare this function, however it is always a 
diagnostic to call it because no printf format specifier can format a 
`nontrivial` object. Ideally there would be a diagnostic on the declaration, 
but I think that it's sufficient as it is.


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

https://reviews.llvm.org/D112579

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/format-strings.c
  clang/test/SemaCXX/attr-format.cpp

Index: clang/test/SemaCXX/attr-format.cpp
===
--- clang/test/SemaCXX/attr-format.cpp
+++ clang/test/SemaCXX/attr-format.cpp
@@ -1,7 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only -Wformat-nonliteral -verify %s
+#include 
+
+int printf(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+
 struct S {
-  static void f(const char*, ...) __attribute__((format(printf, 1, 2)));
-  static const char* f2(const char*) __attribute__((format_arg(1)));
+  static void f(const char *, ...) __attribute__((format(printf, 1, 2)));
+  static const char *f2(const char *) __attribute__((format_arg(1)));
 
   // GCC has a hidden 'this' argument in member functions which is why
   // the format argument is argument 2 here.
@@ -38,6 +42,47 @@
 
 // Make sure we interpret member operator calls as having an implicit
 // this argument.
-void test_operator_call(S s, const char* str) {
+void test_operator_call(S s, const char *str) {
   s("%s", str);
 }
+
+template 
+void format(const char *fmt, Args &&...args) // expected-warning{{GCC requires functions with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+template 
+Arg (Arg ) { return a; }
+
+struct foo {
+  int big[10];
+  foo();
+  ~foo();
+
+  template 
+  void format(const char *const fmt, Args &&...args) // expected-warning{{GCC requires functions with the 'format' attribute to be variadic}}
+  __attribute__((format(printf, 2, 3))) {
+printf(fmt, expand(args)...);
+  }
+};
+
+void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires functions with the 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
+  format("bad format %s"); // expected-warning{{more 

[PATCH] D125254: [clang] Allow all string types for all attribute(format) styles

[Félix Cloutier via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG91ed7e194181: [clang] Allow all string types for all 
attribute(format) styles (authored by fcloutier).

Changed prior to commit:
  https://reviews.llvm.org/D125254?vs=428142=429028#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125254

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-strings-objc.m


Index: clang/test/SemaObjC/format-strings-objc.m
===
--- clang/test/SemaObjC/format-strings-objc.m
+++ clang/test/SemaObjC/format-strings-objc.m
@@ -60,8 +60,23 @@
 }
 
 // Check type validation
-extern void NSLog2(int format, ...) __attribute__((format(__NSString__, 1, 
2))); // expected-error {{format argument not an NSString}}
-extern void CFStringCreateWithFormat2(int *format, ...) 
__attribute__((format(CFString, 1, 2))); // expected-error {{format argument 
not a CFString}}
+extern void NSLog2(int format, ...) __attribute__((format(__NSString__, 1, 
2))); // expected-error {{format argument not a string type}}
+extern void CFStringCreateWithFormat2(int *format, ...) 
__attribute__((format(CFString, 1, 2))); // expected-error {{format argument 
not a string type}}
+
+// Check interoperability of strings
+extern void NSLog3(const char *, ...) __attribute__((format(__NSString__, 1, 
2)));
+extern void CFStringCreateWithFormat3(CFStringRef, ...) 
__attribute__((format(__NSString__, 1, 2)));
+extern void printf2(NSString *format, ...) __attribute__((format(printf, 1, 
2)));
+
+extern NSString *CStringToNSString(const char *) 
__attribute__((format_arg(1)));
+
+void NSLog3(const char *fmt, ...) {
+  NSString *const nsFmt = CStringToNSString(fmt);
+  va_list ap;
+  va_start(ap, fmt);
+  NSLogv(nsFmt, ap);
+  va_end(ap);
+}
 
 //  - Catch use of long long with int arguments.
 void rdar_7068334(void) {
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3661,8 +3661,7 @@
   (!Ty->isPointerType() ||
!Ty->castAs()->getPointeeType()->isCharType())) {
 S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "a string type" << IdxExpr->getSourceRange()
-<< getFunctionOrMethodParamRange(D, 0);
+<< IdxExpr->getSourceRange() << getFunctionOrMethodParamRange(D, 0);
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
@@ -3862,27 +3861,12 @@
   // make sure the format string is really a string
   QualType Ty = getFunctionOrMethodParamType(D, ArgIdx);
 
-  if (Kind == CFStringFormat) {
-if (!isCFStringType(Ty, S.Context)) {
-  S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "a CFString" << IdxExpr->getSourceRange()
-<< getFunctionOrMethodParamRange(D, ArgIdx);
-  return;
-}
-  } else if (Kind == NSStringFormat) {
-// FIXME: do we need to check if the type is NSString*?  What are the
-// semantics?
-if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true)) {
-  S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "an NSString" << IdxExpr->getSourceRange()
-<< getFunctionOrMethodParamRange(D, ArgIdx);
-  return;
-}
-  } else if (!Ty->isPointerType() ||
- !Ty->castAs()->getPointeeType()->isCharType()) {
+  if (!isNSStringType(Ty, S.Context, true) &&
+  !isCFStringType(Ty, S.Context) &&
+  (!Ty->isPointerType() ||
+   !Ty->castAs()->getPointeeType()->isCharType())) {
 S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-  << "a string type" << IdxExpr->getSourceRange()
-  << getFunctionOrMethodParamRange(D, ArgIdx);
+  << IdxExpr->getSourceRange() << getFunctionOrMethodParamRange(D, ArgIdx);
 return;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3108,7 +3108,7 @@
   "strftime format attribute requires 3rd parameter to be 0">;
 def err_format_attribute_requires_variadic : Error<
   "format attribute requires variadic function">;
-def err_format_attribute_not : Error<"format argument not %0">;
+def err_format_attribute_not : Error<"format argument not a string type">;
 def err_format_attribute_result_not : Error<"function does not return %0">;
 def err_format_attribute_implicit_this_format_string : Error<
   "format attribute cannot specify the implicit this argument as the format "


Index: clang/test/SemaObjC/format-strings-objc.m
===
--- clang/test/SemaObjC/format-strings-objc.m
+++ 

[PATCH] D125254: [clang] Allow all string types for all attribute(format) styles

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: NoQ, ahatanak, aaron.ballman.
Herald added a project: All.
fcloutier requested review of this revision.
Herald added a project: clang.

It's not unusual to see functions like this:

  void log(const char *fmt, ...) {
  va_list ap;
  va_start(ap, fmt);
  NSLogv([NSString stringWithCString:fmt], ap);
  va_end(ap);
  }

Currently, such a function can't be annotated with `__attribute__((format))`. 
It can't do `__attribute__((format(printf)))` because `printf` does not accept 
the `%@` format specifier (while this function does support it) and it can't do 
`__attribute__((format(NSString)))` because the format argument is not a 
`NSString *`. It's not always reasonable to ask the owners of these functions 
to change the type of the format argument as it can be an ABI-breaking change. 
The only viable thing to do is to not annotate `log` at all, which precludes 
using `-Wformat-nonliteral` (as it will trigger on the `NSLogv` line) and may 
hide format bugs in users of `log`.

This patch allows all string types for all format kinds and leaves it up to the 
user to convert the format string with a function that has the correct 
`__attribute__((format_arg))` annotation. The example above can now be 
annotated with `__attribute__((format(NSString, 1, 2)))` and pass 
`-Wformat-nonliteral`, provided that `[NSString stringWithCString:]` is 
modified to have `__attribute__((format_arg(1)))`.

It is still a diagnostic to mix format strings with different format styles, so 
for instance you can't pass a `printf`-style format string to `[NSString 
stringWithFormat:]` even if you've converted it to a `NSString *`:

  void log(const char *fmt, ...) __attribute__((format(printf, 1, 2))) {
  va_list ap;
  va_start(ap, fmt);
  NSLogv([NSString stringWithCString:fmt], ap); // warning: format string 
is not a string literal [-Wformat-nonliteral]
  va_end(ap);
  }

rdar://89060618


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125254

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-strings-objc.m


Index: clang/test/SemaObjC/format-strings-objc.m
===
--- clang/test/SemaObjC/format-strings-objc.m
+++ clang/test/SemaObjC/format-strings-objc.m
@@ -60,8 +60,23 @@
 }
 
 // Check type validation
-extern void NSLog2(int format, ...) __attribute__((format(__NSString__, 1, 
2))); // expected-error {{format argument not an NSString}}
-extern void CFStringCreateWithFormat2(int *format, ...) 
__attribute__((format(CFString, 1, 2))); // expected-error {{format argument 
not a CFString}}
+extern void NSLog2(int format, ...) __attribute__((format(__NSString__, 1, 
2))); // expected-error {{format argument not a string type}}
+extern void CFStringCreateWithFormat2(int *format, ...) 
__attribute__((format(CFString, 1, 2))); // expected-error {{format argument 
not a string type}}
+
+// Check interoperability of strings
+extern void NSLog3(const char *, ...) __attribute__((format(__NSString__, 1, 
2)));
+extern void CFStringCreateWithFormat3(const char *, ...) 
__attribute__((format(__NSString__, 1, 2)));
+extern void printf2(NSString *format, ...) __attribute__((format(printf, 1, 
2)));
+
+extern NSString *CStringToNSString(const char *) 
__attribute__((format_arg(1)));
+
+void NSLog3(const char *fmt, ...) {
+  NSString *const nsFmt = CStringToNSString(fmt);
+  va_list ap;
+  va_start(ap, fmt);
+  NSLogv(nsFmt, ap);
+  va_end(ap);
+}
 
 //  - Catch use of long long with int arguments.
 void rdar_7068334(void) {
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3661,8 +3661,7 @@
   (!Ty->isPointerType() ||
!Ty->castAs()->getPointeeType()->isCharType())) {
 S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "a string type" << IdxExpr->getSourceRange()
-<< getFunctionOrMethodParamRange(D, 0);
+<< IdxExpr->getSourceRange() << getFunctionOrMethodParamRange(D, 0);
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
@@ -3862,27 +3861,13 @@
   // make sure the format string is really a string
   QualType Ty = getFunctionOrMethodParamType(D, ArgIdx);
 
-  if (Kind == CFStringFormat) {
-if (!isCFStringType(Ty, S.Context)) {
-  S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "a CFString" << IdxExpr->getSourceRange()
-<< getFunctionOrMethodParamRange(D, ArgIdx);
-  return;
-}
-  } else if (Kind == NSStringFormat) {
-// FIXME: do we need to check if the type is NSString*?  What are the
-// semantics?
-if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true)) {
-  S.Diag(AL.getLoc(), diag::err_format_attribute_not)
-<< "an NSString" << 

[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier accepted this revision.
fcloutier added a comment.
This revision is now accepted and ready to land.

I think that this is a good warning and I'll defer to the experts for what has 
to happen when prototypes merge with K definitions :)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5530
+  "calling function %0 with arguments when function has no prototype">, 
InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;
 def warn_missing_variable_declarations : Warning<

sp: I think this should be called "strict-call-without-prototype" (singular 
call) as most warning flag names seem to prefer singulars.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113636: format_arg attribute does not support nullable instancetype return type

[Félix Cloutier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG12ab3e6c8402: format_arg attribute does not support nullable 
instancetype return type (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113636

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -2,7 +2,10 @@
 
 @interface NSString
 +(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
-+(instancetype)stringWithString:(NSString *)cstr 
__attribute__((format_arg(1)));
+-(instancetype)initWithString:(NSString *)str __attribute__((format_arg(1)));
+
++(instancetype _Nonnull)nonNullableString:(NSString *)str 
__attribute__((format_arg(1)));
++(instancetype _Nullable)nullableString:(NSString *)str 
__attribute__((format_arg(1)));
 @end
 
 @protocol MaybeString
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3389,7 +3389,8 @@
   }
   Ty = getFunctionOrMethodResultType(D);
   // replace instancetype with the class type
-  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+  auto Instancetype = S.Context.getObjCInstanceTypeDecl()->getTypeForDecl();
+  if (Ty->getAs() == Instancetype)
 if (auto *OMD = dyn_cast(D))
   if (auto *Interface = OMD->getClassInterface())
 Ty = S.Context.getObjCObjectPointerType(


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -2,7 +2,10 @@
 
 @interface NSString
 +(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
-+(instancetype)stringWithString:(NSString *)cstr __attribute__((format_arg(1)));
+-(instancetype)initWithString:(NSString *)str __attribute__((format_arg(1)));
+
++(instancetype _Nonnull)nonNullableString:(NSString *)str __attribute__((format_arg(1)));
++(instancetype _Nullable)nullableString:(NSString *)str __attribute__((format_arg(1)));
 @end
 
 @protocol MaybeString
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3389,7 +3389,8 @@
   }
   Ty = getFunctionOrMethodResultType(D);
   // replace instancetype with the class type
-  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+  auto Instancetype = S.Context.getObjCInstanceTypeDecl()->getTypeForDecl();
+  if (Ty->getAs() == Instancetype)
 if (auto *OMD = dyn_cast(D))
   if (auto *Interface = OMD->getClassInterface())
 Ty = S.Context.getObjCObjectPointerType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113636: format_arg attribute does not support nullable instancetype return type

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: NoQ, ahatanak.
fcloutier added a project: clang.
Herald added a reviewer: aaron.ballman.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

Following up with D112670 , it appears that 
the original fix was insufficient: it's not possible to use 
`__attribute__((format_arg))` on methods that return `instancetype` in 
`NSString` when `instancetype` is sugared, as this breaks pointer equality. 
Although this is only possible in limited scenarios because `instancetype` is a 
contextual keyword, it can (crucially) be combined with nullability qualifiers.

rdar://85278860


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113636

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -2,7 +2,10 @@
 
 @interface NSString
 +(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
-+(instancetype)stringWithString:(NSString *)cstr 
__attribute__((format_arg(1)));
+-(instancetype)initWithString:(NSString *)str __attribute__((format_arg(1)));
+
++(instancetype _Nonnull)nonNullableString:(NSString *)str 
__attribute__((format_arg(1)));
++(instancetype _Nullable)nullableString:(NSString *)str 
__attribute__((format_arg(1)));
 @end
 
 @protocol MaybeString
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3389,7 +3389,8 @@
   }
   Ty = getFunctionOrMethodResultType(D);
   // replace instancetype with the class type
-  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+  auto Instancetype = S.Context.getObjCInstanceTypeDecl()->getTypeForDecl();
+  if (Ty->getAs() == Instancetype)
 if (auto *OMD = dyn_cast(D))
   if (auto *Interface = OMD->getClassInterface())
 Ty = S.Context.getObjCObjectPointerType(


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -2,7 +2,10 @@
 
 @interface NSString
 +(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
-+(instancetype)stringWithString:(NSString *)cstr __attribute__((format_arg(1)));
+-(instancetype)initWithString:(NSString *)str __attribute__((format_arg(1)));
+
++(instancetype _Nonnull)nonNullableString:(NSString *)str __attribute__((format_arg(1)));
++(instancetype _Nullable)nullableString:(NSString *)str __attribute__((format_arg(1)));
 @end
 
 @protocol MaybeString
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3389,7 +3389,8 @@
   }
   Ty = getFunctionOrMethodResultType(D);
   // replace instancetype with the class type
-  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+  auto Instancetype = S.Context.getObjCInstanceTypeDecl()->getTypeForDecl();
+  if (Ty->getAs() == Instancetype)
 if (auto *OMD = dyn_cast(D))
   if (auto *Interface = OMD->getClassInterface())
 Ty = S.Context.getObjCObjectPointerType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Thanks Arthur for your feedback.

  void myprintf(const char *fmt, int n) __attribute__((format(printf, 1, 2)));  
// N.B.: int, not unsigned long
  int main() {
  myprintf("%lu", 3uL);  // this should error
  myprintf("%d", 3uL);  // this should not error
  }

This is handled naturally by the current implementation. The integer literal 
undergoes an implicit cast to `int` because that's the type of the `n` 
parameter, and it causes the %lu case to fail and the %d case to succeed.

Your second example is the scenario of concern for adding an attribute like the 
`format_like` attribute that I described in my response to Aaron. I think that 
these two features don't need to be tied together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Thanks for looking, Aaron. You're right that the main utility of the 
aggregation of format warnings is to extend C's type checking because there is 
no other good way, or good place, to do it. I have built hundreds of millions 
of shipping lines of C, C++ and Objective-C, and this change seems like it 
would be an effective fix in several places where we don't currently have 
anywhere else to go.

For variadic templates, you're right that at some final instantiation, the 
compiler will have all the format argument types in hand. What gets lost in 
piping and substitution is actually the format string that Clang must 
type-check against. You can see this in action in the LLVM code base, actually: 
if we turn on -Wformat-nonliteral for files including 
llvm/include/Support/Format.h, the warning will trigger for users of the 
`llvm::format` function. This is because the format string is stored in 
`format_object_base::Fmt` instead of being directly forwarded, and sprinkling 
`constexpr` in strategic places won't resolve this issue because SemaChecking 
has its own custom expression evaluator. Swapping it out for the more common 
ExprConstant stuff is probably not impossible, but it's difficult because 
SemaChecking supports its own kind of symbolism. For instance, it's OK to use a 
format string parameter as the format string argument of another function that 
wants one, and other attributes like `format_arg` can tell you to assume the 
same specifiers as you get from another expression. So, we could fix this, but 
it would be more work, and the same purpose is served by allowing fixed 
arguments to participate in the `format` attribute checking. Verifying that 
users of the LLVM `llvm::format` function are doing the right thing is a better 
experience than letting this bubble up from however many levels of template 
instantiation there is.

Type checking may well actually need to be tested better for the case where 
variadic argument promotions don't happen, but there's a fairly finite number 
of ways it could go wrong, and I still think that's the easier way to go.

I don't think that we can easily distinguish between parameters created by 
variadic templates and fixed parameters from a function without variadic 
templates. I also think that most people who write functions like llvm::format 
aggressively do not want to be in charge of type-checking the format string 
themselves, and would much rather defer to Clang's existing type checker. If 
Clang allows the format attribute to type-check parameters created by variadic 
templates, it follows that the path of least resistance is to allow it on 
functions with fixed arguments.

With that said, there are still use cases for allowing format strings in 
functions with fixed arguments. (Interestingly, it would not be possible to mix 
fixed format arguments with variadic format arguments in a C function, so maybe 
we can prevent that altogether, but it does not remove from the general 
usefulness of the feature). You'll have to take my word for this, but it's one 
of the handful of blockers that we have for very broad adoption of 
-Wformat-nonliteral. The story usually goes like this: there's some function 
`NSString *foo(NSString *fmt, NSMyFoo *obj1, NSMyBar *obj2)` that needs to do 
something with `obj1` and `obj2` before it decides whether it actually wants to 
print them, return something unrelated altogether, throw an exception, etc. 
General limitations of the C language make it difficult to untangle the print 
parts from the logic parts for reasons which, to me, align rather closely to 
compiler capriciousness given how close it is to supporting the feature. This 
is more common in Objective-C code bases as %@ is a universal format specifier 
for all object types.

Another way we could approach this problem is to have a new attribute to say 
that a format argument must have a string that conforms to another constant 
format string. For instance, I think that `NSString *foo(NSString *fmt, NSMyFoo 
*obj1, NSMyBar *obj2) __attribute__((format_like(1, @"hello %@ %@!")))`, by the 
compiler checks that `fmt` has specifiers equivalent to %@ %@, would give us 
roughly the same safety improvements. It would allow for even more fun things, 
like passing a format string to a function that will then itself supply 
arguments to a formatting function.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6a5f7437720e: format_arg attribute should allow instancetype 
in NSString definition (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,14 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
++(instancetype)stringWithString:(NSString *)cstr 
__attribute__((format_arg(1)));
+@end
+
+@protocol MaybeString
+-(instancetype)maybeString:(const char *)cstr __attribute__((format_arg(1))); 
// expected-error {{function does not return string type}}
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,14 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
++(instancetype)stringWithString:(NSString *)cstr __attribute__((format_arg(1)));
+@end
+
+@protocol MaybeString
+-(instancetype)maybeString:(const char *)cstr __attribute__((format_arg(1))); // expected-error {{function does not return string type}}
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 383227.
fcloutier added a comment.

Add test for a protocol method with `format_arg`, second NSString method 
accepting a NSString instead of a C string


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,14 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
++(instancetype)stringWithString:(NSString *)cstr 
__attribute__((format_arg(1)));
+@end
+
+@protocol MaybeString
+-(instancetype)maybeString:(const char *)cstr __attribute__((format_arg(1))); 
// expected-error {{function does not return string type}}
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,14 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
++(instancetype)stringWithString:(NSString *)cstr __attribute__((format_arg(1)));
+@end
+
+@protocol MaybeString
+-(instancetype)maybeString:(const char *)cstr __attribute__((format_arg(1))); // expected-error {{function does not return string type}}
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 383222.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3388,6 +3388,12 @@
 return;
   }
   Ty = getFunctionOrMethodResultType(D);
+  // replace instancetype with the class type
+  if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl())
+if (auto *OMD = dyn_cast(D))
+  if (auto *Interface = OMD->getClassInterface())
+Ty = S.Context.getObjCObjectPointerType(
+QualType(Interface->getTypeForDecl(), 0));
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
   (!Ty->isPointerType() ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added a comment.

Apologies, Phabricator showed the comment below line 197 in the diff, but the 
email showed it to be below line 3404. I can check if the return type is 
`instancetype` in `handleFormatArgAttr` and use that instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3403
   }
   Ty = getFunctionOrMethodResultType(D);
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&

ahatanak wrote:
> Is it possible to just replace `Ty` with the class pointer type here if it is 
> an instancetype instead of defining another function (`isNSStringInterface`) 
> that looks at the class name and determines whether it's `NSString`?
I think that it would be awkward and I'd like to offer modest pushback. You 
need the enclosing `Decl` to make sense of `instancetype` because it's just a 
typedef to `id`. There are three uses of `isNSStringType` and only one could 
benefit from the `Decl` it because the other two don't refer to a return type. 
IMO, the split is the right way to go.

FWIW, it's not so much defining another function that looks at the class name 
so much as moving the look-at-the-class-name logic out of an existing function. 
I took the guts out of `isNSStringType` to make `isNSStringInterface`, but 
Phabricator isn't very good at showing code that moved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 383179.
fcloutier added a comment.

Forgot to run clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112670

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -156,24 +156,42 @@
   return false;
 }
 
+static inline bool isNSStringInterface(ASTContext , ObjCInterfaceDecl *Cls,
+   bool AllowNSAttributedString = false) {
+  if (!Cls)
+return false;
+
+  IdentifierInfo *ClsName = Cls->getIdentifier();
+
+  if (ClsName == ("NSAttributedString"))
+return AllowNSAttributedString;
+  // FIXME: Should we walk the chain of classes?
+  return ClsName == ("NSString") ||
+ ClsName == ("NSMutableString");
+}
+
 static inline bool isNSStringType(QualType T, ASTContext ,
   bool AllowNSAttributedString = false) {
   const auto *PT = T->getAs();
   if (!PT)
 return false;
 
-  ObjCInterfaceDecl *Cls = PT->getObjectType()->getInterface();
-  if (!Cls)
+  return isNSStringInterface(Ctx, PT->getObjectType()->getInterface(),
+ AllowNSAttributedString);
+}
+
+static inline bool
+isInstancetypeNSStringType(ASTContext , QualType T, Decl *D,
+   bool AllowNSAttributedString = false) {
+  if (T.getTypePtr() != Ctx.getObjCInstanceTypeDecl()->getTypeForDecl())
 return false;
 
-  IdentifierInfo* ClsName = Cls->getIdentifier();
+  auto *OMD = dyn_cast(D);
+  if (!OMD)
+return false;
 
-  if (AllowNSAttributedString &&
-  ClsName == ("NSAttributedString"))
-return true;
-  // FIXME: Should we walk the chain of classes?
-  return ClsName == ("NSString") ||
- ClsName == ("NSMutableString");
+  return isNSStringInterface(Ctx, OMD->getClassInterface(),
+ AllowNSAttributedString);
 }
 
 static inline bool isCFStringType(QualType T, ASTContext ) {
@@ -3390,6 +3408,7 @@
   Ty = getFunctionOrMethodResultType(D);
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
+  !isInstancetypeNSStringType(S.Context, Ty, D) &&
   (!Ty->isPointerType() ||
!Ty->castAs()->getPointeeType()->isCharType())) {
 S.Diag(AL.getLoc(), diag::err_format_attribute_result_not)


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -156,24 +156,42 @@
   return false;
 }
 
+static inline bool isNSStringInterface(ASTContext , ObjCInterfaceDecl *Cls,
+   bool AllowNSAttributedString = false) {
+  if (!Cls)
+return false;
+
+  IdentifierInfo *ClsName = Cls->getIdentifier();
+
+  if (ClsName == ("NSAttributedString"))
+return AllowNSAttributedString;
+  // FIXME: Should we walk the chain of classes?
+  return ClsName == ("NSString") ||
+ ClsName == ("NSMutableString");
+}
+
 static inline bool isNSStringType(QualType T, ASTContext ,
   bool AllowNSAttributedString = false) {
   const auto *PT = T->getAs();
   if (!PT)
 return false;
 
-  ObjCInterfaceDecl *Cls = PT->getObjectType()->getInterface();
-  if (!Cls)
+  return isNSStringInterface(Ctx, PT->getObjectType()->getInterface(),
+ AllowNSAttributedString);
+}
+
+static inline bool
+isInstancetypeNSStringType(ASTContext , QualType T, Decl *D,
+   bool AllowNSAttributedString = false) {
+  if (T.getTypePtr() != Ctx.getObjCInstanceTypeDecl()->getTypeForDecl())
 return false;
 
-  IdentifierInfo* ClsName = Cls->getIdentifier();
+  auto 

[PATCH] D112569: -Wformat-nonliteral should not trigger for format strings passed to blocks with __attribute__((format))

[Félix Cloutier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd378a0febc7e: [Sema] Recognize format argument indicated by 
format attribute inside blocks (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112569

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but 
the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 
'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string 
literal}}
+va_end(ap);
+  };
+
+  printf_arg2("foo", "%s string %i\n", "aaa", 123);
+  printf_arg2("%s string\n", "foo", "bar"); // expected-warning{{data argument 
not used by format string}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7784,11 +7784,11 @@
   // }
   if (HasVAListArg) {
 if (const ParmVarDecl *PV = dyn_cast(VD)) {
-  if (const NamedDecl *ND = dyn_cast(PV->getDeclContext())) 
{
+  if (const Decl *D = dyn_cast(PV->getDeclContext())) {
 int PVIndex = PV->getFunctionScopeIndex() + 1;
-for (const auto *PVFormat : ND->specific_attrs()) {
+for (const auto *PVFormat : D->specific_attrs()) {
   // adjust for implicit parameter
-  if (const CXXMethodDecl *MD = dyn_cast(ND))
+  if (const CXXMethodDecl *MD = dyn_cast(D))
 if (MD->isInstance())
   ++PVIndex;
   // We also check if the formats are compatible.


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
+va_end(ap);
+  };
+
+  printf_arg2("foo", "%s string %i\n", "aaa", 123);
+  printf_arg2("%s string\n", "foo", "bar"); // expected-warning{{data argument not used by format string}}
+}
Index: clang/lib/Sema/SemaChecking.cpp

[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: doug.gregor, dcoughlin, rsmith, NoQ, ahatanak.
fcloutier added a project: clang.
Herald added a reviewer: aaron.ballman.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

It has come to my attention that _inside the interface of `NSString_`, using 
`__attribute__((format_arg(__NSString__, N)))` does not work for a method that 
returns `instancetype`. This is a pretty specific problem because not a lot of 
people define `NSString`. Thankfully, the fix isn't very difficult: if checking 
an Objective-C method definition, and it returns `instancetype`, we also check 
if the class being defined is `NSString`.

rdar://84729746


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112670

Files:
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/SemaObjC/format-arg-attribute.m


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr 
__attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -156,26 +156,39 @@
   return false;
 }
 
-static inline bool isNSStringType(QualType T, ASTContext ,
-  bool AllowNSAttributedString = false) {
-  const auto *PT = T->getAs();
-  if (!PT)
-return false;
-
-  ObjCInterfaceDecl *Cls = PT->getObjectType()->getInterface();
+static inline bool isNSStringInterface(ASTContext , ObjCInterfaceDecl 
*Cls, bool AllowNSAttributedString = false) {
   if (!Cls)
 return false;
 
-  IdentifierInfo* ClsName = Cls->getIdentifier();
+  IdentifierInfo *ClsName = Cls->getIdentifier();
 
-  if (AllowNSAttributedString &&
-  ClsName == ("NSAttributedString"))
-return true;
+  if (ClsName == ("NSAttributedString"))
+return AllowNSAttributedString;
   // FIXME: Should we walk the chain of classes?
   return ClsName == ("NSString") ||
  ClsName == ("NSMutableString");
 }
 
+static inline bool isNSStringType(QualType T, ASTContext ,
+  bool AllowNSAttributedString = false) {
+  const auto *PT = T->getAs();
+  if (!PT)
+return false;
+  
+  return isNSStringInterface(Ctx, PT->getObjectType()->getInterface(), 
AllowNSAttributedString);
+}
+
+static inline bool isInstancetypeNSStringType(ASTContext , QualType T, 
Decl *D, bool AllowNSAttributedString = false) {
+  if (T.getTypePtr() != Ctx.getObjCInstanceTypeDecl()->getTypeForDecl())
+return false;
+  
+  auto *OMD = dyn_cast(D);
+  if (!OMD)
+return false;
+  
+  return isNSStringInterface(Ctx, OMD->getClassInterface(), 
AllowNSAttributedString);
+}
+
 static inline bool isCFStringType(QualType T, ASTContext ) {
   const auto *PT = T->getAs();
   if (!PT)
@@ -3390,6 +3403,7 @@
   Ty = getFunctionOrMethodResultType(D);
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
   !isCFStringType(Ty, S.Context) &&
+  !isInstancetypeNSStringType(S.Context, Ty, D) &&
   (!Ty->isPointerType() ||
!Ty->castAs()->getPointeeType()->isCharType())) {
 S.Diag(AL.getLoc(), diag::err_format_attribute_result_not)


Index: clang/test/SemaObjC/format-arg-attribute.m
===
--- clang/test/SemaObjC/format-arg-attribute.m
+++ clang/test/SemaObjC/format-arg-attribute.m
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -156,26 +156,39 @@
   return false;
 }
 
-static inline bool isNSStringType(QualType T, ASTContext ,
-  bool AllowNSAttributedString = false) {
-  const auto *PT = T->getAs();
-  if (!PT)
-return false;
-
-  ObjCInterfaceDecl *Cls = PT->getObjectType()->getInterface();
+static inline bool isNSStringInterface(ASTContext , ObjCInterfaceDecl *Cls, bool AllowNSAttributedString = false) {
   if (!Cls)
 return false;
 
-  IdentifierInfo* ClsName = Cls->getIdentifier();
+  IdentifierInfo *ClsName = Cls->getIdentifier();
 
-  if (AllowNSAttributedString &&
-  ClsName == ("NSAttributedString"))
-return true;
+  if (ClsName == 

[PATCH] D112569: -Wformat-nonliteral should not trigger for format strings passed to blocks with __attribute__((format))

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 382788.
fcloutier added a reviewer: ahatanak.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112569

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but 
the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 
'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string 
literal}}
+va_end(ap);
+  };
+
+  printf_arg2("foo", "%s string %i\n", "aaa", 123);
+  printf_arg2("%s string\n", "foo", "bar"); // expected-warning{{data argument 
not used by format string}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7780,11 +7780,11 @@
   // }
   if (HasVAListArg) {
 if (const ParmVarDecl *PV = dyn_cast(VD)) {
-  if (const NamedDecl *ND = dyn_cast(PV->getDeclContext())) 
{
+  if (const Decl *D = dyn_cast(PV->getDeclContext())) {
 int PVIndex = PV->getFunctionScopeIndex() + 1;
-for (const auto *PVFormat : ND->specific_attrs()) {
+for (const auto *PVFormat : D->specific_attrs()) {
   // adjust for implicit parameter
-  if (const CXXMethodDecl *MD = dyn_cast(ND))
+  if (const CXXMethodDecl *MD = dyn_cast(D))
 if (MD->isInstance())
   ++PVIndex;
   // We also check if the formats are compatible.


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}
+va_end(ap);
+  };
+
+  printf_arg2("foo", "%s string %i\n", "aaa", 123);
+  printf_arg2("%s string\n", "foo", "bar"); // expected-warning{{data argument not used by format string}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7780,11 

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

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: dcoughlin, doug.gregor, rsmith.
fcloutier added a project: clang.
Herald added a reviewer: aaron.ballman.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

Clang only allows you to use `__attribute__((format))` on variadic functions. 
There are legit use cases for `__attribute__((format))` on non-variadic 
functions, such as:

(1) variadic templates

  template
  void print(const char *fmt, Args… &) __attribute__((format(1, 2))); // 
error: format attribute requires variadic function

(2) functions which take fixed arguments and a custom format:

  void print_number_string(const char *fmt, unsigned number, const char 
*string) __attribute__((format(1, 2)));
  // ^error: format attribute requires variadic function
  
  void foo(void) {
  print_number_string(“%08x %s\n”, 0xdeadbeef, “hello”);
  print_number_string(“%d %s”, 0xcafebabe, “bar”);
  }

This change allows Clang users to attach `__attribute__((format))` to 
non-variadic functions, including functions with C++ variadic templates. It 
replaces the error with a GCC compatibility warning and improves the type 
checker to ensure that received arrays are treated like pointers (this is a 
possibility in C++ since references to template types can bind to arrays).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112579

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Sema/attr-format.c
  clang/test/Sema/attr-format.cpp

Index: clang/test/Sema/attr-format.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-format.cpp
@@ -0,0 +1,16 @@
+//RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#include 
+
+template
+void format(const char *fmt, Args &&... args) // expected-warning{{GCC requires functions with 'format' attribute to be variadic}}
+__attribute__((format(printf, 1, 2)));
+
+void do_format() {
+  int x = 123;
+  int  = x;
+  const char *s = "world";
+  format("bare string");
+  format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, _format);
+  format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
+}
Index: clang/test/Sema/attr-format.c
===
--- clang/test/Sema/attr-format.c
+++ clang/test/Sema/attr-format.c
@@ -5,7 +5,7 @@
 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-error {{format attribute requires variadic function}}
+void d(const char *a, int c) __attribute__((format(printf, 1, 2))); // expected-warning {{GCC requires functions with 'format' attribute to be variadic}}
 void e(char *str, int c, ...) __attribute__((format(printf, 2,3))); // expected-error {{format argument not a string type}}
 
 typedef const char* xpto;
@@ -39,7 +39,7 @@
 void a2(const char *a, ...)__attribute__((format(printf0, 1,2))); // no-error
 void b2(const char *a, ...)__attribute__((format(printf0, 1,1))); // expected-error {{'format' attribute parameter 3 is out of bounds}}
 void c2(const char *a, ...)__attribute__((format(printf0, 0,2))); // expected-error {{'format' attribute parameter 2 is out of bounds}}
-void d2(const char *a, int c)  __attribute__((format(printf0, 1,2))); // expected-error {{format attribute requires variadic function}}
+void d2(const char *a, int c)  __attribute__((format(printf0, 1,2))); // expected-warning {{GCC requires functions with 'format' attribute to be variadic}}
 void e2(char *str, int c, ...) __attribute__((format(printf0, 2,3))); // expected-error {{format argument not a string type}}
 
 // FreeBSD usage
@@ -61,7 +61,7 @@
 void a3(const char *a, ...)__attribute__((format(freebsd_kprintf, 1,2))); // no-error
 void b3(const char *a, ...)__attribute__((format(freebsd_kprintf, 1,1))); // expected-error {{'format' attribute parameter 3 is out of bounds}}
 void c3(const char *a, ...)__attribute__((format(freebsd_kprintf, 0,2))); // expected-error {{'format' attribute parameter 2 is out of bounds}}
-void d3(const char *a, int c)  __attribute__((format(freebsd_kprintf, 1,2))); // expected-error {{format attribute requires variadic function}}
+void d3(const char *a, int c)  __attribute__((format(freebsd_kprintf, 1,2))); // expected-warning {{GCC requires functions with 'format' attribute to be variadic}}
 void e3(char *str, int c, ...) __attribute__((format(freebsd_kprintf, 2,3))); // expected-error {{format argument not a string type}}
 
 
@@ -87,3 +87,9 @@
   __attribute__ 

[PATCH] D112569: -Wformat-nonliteral should not trigger for format strings passed to blocks with __attribute__((format))

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 382461.
fcloutier added a comment.

Thanks Artem for pointing out that I was completely misusing 
`getFunctionScopeIndex`. This should be better. I added a test that you can 
pick a non-1 value for the format parameter in blocks.


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

https://reviews.llvm.org/D112569

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but 
the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 
'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string 
literal}}
+va_end(ap);
+  };
+
+  printf_arg2("foo", "%s string %i\n", "aaa", 123);
+  printf_arg2("%s string\n", "foo", "bar"); // expected-warning{{data argument 
not used by format string}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7780,13 +7780,13 @@
   // }
   if (HasVAListArg) {
 if (const ParmVarDecl *PV = dyn_cast(VD)) {
-  if (const NamedDecl *ND = dyn_cast(PV->getDeclContext())) 
{
+  if (const Decl *D = dyn_cast(PV->getDeclContext())) {
 int PVIndex = PV->getFunctionScopeIndex() + 1;
-for (const auto *PVFormat : ND->specific_attrs()) {
-  // adjust for implicit parameter
-  if (const CXXMethodDecl *MD = dyn_cast(ND))
-if (MD->isInstance())
-  ++PVIndex;
+// adjust for implicit parameter
+const CXXMethodDecl *MD = dyn_cast(D);
+if (MD && MD->isInstance())
+  ++PVIndex;
+for (const auto *PVFormat : D->specific_attrs()) {
   // We also check if the formats are compatible.
   // We can't pass a 'scanf' string to a 'printf' function.
   if (PVIndex == PVFormat->getFormatIdx() &&


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,30 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_arg1)(
+  const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_arg1("%s string %i\n", "aaa", 123);
+  printf_arg1("%s string\n", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+
+  void __attribute__((__format__(__printf__, 2, 3))) (^printf_arg2)(
+  const char *, const char *, ...) =
+  ^(const char *not_fmt, const char *fmt, ...)
+  __attribute__((__format__(__printf__, 2, 3))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(not_fmt, ap); // expected-warning{{format string is not a string literal}}

[PATCH] D112569: -Wformat-nonliteral should not trigger for format strings passed to blocks with __attribute__((format))

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: doug.gregor, dcoughlin, rsmith.
fcloutier added a project: clang.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

The checker that implements `-Wformat-nonliteral` does not understand 
`__attribute__((format))` on blocks in the same way that it understands it on 
functions. This works just fine (assuming `#define __printflike(A, B) 
__attribute__((format(printf, A, B)))`):

  void printfblock(const char *fmt, ...) __printflike(1, 2) {
va_list ap;
va_start(ap, fmt);
vprintf(fmt, ap);
va_end(ap);
  }
  
  void foo(void) {
printfblock("%s %i\n", "hello", 1);
  }

However, this incorrectly triggers `-Wformat-nonliteral`:

  void foo(void) {
void (^printfblock)(const char *fmt, ...) __printflike(1, 2) = ^(const 
char *fmt, ...) __printflike(1, 2) {
va_list ap;
va_start(ap, fmt);
vprintf(fmt, ap); // warning: format string is not a string 
literal [-Wformat-nonliteral]
va_end(ap);
};

printfblock("%s %i\n", "hello", 1);
  }

This patch updates `checkFormatStringExpr` so that it can look through 
`BlockDecl`s and find out which parameter is identified as a format string.

rdar://84603673


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112569

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem 
%S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,16 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but 
the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_block)(const 
char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_block("%s string %i\n", "aaa", 123);
+  printf_block("%s string\n", 123); // expected-warning{{format specifies type 
'char *' but the argument has type 'int'}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7780,13 +7780,16 @@
   // }
   if (HasVAListArg) {
 if (const ParmVarDecl *PV = dyn_cast(VD)) {
-  if (const NamedDecl *ND = dyn_cast(PV->getDeclContext())) 
{
-int PVIndex = PV->getFunctionScopeIndex() + 1;
-for (const auto *PVFormat : ND->specific_attrs()) {
+  if (const Decl *D = dyn_cast(PV->getDeclContext())) {
+int PVIndex = 1;
+if (const NamedDecl *ND = dyn_cast(D)) {
+  PVIndex += PV->getFunctionScopeIndex();
   // adjust for implicit parameter
   if (const CXXMethodDecl *MD = dyn_cast(ND))
 if (MD->isInstance())
   ++PVIndex;
+}
+for (const auto *PVFormat : D->specific_attrs()) {
   // We also check if the formats are compatible.
   // We can't pass a 'scanf' string to a 'printf' function.
   if (PVIndex == PVFormat->getFormatIdx() &&


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fblocks -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
 
 #include 
 #include 
@@ -714,3 +714,16 @@
 void test_printf_opaque_ptr(void *op) {
   printf("%s", op); // expected-warning{{format specifies type 'char *' but the argument has type 'void *'}}
 }
+
+void test_block() {
+  void __attribute__((__format__(__printf__, 1, 2))) (^printf_block)(const char *, ...) =
+  ^(const char *fmt, ...) __attribute__((__format__(__printf__, 1, 2))) {
+va_list ap;
+va_start(ap, fmt);
+vprintf(fmt, ap);
+va_end(ap);
+  };
+
+  printf_block("%s string %i\n", "aaa", 123);
+  

[PATCH] D96196: [Sema][NFC] Create Sema::BuildImplicitCast

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
Herald added a subscriber: jfb.
Herald added a reviewer: aaron.ballman.
fcloutier requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

This change creates Sema::BuildImplicitCast, which the rest of Sema now uses to
create implicit casts. The main change in terms of interface is that now,
technically, creating an implicit cast could fail. However, the current
implementation of BuildImplicitCast never does because it simply calls
ImplicitCastExpr::Create.

Nevertheless, Sema users of ImplicitCastExpr::Create, and their downstream users
in several cases, were updated to allow failing, except in some cases (such as
in OpenMP) where existing code already didn't bother to check implicit casts
failing.

rdar://74050758


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96196

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaExprObjC.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp

Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -7236,8 +7236,11 @@
 FoundResult)) {
 if (DiagnoseUseOfDecl(Fn, Arg->getBeginLoc()))
   return ExprError();
-
-Arg = FixOverloadedFunctionReference(Arg, FoundResult, Fn);
+ExprResult OverloadArg =
+FixOverloadedFunctionReference(Arg, FoundResult, Fn);
+if (OverloadArg.isInvalid())
+  return ExprError();
+Arg = OverloadArg.get();
 ArgType = Arg->getType();
   } else
 return ExprError();
@@ -7290,7 +7293,11 @@
 if (DiagnoseUseOfDecl(Fn, Arg->getBeginLoc()))
   return ExprError();
 
-Arg = FixOverloadedFunctionReference(Arg, FoundResult, Fn);
+ExprResult OverloadArg =
+FixOverloadedFunctionReference(Arg, FoundResult, Fn);
+if (OverloadArg.isInvalid())
+  return ExprError();
+Arg = OverloadArg.get();
 ArgType = Arg->getType();
   } else
 return ExprError();
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3187,13 +3187,16 @@
 NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
-Value =
-ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp, Value,
- nullptr, VK_XValue, FPOptionsOverride());
-
+ExprResult AsXValue = S.BuildImplicitCastExpr(
+Value, Value->getType(), CK_NoOp, VK_XValue, FPOptionsOverride());
 // Complete type-checking the initialization of the return type
 // using the constructor we found.
-Res = Seq.Perform(S, Entity, Kind, Value);
+if (AsXValue.isInvalid())
+  Res = ExprError();
+else {
+  Expr *E = AsXValue.get();
+  Res = Seq.Perform(S, Entity, Kind, E);
+}
   }
 
   return NeedSecondOverloadResolution;
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -1754,8 +1754,9 @@
   // FIXME: FixOverloadedFunctionReference has side-effects; we shouldn't
   // be calling it from within an NDEBUG block.
   assert(S.Context.hasSameType(
-FromType,
-S.FixOverloadedFunctionReference(From, AccessPair, Fn)->getType()));
+  FromType, S.FixOverloadedFunctionReference(From, AccessPair, Fn)
+.get()
+->getType()));
 } else {
   return false;
 }
@@ -5935,10 +5936,12 @@
 if (Result.isInvalid())
   return true;
 // Record usage of conversion in an implicit cast.
-From = ImplicitCastExpr::Create(SemaRef.Context, Result.get()->getType(),
-CK_UserDefinedConversion, Result.get(),
-nullptr, Result.get()->getValueKind(),
-SemaRef.CurFPFeatureOverrides());
+Result = SemaRef.BuildImplicitCastExpr(
+Result.get(), Result.get()->getType(), CK_UserDefinedConversion,
+Result.get()->getValueKind(), SemaRef.CurFPFeatureOverrides());
+if (Result.isInvalid())
+  return true;
+From = Result.get();
   }
   return 

[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG554cf3729e65: [clang-tblgen] AnnotateAttr::printPretty has 
spurious comma when no variadic… (authored by fcloutier).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95695

Files:
  clang/test/AST/ast-print-attr.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -773,10 +773,8 @@
 
 void writeValue(raw_ostream ) const override {
   OS << "\";\n";
-  OS << "  bool isFirst = true;\n"
- << "  for (const auto  : " << RangeName << "()) {\n"
- << "if (isFirst) isFirst = false;\n"
- << "else OS << \", \";\n";
+  OS << "  for (const auto  : " << RangeName << "()) {\n"
+ << "DelimitAttributeArgument(OS, IsFirstArgument);\n";
   writeValueImpl(OS);
   OS << "  }\n";
   OS << "  OS << \"";
@@ -1428,10 +1426,12 @@
 return;
   }
 
-  OS << "  switch (getAttributeSpellingListIndex()) {\n"
-"  default:\n"
-"llvm_unreachable(\"Unknown attribute spelling!\");\n"
-"break;\n";
+  OS << "  bool IsFirstArgument = true; (void)IsFirstArgument;\n"
+ << "  unsigned TrailingOmittedArgs = 0; (void)TrailingOmittedArgs;\n"
+ << "  switch (getAttributeSpellingListIndex()) {\n"
+ << "  default:\n"
+ << "llvm_unreachable(\"Unknown attribute spelling!\");\n"
+ << "break;\n";
 
   for (unsigned I = 0; I < Spellings.size(); ++ I) {
 llvm::SmallString<16> Prefix;
@@ -1476,12 +1476,10 @@
 
 Spelling += Name;
 
-OS <<
-  "  case " << I << " : {\n"
-  "OS << \"" << Prefix << Spelling;
+OS << "  case " << I << " : {\n"
+   << "OS << \"" << Prefix << Spelling << "\";\n";
 
 if (Variety == "Pragma") {
-  OS << "\";\n";
   OS << "printPrettyPragma(OS, Policy);\n";
   OS << "OS << \"\\n\";";
   OS << "break;\n";
@@ -1490,19 +1488,18 @@
 }
 
 if (Spelling == "availability") {
-  OS << "(";
+  OS << "OS << \"(";
   writeAvailabilityValue(OS);
-  OS << ")";
+  OS << ")\";\n";
 } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
-  OS << "(";
+  OS << "OS << \"(";
   writeDeprecatedAttrValue(OS, Variety);
-  OS << ")";
+  OS << ")\";\n";
 } else {
   // To avoid printing parentheses around an empty argument list or
   // printing spurious commas at the end of an argument list, we need to
   // determine where the last provided non-fake argument is.
   unsigned NonFakeArgs = 0;
-  unsigned TrailingOptArgs = 0;
   bool FoundNonOptArg = false;
   for (const auto  : llvm::reverse(Args)) {
 if (arg->isFake())
@@ -1516,61 +1513,33 @@
   FoundNonOptArg = true;
   continue;
 }
-if (!TrailingOptArgs++)
-  OS << "\";\n"
- << "unsigned TrailingOmittedArgs = 0;\n";
 OS << "if (" << arg->getIsOmitted() << ")\n"
<< "  ++TrailingOmittedArgs;\n";
   }
-  if (TrailingOptArgs)
-OS << "OS << \"";
-  if (TrailingOptArgs < NonFakeArgs)
-OS << "(";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \"(\";\n"
-   << "OS << \"";
   unsigned ArgIndex = 0;
   for (const auto  : Args) {
 if (arg->isFake())
   continue;
-if (ArgIndex) {
-  if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
-OS << "\";\n"
-   << "if (" << ArgIndex << " < " << NonFakeArgs
-   << " - TrailingOmittedArgs)\n"
-   << "  OS << \", \";\n"
-   << "OS << \"";
-  else
-OS << ", ";
-}
 std::string IsOmitted = arg->getIsOmitted();
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "if (!(" << IsOmitted << ")) {\n"
- << "  OS << \"";
+  OS << "if (!(" << IsOmitted << ")) {\n";
+// Variadic arguments print their own leading comma.
+if (!arg->isVariadic())
+  OS << "DelimitAttributeArgument(OS, IsFirstArgument);\n";
+OS << "OS << \"";
 arg->writeValue(OS);
+OS << "\";\n";
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "}\n"
- << "OS << \"";
+  OS << "}\n";
 ++ArgIndex;
   }
-  if (TrailingOptArgs < NonFakeArgs)
-OS << ")";
-  else if 

[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier added inline comments.



Comment at: clang/test/AST/ast-print-attr.c:31
+// CHECK: int fun_annotate() __attribute__((annotate("annotation")))
+int fun_annotate() __attribute__((annotate("annotation")));

aaron.ballman wrote:
> Can you add a second test that shows we properly print the comma? e.g., `int 
> fun_annotate2() __attribute__((annotate("annotation one", "annotation 
> two")));`
I switched this to use `ownership_holds` and `ownership_returns` because 
`annotate` has a `VariadicExprArgument` and it doesn't know how to print 
expressions (it just prints a pointer value). `ownership_holds` and 
`ownership_returns` are the same attribute, and they have the same 
bug-triggering configuration (a fixed argument followed by a possibly-empty 
list):

```c++
int *fun_returns() __attribute__((ownership_returns(fun_returns)));
void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
```

In the `ownership_returns` case, without my change, Clang prints 
`__attribute__((ownership_returns(foo, )));`.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2252
+  // Helper to print the starting character of an attribute argument. If there
+  // hasn't been an argument yet, it prints an opening parenthese; otherwise it
+  // prints a comma.

aaron.ballman wrote:
> One downside to printing the opening paren is that this can't be used in a 
> generic way for generating any comma-separate list. That said, I think this 
> functionality is clean -- perhaps renaming the function from `Comma` to 
> `PrintAttributeArgListElement` or something would be an improvement?
I went with `DelimitAttributeArgument`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 320904.
fcloutier added a comment.

Address Aaron's feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95695

Files:
  clang/test/AST/ast-print-attr.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -773,10 +773,8 @@
 
 void writeValue(raw_ostream ) const override {
   OS << "\";\n";
-  OS << "  bool isFirst = true;\n"
- << "  for (const auto  : " << RangeName << "()) {\n"
- << "if (isFirst) isFirst = false;\n"
- << "else OS << \", \";\n";
+  OS << "  for (const auto  : " << RangeName << "()) {\n"
+ << "DelimitAttributeArgument(OS, IsFirstArgument);\n";
   writeValueImpl(OS);
   OS << "  }\n";
   OS << "  OS << \"";
@@ -1428,10 +1426,12 @@
 return;
   }
 
-  OS << "  switch (getAttributeSpellingListIndex()) {\n"
-"  default:\n"
-"llvm_unreachable(\"Unknown attribute spelling!\");\n"
-"break;\n";
+  OS << "  bool IsFirstArgument = true; (void)IsFirstArgument;\n"
+ << "  unsigned TrailingOmittedArgs = 0; (void)TrailingOmittedArgs;\n"
+ << "  switch (getAttributeSpellingListIndex()) {\n"
+ << "  default:\n"
+ << "llvm_unreachable(\"Unknown attribute spelling!\");\n"
+ << "break;\n";
 
   for (unsigned I = 0; I < Spellings.size(); ++ I) {
 llvm::SmallString<16> Prefix;
@@ -1476,12 +1476,10 @@
 
 Spelling += Name;
 
-OS <<
-  "  case " << I << " : {\n"
-  "OS << \"" << Prefix << Spelling;
+OS << "  case " << I << " : {\n"
+   << "OS << \"" << Prefix << Spelling << "\";\n";
 
 if (Variety == "Pragma") {
-  OS << "\";\n";
   OS << "printPrettyPragma(OS, Policy);\n";
   OS << "OS << \"\\n\";";
   OS << "break;\n";
@@ -1490,19 +1488,18 @@
 }
 
 if (Spelling == "availability") {
-  OS << "(";
+  OS << "OS << \"(";
   writeAvailabilityValue(OS);
-  OS << ")";
+  OS << ")\";\n";
 } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
-  OS << "(";
+  OS << "OS << \"(";
   writeDeprecatedAttrValue(OS, Variety);
-  OS << ")";
+  OS << ")\";\n";
 } else {
   // To avoid printing parentheses around an empty argument list or
   // printing spurious commas at the end of an argument list, we need to
   // determine where the last provided non-fake argument is.
   unsigned NonFakeArgs = 0;
-  unsigned TrailingOptArgs = 0;
   bool FoundNonOptArg = false;
   for (const auto  : llvm::reverse(Args)) {
 if (arg->isFake())
@@ -1516,61 +1513,33 @@
   FoundNonOptArg = true;
   continue;
 }
-if (!TrailingOptArgs++)
-  OS << "\";\n"
- << "unsigned TrailingOmittedArgs = 0;\n";
 OS << "if (" << arg->getIsOmitted() << ")\n"
<< "  ++TrailingOmittedArgs;\n";
   }
-  if (TrailingOptArgs)
-OS << "OS << \"";
-  if (TrailingOptArgs < NonFakeArgs)
-OS << "(";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \"(\";\n"
-   << "OS << \"";
   unsigned ArgIndex = 0;
   for (const auto  : Args) {
 if (arg->isFake())
   continue;
-if (ArgIndex) {
-  if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
-OS << "\";\n"
-   << "if (" << ArgIndex << " < " << NonFakeArgs
-   << " - TrailingOmittedArgs)\n"
-   << "  OS << \", \";\n"
-   << "OS << \"";
-  else
-OS << ", ";
-}
 std::string IsOmitted = arg->getIsOmitted();
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "if (!(" << IsOmitted << ")) {\n"
- << "  OS << \"";
+  OS << "if (!(" << IsOmitted << ")) {\n";
+// Variadic arguments print their own leading comma.
+if (!arg->isVariadic())
+  OS << "DelimitAttributeArgument(OS, IsFirstArgument);\n";
+OS << "OS << \"";
 arg->writeValue(OS);
+OS << "\";\n";
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "}\n"
- << "OS << \"";
+  OS << "}\n";
 ++ArgIndex;
   }
-  if (TrailingOptArgs < NonFakeArgs)
-OS << ")";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \")\";\n"
-   << "OS << 

[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier updated this revision to Diff 320204.
fcloutier added a comment.

Updating the diff using arcanist, which I'm told produces better results. Sorry 
for the churn!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95695

Files:
  clang/test/AST/ast-print-attr.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -773,10 +773,8 @@
 
 void writeValue(raw_ostream ) const override {
   OS << "\";\n";
-  OS << "  bool isFirst = true;\n"
- << "  for (const auto  : " << RangeName << "()) {\n"
- << "if (isFirst) isFirst = false;\n"
- << "else OS << \", \";\n";
+  OS << "  for (const auto  : " << RangeName << "()) {\n"
+ << "Comma(OS, IsFirstArgument);\n";
   writeValueImpl(OS);
   OS << "  }\n";
   OS << "  OS << \"";
@@ -1428,10 +1426,12 @@
 return;
   }
 
-  OS << "  switch (getAttributeSpellingListIndex()) {\n"
-"  default:\n"
-"llvm_unreachable(\"Unknown attribute spelling!\");\n"
-"break;\n";
+  OS << "  bool IsFirstArgument = true; (void)IsFirstArgument;\n"
+ << "  unsigned TrailingOmittedArgs = 0; (void)TrailingOmittedArgs;\n"
+ << "  switch (getAttributeSpellingListIndex()) {\n"
+ << "  default:\n"
+ << "llvm_unreachable(\"Unknown attribute spelling!\");\n"
+ << "break;\n";
 
   for (unsigned I = 0; I < Spellings.size(); ++ I) {
 llvm::SmallString<16> Prefix;
@@ -1476,12 +1476,10 @@
 
 Spelling += Name;
 
-OS <<
-  "  case " << I << " : {\n"
-  "OS << \"" << Prefix << Spelling;
+OS << "  case " << I << " : {\n"
+   << "OS << \"" << Prefix << Spelling << "\";\n";
 
 if (Variety == "Pragma") {
-  OS << "\";\n";
   OS << "printPrettyPragma(OS, Policy);\n";
   OS << "OS << \"\\n\";";
   OS << "break;\n";
@@ -1490,19 +1488,18 @@
 }
 
 if (Spelling == "availability") {
-  OS << "(";
+  OS << "OS << \"(";
   writeAvailabilityValue(OS);
-  OS << ")";
+  OS << ")\";\n";
 } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
-  OS << "(";
+  OS << "OS << \"(";
   writeDeprecatedAttrValue(OS, Variety);
-  OS << ")";
+  OS << ")\";\n";
 } else {
   // To avoid printing parentheses around an empty argument list or
   // printing spurious commas at the end of an argument list, we need to
   // determine where the last provided non-fake argument is.
   unsigned NonFakeArgs = 0;
-  unsigned TrailingOptArgs = 0;
   bool FoundNonOptArg = false;
   for (const auto  : llvm::reverse(Args)) {
 if (arg->isFake())
@@ -1516,61 +1513,33 @@
   FoundNonOptArg = true;
   continue;
 }
-if (!TrailingOptArgs++)
-  OS << "\";\n"
- << "unsigned TrailingOmittedArgs = 0;\n";
 OS << "if (" << arg->getIsOmitted() << ")\n"
<< "  ++TrailingOmittedArgs;\n";
   }
-  if (TrailingOptArgs)
-OS << "OS << \"";
-  if (TrailingOptArgs < NonFakeArgs)
-OS << "(";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \"(\";\n"
-   << "OS << \"";
   unsigned ArgIndex = 0;
   for (const auto  : Args) {
 if (arg->isFake())
   continue;
-if (ArgIndex) {
-  if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
-OS << "\";\n"
-   << "if (" << ArgIndex << " < " << NonFakeArgs
-   << " - TrailingOmittedArgs)\n"
-   << "  OS << \", \";\n"
-   << "OS << \"";
-  else
-OS << ", ";
-}
 std::string IsOmitted = arg->getIsOmitted();
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "if (!(" << IsOmitted << ")) {\n"
- << "  OS << \"";
+  OS << "if (!(" << IsOmitted << ")) {\n";
+// Variadic arguments print their own leading comma.
+if (!arg->isVariadic())
+  OS << "Comma(OS, IsFirstArgument);\n";
+OS << "OS << \"";
 arg->writeValue(OS);
+OS << "\";\n";
 if (arg->isOptional() && IsOmitted != "false")
-  OS << "\";\n"
- << "}\n"
- << "OS << \"";
+  OS << "}\n";
 ++ArgIndex;
   }
-  if (TrailingOptArgs < NonFakeArgs)
-OS << ")";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << 

[PATCH] D95695: [clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[Félix Cloutier via Phabricator via cfe-commits]

fcloutier created this revision.
fcloutier added reviewers: jkorous, dcoughlin.
fcloutier added a project: clang.
Herald added a subscriber: Charusso.
Herald added a reviewer: aaron.ballman.
fcloutier requested review of this revision.
Herald added a subscriber: cfe-commits.

Since it gained a new `VariadicExprArgument`, `AnnotateAttr`'s `printPretty` no 
longer prints back compilable code when the attribute is only passed a string. 
This is because the comma-printing logic unconditionally prints a comma between 
the first, fixed argument and the `VariadicExprArgument`, which is most likely 
an empty collection.

This diff adds a `Comma` helper to AttrImpl.inc that prints a comma before an 
argument if it isn't the first argument. In the process, it simplifies 
substantially the generation code, and arguably the generated code, too.

rdar://73742471


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95695

Files:
  clang/test/AST/ast-print-attr.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -776,4 +776,2 @@
-  OS << "  bool isFirst = true;\n"
- << "  for (const auto  : " << RangeName << "()) {\n"
- << "if (isFirst) isFirst = false;\n"
- << "else OS << \", \";\n";
+  OS << "  for (const auto  : " << RangeName << "()) {\n"
+ << "Comma(OS, IsFirstArgument);\n";
@@ -1431,4 +1429,6 @@
-  OS << "  switch (getAttributeSpellingListIndex()) {\n"
-"  default:\n"
-"llvm_unreachable(\"Unknown attribute spelling!\");\n"
-"break;\n";
+  OS << "  bool IsFirstArgument = true; (void)IsFirstArgument;\n"
+ << "  unsigned TrailingOmittedArgs = 0; (void)TrailingOmittedArgs;\n"
+ << "  switch (getAttributeSpellingListIndex()) {\n"
+ << "  default:\n"
+ << "llvm_unreachable(\"Unknown attribute spelling!\");\n"
+ << "break;\n";
@@ -1479,3 +1479,2 @@
-OS <<
-  "  case " << I << " : {\n"
-  "OS << \"" << Prefix << Spelling;
+OS << "  case " << I << " : {\n"
+   << "OS << \"" << Prefix << Spelling << "\";\n";
@@ -1484 +1482,0 @@
-  OS << "\";\n";
@@ -1493 +1491 @@
-  OS << "(";
+  OS << "OS << \"(";
@@ -1495 +1493 @@
-  OS << ")";
+  OS << ")\";\n";
@@ -1497 +1495 @@
-  OS << "(";
+  OS << "OS << \"(";
@@ -1499 +1497 @@
-  OS << ")";
+  OS << ")\";\n";
@@ -1505 +1502,0 @@
-  unsigned TrailingOptArgs = 0;
@@ -1519,3 +1515,0 @@
-if (!TrailingOptArgs++)
-  OS << "\";\n"
- << "unsigned TrailingOmittedArgs = 0;\n";
@@ -1525,9 +1518,0 @@
-  if (TrailingOptArgs)
-OS << "OS << \"";
-  if (TrailingOptArgs < NonFakeArgs)
-OS << "(";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \"(\";\n"
-   << "OS << \"";
@@ -1538,10 +1522,0 @@
-if (ArgIndex) {
-  if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
-OS << "\";\n"
-   << "if (" << ArgIndex << " < " << NonFakeArgs
-   << " - TrailingOmittedArgs)\n"
-   << "  OS << \", \";\n"
-   << "OS << \"";
-  else
-OS << ", ";
-}
@@ -1550,3 +1525,5 @@
-  OS << "\";\n"
- << "if (!(" << IsOmitted << ")) {\n"
- << "  OS << \"";
+  OS << "if (!(" << IsOmitted << ")) {\n";
+// Variadic arguments print their own leading comma.
+if (!arg->isVariadic())
+  OS << "Comma(OS, IsFirstArgument);\n";
+OS << "OS << \"";
@@ -1553,0 +1531 @@
+OS << "\";\n";
@@ -1555,3 +1533 @@
-  OS << "\";\n"
- << "}\n"
- << "OS << \"";
+  OS << "}\n";
@@ -1560,7 +1536,3 @@
-  if (TrailingOptArgs < NonFakeArgs)
-OS << ")";
-  else if (TrailingOptArgs)
-OS << "\";\n"
-   << "if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-   << "   OS << \")\";\n"
-   << "OS << \"";
+  if (ArgIndex != 0)
+OS << "if (!IsFirstArgument)\n"
+   << "  OS << \")\";\n";
@@ -1568,6 +1540,3 @@
-
-OS << Suffix + "\";\n";
-
-OS <<
-  "break;\n"
-  "  }\n";
+OS << "OS << \"" << Suffix << "\";\n"
+   << "break;\n"
+   << "  }\n";
@@ -2281,0 +2251,11 @@
+  // Helper to print the starting character of an attribute argument. If there
+  // hasn't been an argument yet, it prints an opening parenthese; otherwise it
+  // prints a comma.
+  OS << "static inline void Comma(raw_ostream& OS, bool& IsFirst) {\n"
+ << "  if (IsFirst) {\n"
+ << "IsFirst = false;\n"
+ << "OS <<