Page MenuHomePhabricator

[Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker
ClosedPublic

Authored by baloghadamsoftware on Aug 11 2020, 6:28 AM.

Details

Summary

llvm::isa<>() and llvm::isa_and_not_null<>() template functions recently became variadic. Unfortunately this causes crashes in case of isa_and_not_null<>() and incorrect behavior in isa<>(). This patch fixes this issue (47037).

Diff Detail

Event Timeline

baloghadamsoftware requested review of this revision.Aug 11 2020, 6:28 AM
baloghadamsoftware added a comment.EditedAug 11 2020, 6:30 AM

This patch intends to fix bug 47037. It is still work in progress with lots of debug printouts.

I described the issue with this patch at the bug report:

The variadic tests fail because the whole Dyanamic Type library is extremely poor design: it stores the pointers and references as they are instead of the underlying types. Unfortunately, cast<> functions which can take pointers have pointers to SubstTemplateTypeParmType but isa<> functions take everything by reference. Thus they have a reference to a SubstTemplateTypeParmType which contains the pointer. The two will never match even if I unpack the reference for the isa<> functions and get to the pointer itself because I cannot create a new pointer type in the cast<> functions where SubstTemplateTypeParmType is inside the pointer. I did not found any method to remove SubstTemplateTypeParmType from a pointer either.

This results that the test evalNonNullParamNonNullReturnReference() passes only by chance in the original version. Even if I fix the parameter which should have been pointer instead of reference the early exit blocks which use dyn_cast<> store a different "from" type for the casts that we retrieve later at the isa<> calls. This way both true and false branches for the isa<> blocks remain alive. If I extend the tests for variadic templates we are not lucky anymore, the tests fail.

whisperity edited the summary of this revision. (Show Details)Aug 11 2020, 6:30 AM

My proposal is the following: do not store the casing info between pointers and references but the underlying types themselves. There are two ways to implement this: in the checkers using it (currently only this one) or behind the API of DynamicCastInfo. Maybe the latter is more appropriate.

To be fully correct we should not only change the DynamicCastInfo but also the DynamicTypeInfo: if the dynamic type of an object behind a pointer is known, then it remains the same also if we get a reference from the pointer or substitute a type for a template type parameter with it. (We can create new pointer or reference type using ASTContext.)

baloghadamsoftware retitled this revision from [Analyzer][WIP] Support for the new variadic isa<> and isa_and_nod_null<> in CastValueChecker to [Analyzer][WIP] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker.Aug 11 2020, 6:41 AM
baloghadamsoftware retitled this revision from [Analyzer][WIP] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker to [Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker.
baloghadamsoftware edited the summary of this revision. (Show Details)

Working version. Debug printouts removed.

NoQ added a comment.Aug 11 2020, 11:18 AM

Thanks!!

Could you also unforget the diff context? >.<

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
270–272

We shouldn't crash when code under analysis doesn't match our expectation.

315

Let's return immediately after the transition instead. Like, generally, it's a good practice to return immediately after a transition if you don't plan any more state splits, because otherwise it's too easy to accidentally introduce unwanted state splits.

482

Looks like a leftover debug print.

gamesh411 added inline comments.Aug 12 2020, 1:10 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
265

Do we intentionally skip the last arg with this loop boundary, or is the loop variable just off-by-one?

Updated according to the comments.

baloghadamsoftware marked 3 inline comments as done.Aug 12 2020, 5:19 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
270–272

The question is whether we can get any other template argument kind for llvm::isa<>() and llvm::isa_and_nonnull<>() than Type or Pack?

315

Now I inserted a return but only if this is a known conversion. If this is an assumption, the we should assume every type in the template argument list.

martong added inline comments.Aug 12 2020, 6:48 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
270–272

I think it is worth to ponder about the below case. What if isa is used in a dependent context (i.e. it is used inside another template)?

/// The template argument is an expression, and we've not resolved it to one
/// of the other forms yet, either because it's dependent or because we're
/// representing a non-canonical template argument (for instance, in a
/// TemplateSpecializationType).
Expression,
NoQ accepted this revision.Aug 12 2020, 2:13 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
270–272

Aha, yup, that's better, thanks.

The question is whether we can get any other template argument kind for llvm::isa<>() and llvm::isa_and_nonnull<>() than Type or Pack?

It's a more general problem: what about a completely unrelated template function that is accidentally called llvm::isa<>() or llvm::isa_and_nonnull<>()? Or what if llvm::isa<>()'s/ llvm::isa_and_nonnull<>()'s implementation changes again? If it's at all possible in any template function, we should take it into account and at least provide an early return, we shouldn't rule out that possibility by looking at the name only.

I think it is worth to ponder about the below case. What if isa is used in a dependent context (i.e. it is used inside another template)?

Ok, this one probably doesn't happen, because we don't ever try to symbolically execute templated ASTs (that aren't fully instantiated).

289

Hmm, is this phabricator's way of displaying tabs?

This revision is now accepted and ready to land.Aug 12 2020, 2:13 PM
whisperity added inline comments.Aug 13 2020, 2:16 AM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
289

I believe it is not displaying tabs, but rather just indicating that the current line changed in a way that only the indentation has changed... instead of marking the old side of the diff red. The HTML element is span.depth-in. A span.depth-out is red, and the arrow points the other way.

But I agree this is a new thing since the version update.

baloghadamsoftware marked 2 inline comments as done.Aug 13 2020, 4:08 AM

@NoQ Thank you for the review. Please also review D85752 after my update and tell me what I have to change there if it is not acceptable yet. This patch depends on that one.

NoQ added inline comments.Aug 15 2020, 3:01 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
289

Aha, ok, nice!

NoQ added a comment.Aug 25 2020, 12:14 PM

Hans is pinging us on Bugzilla because this patch is marked as a release blocker (and i believe that it indeed is). I think we should land these patches.

In D85728#2237006, @NoQ wrote:

Hans is pinging us on Bugzilla because this patch is marked as a release blocker (and i believe that it indeed is). I think we should land these patches.

I believe @baloghadamsoftware is on vacation.
I'm sure he won't be mad if you commit on his behalf.

I was indeed on vacation, so thanks for committing it, @NoQ! I was waiting for agreement for the prerequisite patch then I forgot to notify you that I was going on vacation.