r206827 - Thread safety analysis: misc updates to SExpr handling. Fix to minimal SSA,
Delesley Hutchins
delesley at google.com
Tue Apr 22 07:59:20 PDT 2014
Thanks for the catch! Should be fixed in r206899.
-DeLesley
On Tue, Apr 22, 2014 at 2:44 AM, Timur Iskhodzhanov <timurrrr at google.com> 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 <delesley at google.com>:
>> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
More information about the cfe-commits
mailing list