Reduce false positives in printf/scanf format checker

[AndyG]

Summary

The printf/scanf format checker is a little over-zealous in handling the conditional operator. This patch reduces work by not checking code-paths that are never used and reduces false positives regarding uncovered arguments, for example in the code fragment:

printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

[AndyG]

Richard, David,

Sorry, I've added you as reviewers by proximity to recent relevant changes in SemaChecking.cpp. Hope you don't mind. Please tell me who to redirect to, if there are more applicable reviewers.

Cheers
Andy

[AndyG]

Bump! :o)

[AndyG]

Richard, you have been recommended to me as a suitable reviewer by David. If that's not ok with you, please could you recommend another! Thanks, Andy.

[rtrieu]

Hi Andy,

Could you give some motivation for this patch? From your example:

printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

I would expect the first format string to produce a warning that msg is not used.

[AndyG]

Hi Richard,

Thank you for looking at my patch. I would argue that the code

printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

is valid and should not cause a warning due to unused arguments since at least one code path uses all the arguments. This in my view is the problem with the diagnostic as it currently stands.

I should expect that the following would cause the warning:

printf(minimal ? "%i\n" : "%i: %s\n", code, msg, unused);

which indeed it still does under this patch for the argument 'unused'. Also, where the compiler can determine that a code path is never followed then it will still produce the warning, as in:

printf(1 ? "%i\n" : "%i: %s\n", code, msg);

Cheers,
Andy

[rtrieu]

It looks like you are limiting to one diagnostic per printf, no matter the number of format strings. How is the case when multiple format strings would trigger the warning?

[AndyG]

Yes, but only for the "data argument not used" warning. All other warnings are unaffected by the change, for example:

test.cpp:9:51: warning: format specifies type 'char *' but the argument has type 'int' [-Wformat]                                                  
    printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
                      ~~                          ^~~~
                      %d
test.cpp:9:30: warning: more '%' conversions than data arguments [-Wformat]
    printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
                            ~^
test.cpp:9:57: warning: format specifies type 'int' but the argument has type 'const char *' [-Wformat]
    printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
                                        ~~              ^~~
                                        %s
test.cpp:9:45: warning: more '%' conversions than data arguments [-Wformat]
    printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
                                           ~^
4 warnings generated.

But, as you observed, for the unused argument it only warns for the first instance:

test.cpp:9:53: warning: data argument not used by format string [-Wformat-extra-args]
    printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
                              ~~~~~~~~~             ^
1 warning generated.

This is in comparison to the original diagnostic:

test.cpp:9:48: warning: data argument not used by format string [-Wformat-extra-args]
    printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
                     ~~~~~~                    ^
test.cpp:9:53: warning: data argument not used by format string [-Wformat-extra-args]
    printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
                              ~~~~~~~~~             ^
2 warnings generated.

However, may I ask what the specific concern is here? If this diagnostic appears then the user knows that all the strings do not use this and any subsequent argument. Effectively the original situation doesn't really tell the user anything more useful - it only duplicates the warning and actually, as can be seen above, it is potentially confusing since it suggests two positions for the unused argument(s). Compare this with the new diagnostic which shows only one string, and this string is the first string which uses the most arguments, so it is very clear which arguments are not used.

What do you think? Is this an adequate argument?

[AndyG]

Thoughts? Am I good to go?

Cheers,
Andy

[rtrieu]

Oops, forget to hit send on my last comment.

My concern is something like:

printf(condition ? "first message: %d" : "second message: %d", 5, 10);

There's nothing in the code implying that either message is the one that should be using two arguments, so which one will be highlighted? At that point, we may need to consider rewriting the message to indicate that multiple format strings would be affected.

[AndyG]

In your case, the first string would be highlighted only. Yes, I see what you mean. Is it possible to have multiple ranges for the diagnostic? By which I mean, to produce the following:

test.cpp:x:y: warning: data argument not used by format string [-Wformat-extra-args]
    printf(condition ? "first message: %d" : "second message: %d", 5, 10);
                       ~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~     ^

If so, I think the diagnostic should still track just the strings utilising the most arguments, so this would be the case with three messages, missing out the middle string since it is already using a sub-set of the arguments anyway:

test.cpp:x:y: warning: data argument not used by format string [-Wformat-extra-args]
    printf(a ? "%d %d" : b ? "%d" : "%d %d", 5, 10, 20);
                ~~~~~                ~~~~~          ^

If you think this isn't sufficient, then my suggestion would be to make the range of the diagnostic the complete format expression, and let the programmer assess it themselves.

[rtrieu]

In D15636#343856, @AndyG wrote:

In your case, the first string would be highlighted only. Yes, I see what you mean. Is it possible to have multiple ranges for the diagnostic? By which I mean, to produce the following:

test.cpp:x:y: warning: data argument not used by format string [-Wformat-extra-args]
    printf(condition ? "first message: %d" : "second message: %d", 5, 10);
                       ~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~     ^

If so, I think the diagnostic should still track just the strings utilising the most arguments, so this would be the case with three messages, missing out the middle string since it is already using a sub-set of the arguments anyway:

test.cpp:x:y: warning: data argument not used by format string [-Wformat-extra-args]
    printf(a ? "%d %d" : b ? "%d" : "%d %d", 5, 10, 20);
                ~~~~~                ~~~~~          ^

If you think this isn't sufficient, then my suggestion would be to make the range of the diagnostic the complete format expression, and let the programmer assess it themselves.

I believe your two examples are the way to go, highlighting all the format strings using the most arguments. You can pass multiple SourceRange's to a diagnostic. To do that, in your Diagnose function, store the result of P.Diag in a local reference, then pass in the extra SourceRange's, then pass that reference to EmitFormatDiagnostic.

[AndyG]

All strings matching the highest uncovered argument are now highlighted in the diagnostic.

EDIT: I also rebased against r260222

[AndyG]

Richard, are you happy with this latest version? Can I proceed to commit it?

Thanks.

[rtrieu]

In D15636#352532, @AndyG wrote:

Richard, are you happy with this latest version? Can I proceed to commit it?

Thanks.

No, this is not ready yet. This code is old and needs a bit of clean up, and because your patch touches it, you will be one that needs to clean it up before committing your patch. Mainly, the changes are needed so that you can pass your arg handler properly through the function calls.

include/clang/Sema/Sema.h

9100–9106 ↗

(On Diff #47426)

Remove this from Sema.h and make the function static in SemaChecking.cpp. It is only called from SemaChecking.cpp anyways and doesn't need access to anything in Sema.

lib/Sema/SemaChecking.cpp

3261

Make this a class. Its members needs to be segmented into private and public parts.

3262–3264

This section should be private section.

3266–3268

These should be made public.

3521

Pass in UncoveredArg and update within CheckFormatString function

3597–3603

FirstUncoveredArg is now a private member so there's two paths, either will work:

1. Create two public function for UncoveredArgHandler,

bool UncoveredArgHandler::hasUncoveredArg();
unsigned UncoveredArgHandler::getUncoveredArg();

then use:

if (UncoveredArg.hasUncoveredArg()) {
  unsigned ArgIdx = UncoveredArg.getUncoveredArg() + firstDataArg;

getUncoveredArg() should never return negative numbers as that should remain an implementation detail.

2. Pass both Args and firstDataArg to Diagnose and do the logic there.

3674

Don't change the call signature here, leave it as a zero argument call. Have another function return the first uncovered argument.

3910–3962

Move these two functions up to where the class is defined.

3923–3924

Use a range-based for-loop here.

5016–5017

Make this a static void function.
Add a new first argument "Sema &S".

static void CheckFormatString(Sema &S, const StringLiteral *FExpr,

The other changes for changing this function are listed below. Also, a forward declaration of this function is necessary higher in this file since a previous function will call this function.

5016–5017

FormatStringType and VariadicCallType need to qualified with "Sema::"

5017

Pass in an UncoveredArgHandler here instead of an unsigned.

5029–5030

*this => S
Here and throughout this function

5030–5031

PDiag => S.PDiag
Here and throughout this function

5040–5041

Context => S.Context
Here and throughout this function

5066–5067

FST_* => Sema::FST_*
Here and throughout this function

5081

This line should remain the same. Additional function should be added to check the status of uncovered arguments and pass that to UncoveredArgHandler.

5091

You have modified the Scanf handling function, but there are no additional scanf tests.

test/Sema/format-strings.c

106

Add a test for having the format string as a constant literal defined elsewhere. This should generate a note to point to the original format string. You changes removes this note. I am fine having this listed as a FIXME for a follow up patch.

[AndyG]

I've removed CheckFormatString from Sema.h and make it a static function inside SemaChecking.cpp in r261522. I'll submit a new diff here with the remaining requested changes for this patch when I have a moment.

[AndyG]

Revised patch coming shortly...

lib/Sema/SemaChecking.cpp

3674

DoneProcessing is that function! To move the logic into another function would leave nothing behind :o)

Passing UncoveredArgHandler& into CheckFormatHandler allows this to be worked around.

3910–3962

UncoveredArgHandler::Update has been moved up, but UncoveredArgHandler::Diagnose has been left since it has a dependency on CheckFormatHandler.

3923–3924

Except that it is iterating from the second item in the list. I don't think there is an easier way to do it.

[AndyG]

Patch additionally re-based off r261522.

[AndyG]

lib/Sema/SemaChecking.cpp

3905

Rather than FExpr, I think this should be OrigFormatExpr?

[rtrieu]

A few more comments.

In D15636#358588, @AndyG wrote:

Patch additionally re-based off r261522.

It's usually a bad idea to rebase in the middle of a string of patches. Phabricator isn't aware of revisions, so while a base to latest patch will show up fine, attempting to diff the original patch to the latest will show all the upstream changes as well.

lib/Sema/SemaChecking.cpp

3279

Add an assert that NewFirstUncoveredArg >= 0

3284–3288

Move this to a new function "setAllCovered()"

3823–3833

Use two different function calls depending on the result of the find_first(). This also preserves the existing assert.

signed notCoveredArg = CoveredArgs.find_first();
if (notCoveredArg >= 0) {
  assert((unsigned)notCoveredArg < NumDataArgs);
  UncoveredArg.Update(notCoveredArg, OrigFormatExpr);
} else {
  UncoveredArg.setAllCovered();
}

3905

OrigFormatExpr is probably better.

3923–3924

That raises the question why the first element is different from the rest.

5081

Not exactly what I said, but this works too.

[AndyG]

Updated patch according to the comments made. Also spotted a corner-case where a double-diagnostic was produced, for example in the following where one string has too few arguments and the other string too many:

printf(minimal ? "%i\n" : "%i %s %s\n", code, msg);

Hitherto, this produced two diagnostics: the first (correct) diagnostic that more '%' conversions exist than data arguments, but then a second diagnostic regarding uncovered arguments. The second diagnostic is now suppressed since the format string that has too many does also cover all arguments.

[AndyG]

lib/Sema/SemaChecking.cpp

3923–3924

I've put this as a range-based for-loop as requested. The reason it wasn't before is that one expression is passed through the call to EmitFormatDiagnostic. Ultimately, I think EmitFormatDiagnostic needs to be reworked to support multiple format strings - and this will probably resolve the FIXME relating to the missing diagnostic note in the tests.

[rtrieu]

LGTM
The other issues brought up in this review can go in follow up patches.

lib/Sema/SemaChecking.cpp

 bool Sema::SemaBuiltinSetjmp(CallExpr *TheCall) {
   if (!Context.getTargetInfo().hasSjLjLowering())
     return Diag(TheCall->getLocStart(), diag::err_builtin_setjmp_unsupported)
              << SourceRange(TheCall->getLocStart(), TheCall->getLocEnd());
   return false;
 }
 
 namespace {
+class UncoveredArgHandler {
+  enum { Unknown = -1, AllCovered = -2 };
+  signed FirstUncoveredArg;
+  SmallVector<const Expr *, 4> DiagnosticExprs;
+
+public:
+  UncoveredArgHandler() : FirstUncoveredArg(Unknown) { }
+
+  bool hasUncoveredArg() const {
+    return (FirstUncoveredArg >= 0);
+  }
+
+  unsigned getUncoveredArg() const {
+    assert(hasUncoveredArg() && "no uncovered argument");
+    return FirstUncoveredArg;
+  }
+
+  void Update(signed NewFirstUncoveredArg, const Expr *StrExpr) {
+    // Don't update if a previous string covers all arguments.
+    if (FirstUncoveredArg == AllCovered)
+      return;
+
+    if (NewFirstUncoveredArg < 0) {
+      // A string has been found with all arguments covered, so clear out
+      // the diagnostics.
+      FirstUncoveredArg = AllCovered;
+      DiagnosticExprs.clear();
+      return;
+    }
+
+    // UncoveredArgHandler tracks the highest uncovered argument index
+    // and with it all the strings that match this index.
+    if (NewFirstUncoveredArg == FirstUncoveredArg)
+      DiagnosticExprs.push_back(StrExpr);
+    else if (NewFirstUncoveredArg > FirstUncoveredArg) {
+      DiagnosticExprs.clear();
+      DiagnosticExprs.push_back(StrExpr);
+      FirstUncoveredArg = NewFirstUncoveredArg;
+    }
+  }
+
+  void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr);
+};
+
 enum StringLiteralCheckType {
   SLCT_NotALiteral,
   SLCT_UncheckedLiteral,
   SLCT_CheckedLiteral
 };
 } // end anonymous namespace
 
 static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
                               const Expr *OrigFormatExpr,
                               ArrayRef<const Expr *> Args,
                               bool HasVAListArg, unsigned format_idx,
                               unsigned firstDataArg,
                               Sema::FormatStringType Type,
                               bool inFunctionCall,
                               Sema::VariadicCallType CallType,
-                              llvm::SmallBitVector &CheckedVarArgs);
+                              llvm::SmallBitVector &CheckedVarArgs,
+                              UncoveredArgHandler &UncoveredArg);
 
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
 // format string, we will usually need to emit a warning.
 // True string literals are then checked by CheckFormatString.
 static StringLiteralCheckType
 checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
                       bool HasVAListArg, unsigned format_idx,
                       unsigned firstDataArg, Sema::FormatStringType Type,
                       Sema::VariadicCallType CallType, bool InFunctionCall,
-                      llvm::SmallBitVector &CheckedVarArgs) {
+                      llvm::SmallBitVector &CheckedVarArgs,
+                      UncoveredArgHandler &UncoveredArg) {
  tryAgain:
   if (E->isTypeDependent() || E->isValueDependent())
     return SLCT_NotALiteral;
 
   E = E->IgnoreParenCasts();
 
   if (E->isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull))
     // Technically -Wformat-nonliteral does not warn about this case.
     // The behavior of printf and friends in this case is implementation
     // dependent.  Ideally if the format string cannot be null then
     // it should have a 'nonnull' attribute in the function prototype.
     return SLCT_UncheckedLiteral;
 
   switch (E->getStmtClass()) {
   case Stmt::BinaryConditionalOperatorClass:
   case Stmt::ConditionalOperatorClass: {
     // The expression is a literal if both sub-expressions were, and it was
     // completely checked only if both sub-expressions were checked.
     const AbstractConditionalOperator *C =
         cast<AbstractConditionalOperator>(E);
-    StringLiteralCheckType Left =
-        checkFormatStringExpr(S, C->getTrueExpr(), Args,
+
+    // Determine whether it is necessary to check both sub-expressions, for
+    // example, because the condition expression is a constant that can be
+    // evaluated at compile time.
+    bool CheckLeft = true, CheckRight = true;
+
+    bool Cond;
+    if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
+      if (Cond)
+        CheckRight = false;
+      else
+        CheckLeft = false;
+    }
+
+    StringLiteralCheckType Left;
+    if (!CheckLeft)
+      Left = SLCT_UncheckedLiteral;
+    else {
+      Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
                                    HasVAListArg, format_idx, firstDataArg,
-                              Type, CallType, InFunctionCall, CheckedVarArgs);
-    if (Left == SLCT_NotALiteral)
-      return SLCT_NotALiteral;
+                                   Type, CallType, InFunctionCall,
+                                   CheckedVarArgs, UncoveredArg);
+      if (Left == SLCT_NotALiteral || !CheckRight)
+        return Left;
+    }
+
     StringLiteralCheckType Right =
         checkFormatStringExpr(S, C->getFalseExpr(), Args,
                               HasVAListArg, format_idx, firstDataArg,
-                              Type, CallType, InFunctionCall, CheckedVarArgs);
-    return Left < Right ? Left : Right;
+                              Type, CallType, InFunctionCall, CheckedVarArgs,
+                              UncoveredArg);
+
+    return (CheckLeft && Left < Right) ? Left : Right;
   }
 
   case Stmt::ImplicitCastExprClass: {
     E = cast<ImplicitCastExpr>(E)->getSubExpr();
     goto tryAgain;
   }
 
   case Stmt::OpaqueValueExprClass:
if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
           // Look through initializers like const char c[] = { "foo" }
           if (const InitListExpr *InitList = dyn_cast<InitListExpr>(Init)) {
             if (InitList->isStringLiteralInit())
               Init = InitList->getInit(0)->IgnoreParenImpCasts();
           }
           return checkFormatStringExpr(S, Init, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
-                                       /*InFunctionCall*/false, CheckedVarArgs);
+                                       /*InFunctionCall*/false, CheckedVarArgs,
+                                       UncoveredArg);
         }
       }
 
       // For vprintf* functions (i.e., HasVAListArg==true), we add a
       // special check to see if the format string is a function parameter
       // of the function calling the printf function.  If the function
       // has an attribute indicating it is a printf-like function, then we
       // should suppress warnings concerning non-literals being used in a call
if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
         if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
           if (MD->isInstance())
             --ArgIndex;
         const Expr *Arg = CE->getArg(ArgIndex - 1);
 
         return checkFormatStringExpr(S, Arg, Args,
                                      HasVAListArg, format_idx, firstDataArg,
                                      Type, CallType, InFunctionCall,
-                                     CheckedVarArgs);
+                                     CheckedVarArgs, UncoveredArg);
       } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
         unsigned BuiltinID = FD->getBuiltinID();
         if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
             BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {
           const Expr *Arg = CE->getArg(0);
           return checkFormatStringExpr(S, Arg, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
-                                       InFunctionCall, CheckedVarArgs);
+                                       InFunctionCall, CheckedVarArgs,
+                                       UncoveredArg);
         }
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass: {
     const StringLiteral *StrE = nullptr;
 
     if (const ObjCStringLiteral *ObjCFExpr = dyn_cast<ObjCStringLiteral>(E))
       StrE = ObjCFExpr->getString();
     else
       StrE = cast<StringLiteral>(E);
 
     if (StrE) {
       CheckFormatString(S, StrE, E, Args, HasVAListArg, format_idx,
                         firstDataArg, Type, InFunctionCall, CallType,
-                        CheckedVarArgs);
+                        CheckedVarArgs, UncoveredArg);
       return SLCT_CheckedLiteral;
     }
 
     return SLCT_NotALiteral;
   }
 
   default:
     return SLCT_NotALiteral;
bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
   // are string literals: (1) permits the checking of format strings by
   // the compiler and thereby (2) can practically remove the source of
   // many format string exploits.
 
   // Format string can be either ObjC string (e.g. @"%d") or
   // C string (e.g. "%d")
   // ObjC string uses the same format specifiers as C string, so we can use
   // the same format string checking logic for both ObjC and C strings.
+  UncoveredArgHandler UncoveredArg;
   StringLiteralCheckType CT =
       checkFormatStringExpr(*this, OrigFormatExpr, Args, HasVAListArg,
                             format_idx, firstDataArg, Type, CallType,
-                            /*IsFunctionCall*/true, CheckedVarArgs);
+                            /*IsFunctionCall*/true, CheckedVarArgs,
+                            UncoveredArg);
+
+  // Generate a diagnostic where an uncovered argument is detected.
+  if (UncoveredArg.hasUncoveredArg()) {
+    unsigned ArgIdx = UncoveredArg.getUncoveredArg() + firstDataArg;
+    assert(ArgIdx < Args.size() && "ArgIdx outside bounds");
+    UncoveredArg.Diagnose(*this, /*IsFunctionCall*/true, Args[ArgIdx]);
+  }
+
   if (CT != SLCT_NotALiteral)
     // Literal format string found, check done!
     return CT == SLCT_CheckedLiteral;
 
   // Strftime is particular as it always uses a single 'time' argument,
   // so it is safe to pass a non-literal string.
   if (Type == FST_Strftime)
     return false;
protected:
   ArrayRef<const Expr *> Args;
   unsigned FormatIdx;
   llvm::SmallBitVector CoveredArgs;
   bool usesPositionalArgs;
   bool atFirstArg;
   bool inFunctionCall;
   Sema::VariadicCallType CallType;
   llvm::SmallBitVector &CheckedVarArgs;
+  UncoveredArgHandler &UncoveredArg;
 
 public:
   CheckFormatHandler(Sema &s, const StringLiteral *fexpr,
                      const Expr *origFormatExpr, unsigned firstDataArg,
                      unsigned numDataArgs, const char *beg, bool hasVAListArg,
                      ArrayRef<const Expr *> Args,
                      unsigned formatIdx, bool inFunctionCall,
                      Sema::VariadicCallType callType,
-                     llvm::SmallBitVector &CheckedVarArgs)
+                     llvm::SmallBitVector &CheckedVarArgs,
+                     UncoveredArgHandler &UncoveredArg)
     : S(s), FExpr(fexpr), OrigFormatExpr(origFormatExpr),
       FirstDataArg(firstDataArg), NumDataArgs(numDataArgs),
       Beg(beg), HasVAListArg(hasVAListArg),
       Args(Args), FormatIdx(formatIdx),
       usesPositionalArgs(false), atFirstArg(true),
       inFunctionCall(inFunctionCall), CallType(callType),
-      CheckedVarArgs(CheckedVarArgs) {
+      CheckedVarArgs(CheckedVarArgs), UncoveredArg(UncoveredArg) {
     CoveredArgs.resize(numDataArgs);
     CoveredArgs.reset();
   }
 
   void DoneProcessing();
 
   void HandleIncompleteSpecifier(const char *startSpecifier,
                                  unsigned specifierLen) override;
 
   void HandleInvalidLengthModifier(
                            const analyze_format_string::FormatSpecifier &FS,
                            const analyze_format_string::ConversionSpecifier &CS,
                            const char *startSpecifier, unsigned specifierLen,

 
 // Note that this may return NULL if there was an error parsing or building
 // one of the argument expressions.
 const Expr *CheckFormatHandler::getDataArg(unsigned i) const {
   return Args[FirstDataArg + i];
 }
 
 void CheckFormatHandler::DoneProcessing() {
-    // Does the number of data arguments exceed the number of
-    // format conversions in the format string?
   if (!HasVAListArg) {
       // Find any arguments that weren't covered.
     CoveredArgs.flip();
-    signed notCoveredArg = CoveredArgs.find_first();
-    if (notCoveredArg >= 0) {
-      assert((unsigned)notCoveredArg < NumDataArgs);
-      if (const Expr *E = getDataArg((unsigned) notCoveredArg)) {
-        SourceLocation Loc = E->getLocStart();
-        if (!S.getSourceManager().isInSystemMacro(Loc)) {
-          EmitFormatDiagnostic(S.PDiag(diag::warn_printf_data_arg_not_used),
-                               Loc, /*IsStringLocation*/false,
-                               getFormatStringRange());
-        }
-      }
-    }
-  }
+    UncoveredArg.Update(CoveredArgs.find_first(), FExpr);
+
+void UncoveredArgHandler::Diagnose(Sema &S, bool IsFunctionCall,
+                                   const Expr *ArgExpr) {
+  assert(hasUncoveredArg() && DiagnosticExprs.size() > 0 &&
+         "Invalid state");
+
+  if (!ArgExpr)
+    return;
+
+  SourceLocation Loc = ArgExpr->getLocStart();
+
+  if (S.getSourceManager().isInSystemMacro(Loc))
+    return;
+
+  PartialDiagnostic PDiag = S.PDiag(diag::warn_printf_data_arg_not_used);
+  for (unsigned i = 1; i < DiagnosticExprs.size(); ++i)
+    PDiag << DiagnosticExprs[i]->getSourceRange();
+
+  CheckFormatHandler::EmitFormatDiagnostic(
+                                  S, IsFunctionCall, DiagnosticExprs[0],
+                                  PDiag, Loc, /*IsStringLocation*/false,
+                                  DiagnosticExprs[0]->getSourceRange());
 }
 
 bool
 CheckFormatHandler::HandleInvalidConversionSpecifier(unsigned argIndex,
                                                      SourceLocation Loc,
                                                      const char *startSpec,
                                                      unsigned specifierLen,
                                                      const char *csStart,
                                                      unsigned csLen) {
   bool keepGoing = true;
   if (argIndex < NumDataArgs) {
     // Consider the argument coverered, even though the specifier doesn't
     // make sense.
     CoveredArgs.set(argIndex);
   }
   else {
     // If argIndex exceeds the number of data arguments we
     // don't issue a warning because that is just a cascade of warnings (and
     // they may have intended '%%' anyway). We don't want to continue processing
     // the format string after this point, however, as we will like just get
     // gibberish when trying to match arguments.
     keepGoing = false;
   }
   
   EmitFormatDiagnostic(S.PDiag(diag::warn_format_invalid_conversion)
                          << StringRef(csStart, csLen),
                        Loc, /*IsStringLocation*/true,
                        getSpecifierRange(startSpec, specifierLen));
   
   return keepGoing;
 }
 
 void
 CheckFormatHandler::HandlePositionalNonpositionalArgs(SourceLocation Loc,
                                                       const char *startSpec,
                                                       unsigned specifierLen) {
   EmitFormatDiagnostic(
     S.PDiag(diag::warn_format_mix_positional_nonpositional_args),
     Loc, /*isStringLoc*/true, getSpecifierRange(startSpec, specifierLen));
 }
 

 public:
   CheckPrintfHandler(Sema &s, const StringLiteral *fexpr,
                      const Expr *origFormatExpr, unsigned firstDataArg,
                      unsigned numDataArgs, bool isObjC,
                      const char *beg, bool hasVAListArg,
                      ArrayRef<const Expr *> Args,
                      unsigned formatIdx, bool inFunctionCall,
                      Sema::VariadicCallType CallType,
-                     llvm::SmallBitVector &CheckedVarArgs)
+                     llvm::SmallBitVector &CheckedVarArgs,
+                     UncoveredArgHandler &UncoveredArg)
     : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg,
                          numDataArgs, beg, hasVAListArg, Args,
-                         formatIdx, inFunctionCall, CallType, CheckedVarArgs),
+                         formatIdx, inFunctionCall, CallType, CheckedVarArgs,
+                         UncoveredArg),
       ObjCContext(isObjC)
   {}
 
   bool HandleInvalidPrintfConversionSpecifier(
                                       const analyze_printf::PrintfSpecifier &FS,
                                       const char *startSpecifier,
                                       unsigned specifierLen) override;
 

 class CheckScanfHandler : public CheckFormatHandler {
 public:
   CheckScanfHandler(Sema &s, const StringLiteral *fexpr,
                     const Expr *origFormatExpr, unsigned firstDataArg,
                     unsigned numDataArgs, const char *beg, bool hasVAListArg,
                     ArrayRef<const Expr *> Args,
                     unsigned formatIdx, bool inFunctionCall,
                     Sema::VariadicCallType CallType,
-                    llvm::SmallBitVector &CheckedVarArgs)
+                    llvm::SmallBitVector &CheckedVarArgs,
+                    UncoveredArgHandler &UncoveredArg)
     : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg,
                          numDataArgs, beg, hasVAListArg,
                          Args, formatIdx, inFunctionCall, CallType,
-                         CheckedVarArgs)
+                         CheckedVarArgs, UncoveredArg)
   {}
   
   bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS,
                             const char *startSpecifier,
                             unsigned specifierLen) override;
   
   bool HandleInvalidScanfConversionSpecifier(
           const analyze_scanf::ScanfSpecifier &FS,
EmitFormatDiagnostic(S.PDiag(diag)
                              << Ex->getType() << false << Ex->getSourceRange(),
                          Ex->getLocStart(),
                          /*IsStringLocation*/ false,
                          getSpecifierRange(startSpecifier, specifierLen));
   }
 
   return true;
 }
 
 static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
                               const Expr *OrigFormatExpr,
                               ArrayRef<const Expr *> Args,
                               bool HasVAListArg, unsigned format_idx,
                               unsigned firstDataArg,
                               Sema::FormatStringType Type,
                               bool inFunctionCall,
                               Sema::VariadicCallType CallType,
-                              llvm::SmallBitVector &CheckedVarArgs) {
+                              llvm::SmallBitVector &CheckedVarArgs,
+                              UncoveredArgHandler &UncoveredArg) {
   // CHECK: is the format string a wide literal?
   if (!FExpr->isAscii() && !FExpr->isUTF8()) {
     CheckFormatHandler::EmitFormatDiagnostic(
       S, inFunctionCall, Args[format_idx],
       S.PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(),
       /*IsStringLocation*/true, OrigFormatExpr->getSourceRange());
     return;
   }
 
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();
   const char *Str = StrRef.data();
   // Account for cases where the string literal is truncated in a declaration.
   const ConstantArrayType *T =
     S.Context.getAsConstantArrayType(FExpr->getType());
   assert(T && "String literal not of constant array type!");
   size_t TypeSize = T->getSize().getZExtValue();
   size_t StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size());
   const unsigned numDataArgs = Args.size() - firstDataArg;
 
   // Emit a warning if the string literal is truncated and does not contain an
   // embedded null character.
   if (TypeSize <= StrRef.size() &&
       StrRef.substr(0, TypeSize).find('\0') == StringRef::npos) {
     CheckFormatHandler::EmitFormatDiagnostic(
         S, inFunctionCall, Args[format_idx],
         S.PDiag(diag::warn_printf_format_string_not_null_terminated),
         FExpr->getLocStart(),
         /*IsStringLocation=*/true, OrigFormatExpr->getSourceRange());
     return;
   }
 
   // CHECK: empty format string?
   if (StrLen == 0 && numDataArgs > 0) {
     CheckFormatHandler::EmitFormatDiagnostic(
       S, inFunctionCall, Args[format_idx],
       S.PDiag(diag::warn_empty_format_string), FExpr->getLocStart(),
       /*IsStringLocation*/true, OrigFormatExpr->getSourceRange());
     return;
   }
 
   if (Type == Sema::FST_Printf || Type == Sema::FST_NSString ||
       Type == Sema::FST_FreeBSDKPrintf || Type == Sema::FST_OSTrace) {
     CheckPrintfHandler H(S, FExpr, OrigFormatExpr, firstDataArg,
                          numDataArgs, (Type == Sema::FST_NSString ||
                                        Type == Sema::FST_OSTrace),
                          Str, HasVAListArg, Args, format_idx,
-                         inFunctionCall, CallType, CheckedVarArgs);
+                         inFunctionCall, CallType, CheckedVarArgs,
+                         UncoveredArg);
 
     if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen,
                                                   S.getLangOpts(),
                                                   S.Context.getTargetInfo(),
                                             Type == Sema::FST_FreeBSDKPrintf))
       H.DoneProcessing();
   } else if (Type == Sema::FST_Scanf) {
     CheckScanfHandler H(S, FExpr, OrigFormatExpr, firstDataArg, numDataArgs,
                         Str, HasVAListArg, Args, format_idx,
-                        inFunctionCall, CallType, CheckedVarArgs);
+                        inFunctionCall, CallType, CheckedVarArgs,
+                        UncoveredArg);
 
     if (!analyze_format_string::ParseScanfString(H, Str, Str + StrLen,
                                                  S.getLangOpts(),
                                                  S.Context.getTargetInfo()))
       H.DoneProcessing();
   } // TODO: handle other formats
 }
 
 bool Sema::FormatStringHasSArg(const StringLiteral *FExpr) {
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();
   const char *Str = StrRef.data();
   // Account for cases where the string literal is truncated in a declaration.

test/Sema/format-strings-scanf.c

 int sscanf(const char * restrict, const char * restrict, ...) ;
 int my_scanf(const char * restrict, ...) __attribute__((__format__(__scanf__, 1, 2)));
 
 int vscanf(const char * restrict, va_list);
 int vfscanf(FILE * restrict, const char * restrict, va_list);
 int vsscanf(const char * restrict, const char * restrict, va_list);
 
 void test(const char *s, int *i) {
-  scanf(s, i); // expected-warning{{ormat string is not a string literal}}
+  scanf(s, i); // expected-warning{{format string is not a string literal}}
   scanf("%0d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%00d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%d%[asdfasdfd", i, s); // expected-warning{{no closing ']' for '%[' in scanf format string}}
 
   unsigned short s_x;
   scanf ("%" "hu" "\n", &s_x); // no-warning
   scanf("%y", i); // expected-warning{{invalid conversion specifier 'y'}}
   scanf("%%"); // no-warning
void test_qualifiers(const int *cip, volatile int* vip,
   scanf("%n", vip); // No warning.
   scanf("%c", vcp); // No warning.
 
   typedef int* ip_t;
   typedef const int* cip_t;
   scanf("%d", (ip_t)0); // No warning.
   scanf("%d", (cip_t)0); // expected-warning{{format specifies type 'int *' but the argument has type 'cip_t' (aka 'const int *')}}
 }
+
+void check_conditional_literal(char *s, int *i) {
+  scanf(0 ? "%s" : "%d", i); // no warning
+  scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char *'}}
+  scanf(0 ? "%d %d" : "%d", i); // no warning
+  scanf(1 ? "%d %d" : "%d", i); // expected-warning{{more '%' conversions than data arguments}}
+  scanf(0 ? "%d %d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(1 ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(i ? "%s" : "%d", s); // expected-warning{{format specifies type 'int *'}}
+}

test/Sema/format-strings.c

void check_string_literal( FILE* fp, const char* s, char *buf, ... ) {
   vsprintf(buf,s,ap); // expected-warning {{format string is not a string lit}}
   vsnprintf(buf,2,s,ap); // expected-warning {{format string is not a string lit}}
   vsnprintf(buf,2,global_fmt,ap); // expected-warning {{format string is not a string literal}}
   __builtin___vsnprintf_chk(buf,2,0,-1,s,ap); // expected-warning {{format string is not a string lit}}
   __builtin___vsnprintf_chk(buf,2,0,-1,global_fmt,ap); // expected-warning {{format string is not a string literal}}
 
   vscanf(s, ap); // expected-warning {{format string is not a string literal}}
 
+  const char *const fmt = "%d"; // FIXME -- defined here
+  printf(fmt, 1, 2); // expected-warning{{data argument not used}}
+
   // rdar://6079877
   printf("abc"
          "%*d", 1, 1); // no-warning
   printf("abc\
 def"
          "%*d", 1, 1); // no-warning
          
   // <rdar://problem/6079850>, allow 'unsigned' (instead of 'int') to be used for both
void check_string_literal2( FILE* fp, const char* s, char *buf, ... ) {
   vscanf(s, ap); // expected-warning {{format string is not a string literal}}
 }
 
 void check_conditional_literal(const char* s, int i) {
   printf(i == 1 ? "yes" : "no"); // no-warning
   printf(i == 0 ? (i == 1 ? "yes" : "no") : "dont know"); // no-warning
   printf(i == 0 ? (i == 1 ? s : "no") : "dont know"); // expected-warning{{format string is not a string literal}}
   printf("yes" ?: "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(0 ? "yes %s" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no %s", 1); // expected-warning{{format specifies type 'char *'}}
+
+  printf(0 ? "yes" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes" : "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes %d" : "no", 1); // no-warning
+  printf(i ? "yes" : "no %d", 1); // no-warning
+  printf(i ? "yes %s" : "no %d", 1); // expected-warning{{format specifies type 'char *'}}
+  printf(i ? "yes" : "no %d", 1, 2); // expected-warning{{data argument not used by format string}}
+
+  printf(i ? "%*s" : "-", i, s); // no-warning
+  printf(i ? "yes" : 0 ? "no %*d" : "dont know %d", 1, 2); // expected-warning{{data argument not used by format string}}
 }
 
 void check_writeback_specifier()
 {
   int x;
   char *b;
   printf("%n", b); // expected-warning{{format specifies type 'int *' but the argument has type 'char *'}}
   printf("%n", &x); // no-warning
 
   printf("%hhn", (signed char*)0); // no-warning
void pr9751() {
   printf("%hhs", "a"); // expected-warning{{length modifier 'hh' results in undefined behavior or no effect with 's' conversion specifier}}
 
   const char kFormat16[] = "%-0d"; // expected-note{{format string is defined here}}
   printf(kFormat16, 5); // expected-warning{{flag '0' is ignored when flag '-' is present}}
   printf("%-0d", 5); // expected-warning{{flag '0' is ignored when flag '-' is present}}
 
   // Make sure that the "format string is defined here" note is not emitted
   // when the original string is within the argument expression.
-  printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}}
+  printf(1 ? "yes %d" : "no %d"); // expected-warning{{more '%' conversions than data arguments}}
 
   const char kFormat17[] = "%hu"; // expected-note{{format string is defined here}}}
   printf(kFormat17, (int[]){0}); // expected-warning{{format specifies type 'unsigned short' but the argument}}
 
   printf("%a", (long double)0); // expected-warning{{format specifies type 'double' but the argument has type 'long double'}}
 
   // Test braced char[] initializers.
   const char kFormat18[] = { "%lld" }; // expected-note{{format string is defined here}}