[cfe-dev] r345099 - [analyzer] Trust summaries for OSObject::retain and OSObject::release
George Karpenkov via cfe-dev
cfe-dev at lists.llvm.org
Mon Nov 26 15:23:46 PST 2018
The error is indeed strange.
The body is declared as
LazyDeclStmtPtr Body;
where
using LazyDeclStmtPtr =
LazyOffsetPtr<Stmt, uint64_t, &ExternalASTSource::GetExternalDeclStmt>;
where
template<typename T, typename OffsT, T* (ExternalASTSource::*Get)(OffsT Offset)>
struct LazyOffsetPtr {
mutable uint64_t Ptr = 0;
(…)
explicit operator bool() const { return Ptr != 0; }
(…)
}
so it does not seem like it can be uninitialized.
Sadly on macOS I don’t have either valgrind or msan,
so I can’t reproduce the failure.
Do you think you could debug further?
Is “Body” indeed uninitialized at use time? (e.g. if you print it..)
A stacktrace from a debug build should be helpful.
Thanks,
George
> On Nov 26, 2018, at 12:03 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
>
> 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 <mailto:cfe-commits at lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
> <memcpy.c>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181126/734b0731/attachment.html>
More information about the cfe-dev
mailing list