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).
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.
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.)
Could you also unforget the diff context? >.<
We shouldn't crash when code under analysis doesn't match our expectation.
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.
Looks like a leftover debug print.
The question is whether we can get any other template argument kind for llvm::isa<>() and llvm::isa_and_nonnull<>() than Type or Pack?
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.
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,
Aha, yup, that's better, thanks.
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.
Ok, this one probably doesn't happen, because we don't ever try to symbolically execute templated ASTs (that aren't fully instantiated).
Hmm, is this phabricator's way of displaying tabs?
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.