On Jan 13, 2012, at 10:18 PM, Chris Lattner wrote: > > On Jan 13, 2012, at 1:52 PM, Anna Zaks wrote: > >> Author: zaks >> Date: Fri Jan 13 15:52:01 2012 >> New Revision: 148142 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=148142&view=rev >> Log: >> Move identification of memory setting and copying functions (memset, >> memcmp, strncmp,..) out of Sema and into FunctionDecl so that the logic >> could be reused in the analyzer. > > Hi Anna, > > Is there a reason not to just use Builtin ID's directly? Why have another > enum? >
Good point. The reason for the new enum is that it provides better readability: documents what the method handles; and in all the places where the value is used, makes it obvious that it's not just a single builtin. On the other hand, it's probably more undesirable to grow FunctionDecl.. I'll patch to use the built-in ID. Thanks! Anna. > -Chris > >> >> Modified: >> cfe/trunk/include/clang/AST/Decl.h >> cfe/trunk/include/clang/Sema/Sema.h >> cfe/trunk/lib/AST/Decl.cpp >> cfe/trunk/lib/Sema/SemaChecking.cpp >> >> Modified: cfe/trunk/include/clang/AST/Decl.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=148142&r1=148141&r2=148142&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/AST/Decl.h (original) >> +++ cfe/trunk/include/clang/AST/Decl.h Fri Jan 13 15:52:01 2012 >> @@ -1968,6 +1968,27 @@ >> /// definition of a member function. >> virtual bool isOutOfLine() const; >> >> + /// \brief Enumeration used to identify memory setting or copying >> functions >> + /// identified by getMemoryFunctionKind(). >> + enum MemoryFunctionKind { >> + MFK_Memset, >> + MFK_Memcpy, >> + MFK_Memmove, >> + MFK_Memcmp, >> + MFK_Strncpy, >> + MFK_Strncmp, >> + MFK_Strncasecmp, >> + MFK_Strncat, >> + MFK_Strndup, >> + MFK_Strlcpy, >> + MFK_Strlcat, >> + MFK_Invalid >> + }; >> + >> + /// \brief If the given function is a memory copy or setting function, >> return >> + /// it's kind. If the function is not a memory function, returns >> MFK_Invalid. >> + MemoryFunctionKind getMemoryFunctionKind(); >> + >> // Implement isa/cast/dyncast/etc. >> static bool classof(const Decl *D) { return classofKind(D->getKind()); } >> static bool classof(const FunctionDecl *D) { return true; } >> >> Modified: cfe/trunk/include/clang/Sema/Sema.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=148142&r1=148141&r2=148142&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Sema/Sema.h (original) >> +++ cfe/trunk/include/clang/Sema/Sema.h Fri Jan 13 15:52:01 2012 >> @@ -6311,21 +6311,8 @@ >> unsigned format_idx, unsigned firstDataArg, >> bool isPrintf); >> >> - /// \brief Enumeration used to describe which of the memory setting or >> copying >> - /// functions is being checked by \c CheckMemaccessArguments(). >> - enum CheckedMemoryFunction { >> - CMF_Memset, >> - CMF_Memcpy, >> - CMF_Memmove, >> - CMF_Memcmp, >> - CMF_Strncpy, >> - CMF_Strncmp, >> - CMF_Strncasecmp, >> - CMF_Strncat, >> - CMF_Strndup >> - }; >> - >> - void CheckMemaccessArguments(const CallExpr *Call, CheckedMemoryFunction >> CMF, >> + void CheckMemaccessArguments(const CallExpr *Call, >> + FunctionDecl::MemoryFunctionKind CMF, >> IdentifierInfo *FnName); >> >> void CheckStrlcpycatArguments(const CallExpr *Call, >> >> Modified: cfe/trunk/lib/AST/Decl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=148142&r1=148141&r2=148142&view=diff >> ============================================================================== >> --- cfe/trunk/lib/AST/Decl.cpp (original) >> +++ cfe/trunk/lib/AST/Decl.cpp Fri Jan 13 15:52:01 2012 >> @@ -2302,6 +2302,83 @@ >> return SourceRange(getOuterLocStart(), EndRangeLoc); >> } >> >> +FunctionDecl::MemoryFunctionKind FunctionDecl::getMemoryFunctionKind() { >> + IdentifierInfo *FnInfo = getIdentifier(); >> + >> + if (!FnInfo) >> + return MFK_Invalid; >> + >> + // Builtin handling. >> + switch (getBuiltinID()) { >> + case Builtin::BI__builtin_memset: >> + case Builtin::BI__builtin___memset_chk: >> + case Builtin::BImemset: >> + return MFK_Memset; >> + >> + case Builtin::BI__builtin_memcpy: >> + case Builtin::BI__builtin___memcpy_chk: >> + case Builtin::BImemcpy: >> + return MFK_Memcpy; >> + >> + case Builtin::BI__builtin_memmove: >> + case Builtin::BI__builtin___memmove_chk: >> + case Builtin::BImemmove: >> + return MFK_Memmove; >> + >> + case Builtin::BIstrlcpy: >> + return MFK_Strlcpy; >> + case Builtin::BIstrlcat: >> + return MFK_Strlcat; >> + >> + case Builtin::BI__builtin_memcmp: >> + return MFK_Memcmp; >> + >> + case Builtin::BI__builtin_strncpy: >> + case Builtin::BI__builtin___strncpy_chk: >> + case Builtin::BIstrncpy: >> + return MFK_Strncpy; >> + >> + case Builtin::BI__builtin_strncmp: >> + return MFK_Strncmp; >> + >> + case Builtin::BI__builtin_strncasecmp: >> + return MFK_Strncasecmp; >> + >> + case Builtin::BI__builtin_strncat: >> + case Builtin::BIstrncat: >> + return MFK_Strncat; >> + >> + case Builtin::BI__builtin_strndup: >> + case Builtin::BIstrndup: >> + return MFK_Strndup; >> + >> + default: >> + if (getLinkage() == ExternalLinkage && >> + (!getASTContext().getLangOptions().CPlusPlus || isExternC())) { >> + if (FnInfo->isStr("memset")) >> + return MFK_Memset; >> + else if (FnInfo->isStr("memcpy")) >> + return MFK_Memcpy; >> + else if (FnInfo->isStr("memmove")) >> + return MFK_Memmove; >> + else if (FnInfo->isStr("memcmp")) >> + return MFK_Memcmp; >> + else if (FnInfo->isStr("strncpy")) >> + return MFK_Strncpy; >> + else if (FnInfo->isStr("strncmp")) >> + return MFK_Strncmp; >> + else if (FnInfo->isStr("strncasecmp")) >> + return MFK_Strncasecmp; >> + else if (FnInfo->isStr("strncat")) >> + return MFK_Strncat; >> + else if (FnInfo->isStr("strndup")) >> + return MFK_Strndup; >> + } >> + break; >> + } >> + return MFK_Invalid; >> +} >> + >> //===----------------------------------------------------------------------===// >> // FieldDecl Implementation >> //===----------------------------------------------------------------------===// >> >> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=148142&r1=148141&r2=148142&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jan 13 15:52:01 2012 >> @@ -480,88 +480,15 @@ >> TheCall->getCallee()->getLocStart()); >> } >> >> - // Builtin handling >> - int CMF = -1; >> - switch (FDecl->getBuiltinID()) { >> - case Builtin::BI__builtin_memset: >> - case Builtin::BI__builtin___memset_chk: >> - case Builtin::BImemset: >> - CMF = CMF_Memset; >> - break; >> - >> - case Builtin::BI__builtin_memcpy: >> - case Builtin::BI__builtin___memcpy_chk: >> - case Builtin::BImemcpy: >> - CMF = CMF_Memcpy; >> - break; >> - >> - case Builtin::BI__builtin_memmove: >> - case Builtin::BI__builtin___memmove_chk: >> - case Builtin::BImemmove: >> - CMF = CMF_Memmove; >> - break; >> + FunctionDecl::MemoryFunctionKind CMF = FDecl->getMemoryFunctionKind(); >> + if (CMF == FunctionDecl::MFK_Invalid) >> + return false; >> >> - case Builtin::BIstrlcpy: >> - case Builtin::BIstrlcat: >> + // Handle memory setting and copying functions. >> + if (CMF == FunctionDecl::MFK_Strlcpy || CMF == FunctionDecl::MFK_Strlcat) >> CheckStrlcpycatArguments(TheCall, FnInfo); >> - break; >> - >> - case Builtin::BI__builtin_memcmp: >> - CMF = CMF_Memcmp; >> - break; >> - >> - case Builtin::BI__builtin_strncpy: >> - case Builtin::BI__builtin___strncpy_chk: >> - case Builtin::BIstrncpy: >> - CMF = CMF_Strncpy; >> - break; >> - >> - case Builtin::BI__builtin_strncmp: >> - CMF = CMF_Strncmp; >> - break; >> - >> - case Builtin::BI__builtin_strncasecmp: >> - CMF = CMF_Strncasecmp; >> - break; >> - >> - case Builtin::BI__builtin_strncat: >> - case Builtin::BIstrncat: >> - CMF = CMF_Strncat; >> - break; >> - >> - case Builtin::BI__builtin_strndup: >> - case Builtin::BIstrndup: >> - CMF = CMF_Strndup; >> - break; >> - >> - default: >> - if (FDecl->getLinkage() == ExternalLinkage && >> - (!getLangOptions().CPlusPlus || FDecl->isExternC())) { >> - if (FnInfo->isStr("memset")) >> - CMF = CMF_Memset; >> - else if (FnInfo->isStr("memcpy")) >> - CMF = CMF_Memcpy; >> - else if (FnInfo->isStr("memmove")) >> - CMF = CMF_Memmove; >> - else if (FnInfo->isStr("memcmp")) >> - CMF = CMF_Memcmp; >> - else if (FnInfo->isStr("strncpy")) >> - CMF = CMF_Strncpy; >> - else if (FnInfo->isStr("strncmp")) >> - CMF = CMF_Strncmp; >> - else if (FnInfo->isStr("strncasecmp")) >> - CMF = CMF_Strncasecmp; >> - else if (FnInfo->isStr("strncat")) >> - CMF = CMF_Strncat; >> - else if (FnInfo->isStr("strndup")) >> - CMF = CMF_Strndup; >> - } >> - break; >> - } >> - >> - // Memset/memcpy/memmove handling >> - if (CMF != -1) >> - CheckMemaccessArguments(TheCall, CheckedMemoryFunction(CMF), FnInfo); >> + else >> + CheckMemaccessArguments(TheCall, CMF, FnInfo); >> >> return false; >> } >> @@ -2500,16 +2427,17 @@ >> /// >> /// \param Call The call expression to diagnose. >> void Sema::CheckMemaccessArguments(const CallExpr *Call, >> - CheckedMemoryFunction CMF, >> + FunctionDecl::MemoryFunctionKind CMF, >> IdentifierInfo *FnName) { >> // It is possible to have a non-standard definition of memset. Validate >> // we have enough arguments, and if not, abort further checking. >> - unsigned ExpectedNumArgs = (CMF == CMF_Strndup ? 2 : 3); >> + unsigned ExpectedNumArgs = (CMF == FunctionDecl::MFK_Strndup ? 2 : 3); >> if (Call->getNumArgs() < ExpectedNumArgs) >> return; >> >> - unsigned LastArg = (CMF == CMF_Memset || CMF == CMF_Strndup ? 1 : 2); >> - unsigned LenArg = (CMF == CMF_Strndup ? 1 : 2); >> + unsigned LastArg = (CMF == FunctionDecl::MFK_Memset || >> + CMF == FunctionDecl::MFK_Strndup ? 1 : 2); >> + unsigned LenArg = (CMF == FunctionDecl::MFK_Strndup ? 1 : 2); >> const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts(); >> >> // We have special checking when the length is a sizeof expression. >> @@ -2553,7 +2481,8 @@ >> if (Context.getTypeSize(PointeeTy) == Context.getCharWidth()) >> ActionIdx = 2; // If the pointee's size is sizeof(char), >> // suggest an explicit length. >> - unsigned DestSrcSelect = (CMF == CMF_Strndup ? 1 : ArgIdx); >> + unsigned DestSrcSelect = >> + (CMF == FunctionDecl::MFK_Strndup ? 1 : ArgIdx); >> DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest, >> PDiag(diag::warn_sizeof_pointer_expr_memaccess) >> << FnName << DestSrcSelect << ActionIdx >> @@ -2583,12 +2512,15 @@ >> DiagRuntimeBehavior( >> Dest->getExprLoc(), Dest, >> PDiag(diag::warn_dyn_class_memaccess) >> - << (CMF == CMF_Memcmp ? ArgIdx + 2 : ArgIdx) << FnName << >> PointeeTy >> + << (CMF == FunctionDecl::MFK_Memcmp ? ArgIdx + 2 : ArgIdx) >> + << FnName << PointeeTy >> // "overwritten" if we're warning about the destination for any >> call >> // but memcmp; otherwise a verb appropriate to the call. >> - << (ArgIdx == 0 && CMF != CMF_Memcmp ? 0 : (unsigned)CMF) >> + << (ArgIdx == 0 && >> + CMF != FunctionDecl::MFK_Memcmp ? 0 : (unsigned)CMF) >> << Call->getCallee()->getSourceRange()); >> - else if (PointeeTy.hasNonTrivialObjCLifetime() && CMF != CMF_Memset) >> + else if (PointeeTy.hasNonTrivialObjCLifetime() && >> + CMF != FunctionDecl::MFK_Memset) >> DiagRuntimeBehavior( >> Dest->getExprLoc(), Dest, >> PDiag(diag::warn_arc_object_memaccess) >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
