Page MenuHomePhabricator

[analyzer] CastValueChecker: Model inheritance
Needs ReviewPublic

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

Details

Reviewers
NoQ
Summary

The key is CXXRecordDecl::isDerivedFrom().

Diff Detail

Event Timeline

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

That could be helpful, but I am not sure.

255–256

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

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

Probably a consistent way of MemRegion handling is more appropriate.

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

Ty(Ty) is the idiom here.

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

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).

209

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

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

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.Tue, Sep 3, 2:10 PM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
42

*cries in pointer chasing*

Charusso updated this revision to Diff 219391.Mon, Sep 9, 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
161

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.

209

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.Mon, Sep 9, 7:57 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
136

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.

209

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.Mon, Sep 9, 8:14 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
161

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).