Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -9102,7 +9102,8 @@ unsigned format_idx, unsigned firstDataArg, FormatStringType Type, bool inFunctionCall, VariadicCallType CallType, - llvm::SmallBitVector &CheckedVarArgs); + llvm::SmallBitVector &CheckedVarArgs, + signed &FirstUncoveredArg); bool FormatStringHasSArg(const StringLiteral *FExpr); Index: lib/Sema/SemaChecking.cpp =================================================================== --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -3233,6 +3233,16 @@ } namespace { +struct UncoveredArgHandler { + enum { Unknown = -1, AllCovered = -2 }; + signed FirstUncoveredArg; + SmallVector DiagnosticExprs; + + UncoveredArgHandler() : FirstUncoveredArg(Unknown) { } + void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr); + void Update(signed NewFirstUncoveredArg, const Expr *StrExpr); +}; + enum StringLiteralCheckType { SLCT_NotALiteral, SLCT_UncheckedLiteral, @@ -3249,7 +3259,8 @@ 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; @@ -3270,17 +3281,39 @@ // completely checked only if both sub-expressions were checked. const AbstractConditionalOperator *C = cast(E); - StringLiteralCheckType Left = - checkFormatStringExpr(S, C->getTrueExpr(), Args, - HasVAListArg, format_idx, firstDataArg, - Type, CallType, InFunctionCall, CheckedVarArgs); - if (Left == SLCT_NotALiteral) - return SLCT_NotALiteral; + + // 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, 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: { @@ -3331,7 +3364,8 @@ return checkFormatStringExpr(S, Init, Args, HasVAListArg, format_idx, firstDataArg, Type, CallType, - /*InFunctionCall*/false, CheckedVarArgs); + /*InFunctionCall*/false, CheckedVarArgs, + UncoveredArg); } } @@ -3386,7 +3420,7 @@ return checkFormatStringExpr(S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type, CallType, InFunctionCall, - CheckedVarArgs); + CheckedVarArgs, UncoveredArg); } else if (const FunctionDecl *FD = dyn_cast(ND)) { unsigned BuiltinID = FD->getBuiltinID(); if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString || @@ -3395,7 +3429,8 @@ return checkFormatStringExpr(S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type, CallType, - InFunctionCall, CheckedVarArgs); + InFunctionCall, CheckedVarArgs, + UncoveredArg); } } } @@ -3412,8 +3447,11 @@ StrE = cast(E); if (StrE) { + signed FirstUncoveredArg = UncoveredArgHandler::Unknown; S.CheckFormatString(StrE, E, Args, HasVAListArg, format_idx, firstDataArg, - Type, InFunctionCall, CallType, CheckedVarArgs); + Type, InFunctionCall, CallType, CheckedVarArgs, + FirstUncoveredArg); + UncoveredArg.Update(FirstUncoveredArg, StrE); return SLCT_CheckedLiteral; } @@ -3481,10 +3519,20 @@ // 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.FirstUncoveredArg >= 0) { + unsigned ArgIdx = UncoveredArg.FirstUncoveredArg + 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; @@ -3552,7 +3600,7 @@ CoveredArgs.reset(); } - void DoneProcessing(); + void DoneProcessing(signed &FirstUncoveredArg); void HandleIncompleteSpecifier(const char *startSpecifier, unsigned specifierLen) override; @@ -3777,27 +3825,73 @@ return Args[FirstDataArg + i]; } -void CheckFormatHandler::DoneProcessing() { +void CheckFormatHandler::DoneProcessing(signed &FirstUncoveredArg) { // 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()); - } + signed Found = CoveredArgs.find_first(); + FirstUncoveredArg = + (Found >= 0) ? Found : UncoveredArgHandler::AllCovered; + } +} + +void UncoveredArgHandler::Update(signed NewFirstUncoveredArg, + const Expr *StrExpr) { + // Don't update if a previous string covers all arguments. + if (FirstUncoveredArg == AllCovered) + return; + + switch (NewFirstUncoveredArg) { + case Unknown: + // The string was not fully analysed. Do nothing. + break; + + case UncoveredArgHandler::AllCovered: + // A string has been found with all arguments covered, so clear out + // the diagnostics. + FirstUncoveredArg = AllCovered; + DiagnosticExprs.clear(); + break; + + default: + // 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; } - } + break; } } +void UncoveredArgHandler::Diagnose(Sema &S, bool IsFunctionCall, + const Expr *ArgExpr) { + assert(FirstUncoveredArg >= 0 && 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, @@ -4891,7 +4985,8 @@ bool HasVAListArg, unsigned format_idx, unsigned firstDataArg, FormatStringType Type, bool inFunctionCall, VariadicCallType CallType, - llvm::SmallBitVector &CheckedVarArgs) { + llvm::SmallBitVector &CheckedVarArgs, + signed &FirstUncoveredArg) { // CHECK: is the format string a wide literal? if (!FExpr->isAscii() && !FExpr->isUTF8()) { @@ -4944,7 +5039,7 @@ getLangOpts(), Context.getTargetInfo(), Type == FST_FreeBSDKPrintf)) - H.DoneProcessing(); + H.DoneProcessing(FirstUncoveredArg); } else if (Type == FST_Scanf) { CheckScanfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, numDataArgs, Str, HasVAListArg, Args, format_idx, @@ -4953,7 +5048,7 @@ if (!analyze_format_string::ParseScanfString(H, Str, Str + StrLen, getLangOpts(), Context.getTargetInfo())) - H.DoneProcessing(); + H.DoneProcessing(FirstUncoveredArg); } // TODO: handle other formats } Index: test/Sema/format-strings.c =================================================================== --- test/Sema/format-strings.c +++ test/Sema/format-strings.c @@ -86,6 +86,19 @@ 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() @@ -519,7 +532,7 @@ // 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}}