[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D25475: [analyzer] Add a new SVal to support pointer-to-member operations.
From:       Devin Coughlin via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2016-11-30 22:08:08
Message-ID: 8d6d2933153acf7028f10f982810f9f9 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]