[PATCH] D132568: [clang][Sema] check default argument promotions for printf

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D132568#3746551 <https://reviews.llvm.org/D132568#3746551>, @aaron.ballman wrote:

> Thank you for the patch, but it'd be really helpful to me as a reviewer if you and @nickdesaulniers could coordinate so there's only one patch trying to address #57102 instead of two competing patches (I'm happy to review whichever patch comes out). From a quick look over the test case changes, this patch looks like it's more in line with how I'd expect to resolve that issue compared to D132266 <https://reviews.llvm.org/D132266>.

The addition to clang/test/Sema/format-strings.c looks like it will resolve Linus' complaints in https://github.com/llvm/llvm-project/issues/57102; I'm happy to abandon D132266 <https://reviews.llvm.org/D132266> in preference of this.

> However, the usage of %hhd corresponding to short is relatively rare, and it is more likely to be a misuse.

Do we want to encode that in `test_promotion` in `clang/test/Sema/format-strings.c`? Seems like tests on shorts are missing.



================
Comment at: clang/lib/AST/FormatString.cpp:359
 
-      if (const BuiltinType *BT = argTy->getAs<BuiltinType>())
+      if (const BuiltinType *BT = argTy->getAs<BuiltinType>()) {
         switch (BT->getKind()) {
----------------
might as well make this `auto` while you're here.


================
Comment at: clang/lib/AST/FormatString.cpp:408-410
+                T == C.ShortTy || T == C.UnsignedShortTy) {
+              return MatchPromotion;
+            }
----------------
`{}` aren't necessary


================
Comment at: clang/lib/AST/FormatString.cpp:414-417
+            if (T == C.SignedCharTy || T == C.UnsignedCharTy || T == C.IntTy ||
+                T == C.UnsignedIntTy) {
+              return NoMatchPromotionTypeConfusion;
+            }
----------------
`{}` aren't necessary


================
Comment at: clang/lib/AST/FormatString.cpp:426-428
+            if (T == C.IntTy || T == C.UnsignedIntTy) {
+              return NoMatchPromotionTypeConfusion;
+            }
----------------
`{}` aren't necessary


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10084-10085
 
-  analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
-  if (Match == analyze_printf::ArgType::Match)
+  ArgType::MatchKind ImplicitMatch = ArgType::NoMatch,
+                     Match = AT.matchesType(S.Context, ExprTy);
+  if (Match == ArgType::Match)
----------------
I think it would be slightly more readable to make these two assignments two dedicated statements.


================
Comment at: clang/test/Sema/format-strings-scanf.c:271-272
+  scanf("%llx", &i); // expected-warning{{format specifies type 'unsigned long long *' but the argument has type 'int *'}}
+  scanf("%hf", // expected-warning{{length modifier 'h' results in undefined behavior or no effect with 'f' conversion specifier}}
+  &sc); // Is this a clang bug ?
+  scanf("%s", i); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
----------------
Hmm...


================
Comment at: clang/test/Sema/format-strings-scanf.c:274
+  scanf("%s", i); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
+
+}
----------------
delete empty newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132568