AndyG created this revision.
AndyG added a reviewer: cfe-commits.
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);
http://reviews.llvm.org/D15636
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
test/Sema/format-strings.c
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}}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3004,6 +3004,15 @@
}
namespace {
+struct UncoveredArgHandler {
+ enum { Unknown = -1, AllCovered = -2 };
+ signed FirstUncoveredArg;
+ const Expr *DiagnosticExpr;
+
+ UncoveredArgHandler() : FirstUncoveredArg(Unknown), DiagnosticExpr(0) { }
+ void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr);
+};
+
enum StringLiteralCheckType {
SLCT_NotALiteral,
SLCT_UncheckedLiteral,
@@ -3020,7 +3029,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;
@@ -3041,17 +3051,39 @@
// completely checked only if both sub-expressions were checked.
const AbstractConditionalOperator *C =
cast<AbstractConditionalOperator>(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: {
@@ -3102,7 +3134,8 @@
return checkFormatStringExpr(S, Init, Args,
HasVAListArg, format_idx,
firstDataArg, Type, CallType,
- /*InFunctionCall*/false, CheckedVarArgs);
+ /*InFunctionCall*/false, CheckedVarArgs,
+ UncoveredArg);
}
}
@@ -3157,16 +3190,17 @@
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);
}
}
}
@@ -3183,8 +3217,12 @@
StrE = cast<StringLiteral>(E);
if (StrE) {
+ signed OldUncoveredArg = UncoveredArg.FirstUncoveredArg;
S.CheckFormatString(StrE, E, Args, HasVAListArg, format_idx, firstDataArg,
- Type, InFunctionCall, CallType, CheckedVarArgs);
+ Type, InFunctionCall, CallType, CheckedVarArgs,
+ UncoveredArg.FirstUncoveredArg);
+ if (UncoveredArg.FirstUncoveredArg != OldUncoveredArg)
+ UncoveredArg.DiagnosticExpr = E;
return SLCT_CheckedLiteral;
}
@@ -3252,10 +3290,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;
@@ -3323,7 +3371,7 @@
CoveredArgs.reset();
}
- void DoneProcessing();
+ void DoneProcessing(signed &FirstUncoveredArg);
void HandleIncompleteSpecifier(const char *startSpecifier,
unsigned specifierLen) override;
@@ -3548,27 +3596,39 @@
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();
+ if (Found < 0)
+ FirstUncoveredArg = UncoveredArgHandler::AllCovered;
+ else if (FirstUncoveredArg != UncoveredArgHandler::AllCovered)
+ FirstUncoveredArg = std::max(FirstUncoveredArg, Found);
}
}
+void UncoveredArgHandler::Diagnose(Sema &S, bool IsFunctionCall,
+ const Expr *ArgExpr) {
+ assert(FirstUncoveredArg >= 0 && DiagnosticExpr && "Invalid state");
+
+ if (!ArgExpr)
+ return;
+
+ SourceLocation Loc = ArgExpr->getLocStart();
+
+ if (S.getSourceManager().isInSystemMacro(Loc))
+ return;
+
+ CheckFormatHandler::EmitFormatDiagnostic(
+ S, IsFunctionCall, DiagnosticExpr,
+ S.PDiag(diag::warn_printf_data_arg_not_used),
+ Loc, /*IsStringLocation*/false,
+ DiagnosticExpr->getSourceRange());
+}
+
bool
CheckFormatHandler::HandleInvalidConversionSpecifier(unsigned argIndex,
SourceLocation Loc,
@@ -4662,7 +4722,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()) {
@@ -4715,16 +4776,16 @@
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,
inFunctionCall, CallType, CheckedVarArgs);
if (!analyze_format_string::ParseScanfString(H, Str, Str + StrLen,
getLangOpts(),
Context.getTargetInfo()))
- H.DoneProcessing();
+ H.DoneProcessing(FirstUncoveredArg);
} // TODO: handle other formats
}
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -9041,7 +9041,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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
