Page MenuHomePhabricator

[DWARFASTParserClang] Always complete types read from a module/PCH AST context.
ClosedPublic

Authored by friss on Feb 21 2018, 12:52 PM.

Details

Summary

The modified test would just crash without the code change. The reason is that
we would try to extend the Foo type imported from the PCH debug info when adding the
Foo::Bar definitiion to it. This will crash if the type is not complete.

I pondered moving the CompleteType call inside of CopyType, but CopytType seems
to be used as a lower-level building block in other places so I decided not to.

ClangASTImporter is kinda scary. It has no comments and interacts with the Clang
ASTs which are not exactly easy to deal with. Any insight appreciated.

Diff Detail

Repository
rL LLVM

Event Timeline

friss created this revision.Feb 21 2018, 12:52 PM
clayborg requested changes to this revision.Feb 22 2018, 8:12 AM

Sean was the person with the most experience working on the expression parser and he wrote the clang AST importer, but Jim is now the expression parser expert. Jim: and ideas on how this really should be fixed?

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
182–184 ↗(On Diff #135316)

We typically should never need to do the completion of a type manually like this. Many times we have variables that are like "foo *". For this variable, we might never need to complete the type since it is only a pointer. Furthermore, always completing a type will make LLDB very slow as every type we touch is now completing as soon as we import it. So while this patch might work, it will really affect LLDB performance. I think the right fix is probably elsewhere where someone isn't checking if the type is complete before doing the work that crashes clang...

This revision now requires changes to proceed.Feb 22 2018, 8:12 AM
friss added a comment.Feb 22 2018, 9:59 AM

Note that this code path is only triggered when importing debug info from a different AST context, it is not the common codepath. The issue in this case is that LLDB is crashing when using the incomplete Decl as the DeclContext for another one. I guess I could add calls to RequireCompleteType before every use of a DeclContext. But that would be costly too I think, mapping from a DeclContext to a CompilerType is not trivial.

jfb added a subscriber: jfb.Feb 22 2018, 10:38 AM
friss updated this revision to Diff 138733.Mar 16 2018, 10:45 AM

I experimented quite a bit with different approches to fix this bug
without always completing the types right after importing them from
the external AST.

This is the most principled approach I came up with. I applied the
new helper in only one place (the only one we have seen failing),
but we could potentially find more uses for it in the future.

friss updated this revision to Diff 138734.Mar 16 2018, 10:55 AM

Adding back a hunk I didn't mean to delete.

Much better. Make sure Jim is ok with this as I am ok with it if he is.

jingham accepted this revision.Mar 16 2018, 12:15 PM

I can't see a narrower way to do this. This sort of change makes me really wish we had "how many dies did you complete" metrics and some stress tests, so we can tell if we've just made some operation unreasonably more expensive and have to go back and really put our thinking caps on. But I think this should be fine.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 16 2018, 3:14 PM
This revision was automatically updated to reflect the committed changes.