These are comments about everything besides the actual checking code. I will review that next.
1. Double warnings are coming up because the format strings are being checked twice. Clang already knows that `__builtin___sprintf_chk` has a format string, so that gets checked with the old diagnostics. Instead of calling the checks directly, modify the existing path to carry extra information along. - Add an argument `bool IsSprintf` at the end of Sema::CheckFormatArguments - When Sema::CheckFormatArguments is called in Sema::checkCall, set that argument to true if `FDecl` is a FunctionDecl that is the function `__builtin___sprintf_chk` - In Sema::CheckFormatArguments, if IsSprintf is true, set the type to FST_Sprintf, otherwise, use the current type. 2. From testing, your patch looks limited in a few areas. The limitations of this warning should be documented with your CheckSprintfHandler class. For instance, non-specifiers text in the format string isn't counted. There's two ways to go from here: 3. See http://llvm.org/docs/Phabricator.html about how to add full context to your patches. Full context makes reviewing patches easier. ================ Comment at: include/clang/Sema/Sema.h:8524-8527 @@ -8523,2 +8523,6 @@ + void CheckSprintfMemAlloc(FunctionDecl *FDecl, CallExpr *TheCall, + unsigned BufIdx, unsigned FmtIdx, + unsigned DataIdx); + bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall, ---------------- Remove. ================ Comment at: include/clang/Sema/Sema.h:8569 @@ -8564,2 +8568,3 @@ FST_Printf, + FST_Sprintf, FST_NSString, ---------------- Add comment that this is not a format string attribute, but extra checking on top of a printf format string. ================ Comment at: lib/Sema/SemaChecking.cpp:483-485 @@ -482,2 +482,5 @@ break; + case Builtin::BI__builtin___sprintf_chk: + CheckSprintfMemAlloc(FDecl, TheCall, 0, 3, 4); + break; case Builtin::BI__builtin___memccpy_chk: ---------------- Remove. ================ Comment at: lib/Sema/SemaChecking.cpp:1323-1356 @@ -1319,2 +1322,36 @@ +void Sema::CheckSprintfMemAlloc(FunctionDecl *FDecl, CallExpr *TheCall, + unsigned BufIdx, unsigned FmtIdx, + unsigned DataIdx) { + unsigned NumArgs = TheCall->getNumArgs(); + if (NumArgs <= BufIdx || + NumArgs <= FmtIdx || + NumArgs <= DataIdx) + return; + + const Expr *FmtArg = TheCall->getArg(FmtIdx); + if (!isa<StringLiteral>(FmtArg->IgnoreImplicit())) + return; + + const FunctionProtoType *Proto = + FDecl->getType()->castAs<FunctionProtoType>(); + VariadicCallType CallType = getVariadicCallType(FDecl, Proto, + TheCall->getCallee()); + llvm::ArrayRef<const Expr*> Args = + llvm::makeArrayRef(TheCall->getArgs(), NumArgs); + llvm::SmallBitVector CheckedVarArgs; + if (FDecl) { + for (const auto *I : FDecl->specific_attrs<FormatAttr>()) { + // Only create vector if there are format attributes. + CheckedVarArgs.resize(Args.size()); + CheckFormatArguments(I, Args, /*IsMemberFunction=*/false, CallType, + TheCall->getLocStart(), FDecl->getSourceRange(), + CheckedVarArgs); + } + } + const StringLiteral *SL = cast<StringLiteral>(FmtArg->IgnoreImplicit()); + CheckFormatString(SL, FmtArg, Args, true, FmtIdx, DataIdx, FST_Sprintf, + /*inFunctionCall=*/true, CallType, CheckedVarArgs); +} + bool Sema::CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation lbrac, ---------------- Remove. ================ Comment at: lib/Sema/SemaChecking.cpp:3395 @@ -3357,3 +3394,3 @@ -}; +}; } ---------------- Don't make unrelated whitespace changes in your patch. ================ Comment at: lib/Sema/SemaChecking.cpp:4091-4094 @@ +4090,6 @@ +static const ConstantArrayType* GetArrayType(Sema &S, const Expr *Array) { + if (isa<DeclRefExpr>(Array)) { + QualType ArrayTy = cast<DeclRefExpr>(Array)->getDecl()->getType(); + if (ArrayTy->isConstantArrayType()) + return S.Context.getAsConstantArrayType(Array->getType()); + } ---------------- Do you care that the expression is exactly a DeclRefExpr? It doens't seem important to this warning and you can remove everything except the return statement. ================ Comment at: lib/Sema/SemaChecking.cpp:4106 @@ +4105,3 @@ + const Expr *OrigFormatExpr, unsigned FirstDataArg, + unsigned numDataArgs, bool isObjC, + const char *Begin, bool hasVAListArg, ---------------- Remove isObjC parameter. ================ Comment at: lib/Sema/SemaChecking.cpp:4113 @@ +4112,3 @@ + : CheckPrintfHandler(S, FExpr, OrigFormatExpr, FirstDataArg, numDataArgs, + isObjC, Begin, hasVAListArg, Args, FormatIdx, + inFunctionCall, CallType, CheckedVarArgs), ---------------- Replace `isObjC` with `/*isObjC=*/ false`. ================ Comment at: lib/Sema/SemaChecking.cpp:4124 @@ +4123,3 @@ + + bool Init() { + if (const ConstantArrayType *StorageBufTy = ---------------- What is this function indicating? It needs a better name, a comment, or both. ================ Comment at: lib/Sema/SemaChecking.cpp:4136-4137 @@ +4135,4 @@ + Sema &S; + llvm::APInt StorageBufSize; + llvm::APInt StorageRequired; + }; ---------------- Why are APInt's used here instead of an integer type? ================ Comment at: lib/Sema/SemaChecking.cpp:4250-4251 @@ +4249,4 @@ + if (StorageBufSize.ule(StorageRequired)) { + //S.Diag(Args[0]->getLocStart(), diag::warn_sprintf_chk_overflow) + S.Diag(Args[0]->getLocStart(), diag::warn_memcpy_chk_overflow) + << getSpecifierRange(startSpecifier, specifierLen) ---------------- A new diagnostic message should be created with more information than the current overflow diagnostic, and added to the format warning group. ================ Comment at: lib/Sema/SemaChecking.cpp:4512 @@ +4511,3 @@ + numDataArgs, + (Type == FST_NSString || Type == FST_OSTrace), + Str, HasVAListArg, Args, format_idx, ---------------- Remove this line and remove this parameter from the constructor, since the type is known at this point. ================ Comment at: lib/Sema/SemaChecking.cpp:4519 @@ +4518,3 @@ + Context.getTargetInfo(), + Type == FST_FreeBSDKPrintf)) + H.DoneProcessing(); ---------------- Replace with `/*isFreeBSDKPrintf=*/false)` ================ Comment at: test/SemaCXX/sprintf-chk.cpp:1 @@ +1,2 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -fsyntax-only %s +void test() { ---------------- There is nothing C++ about this. Move test to test/Sema/ directory. ================ Comment at: test/SemaCXX/sprintf-chk.cpp:13 @@ +12,3 @@ + + __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%d%d", 10, 10); // expected-warning {{will always overflow destination buffer}} + __builtin___sprintf_chk(bufA, 0, __builtin_object_size(bufA, 0), "%i%i", 10, 10); // expected-warning {{will always overflow destination buffer}} ---------------- Move the expected-warning to the next line and use the @-1 to declare the warning comes from the previous line. line_generating_warning(); // expected-warning@-1 {{warning message}} Also, make sure that at least one of the expected-warning checks the whole warning message. Right now, they all check truncated versions of the warning. http://reviews.llvm.org/D9665 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
