Thanks for the catch! Should be fixed in r206899. -DeLesley
On Tue, Apr 22, 2014 at 2:44 AM, Timur Iskhodzhanov <[email protected]> wrote: > Hi, > > Can you please take a look at the warning I get using MSVC? > tools\clang\lib\analysis\threadsafetycommon.cpp(368) : warning C4715: > 'clang::threadSafety::SExprBuilder::translateBinaryOperator' : not all > control paths return a value > > 2014-04-22 3:18 GMT+04:00 DeLesley Hutchins <[email protected]>: >> Author: delesley >> Date: Mon Apr 21 18:18:18 2014 >> New Revision: 206827 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=206827&view=rev >> Log: >> Thread safety analysis: misc updates to SExpr handling. Fix to minimal SSA, >> function parameters, and compound assignment. >> >> Modified: >> cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyCommon.h >> cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h >> cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h >> cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp >> >> Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyCommon.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyCommon.h?rev=206827&r1=206826&r2=206827&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyCommon.h (original) >> +++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Mon Apr >> 21 18:18:18 2014 >> @@ -192,7 +192,9 @@ public: >> const CFG *getGraph() const { return CFGraph; } >> CFG *getGraph() { return CFGraph; } >> >> - const NamedDecl *getDecl() const { return >> cast<NamedDecl>(ACtx->getDecl()); } >> + const FunctionDecl *getDecl() const { >> + return dyn_cast<FunctionDecl>(ACtx->getDecl()); >> + } >> >> const PostOrderCFGView *getSortedGraph() const { return SortedGraph; } >> >> @@ -237,6 +239,10 @@ public: >> // FIXME: we don't always have a self-variable. >> SelfVar = new (Arena) til::Variable(til::Variable::VK_SFun); >> } >> + ~SExprBuilder() { >> + if (CallCtx) >> + delete CallCtx; >> + } >> >> // Translate a clang statement or expression to a TIL expression. >> // Also performs substitution of variables; Ctx provides the context. >> @@ -251,7 +257,7 @@ public: >> } >> >> const til::SCFG *getCFG() const { return Scfg; } >> - til::SCFG *getCFF() { return Scfg; } >> + til::SCFG *getCFG() { return Scfg; } >> >> private: >> til::SExpr *translateDeclRefExpr(const DeclRefExpr *DRE, >> @@ -265,6 +271,9 @@ private: >> CallingContext *Ctx); >> til::SExpr *translateUnaryOperator(const UnaryOperator *UO, >> CallingContext *Ctx); >> + til::SExpr *translateBinAssign(til::TIL_BinaryOpcode Op, >> + const BinaryOperator *BO, >> + CallingContext *Ctx); >> til::SExpr *translateBinaryOperator(const BinaryOperator *BO, >> CallingContext *Ctx); >> til::SExpr *translateCastExpr(const CastExpr *CE, CallingContext *Ctx); >> @@ -320,7 +329,7 @@ private: >> // We implement the CFGVisitor API >> friend class CFGWalker; >> >> - void enterCFG(CFG *Cfg, const NamedDecl *D, const CFGBlock *First); >> + void enterCFG(CFG *Cfg, const FunctionDecl *D, const CFGBlock *First); >> void enterCFGBlock(const CFGBlock *B); >> bool visitPredecessors() { return true; } >> void handlePredecessor(const CFGBlock *Pred); >> >> Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h?rev=206827&r1=206826&r2=206827&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h (original) >> +++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTIL.h Mon Apr 21 >> 18:18:18 2014 >> @@ -222,6 +222,7 @@ public: >> Id = static_cast<unsigned short>(I); >> } >> void setClangDecl(const clang::ValueDecl *VD) { Cvdecl = VD; } >> + void setDefinition(SExpr *E); >> >> template <class V> typename V::R_SExpr traverse(V &Visitor) { >> // This routine is only called for variable references. >> @@ -316,7 +317,8 @@ private: >> SExprRef *Location; >> }; >> >> -void SExprRef::attach() { >> + >> +inline void SExprRef::attach() { >> if (!Ptr) >> return; >> >> @@ -328,45 +330,49 @@ void SExprRef::attach() { >> } >> } >> >> -void SExprRef::detach() { >> +inline void SExprRef::detach() { >> if (Ptr && Ptr->opcode() == COP_Variable) { >> cast<Variable>(Ptr)->detachVar(); >> } >> } >> >> -SExprRef::SExprRef(SExpr *P) : Ptr(P) { >> +inline SExprRef::SExprRef(SExpr *P) : Ptr(P) { >> attach(); >> } >> >> -SExprRef::~SExprRef() { >> +inline SExprRef::~SExprRef() { >> detach(); >> } >> >> -void SExprRef::reset(SExpr *P) { >> +inline void SExprRef::reset(SExpr *P) { >> detach(); >> Ptr = P; >> attach(); >> } >> >> >> -Variable::Variable(VariableKind K, SExpr *D, const clang::ValueDecl *Cvd) >> +inline Variable::Variable(VariableKind K, SExpr *D, const clang::ValueDecl >> *Cvd) >> : SExpr(COP_Variable), Definition(D), Cvdecl(Cvd), >> BlockID(0), Id(0), NumUses(0) { >> Flags = K; >> } >> >> -Variable::Variable(SExpr *D, const clang::ValueDecl *Cvd) >> +inline Variable::Variable(SExpr *D, const clang::ValueDecl *Cvd) >> : SExpr(COP_Variable), Definition(D), Cvdecl(Cvd), >> BlockID(0), Id(0), NumUses(0) { >> Flags = VK_Let; >> } >> >> -Variable::Variable(const Variable &Vd, SExpr *D) // rewrite constructor >> +inline Variable::Variable(const Variable &Vd, SExpr *D) // rewrite >> constructor >> : SExpr(Vd), Definition(D), Cvdecl(Vd.Cvdecl), >> BlockID(0), Id(0), NumUses(0) { >> Flags = Vd.kind(); >> } >> >> +inline void Variable::setDefinition(SExpr *E) { >> + Definition.reset(E); >> +} >> + >> void Future::force() { >> Status = FS_evaluating; >> SExpr *R = create(); >> @@ -376,6 +382,7 @@ void Future::force() { >> Status = FS_done; >> } >> >> + >> // Placeholder for C++ expressions that cannot be represented in the TIL. >> class Undefined : public SExpr { >> public: >> >> Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h?rev=206827&r1=206826&r2=206827&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h >> (original) >> +++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h Mon Apr >> 21 18:18:18 2014 >> @@ -396,7 +396,12 @@ public: >> // Pretty printer for TIL expressions >> template <typename Self, typename StreamType> >> class PrettyPrinter { >> +private: >> + bool Verbose; // Print out additional information >> + >> public: >> + PrettyPrinter(bool V = false) : Verbose(V) { } >> + >> static void print(SExpr *E, StreamType &SS) { >> Self printer; >> printer.printSExpr(E, SS, Prec_MAX); >> @@ -530,17 +535,21 @@ protected: >> SS << E->clangDecl()->getNameAsString(); >> } >> >> - void printVariable(Variable *E, StreamType &SS, bool IsVarDecl = false) { >> - SS << E->name() << E->getBlockID() << "_" << E->getID(); >> - if (IsVarDecl) >> - return; >> - >> - SExpr *V = getCanonicalVal(E); >> - if (V != E) { >> - SS << "{"; >> - printSExpr(V, SS, Prec_MAX); >> - SS << "}"; >> + void printVariable(Variable *V, StreamType &SS, bool IsVarDecl = false) { >> + SExpr* E = nullptr; >> + if (!IsVarDecl) { >> + E = getCanonicalVal(V); >> + if (E != V) { >> + printSExpr(E, SS, Prec_Atom); >> + if (Verbose) { >> + SS << " /*"; >> + SS << V->name() << V->getBlockID() << "_" << V->getID(); >> + SS << "*/"; >> + } >> + return; >> + } >> } >> + SS << V->name() << V->getBlockID() << "_" << V->getID(); >> } >> >> void printFunction(Function *E, StreamType &SS, unsigned sugared = 0) { >> >> Modified: cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp?rev=206827&r1=206826&r2=206827&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp (original) >> +++ cfe/trunk/lib/Analysis/ThreadSafetyCommon.cpp Mon Apr 21 18:18:18 2014 >> @@ -47,11 +47,12 @@ SExpr *getCanonicalVal(SExpr *E) { >> if (V->kind() != Variable::VK_Let) >> return V; >> D = V->definition(); >> - if (auto *V2 = dyn_cast<Variable>(D)) { >> + auto *V2 = dyn_cast<Variable>(D); >> + if (V2) >> V = V2; >> - continue; >> - } >> - } while(false); >> + else >> + break; >> + } while (true); >> >> if (ThreadSafetyTIL::isTrivial(D)) >> return D; >> @@ -75,7 +76,7 @@ SExpr *getCanonicalVal(SExpr *E) { >> // canonical definition. If so, mark the Phi node as redundant. >> // getCanonicalVal() will recursively call simplifyIncompletePhi(). >> void simplifyIncompleteArg(Variable *V, til::Phi *Ph) { >> - assert(!Ph && Ph->status() == Phi::PH_Incomplete); >> + assert(Ph && Ph->status() == Phi::PH_Incomplete); >> >> // eliminate infinite recursion -- assume that this node is not redundant. >> Ph->setStatus(Phi::PH_MultiVal); >> @@ -90,8 +91,21 @@ void simplifyIncompleteArg(Variable *V, >> } >> } >> Ph->setStatus(Phi::PH_SingleVal); >> + // Eliminate Redundant Phi node. >> + V->setDefinition(Ph->values()[0]); >> +} >> + >> + >> +// Return true if E is a variable that points to an incomplete Phi node. >> +inline bool isIncompleteVar(SExpr *E) { >> + if (Variable *V = dyn_cast<Variable>(E)) { >> + if (Phi *Ph = dyn_cast<Phi>(V->definition())) >> + return Ph->status() == Phi::PH_Incomplete; >> + } >> + return false; >> } >> >> + >> } // end namespace til >> >> >> @@ -140,6 +154,7 @@ til::SExpr *SExprBuilder::translate(cons >> case Stmt::UnaryOperatorClass: >> return translateUnaryOperator(cast<UnaryOperator>(S), Ctx); >> case Stmt::BinaryOperatorClass: >> + case Stmt::CompoundAssignOperatorClass: >> return translateBinaryOperator(cast<BinaryOperator>(S), Ctx); >> >> case Stmt::ArraySubscriptExprClass: >> @@ -277,6 +292,32 @@ til::SExpr *SExprBuilder::translateUnary >> } >> >> >> +til::SExpr *SExprBuilder::translateBinAssign(til::TIL_BinaryOpcode Op, >> + const BinaryOperator *BO, >> + CallingContext *Ctx) { >> + const Expr *LHS = BO->getLHS(); >> + const Expr *RHS = BO->getRHS(); >> + til::SExpr *E0 = translate(LHS, Ctx); >> + til::SExpr *E1 = translate(RHS, Ctx); >> + >> + const ValueDecl *VD = nullptr; >> + til::SExpr *CV = nullptr; >> + if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(LHS)) { >> + VD = DRE->getDecl(); >> + CV = lookupVarDecl(VD); >> + } >> + >> + if (Op != BO_Assign) { >> + til::SExpr *Arg = CV ? CV : new (Arena) til::Load(E0); >> + E1 = new (Arena) til::BinaryOp(Op, Arg, E1); >> + E1 = addStatement(E1, nullptr, VD); >> + } >> + if (VD && CV) >> + return updateVarDecl(VD, E1); >> + return new (Arena) til::Store(E0, E1); >> +} >> + >> + >> til::SExpr *SExprBuilder::translateBinaryOperator(const BinaryOperator *BO, >> CallingContext *Ctx) { >> switch (BO->getOpcode()) { >> @@ -306,35 +347,24 @@ til::SExpr *SExprBuilder::translateBinar >> til::BinaryOp(BO->getOpcode(), translate(BO->getLHS(), Ctx), >> translate(BO->getRHS(), Ctx)); >> >> - case BO_Assign: { >> - const Expr *LHS = BO->getLHS(); >> - if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(LHS)) { >> - const Expr *RHS = BO->getRHS(); >> - til::SExpr *E1 = translate(RHS, Ctx); >> - return updateVarDecl(DRE->getDecl(), E1); >> - } >> - til::SExpr *E0 = translate(LHS, Ctx); >> - til::SExpr *E1 = translate(BO->getRHS(), Ctx); >> - return new (Arena) til::Store(E0, E1); >> - } >> - case BO_MulAssign: >> - case BO_DivAssign: >> - case BO_RemAssign: >> - case BO_AddAssign: >> - case BO_SubAssign: >> - case BO_ShlAssign: >> - case BO_ShrAssign: >> - case BO_AndAssign: >> - case BO_XorAssign: >> - case BO_OrAssign: >> - return new (Arena) til::Undefined(BO); >> + case BO_Assign: return translateBinAssign(BO_Assign, BO, Ctx); >> + case BO_MulAssign: return translateBinAssign(BO_Mul, BO, Ctx); >> + case BO_DivAssign: return translateBinAssign(BO_Div, BO, Ctx); >> + case BO_RemAssign: return translateBinAssign(BO_Rem, BO, Ctx); >> + case BO_AddAssign: return translateBinAssign(BO_Add, BO, Ctx); >> + case BO_SubAssign: return translateBinAssign(BO_Sub, BO, Ctx); >> + case BO_ShlAssign: return translateBinAssign(BO_Shl, BO, Ctx); >> + case BO_ShrAssign: return translateBinAssign(BO_Shr, BO, Ctx); >> + case BO_AndAssign: return translateBinAssign(BO_And, BO, Ctx); >> + case BO_XorAssign: return translateBinAssign(BO_Xor, BO, Ctx); >> + case BO_OrAssign: return translateBinAssign(BO_Or, BO, Ctx); >> >> case BO_Comma: >> - // TODO: handle LHS >> + // The clang CFG should have already processed both sides. >> return translate(BO->getRHS(), Ctx); >> - } >> >> return new (Arena) til::Undefined(BO); >> + } >> } >> >> >> @@ -495,14 +525,16 @@ void SExprBuilder::makePhiNodeVar(unsign >> >> // Make a new phi node: phi(..., E) >> // All phi args up to the current index are set to the current value. >> + til::SExpr *CurrE = CurrentLVarMap[i].second; >> til::Phi *Ph = new (Arena) til::Phi(Arena, NPreds); >> Ph->values().setValues(NPreds, nullptr); >> for (unsigned PIdx = 0; PIdx < ArgIndex; ++PIdx) >> - Ph->values()[PIdx] = CurrentLVarMap[i].second; >> + Ph->values()[PIdx] = CurrE; >> if (E) >> Ph->values()[ArgIndex] = E; >> - if (!E) { >> - // This is a non-minimal SSA node, which may be removed later. >> + // If E is from a back-edge, or either E or CurrE are incomplete, then >> + // mark this node as incomplete; we may need to remove it later. >> + if (!E || isIncompleteVar(E) || isIncompleteVar(CurrE)) { >> Ph->setStatus(til::Phi::PH_Incomplete); >> } >> >> @@ -601,8 +633,7 @@ void SExprBuilder::mergePhiNodesBackEdge >> } >> >> >> - >> -void SExprBuilder::enterCFG(CFG *Cfg, const NamedDecl *D, >> +void SExprBuilder::enterCFG(CFG *Cfg, const FunctionDecl *FD, >> const CFGBlock *First) { >> // Perform initial setup operations. >> unsigned NBlocks = Cfg->getNumBlockIDs(); >> @@ -616,22 +647,35 @@ void SExprBuilder::enterCFG(CFG *Cfg, co >> auto *BB = new (Arena) til::BasicBlock(Arena, 0, B->size()); >> BlockMap[B->getBlockID()] = BB; >> } >> - CallCtx = new SExprBuilder::CallingContext(D); >> + CallCtx = new SExprBuilder::CallingContext(FD); >> + >> + CurrentBB = lookupBlock(&Cfg->getEntry()); >> + for (auto *Pm : FD->parameters()) { >> + QualType T = Pm->getType(); >> + if (!T.isTrivialType(Pm->getASTContext())) >> + continue; >> + >> + // Add parameters to local variable map. >> + // FIXME: right now we emulate params with loads; that should be fixed. >> + til::SExpr *Lp = new (Arena) til::LiteralPtr(Pm); >> + til::SExpr *Ld = new (Arena) til::Load(Lp); >> + til::SExpr *V = addStatement(Ld, nullptr, Pm); >> + addVarDecl(Pm, V); >> + } >> } >> >> >> void SExprBuilder::enterCFGBlock(const CFGBlock *B) { >> // Intialize TIL basic block and add it to the CFG. >> - CurrentBB = BlockMap[B->getBlockID()]; >> + CurrentBB = lookupBlock(B); >> CurrentBB->setNumPredecessors(B->pred_size()); >> Scfg->add(CurrentBB); >> >> CurrentBlockInfo = &BBInfo[B->getBlockID()]; >> - CurrentArguments.clear(); >> - CurrentInstructions.clear(); >> >> // CurrentLVarMap is moved to ExitMap on block exit. >> - assert(!CurrentLVarMap.valid() && "CurrentLVarMap already initialized."); >> + // FIXME: the entry block will hold function parameters. >> + // assert(!CurrentLVarMap.valid() && "CurrentLVarMap already >> initialized."); >> } >> >> >> @@ -721,6 +765,8 @@ void SExprBuilder::handleSuccessorBackEd >> >> >> void SExprBuilder::exitCFGBlock(const CFGBlock *B) { >> + CurrentArguments.clear(); >> + CurrentInstructions.clear(); >> CurrentBlockInfo->ExitMap = std::move(CurrentLVarMap); >> CurrentBB = nullptr; >> CurrentBlockInfo = nullptr; >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
