r345099 - [analyzer] Trust summaries for OSObject::retain and OSObject::release
Mikael Holmén via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 26 00:03:36 PST 2018
Hi again,
Do you have any opinion about the below valgrind complaint that starts
appearing with this patch?
valgrind still complains on it on current trunk.
I see it when compiling with clang 3.6.0. I've also tried gcc 5.4.0 but
then I don't get it.
Regards,
Mikael
On 11/21/18 8:33 AM, Mikael Holmén via cfe-commits wrote:
> 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
>>
>>
>> _______________________________________________
>> 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/20181126/10bfa372/attachment-0001.c>
More information about the cfe-commits
mailing list