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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
#include <string.h> char a[2]; void foo() { memcpy(a, "ab", 2); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits