Page MenuHomePhabricator

[Analyzer] Store the pointed/referenced type for dynamic casts
ClosedPublic

Authored by baloghadamsoftware on Aug 11 2020, 9:52 AM.

Details

Summary

The successfulness of a dynamic cast depends only on the C++ class, not the pointer or reference. Thus if *A is a *B, then &A is a &B, const *A is a const *B etc. This patch changes DynamicCastInfo to store and check the cast between the unqualified pointed/referenced types. It also removes e.g. SubstTemplateTypeParmType from both the pointer and the pointed type.

Diff Detail

Event Timeline

baloghadamsoftware requested review of this revision.Aug 11 2020, 9:52 AM
NoQ added a comment.Aug 11 2020, 11:08 AM

Yes, i completely agree that this is the right decision, thanks!

clang/lib/StaticAnalyzer/Core/DynamicType.cpp
71–72

Just canonicalize instead. The result doesn't depend on sugar either.

xazax.hun added inline comments.Aug 11 2020, 11:11 AM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
73

Is this doing what you intended? What about a reference to a pointer? Wouldn't you do too much unboxing?

Also, I think a function returning a value would be more conventional.

Other sugars like typedefs cannot interfere? I think this patch might benefit from additional test coverage. I also see no tests for template substitutions.

@baloghadamsoftware Maybe there is a typo in the summary of the patch

then &B is a &B

Shouldn't this be "&A is a &B"?

Updated according to the comments.

baloghadamsoftware edited the summary of this revision. (Show Details)Aug 12 2020, 4:25 AM
baloghadamsoftware marked an inline comment as done.Aug 12 2020, 4:28 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
73

Reference to pointer cast using LLVM's cast functions are syntactically invalid, they do not compile.

For QualType in-place modification is usual, since we use it by value.

I see no test coverage for this particular part of the analyzer specifically, it seems that its is only tested indirectly in the tests for CastValueChecker.

NoQ accepted this revision.Aug 15 2020, 3:01 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
73

The usual idiom is

if (Ty->isPointerType() || Ty->isReferenceType())
  Ty = Ty->getPointeeType();

It makes sure that you don't unwrap things twice. Also there's no need to canonicalize before unwrapping the pointee type; all these methods automatically operate over the canonical type.

You might want to add tests for typedefs.

This revision is now accepted and ready to land.Aug 15 2020, 3:01 PM
NoQ added inline comments.Aug 27 2020, 11:43 AM
clang/lib/StaticAnalyzer/Core/DynamicType.cpp
73

Oof, this function is actually being fed references-to-pointers. I'll add a fixme.