Page MenuHomePhabricator

[analyzer] cover more cases where a Loc can be bound to constants
ClosedPublic

Authored by r.stahl on Apr 18 2018, 8:43 AM.

Details

Summary
  • SValBuilder::getConstantVal stopped processing an initialization if there are intermediate casts. Now additionally handles CStyleCastExpr, CXXConstCastExpr, CXXReinterpretCastExpr, CXXStaticCastExpr, CXXFunctionalCastExpr. Additional CastKinds are CK_NoOp and CK_IntegralToPointer - the others are already handled by EvaluateAsInt.
  • RegionStoreManager::getBindingForElement now resolves a constant array initialization.
  • RegionStoreManager::getBindingForField now resolves constant in-class initializers and constant element-wise initialization.
  • Added tests for all new cases.

Diff Detail

Event Timeline

r.stahl created this revision.Apr 18 2018, 8:43 AM

Not sure how to proceed about these:

  • CXXDynamicCastExpr: I don't think evalCast would handle this correctly, does it?
  • ObjCBridgedCastExpr: I don't know ObjectiveC
  • CastKinds for floating point: Floats cannot yet be handled by the analyzer, right?
MTC added a subscriber: MTC.Apr 18 2018, 7:06 PM
MTC added a comment.Apr 18 2018, 7:34 PM

Test files for initialization missing? : )

NoQ added a comment.Apr 19 2018, 6:45 PM

Looks great, thanks!

CXXDynamicCastExpr: I don't think evalCast would handle this correctly, does it?

Seems so. It is partially supported by StoreManager::attemptDownCast(). I also see relatively little urgency in handling it here because the whole point of dynamic_cast is to be unknown in compile-time; why would anybody need to dynamic_cast a compile-time null pointer?

Also for what-does-what sorts of answers you can consult the code in ExprEngine::Visit* which is essentially a huge switch by statement kinds.

ObjCBridgedCastExpr: I don't know ObjectiveC

Same here, it's quite pointless to have a constant null pointer casted from one language to another.

CastKinds for floating point: Floats cannot yet be handled by the analyzer, right?

Yep, that's right.

In D45774#1071666, @MTC wrote:

Test files for initialization missing? : )

Yep, seems so. This is stuff like

struct S {
  int x = 0;
};

I'm not sure it even needs to be supported explicitly. If the field is static, it most likely acts like a simple global variable (i.e. a VarDecl, not a FieldDecl). If the field is non-static, we should pick-up the in-class initializer in the constructor. Also if it's non-static and const, it is unlikely to have a constant initializer, because why would anybody copy the same constant into every instance of the class.

lib/StaticAnalyzer/Core/RegionStore.cpp
1638

Using const auto * is recommended with dyn_cast (also a plain auto with getAs and such), so that not to write down the type twice. We have a lot of old code around, but the new code should probably follow this recommendation.

1642–1645

Hmm, we might as well want to return an UndefinedVal() when the index is out of bounds of the actual array. The size of the array can be retrieved from VD. Though if you decide to do that, please put it into a separate patch :)

1723

This method crashes when the index is out of range. Not sure - is it possible to skip a few fields in the list? Would the list's AST automatically contain any placeholders in this case?

lib/StaticAnalyzer/Core/RegionStore.cpp
1720

style note -- by LLVM coding standards {'s are usually omitted when there's only a single statement inside the guard. Would help to avoid giant towers of }'s

r.stahl updated this revision to Diff 145156.May 4 2018, 12:24 AM
r.stahl marked 3 inline comments as done.

addressed the comments, thanks!

If it looks good, please commit for me.

r.stahl added inline comments.May 4 2018, 12:26 AM
lib/StaticAnalyzer/Core/RegionStore.cpp
1723

Good catch, but I don't think this is an issue. When testing InitListExpr with missing inits, they get filled with ImplicitValueInitExpr.

NoQ accepted this revision.May 4 2018, 1:22 PM

Yup, thanks! I'll commit.

This revision is now accepted and ready to land.May 4 2018, 1:22 PM
NoQ added a comment.May 4 2018, 1:51 PM

Two questions for the future:

  • Should we skip the initializer binding for local variables (and fields/elements of local variables) entirely? Cause we can load from them anyway. And keeping the Store small might be good for performance.
  • Just in case, do you accidentally plan supporting nested initializer lists?
This revision was automatically updated to reflect the committed changes.
xazax.hun added inline comments.May 5 2018, 1:01 AM
lib/StaticAnalyzer/Core/RegionStore.cpp
1642–1645

+1 for the UndefVal patch :)

In D45774#1088453, @NoQ wrote:

Two questions for the future:

  • Should we skip the initializer binding for local variables (and fields/elements of local variables) entirely? Cause we can load from them anyway. And keeping the Store small might be good for performance.

I don't think that I can judge this.

  • Just in case, do you accidentally plan supporting nested initializer lists?

Not at the moment.