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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

PointerToMemberData looks like it is on the right track! One thing that is still missing is using the base specifier list to figure out the correct subobject field to read and write from. This is important when there is non-virtual multiple inheritance, as there will be multiple copies of the same field in the same object.

Here are is an example where the current patch is not quite right; both of these should evaluate to true:

  void clang_analyzer_eval(int);
  
  struct B {
    int f;
  };
  struct L1 : public B { };
  struct R1 : public B { };
  struct M : public L1, R1 { };
  struct L2 : public M { };
  struct R2 : public M { };
  struct D2 : public L2, R2 { };
  
  
  void diamond() {
    M m;
  
    static_cast<L1 *>(&m)->f = 7;
    static_cast<R1 *>(&m)->f = 16;
  
    int L1::* pl1 = &B::f;
    int M::* pm_via_l1 = pl1;
  
    int R1::* pr1 = &B::f;
    int M::* pm_via_r1 = pr1;
  
    clang_analyzer_eval(m.*(pm_via_l1) == 7); // expected-warning {{TRUE}}
    clang_analyzer_eval(m.*(pm_via_r1) == 16); // expected-warning {{TRUE}}
  }

I suspect you'll need to do something similar to what StoreManager::evalDerivedToBase() does (or maybe just use that function) to figure out the correct location to read and write from when accessing via a pointer to member.

It would also be good to add some tests for the double diamond scenario to ensure the list of path specifiers is constructed in the right order. For example:

  void double_diamond() {
    D2 d2;
  
    static_cast<L1 *>(static_cast<L2 *>(&d2))->f = 1;
    static_cast<L1 *>(static_cast<R2 *>(&d2))->f = 2;
    static_cast<R1 *>(static_cast<L2 *>(&d2))->f = 3;
    static_cast<R1 *>(static_cast<R2 *>(&d2))->f = 4;
  
    clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int L2::*>(static_cast<int L1::*>(&B::f)))) == 1); // expected-warning {{TRUE}}
    clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int R2::*>(static_cast<int L1::*>(&B::f)))) == 2); // expected-warning {{TRUE}}
    clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int L2::*>(static_cast<int R1::*>(&B::f)))) == 3); // expected-warning {{TRUE}}
    clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int R2::*>(static_cast<int R1::*>(&B::f)))) == 4); // expected-warning {{TRUE}}
  }



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:217
+
+  llvm::ImmutableList<const CXXBaseSpecifier *> consCXXBase(
+      const CXXBaseSpecifier *CBS,
----------------
NoQ wrote:
> Hmm, is it "construct"? Or "constrain"? I think a verbose name wouldn't hurt.
> In fact, i suspect that `consVals` are rather `conSVals`, where `con` stands for "concatenate".
In functional paradigms, 'cons' is used to mean prepending an item the the beginning of a linked list. https://en.wikipedia.org/wiki/Cons

In my opinion, 'prepend' is better than 'cons', which I find super-confusing. I don't think 'concatenate' is quite right, since typically that operation combines two (or more) lists. 


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:322
+          Bldr.generateNode(CastE, Pred, state);
+          continue;
+        }
----------------
These conditional fallthroughs make me very, very uncomfortable because they will broken if any of the intervening cases get special handling in the future.

I think it would safer to factor out code in the "destination" case (here 'CK_LValueBitCast') into a function, call it directly, and then continue regardless of the branch.

Another possibility is to use gotos to directly jump to the default 'destination' case.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:472
+        }
+        // If getAs failed just fall through to default behaviour.
+      }
----------------
I think it would be good to be explicit about this fallthrough behavior, as well.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:899
+    case UO_AddrOf: {
+      // Process pointer-to-member address operation
+      const Expr *Ex = U->getSubExpr()->IgnoreParens();
----------------
Just sticking this in the middle of a fallthrough cascade seems quite brittle. For example, it relies on the sub expression of a unary deref never being a DeclRefExpr to a field. This may be guaranteed by Sema (?) but if so it is quite a non-obvious invariant to rely upon.

I think it would be better the restructure this so that the AddrOf case doesn't fall in the the middle of a fallthrough cascade. You could either factor out the default behavior into a function or use a goto.


================
Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:298
 
+  case Stmt::UnaryOperatorClass: {
+    const UnaryOperator *UO = dyn_cast<UnaryOperator>(E);
----------------
Can this be removed? There are no tests for it.


https://reviews.llvm.org/D25475