Le 17 janv. 2012 à 21:38, Eli Friedman a écrit : > On Tue, Jan 17, 2012 at 12:03 PM, Jean-Daniel Dupas > <[email protected]> wrote: >> Author: jddupas >> Date: Tue Jan 17 14:03:31 2012 >> New Revision: 148324 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=148324&view=rev >> Log: >> Fix a couples of issues in format strings checking. >> PR 10274: format function attribute with the NSString archetype yields no >> compiler warnings >> PR 10275: format function attribute isn't checked in Objective-C methods >> >> >> Modified: >> cfe/trunk/include/clang/Sema/Sema.h >> cfe/trunk/lib/Sema/SemaChecking.cpp >> cfe/trunk/lib/Sema/SemaExprObjC.cpp >> cfe/trunk/test/SemaObjC/format-strings-objc.m >> >> Modified: cfe/trunk/include/clang/Sema/Sema.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=148324&r1=148323&r2=148324&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Sema/Sema.h (original) >> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Jan 17 14:03:31 2012 >> @@ -6281,9 +6281,12 @@ >> bool AllowOnePastEnd=true, bool IndexNegated=false); >> void CheckArrayAccess(const Expr *E); >> bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall); >> + bool CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation loc, >> + Expr **Args, unsigned NumArgs); >> bool CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall); >> >> - bool CheckablePrintfAttr(const FormatAttr *Format, CallExpr *TheCall); >> + bool CheckablePrintfAttr(const FormatAttr *Format, Expr **Args, >> + unsigned NumArgs, bool IsCXXMemberCall); >> bool CheckObjCString(Expr *Arg); >> >> ExprResult CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall); >> @@ -6307,13 +6310,13 @@ >> bool SemaBuiltinConstantArg(CallExpr *TheCall, int ArgNum, >> llvm::APSInt &Result); >> >> - bool SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, >> + bool SemaCheckStringLiteral(const Expr *E, Expr **Args, unsigned NumArgs, >> bool HasVAListArg, unsigned format_idx, >> unsigned firstDataArg, bool isPrintf, >> bool inFunctionCall = true); >> >> void CheckFormatString(const StringLiteral *FExpr, const Expr >> *OrigFormatExpr, >> - const CallExpr *TheCall, bool HasVAListArg, >> + Expr **Args, unsigned NumArgs, bool HasVAListArg, >> unsigned format_idx, unsigned firstDataArg, >> bool isPrintf, bool inFunctionCall); >> >> @@ -6321,9 +6324,14 @@ >> const Expr * const *ExprArgs, >> SourceLocation CallSiteLoc); >> >> - void CheckPrintfScanfArguments(const CallExpr *TheCall, bool HasVAListArg, >> - unsigned format_idx, unsigned firstDataArg, >> - bool isPrintf); >> + void CheckFormatArguments(const FormatAttr *Format, CallExpr *TheCall); >> + void CheckFormatArguments(const FormatAttr *Format, Expr **Args, >> + unsigned NumArgs, bool IsCXXMember, >> + SourceLocation Loc, SourceRange Range); >> + void CheckPrintfScanfArguments(Expr **Args, unsigned NumArgs, >> + bool HasVAListArg, unsigned format_idx, >> + unsigned firstDataArg, bool isPrintf, >> + SourceLocation Loc, SourceRange range); >> >> void CheckMemaccessArguments(const CallExpr *Call, >> unsigned BId, >> >> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=148324&r1=148323&r2=148324&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Jan 17 14:03:31 2012 >> @@ -44,23 +44,24 @@ >> return SL->getLocationOfByte(ByteNo, PP.getSourceManager(), >> PP.getLangOptions(), PP.getTargetInfo()); >> } >> - >> >> -/// CheckablePrintfAttr - does a function call have a "printf" attribute >> -/// and arguments that merit checking? >> -bool Sema::CheckablePrintfAttr(const FormatAttr *Format, CallExpr *TheCall) >> { >> - if (Format->getType() == "printf") return true; >> - if (Format->getType() == "printf0") { >> +bool Sema::CheckablePrintfAttr(const FormatAttr *Format, Expr **Args, >> + unsigned NumArgs, bool IsCXXMemberCall) { >> + StringRef Type = Format->getType(); >> + // FIXME: add support for "CFString" Type. They are not string literal >> though, >> + // so they need special handling. >> + if (Type == "printf" || Type == "NSString") return true; >> + if (Type == "printf0") { >> // printf0 allows null "format" string; if so don't check format/args >> unsigned format_idx = Format->getFormatIdx() - 1; >> // Does the index refer to the implicit object argument? >> - if (isa<CXXMemberCallExpr>(TheCall)) { >> + if (IsCXXMemberCall) { >> if (format_idx == 0) >> return false; >> --format_idx; >> } >> - if (format_idx < TheCall->getNumArgs()) { >> - Expr *Format = TheCall->getArg(format_idx)->IgnoreParenCasts(); >> + if (format_idx < NumArgs) { >> + Expr *Format = Args[format_idx]->IgnoreParenCasts(); >> if (!Format->isNullPointerConstant(Context, >> Expr::NPC_ValueDependentIsNull)) >> return true; >> @@ -463,16 +464,7 @@ >> for (specific_attr_iterator<FormatAttr> >> i = FDecl->specific_attr_begin<FormatAttr>(), >> e = FDecl->specific_attr_end<FormatAttr>(); i != e ; ++i) { >> - >> - const FormatAttr *Format = *i; >> - const bool b = Format->getType() == "scanf"; >> - if (b || CheckablePrintfAttr(Format, TheCall)) { >> - bool HasVAListArg = Format->getFirstArg() == 0; >> - CheckPrintfScanfArguments(TheCall, HasVAListArg, >> - Format->getFormatIdx() - 1, >> - HasVAListArg ? 0 : Format->getFirstArg() - >> 1, >> - !b); >> - } >> + CheckFormatArguments(*i, TheCall); >> } >> >> for (specific_attr_iterator<NonNullAttr> >> @@ -495,6 +487,26 @@ >> return false; >> } >> >> +bool Sema::CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation lbrac, >> + Expr **Args, unsigned NumArgs) { >> + for (specific_attr_iterator<FormatAttr> >> + i = Method->specific_attr_begin<FormatAttr>(), >> + e = Method->specific_attr_end<FormatAttr>(); i != e ; ++i) { >> + >> + CheckFormatArguments(*i, Args, NumArgs, false, lbrac, >> + Method->getSourceRange()); >> + } >> + >> + // diagnose nonnull arguments. >> + for (specific_attr_iterator<NonNullAttr> >> + i = Method->specific_attr_begin<NonNullAttr>(), >> + e = Method->specific_attr_end<NonNullAttr>(); i != e; ++i) { >> + CheckNonNullArguments(*i, Args, lbrac); >> + } >> + >> + return false; >> +} >> + >> bool Sema::CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall) { >> // Printf checking. >> const FormatAttr *Format = NDecl->getAttr<FormatAttr>(); >> @@ -509,13 +521,7 @@ >> if (!Ty->isBlockPointerType()) >> return false; >> >> - const bool b = Format->getType() == "scanf"; >> - if (!b && !CheckablePrintfAttr(Format, TheCall)) >> - return false; >> - >> - bool HasVAListArg = Format->getFirstArg() == 0; >> - CheckPrintfScanfArguments(TheCall, HasVAListArg, Format->getFormatIdx() - >> 1, >> - HasVAListArg ? 0 : Format->getFirstArg() - 1, >> !b); >> + CheckFormatArguments(Format, TheCall); >> >> return false; >> } >> @@ -1368,8 +1374,8 @@ >> } >> >> // Handle i > 1 ? "x" : "y", recursively. >> -bool Sema::SemaCheckStringLiteral(const Expr *E, const CallExpr *TheCall, >> - bool HasVAListArg, >> +bool Sema::SemaCheckStringLiteral(const Expr *E, Expr **Args, >> + unsigned NumArgs, bool HasVAListArg, >> unsigned format_idx, unsigned firstDataArg, >> bool isPrintf, bool inFunctionCall) { >> tryAgain: >> @@ -1382,10 +1388,10 @@ >> case Stmt::BinaryConditionalOperatorClass: >> case Stmt::ConditionalOperatorClass: { >> const AbstractConditionalOperator *C = >> cast<AbstractConditionalOperator>(E); >> - return SemaCheckStringLiteral(C->getTrueExpr(), TheCall, HasVAListArg, >> + return SemaCheckStringLiteral(C->getTrueExpr(), Args, NumArgs, >> HasVAListArg, >> format_idx, firstDataArg, isPrintf, >> inFunctionCall) >> - && SemaCheckStringLiteral(C->getFalseExpr(), TheCall, HasVAListArg, >> + && SemaCheckStringLiteral(C->getFalseExpr(), Args, NumArgs, >> HasVAListArg, >> format_idx, firstDataArg, isPrintf, >> inFunctionCall); >> } >> @@ -1433,7 +1439,7 @@ >> >> if (isConstant) { >> if (const Expr *Init = VD->getAnyInitializer()) >> - return SemaCheckStringLiteral(Init, TheCall, >> + return SemaCheckStringLiteral(Init, Args, NumArgs, >> HasVAListArg, format_idx, >> firstDataArg, >> isPrintf, /*inFunctionCall*/false); >> } >> @@ -1474,7 +1480,7 @@ >> unsigned ArgIndex = FA->getFormatIdx(); >> const Expr *Arg = CE->getArg(ArgIndex - 1); >> >> - return SemaCheckStringLiteral(Arg, TheCall, HasVAListArg, >> + return SemaCheckStringLiteral(Arg, Args, NumArgs, HasVAListArg, >> format_idx, firstDataArg, isPrintf, >> inFunctionCall); >> } >> @@ -1494,7 +1500,7 @@ >> StrE = cast<StringLiteral>(E); >> >> if (StrE) { >> - CheckFormatString(StrE, E, TheCall, HasVAListArg, format_idx, >> + CheckFormatString(StrE, E, Args, NumArgs, HasVAListArg, format_idx, >> firstDataArg, isPrintf, inFunctionCall); >> return true; >> } >> @@ -1523,37 +1529,52 @@ >> >> /// CheckPrintfScanfArguments - Check calls to printf and scanf (and similar >> /// functions) for correct use of format strings. >> -void >> -Sema::CheckPrintfScanfArguments(const CallExpr *TheCall, bool HasVAListArg, >> - unsigned format_idx, unsigned firstDataArg, >> - bool isPrintf) { >> - >> - const Expr *Fn = TheCall->getCallee(); >> - >> +void Sema::CheckFormatArguments(const FormatAttr *Format, CallExpr >> *TheCall) { >> + bool IsCXXMember = false; >> // The way the format attribute works in GCC, the implicit this argument >> // of member functions is counted. However, it doesn't appear in our own >> // lists, so decrement format_idx in that case. >> if (isa<CXXMemberCallExpr>(TheCall)) { >> - const CXXMethodDecl *method_decl = >> - dyn_cast<CXXMethodDecl>(TheCall->getCalleeDecl()); >> - if (method_decl && method_decl->isInstance()) { >> - // Catch a format attribute mistakenly referring to the object >> argument. >> + const CXXMethodDecl *method_decl = >> + dyn_cast<CXXMethodDecl>(TheCall->getCalleeDecl()); >> + IsCXXMember = method_decl && method_decl->isInstance(); >> + } >> + CheckFormatArguments(Format, TheCall->getArgs(), TheCall->getNumArgs(), >> + IsCXXMember, TheCall->getRParenLoc(), >> + TheCall->getCallee()->getSourceRange()); >> +} >> + >> +void Sema::CheckFormatArguments(const FormatAttr *Format, Expr **Args, >> + unsigned NumArgs, bool IsCXXMember, >> + SourceLocation Loc, SourceRange Range) { >> + const bool b = Format->getType() == "scanf"; >> + if (b || CheckablePrintfAttr(Format, Args, NumArgs, IsCXXMember)) { >> + bool HasVAListArg = Format->getFirstArg() == 0; >> + unsigned format_idx = Format->getFormatIdx() - 1; >> + unsigned firstDataArg = HasVAListArg ? 0 : Format->getFirstArg() - 1; >> + if (IsCXXMember) { >> if (format_idx == 0) >> return; >> --format_idx; >> if(firstDataArg != 0) >> --firstDataArg; >> } >> + CheckPrintfScanfArguments(Args, NumArgs, HasVAListArg, format_idx, >> + firstDataArg, !b, Loc, Range); >> } >> +} >> >> +void Sema::CheckPrintfScanfArguments(Expr **Args, unsigned NumArgs, >> + bool HasVAListArg, unsigned format_idx, >> + unsigned firstDataArg, bool isPrintf, >> + SourceLocation Loc, SourceRange Range) >> { >> // CHECK: printf/scanf-like function is called with no format string. >> - if (format_idx >= TheCall->getNumArgs()) { >> - Diag(TheCall->getRParenLoc(), diag::warn_missing_format_string) >> - << Fn->getSourceRange(); >> + if (format_idx >= NumArgs) { >> + Diag(Loc, diag::warn_missing_format_string) << Range; >> return; >> } >> >> - const Expr *OrigFormatExpr = >> TheCall->getArg(format_idx)->IgnoreParenCasts(); >> + const Expr *OrigFormatExpr = Args[format_idx]->IgnoreParenCasts(); >> >> // CHECK: format string is not a string literal. >> // >> @@ -1567,18 +1588,18 @@ >> // 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. >> - if (SemaCheckStringLiteral(OrigFormatExpr, TheCall, HasVAListArg, >> format_idx, >> - firstDataArg, isPrintf)) >> + if (SemaCheckStringLiteral(OrigFormatExpr, Args, NumArgs, HasVAListArg, >> + format_idx, firstDataArg, isPrintf)) >> return; // Literal format string found, check done! >> >> // If there are no arguments specified, warn with -Wformat-security, >> otherwise >> // warn only with -Wformat-nonliteral. >> - if (TheCall->getNumArgs() == format_idx+1) >> - Diag(TheCall->getArg(format_idx)->getLocStart(), >> + if (NumArgs == format_idx+1) >> + Diag(Args[format_idx]->getLocStart(), >> diag::warn_format_nonliteral_noargs) >> << OrigFormatExpr->getSourceRange(); >> else >> - Diag(TheCall->getArg(format_idx)->getLocStart(), >> + Diag(Args[format_idx]->getLocStart(), >> diag::warn_format_nonliteral) >> << OrigFormatExpr->getSourceRange(); >> } >> @@ -1594,7 +1615,8 @@ >> const bool IsObjCLiteral; >> const char *Beg; // Start of format string. >> const bool HasVAListArg; >> - const CallExpr *TheCall; >> + const Expr * const *Args; >> + const unsigned NumArgs; >> unsigned FormatIdx; >> llvm::BitVector CoveredArgs; >> bool usesPositionalArgs; >> @@ -1605,14 +1627,14 @@ >> const Expr *origFormatExpr, unsigned firstDataArg, >> unsigned numDataArgs, bool isObjCLiteral, >> const char *beg, bool hasVAListArg, >> - const CallExpr *theCall, unsigned formatIdx, >> - bool inFunctionCall) >> + Expr **args, unsigned numArgs, >> + unsigned formatIdx, bool inFunctionCall) >> : S(s), FExpr(fexpr), OrigFormatExpr(origFormatExpr), >> FirstDataArg(firstDataArg), >> NumDataArgs(numDataArgs), >> IsObjCLiteral(isObjCLiteral), Beg(beg), >> HasVAListArg(hasVAListArg), >> - TheCall(theCall), FormatIdx(formatIdx), >> + Args(args), NumArgs(numArgs), FormatIdx(formatIdx), >> usesPositionalArgs(false), atFirstArg(true), >> inFunctionCall(inFunctionCall) { >> CoveredArgs.resize(numDataArgs); >> @@ -1727,7 +1749,7 @@ >> } >> >> const Expr *CheckFormatHandler::getDataArg(unsigned i) const { >> - return TheCall->getArg(FirstDataArg + i); >> + return Args[FirstDataArg + i]; >> } >> >> void CheckFormatHandler::DoneProcessing() { >> @@ -1811,7 +1833,7 @@ >> bool IsStringLocation, >> Range StringRange, >> FixItHint FixIt) { >> - EmitFormatDiagnostic(S, inFunctionCall, TheCall->getArg(FormatIdx), PDiag, >> + EmitFormatDiagnostic(S, inFunctionCall, Args[FormatIdx], PDiag, >> Loc, IsStringLocation, StringRange, FixIt); >> } >> >> @@ -1870,11 +1892,11 @@ >> const Expr *origFormatExpr, unsigned firstDataArg, >> unsigned numDataArgs, bool isObjCLiteral, >> const char *beg, bool hasVAListArg, >> - const CallExpr *theCall, unsigned formatIdx, >> - bool inFunctionCall) >> + Expr **Args, unsigned NumArgs, >> + unsigned formatIdx, bool inFunctionCall) >> : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg, >> numDataArgs, isObjCLiteral, beg, hasVAListArg, >> - theCall, formatIdx, inFunctionCall) {} >> + Args, NumArgs, formatIdx, inFunctionCall) {} >> >> >> bool HandleInvalidPrintfConversionSpecifier( >> @@ -2194,11 +2216,11 @@ >> const Expr *origFormatExpr, unsigned firstDataArg, >> unsigned numDataArgs, bool isObjCLiteral, >> const char *beg, bool hasVAListArg, >> - const CallExpr *theCall, unsigned formatIdx, >> - bool inFunctionCall) >> + Expr **Args, unsigned NumArgs, >> + unsigned formatIdx, bool inFunctionCall) >> : CheckFormatHandler(s, fexpr, origFormatExpr, firstDataArg, >> numDataArgs, isObjCLiteral, beg, hasVAListArg, >> - theCall, formatIdx, inFunctionCall) {} >> + Args, NumArgs, formatIdx, inFunctionCall) {} >> >> bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS, >> const char *startSpecifier, >> @@ -2341,14 +2363,15 @@ >> >> void Sema::CheckFormatString(const StringLiteral *FExpr, >> const Expr *OrigFormatExpr, >> - const CallExpr *TheCall, bool HasVAListArg, >> - unsigned format_idx, unsigned firstDataArg, >> - bool isPrintf, bool inFunctionCall) { >> + Expr **Args, unsigned NumArgs, >> + bool HasVAListArg, unsigned format_idx, >> + unsigned firstDataArg, bool isPrintf, >> + bool inFunctionCall) { >> >> // CHECK: is the format string a wide literal? >> if (!FExpr->isAscii()) { >> CheckFormatHandler::EmitFormatDiagnostic( >> - *this, inFunctionCall, TheCall->getArg(format_idx), >> + *this, inFunctionCall, Args[format_idx], >> PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(), >> /*IsStringLocation*/true, OrigFormatExpr->getSourceRange()); >> return; >> @@ -2358,12 +2381,12 @@ >> StringRef StrRef = FExpr->getString(); >> const char *Str = StrRef.data(); >> unsigned StrLen = StrRef.size(); >> - const unsigned numDataArgs = TheCall->getNumArgs() - firstDataArg; >> + const unsigned numDataArgs = NumArgs - firstDataArg; >> >> // CHECK: empty format string? >> if (StrLen == 0 && numDataArgs > 0) { >> CheckFormatHandler::EmitFormatDiagnostic( >> - *this, inFunctionCall, TheCall->getArg(format_idx), >> + *this, inFunctionCall, Args[format_idx], >> PDiag(diag::warn_empty_format_string), FExpr->getLocStart(), >> /*IsStringLocation*/true, OrigFormatExpr->getSourceRange()); >> return; >> @@ -2372,7 +2395,7 @@ >> if (isPrintf) { >> CheckPrintfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, >> numDataArgs, isa<ObjCStringLiteral>(OrigFormatExpr), >> - Str, HasVAListArg, TheCall, format_idx, >> + Str, HasVAListArg, Args, NumArgs, format_idx, >> inFunctionCall); >> >> if (!analyze_format_string::ParsePrintfString(H, Str, Str + StrLen, >> @@ -2382,7 +2405,7 @@ >> else { >> CheckScanfHandler H(*this, FExpr, OrigFormatExpr, firstDataArg, >> numDataArgs, isa<ObjCStringLiteral>(OrigFormatExpr), >> - Str, HasVAListArg, TheCall, format_idx, >> + Str, HasVAListArg, Args, NumArgs, format_idx, >> inFunctionCall); >> >> if (!analyze_format_string::ParseScanfString(H, Str, Str + StrLen, >> >> Modified: cfe/trunk/lib/Sema/SemaExprObjC.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprObjC.cpp?rev=148324&r1=148323&r2=148324&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaExprObjC.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaExprObjC.cpp Tue Jan 17 14:03:31 2012 >> @@ -453,14 +453,12 @@ >> Args[NumArgs-1]->getLocEnd()); >> } >> } >> - // diagnose nonnull arguments. >> - for (specific_attr_iterator<NonNullAttr> >> - i = Method->specific_attr_begin<NonNullAttr>(), >> - e = Method->specific_attr_end<NonNullAttr>(); i != e; ++i) { >> - CheckNonNullArguments(*i, Args, lbrac); >> - } >> >> DiagnoseSentinelCalls(Method, lbrac, Args, NumArgs); >> + >> + // Do additional checkings on method. >> + IsError |= CheckObjCMethodCall(Method, lbrac, Args, NumArgs); >> + >> return IsError; >> } >> >> >> Modified: cfe/trunk/test/SemaObjC/format-strings-objc.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/format-strings-objc.m?rev=148324&r1=148323&r2=148324&view=diff >> ============================================================================== >> --- cfe/trunk/test/SemaObjC/format-strings-objc.m (original) >> +++ cfe/trunk/test/SemaObjC/format-strings-objc.m Tue Jan 17 14:03:31 2012 >> @@ -63,3 +63,22 @@ >> printf("%p", y); // no-warning >> } >> >> +// <rdar://problem/10696348>, PR 10274 - CFString and NSString formats are >> ignored >> +extern void MyNSLog(NSString *format, ...) >> __attribute__((format(__NSString__, 1, 2))); >> +extern void MyCFStringCreateWithFormat(CFStringRef format, ...) >> __attribute__((format(__CFString__, 1, 2))); >> + >> +void check_mylog() { >> + MyNSLog(@"%@"); // expected-warning {{more '%' conversions than data >> arguments}} >> + // FIXME: find a way to test CFString too, but I don't know how to create >> constant CFString. > > A constant CFString is written as CFSTR("foo"), which expands to a > call to __builtin___CFStringMakeConstantString. > > -Eli
Yes, I'm working on that part, but as __builtin___CFStringMakeConstantString is not interpreted as a string literal, it needs some special handling in the format checker. -- Jean-Daniel _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
