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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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
----------------
inclyc wrote:
> 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.
No worries!


================
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 ?
+
----------------
inclyc wrote:
> 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` ?
> So maybe we can just keep clang as-is?

I think let's keep clang as-is for the moment so we can keep the concerns separate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132568