[PATCH] D25475: [analyzer] Add a new SVal to support pointer-to-member operations.

[Kirill Romanenkov via cfe-commits]

kromanenkov added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:462
+      case CK_ReinterpretMemberPointer: {
+        const Expr *UOExpr = CastE->getSubExpr()->IgnoreParenCasts();
+        assert(isa<UnaryOperator>(UOExpr) &&
----------------
dcoughlin wrote:
> dcoughlin wrote:
> > dcoughlin wrote:
> > > I don't think pattern matching on the sub expression to find the referred-to declaration is the right thing to do here. It isn't always the case that the casted expression will be a unary pointer to member operation. For example, this is perfectly fine and triggers an assertion failure on your patch:
> > > 
> > > ```
> > > struct B {
> > >   int f;
> > > };
> > > 
> > > struct D : public B {
> > >   int g;
> > > };
> > > 
> > > void foo() {
> > >   D d;
> > >   d.f = 7;
> > > 
> > >   int B::* pfb = &B::f;
> > >   int D::* pfd = pfb;
> > >   int v = d.*pfd;
> > > }
> > > ```
> > > Note that you can't just propagate the value already computed for the subexpression. Here is a particularly annoying example from the C++ spec:
> > > 
> > > ```
> > > struct B {
> > >   int f;
> > > };
> > > struct L : public B { };
> > > struct R : public B { };
> > > struct D : public L, R { };
> > > 
> > > void foo() {
> > >   D d;
> > > 
> > >   int B::* pb = &B::f;
> > >   int L::* pl = pb;
> > >   int R::* pr = pb;
> > > 
> > >   int D::* pdl = pl;
> > >   int D::* pdr = pr;
> > > 
> > >   clang_analyzer_eval(pdl == pdr); // FALSE
> > >   clang_analyzer_eval(pb == pl); // TRUE
> > > }
> > > ```
> > > My guess is this will require accumulating CXXBasePath s or something similar for each cast. I don't know how to do this efficiently. 
> > > I don't know how to do this efficiently.
> > 
> > Jordan suggested storing this in a bump-pointer allocated object with a lifetime of the AnalysisDeclContext. The common case is no multiple inheritance, so that should be the fast case.
> > 
> > Maybe the Data could be a pointer union between a DeclaratorDecl and an immutable linked list (with sharing) of CXXBaseSpecifiers from the CastExprs in the AST. The storage for this could be managed with a new manager in SValBuilder.
> (The bump pointer-allocated thing would have to have the Decl as well.)
> 
> This could also probably live in BasicValueFactory. The extra data would be similar to LazyCompoundValData.
My understanding is that PointerToMember SVal should be represented similar to LazyCompoundVal SVal. If I got you right, PointerToMember SVal should be constructed  in VisitUnaryOperator and  VisitCast (CK_BaseToDerivedMemberPointer and so on) just add CXXBaseSpecifiers to this SVal?
What do you mean by sharing of immutable linked list?


https://reviews.llvm.org/D25475