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
