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

[YingChi Long via Phabricator via cfe-commits]

inclyc added inline comments.


================
Comment at: clang/lib/AST/FormatString.cpp:422
+          case BuiltinType::Bool:
+            // Don't warn printf("%hd", [char])
+            // https://reviews.llvm.org/D66186
----------------
aaron.ballman wrote:
> The comment is talking about passing a `char` (promoted to `int`) then printed as a `short` but the check is looking for the type to be printed as an `int`; that doesn't seem like a type confusion situation so much as a match due to promotion. Should the test below be looking for a short type instead of an int type?
> The comment is talking about passing a `char` (promoted to `int`) then printed as a `short` but the check is looking for the type to be printed as an `int`; that doesn't seem like a type confusion situation so much as a match due to promotion. Should the test below be looking for a short type instead of an int type?

Sorry for this (sometimes shit happens). There is a lot of redundant logic in this place, I will upload the corrected code immediately.


================
Comment at: clang/test/Sema/format-strings-scanf.c:289-290
+  // ill-formed floats
+  scanf("%hf", // expected-warning{{length modifier 'h' results in undefined behavior or no effect with 'f' conversion specifier}}
+  &sc); // Is this a clang bug ?
+
----------------
aaron.ballman wrote:
> inclyc wrote:
> > aaron.ballman wrote:
> > > Is what a clang bug?
> > > 
> > > Also, what is with the "or no effect" in that wording? "This could cause major issues or nothing bad at all might happen" is a very odd way to word a diagnostic. :-D (Maybe something to look into improving in a later patch.)
> > > Is what a clang bug?
> > 
> > Look at `sc` which is a `signed char`, but scanf need a pointer `float *`, I add this comment because I think there should be a warning like 
> > ```
> > format specifies type 'float *' but the argument has type 'signed short *'
> > ```
> > 
> > See printf tests below
> Oh I see what you're saying! There are two issues with the code: one is that the format specifier itself is UB and the other is that the argument passed to this confused format specifier does not agree.
> 
> To me, the priority is the invalid format specifier; unless the user corrects that, we're not really certain what they were aiming to scan in. And I think it's *probably* more likely that the user made a typo in the format specifier instead of trying to scan a half float (or whatever they thought they were scanning) into a `signed char`, so it seems defensible to silence the mismatched specifier diagnostic.
> 
> WDYT?
```  
printf("%hf", // expected-warning{{length modifier 'h' results in undefined behavior or no effect with 'f' conversion specifier}}
  sc); // expected-warning{{format specifies type 'double' but the argument has type 'signed char'}}
```

Then clang seems to have two different behaviors to `printf` and `scanf`, and for `printf` it will give the kind of diagnosis I said. I don't think this problem is particularly serious, because the key point is that using `%hf` UB. So maybe we can just keep clang as-is? Or we should consider implement the same warning in `scanf` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132568