r345099 - [analyzer] Trust summaries for OSObject::retain and OSObject::release
Mikael Holmén via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 20 23:33:06 PST 2018
Hi George,
I noticed that valgrind started complaining in one case with this patch.
I've no idea if it's really due to something in the patch or if it's
something old that surfaced or if it's a false flag.
Anyway, with this patch the following
valgrind clang-tidy -checks='-*,clang-analyzer-*' 'memcpy.c' -- -O0
gives me
==18829== Memcheck, a memory error detector
==18829== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==18829== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==18829== Command: build-all/bin/clang-tidy -checks=-*,clang-analyzer-*
memcpy.c -- -O0
==18829==
==18829== Conditional jump or move depends on uninitialised value(s)
==18829== at 0xE580DF:
clang::ento::RetainSummaryManager::canEval(clang::CallExpr const*,
clang::FunctionDecl const*, bool&) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xD034AA:
clang::ento::retaincountchecker::RetainCountChecker::evalCall(clang::CallExpr
const*, clang::ento::CheckerContext&) const (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDCBCD7:
clang::ento::CheckerManager::runCheckersForEvalCall(clang::ento::ExplodedNodeSet&,
clang::ento::ExplodedNodeSet const&, clang::ento::CallEvent const&,
clang::ento::ExprEngine&) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xE033D5:
clang::ento::ExprEngine::evalCall(clang::ento::ExplodedNodeSet&,
clang::ento::ExplodedNode*, clang::ento::CallEvent const&) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xE03165:
clang::ento::ExprEngine::VisitCallExpr(clang::CallExpr const*,
clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDE3D9A: clang::ento::ExprEngine::Visit(clang::Stmt
const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDDEFD1:
clang::ento::ExprEngine::ProcessStmt(clang::Stmt const*,
clang::ento::ExplodedNode*) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDDEBBC:
clang::ento::ExprEngine::processCFGElement(clang::CFGElement,
clang::ento::ExplodedNode*, unsigned int,
clang::ento::NodeBuilderContext*) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDD3154:
clang::ento::CoreEngine::HandlePostStmt(clang::CFGBlock const*, unsigned
int, clang::ento::ExplodedNode*) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xDD24D3:
clang::ento::CoreEngine::ExecuteWorkList(clang::LocationContext const*,
unsigned int, llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>)
(in /data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xB8E90E: (anonymous
namespace)::AnalysisConsumer::HandleCode(clang::Decl*, unsigned int,
clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl
const*, llvm::DenseMapInfo<clang::Decl const*> >*) (in
/data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829== by 0xB89943: (anonymous
namespace)::AnalysisConsumer::HandleTranslationUnit(clang::ASTContext&)
(in /data/repo/llvm-patch/build-all/bin/clang-tidy)
==18829==
The call to
const FunctionDecl* FDD = FD->getDefinition();
in RetainSummaryManager::canEval eventually ends up in
bool isThisDeclarationADefinition() const {
return isDeletedAsWritten() || isDefaulted() || Body ||
hasSkippedBody() ||
isLateTemplateParsed() || willHaveBody() || hasDefiningAttr();
}
And here it seems to be the access of "Body" that valgrind complains on.
If I simply comment out "Body" the complaint is gone.
I really have no clue about this code, but perhaps this makes some sense
to you? Or perhaps to someone else?
Regards,
Mikael
On 10/24/18 1:11 AM, George Karpenkov via cfe-commits wrote:
> Author: george.karpenkov
> Date: Tue Oct 23 16:11:30 2018
> New Revision: 345099
>
> URL: http://llvm.org/viewvc/llvm-project?rev=345099&view=rev
> Log:
> [analyzer] Trust summaries for OSObject::retain and OSObject::release
>
> Refactor the way in which summaries are consumed for safeMetaCast
>
> Differential Revision: https://reviews.llvm.org/D53549
>
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
> cfe/trunk/test/Analysis/osobject-retain-release.cpp
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp?rev=345099&r1=345098&r2=345099&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp Tue Oct 23 16:11:30 2018
> @@ -776,31 +776,27 @@ bool RetainCountChecker::evalCall(const
>
> const LocationContext *LCtx = C.getLocationContext();
>
> - // Process OSDynamicCast: should just return the first argument.
> - // For now, tresting the cast as a no-op, and disregarding the case where
> - // the output becomes null due to the type mismatch.
> - if (FD->getNameAsString() == "safeMetaCast") {
> - state = state->BindExpr(CE, LCtx,
> - state->getSVal(CE->getArg(0), LCtx));
> - C.addTransition(state);
> - return true;
> - }
> -
> // See if it's one of the specific functions we know how to eval.
> if (!SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation))
> return false;
>
> // Bind the return value.
> - SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
> - if (RetVal.isUnknown() ||
> - (hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
> + // For now, all the functions which we can evaluate and which take
> + // at least one argument are identities.
> + if (CE->getNumArgs() >= 1) {
> + SVal RetVal = state->getSVal(CE->getArg(0), LCtx);
> +
> // If the receiver is unknown or the function has
> // 'rc_ownership_trusted_implementation' annotate attribute, conjure a
> // return value.
> - SValBuilder &SVB = C.getSValBuilder();
> - RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
> + if (RetVal.isUnknown() ||
> + (hasTrustedImplementationAnnotation && !ResultTy.isNull())) {
> + SValBuilder &SVB = C.getSValBuilder();
> + RetVal =
> + SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount());
> + }
> + state = state->BindExpr(CE, LCtx, RetVal, false);
> }
> - state = state->BindExpr(CE, LCtx, RetVal, false);
>
> C.addTransition(state);
> return true;
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=345099&r1=345098&r2=345099&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Tue Oct 23 16:11:30 2018
> @@ -102,9 +102,6 @@ RetainSummaryManager::generateSummary(co
> return getPersistentStopSummary();
> }
>
> - // [PR 3337] Use 'getAs<FunctionType>' to strip away any typedefs on the
> - // function's type.
> - const FunctionType *FT = FD->getType()->getAs<FunctionType>();
> const IdentifierInfo *II = FD->getIdentifier();
> if (!II)
> return getDefaultSummary();
> @@ -115,7 +112,8 @@ RetainSummaryManager::generateSummary(co
> // down below.
> FName = FName.substr(FName.find_first_not_of('_'));
>
> - // Inspect the result type.
> + // Inspect the result type. Strip away any typedefs.
> + const auto *FT = FD->getType()->getAs<FunctionType>();
> QualType RetTy = FT->getReturnType();
> std::string RetTyName = RetTy.getAsString();
>
> @@ -506,12 +504,6 @@ bool RetainSummaryManager::isTrustedRefe
> bool RetainSummaryManager::canEval(const CallExpr *CE,
> const FunctionDecl *FD,
> bool &hasTrustedImplementationAnnotation) {
> - // For now, we're only handling the functions that return aliases of their
> - // arguments: CFRetain (and its families).
> - // Eventually we should add other functions we can model entirely,
> - // such as CFRelease, which don't invalidate their arguments or globals.
> - if (CE->getNumArgs() != 1)
> - return false;
>
> IdentifierInfo *II = FD->getIdentifier();
> if (!II)
> @@ -533,6 +525,13 @@ bool RetainSummaryManager::canEval(const
> return isRetain(FD, FName) || isAutorelease(FD, FName) ||
> isMakeCollectable(FName);
>
> + // Process OSDynamicCast: should just return the first argument.
> + // For now, treating the cast as a no-op, and disregarding the case where
> + // the output becomes null due to the type mismatch.
> + if (TrackOSObjects && FName == "safeMetaCast") {
> + return true;
> + }
> +
> const FunctionDecl* FDD = FD->getDefinition();
> if (FDD && isTrustedReferenceCountImplementation(FDD)) {
> hasTrustedImplementationAnnotation = true;
> @@ -540,6 +539,12 @@ bool RetainSummaryManager::canEval(const
> }
> }
>
> + if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
> + const CXXRecordDecl *Parent = MD->getParent();
> + if (TrackOSObjects && Parent && isOSObjectSubclass(Parent))
> + return FName == "release" || FName == "retain";
> + }
> +
> return false;
>
> }
>
> Modified: cfe/trunk/test/Analysis/osobject-retain-release.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/osobject-retain-release.cpp?rev=345099&r1=345098&r2=345099&view=diff
> ==============================================================================
> --- cfe/trunk/test/Analysis/osobject-retain-release.cpp (original)
> +++ cfe/trunk/test/Analysis/osobject-retain-release.cpp Tue Oct 23 16:11:30 2018
> @@ -9,7 +9,7 @@ struct OSMetaClass;
>
> struct OSObject {
> virtual void retain();
> - virtual void release();
> + virtual void release() {};
> virtual ~OSObject(){}
>
> static OSObject *generateObject(int);
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: memcpy.c
Type: text/x-csrc
Size: 80 bytes
Desc: memcpy.c
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181121/a264f9ea/attachment.c>
More information about the cfe-commits
mailing list