Page MenuHomePhabricator

[lldb] Fix another crash in covariant type handling
ClosedPublic

Authored by labath on Mar 26 2020, 5:33 AM.

Details

Summary

D73024 seems to have fixed one set crash, but it introduced another.
Namely, if a class contains a covariant method returning itself, the
logic in MaybeCompleteReturnType could cause us to attempt a recursive
import, which would result in an assertion failure in
clang::DeclContext::removeDecl.

For some reason, this only manifested itself if the class contained at
least two member variables, and the class itself was imported as a
result of a recursive covariant import.

This patch fixes the crash by not attempting to import classes which are
already completed in MaybeCompleteReturnType. However, it's not clear to
me if this is the right fix, or if this should be handled automatically
by functions lower in the stack.

Diff Detail

Event Timeline

labath created this revision.Mar 26 2020, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 5:33 AM
teemperor accepted this revision.Mar 26 2020, 5:48 AM

To summarize IRC: I think the underlying cause here is that we get the Imported callback from the ASTImporter and then recursively start a completely new import process from that. I don't think that's actually supported by the ASTImporter so we probably should do the same thing we do elsewhere and queue all decls that need to be imported (see CompleteTagDeclsScope). But this improves the overall situation so this should land until this is properly fixed.

This revision is now accepted and ready to land.Mar 26 2020, 5:48 AM
shafik accepted this revision.Mar 26 2020, 9:58 AM

Thanks for the review, and a pointer to CompleteTagDeclsScope. While looking at this, I got some ideas on how to reduce the number of chained imports here (nothing magical -- just avoid importing the type if the base and derived return types are identical (no covariance). I'll try to remove the recursive imports before hacking on this further (but I don't know when exactly that will happen).

This revision was automatically updated to reflect the committed changes.