Page MenuHomePhabricator

[ASTImporter] Fix isa cast assert
ClosedPublic

Authored by martong on Apr 24 2018, 9:05 AM.

Details

Summary

Do early return if we can't import the found decl for a member expr.
This follows the pre-existing scheme, e.g with E->getMemberDecl().
E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. It may involve a lookup in case of a using directive which refers to a member function in a base class template.

We faced this assert during the CTU analysis of google::protobuf v3.5.2. We tried hard to synthesize a minimal test example both by hand and by executing creduce on multiple files. Unfortunately, we were unable to reduce to such a minimal example, yet. Nevertheless, this fix solved the problem in protobuf.
To reproduce the error one must execute the analyzer with -Xclang -analyzer-config -Xclang experimental-enable-naive-ctu-analysis=true -Xclang -analyzer-config -Xclang ctu-dir=/path/to/ctu_dir

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Apr 24 2018, 9:05 AM
a.sidorin accepted this revision.Apr 24 2018, 10:06 AM

This LGTM, but could you please add a test?

This revision is now accepted and ready to land.Apr 24 2018, 10:06 AM

Hi Aleksei,

Thanks for the review.

We faced this assert during the CTU analysis of protobuf. We tried hard to synthesize a minimal test example both by hand and by executing creduce on multiple files. Unfortunately, we were unable to reduce to such a minimal example, yet. Nevertheless, this fix solved the problem in protobuf.

E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. (Note, it may involve a lookup in case of a using directive which refers to a member function in a base class template.)

I hoped that this patch could be accepted without tests, since it is very small and the changes are pretty straight forward, so I thought it can be easily verified by a review.

E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. (Note, it may involve a lookup in case of a using directive which refers to a member function in a base class template.)

Yes, a pretty weird example. Unfortunately, they are pretty common for XTU.

I hoped that this patch could be accepted without tests, since it is very small and the changes are pretty straight forward, so I thought it can be easily verified by a review.

Yes, this is the reason why I have accepted it initially.
I think we can accept this patch without a test but I'd like to get a somebody else's review. @xazax.hun, @szepet, could you take a look?
@martong Could you include the reason why this patch lacks tests into the commit message (including description of the situation where the code fails)?

xazax.hun accepted this revision.Apr 25 2018, 4:06 AM

With a sufficiently detailed commit message, i.e.: what version of a project should be cheked out and how the analyzer needs to be ivoked to reproduce the problem I am ok with committing this without a test.

szepet accepted this revision.Apr 25 2018, 10:05 AM

Yepp, pretty straightforward check for something we were not aware previously (but unfortunately encountered it).

martong edited the summary of this revision. (Show Details)Apr 25 2018, 10:09 AM
martong edited the summary of this revision. (Show Details)

Confirming LGTM, no objections. Thank you!

Guys, I still don't have commit rights, could you please help me with the commit? (I think one more good quality patch and then I could request commit rights for myself ...)

ping...
@xazax.hun could you please help me with the commit?

This revision was automatically updated to reflect the committed changes.