Page MenuHomePhabricator

[analyzer] CastValueChecker: Model inheritance
Needs ReviewPublic

Authored by Charusso on Sep 2 2019, 8:09 AM.

Details

Reviewers
NoQ
Summary

The key is CXXRecordDecl::isDerivedFrom().

Diff Detail

Event Timeline

Charusso created this revision.Sep 2 2019, 8:09 AM
Charusso marked 2 inline comments as done.Sep 2 2019, 8:14 AM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
216

That could be helpful, but I am not sure.

244–245

Previously possible false positives happened because we kept flow the modeling.

clang/lib/StaticAnalyzer/Core/DynamicType.cpp
46

Probably a consistent way of MemRegion handling is more appropriate.

NoQ added inline comments.Sep 3 2019, 1:43 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h
24

Ty(Ty) is the idiom here.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
153

Counterexample:

struct A {};
struct B : A {};
struct C : A, B {};

Downcast from C to B should succeed, even though they have a common ancestor A (which has the same CXXRecordDecl but currently isn't the same object within C, but can be, if B declares A as a virtual base).

201

Omg lol nice. Did you try to figure out how do other people normally do it?

NoQ added inline comments.Sep 3 2019, 1:44 PM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
46

Pointer casts are quite a can of worms. I suggest ignoring it for now and dealing with it as needed. I.e., your code looks good... for now.

Szelethus added inline comments.Sep 3 2019, 2:10 PM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
46

*cries in pointer chasing*

Charusso updated this revision to Diff 219391.Sep 9 2019, 10:34 AM
Charusso marked 5 inline comments as done.
  • Fix.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h
24

Good to know, thanks!

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
153

So, even it is some kind of anti-pattern as a warning arrive immediately, now I allow B to C downcasts. Could you explain me more about that virtual idea, please? Based on this possible use-case in my mind two classes are on the same level as every of their bases/vbases are equals.

201

There is no function for that in ADT/StringExtras.h + grep did not help, so I realized it is a common way to match vowels. Do you know a better solution?

NoQ added inline comments.Sep 9 2019, 7:57 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
154

Using a flag here is unnecessary because you're returning true without doing anything else whenever the flag is set.

So, basically, your code says "if at least one current base is different from at least one previous base, the cast succeeds".

In particular, this function would return true whenever CurrentRD and PreviousRD are from completely unrelated class hierarchies. It sounds like whatever isSucceededDowncast() was supposed to do, it's not doing it right.

201

I just realized that this is actually incorrect and the correct solution is pretty hard to implement because the actual "a vs. an" rule deals with sounds, not letters. Eg.:

Clang is an "LLVM native" C/C++/Objective-C compiler

has an "an" because it's read as "an el-el-vee-am".

NoQ added inline comments.Sep 9 2019, 8:14 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
153

Could you explain me more about that virtual idea, please?

In the following variation:

struct A {};
struct B : virtual A {};
struct C : A, B {};

we have C contain only one instance of A: the layout of B within C would merely refer to the instance of A within C instead of making its own copy. In particular, the constructor of B within the constructor of C would not initialize C's instance of A because the constructor of C will invoke the constructor of A directly (cf. D61816).

Charusso updated this revision to Diff 221070.Sep 20 2019, 11:03 AM
Charusso marked 9 inline comments as done.
  • Try to do the math.
  • Create a consistent dynamic type API.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
153

That is a cool example, thanks you!

201

That is a cool idea, I will adjust StringExtras.h after that patch, for now please let us use that simple heuristic.

Szelethus added inline comments.Sep 20 2019, 11:24 AM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
99

Hmm, I think this function answers the question, at least in the standard library sense, whether Y std::is_base_of of X, whereas "is derived from?" is another term that is used by clang and seems to not allow equality of types. Should we rename this to isBaseOf?

Charusso updated this revision to Diff 222370.EditedSep 30 2019, 1:44 AM
Charusso marked 2 inline comments as done.
  • When we have no information whether the cast succeeds or not we assume both.
  • CastVisitor is the new facility which decides whether the assumptions are appropriate.
  • Removed the vowels-related string-formatting.
  • Moved the API changes to D68199.
  • The math is still WIP.
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
99

I have left the equality by a mistake. It is wanted to be a wrapper of isDerivedFrom(), which does not allow equality. Thanks for the notice!

NoQ added a comment.Sep 30 2019, 1:06 PM
  • CastVisitor is the new facility which decides whether the assumptions are appropriate.
  • The math is still WIP.

You need the math to decide whether delaying the decision to a visitor is the right approach.

clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

Why is (isa<B>(a) && isa<C>(a)) deemed possible in the first test but not in the second test? o_o

Charusso marked 2 inline comments as done.Sep 30 2019, 1:13 PM
In D67079#1688648, @NoQ wrote:
  • CastVisitor is the new facility which decides whether the assumptions are appropriate.
  • The math is still WIP.

You need the math to decide whether delaying the decision to a visitor is the right approach.

Yes, Soon(tm).

clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

In test_downcast() we assume that a is a record type of D where D is a B and D is a C. However in test_downcast_infeasible() if a is not a record type of D is cannot be both B and C at the same time. That is the purpose of CastVisitor.

NoQ added inline comments.Sep 30 2019, 1:17 PM
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

I mean, it contradicts to how the program *actually* works, so we should either not do that, or provide a reeeeeaaaaaallly compelling explanation of why we do this (as in "Extraordinary Claims Require Extraordinary Evidence").

Charusso marked 3 inline comments as done.Sep 30 2019, 1:39 PM
Charusso added inline comments.
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

Are you sure it does not model the program? I have an Apple class and I have a Pen class, until it is not defining an ApplePen class it is a false assumption to say they are defining an ApplePen class. I wanted to prefetch that information before the modeling starts, but it was an impossible challenge for me, so I have picked that CastVisitor-based post-elimination idea. In the real world I have removed only two false assumptions with the visitor from 1200 reports of LLVM so an ApplePen is very rare (https://www.youtube.com/watch?v=Ct6BUPvE2sM).

NoQ added inline comments.Sep 30 2019, 1:44 PM
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

I'm sure that the possibility of taking a true branch in if (x) only depends on the value of x, not on the contents of the branch.

NoQ added inline comments.Sep 30 2019, 2:03 PM
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

I.e., i see the point that you're trying to make (roughly), but it still requires extraordinary evindence and the way you've written the test clearly demonstrates why it needs an extraordinary evidence.

What you were trying to say is "imagine there's no class D, then we probably shouldn't consider the situation that it exists". But in this test the class exists, and you've just written a test about it, and then you're telling me that it doesn't exist. Which is exactly the problem with not considering that D exists. And this is exactly why choosing such approach requires a discussion.

One reason why i doubt the visitor-based solution is that the existence of D is not a path-sensitive fact; it either exists or not, it is irrelevant whether it was mentioned in the current path. Therefore scanning all classes in the translation unit, as opposed to classes mentioned on the current path, seems more precise.

Charusso marked 4 inline comments as done.Sep 30 2019, 2:13 PM
Charusso added inline comments.
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp
25–27 ↗(On Diff #222370)

Well, that is a better idea, yes. I really like your observation about if (x), thanks for them! During the Summer I have learnt a lot, but a lot about the issue of the one translation unit, so that is why I was very afraid of using it. Let us give it a try.

Charusso updated this revision to Diff 222685.Oct 1 2019, 1:33 PM
Charusso marked an inline comment as done.
  • Use the TU's Decls instead of the gathered casts' Decls.
  • The math is still missing.
Charusso updated this revision to Diff 222931.Oct 2 2019, 4:15 PM
  • Rebase.
Charusso updated this revision to Diff 222932.Oct 2 2019, 4:19 PM
  • Rebase properly.

Well, the rebase is a little-bit weird, sorry for the inconvenience.