[PATCH] D69933: [ASTImporter] Limit imports of structs

Tue Jan 21 17:17:05 PST 2020

clayborg added a comment.

Clang AST contexts know how to complete types and is done via the external AST source code that will ask a type to complete itself. Each object file has an AST that knows how to lazily complete a type when and only when it is needed. Each object file also only knows about the type information in the binary itself. So if we have a forward declaration to "Foo" with something like "struct Foo;" that is all the object file AST will ever know about this type. This is required because each module can be re-used on subsequent debug sessions if they haven't changed. So if we have a forward declaration for "Foo" in the AST for "bbb.so" that is ok. We don't want to copy some definition for "Foo" from "foo.so" over into bbb.so's AST context because if we run again and we get a new foo.so we would have to reload bbb.so because its copy of "Foo" might be out of date. And we would need to track these interactions.

When we run expressions, we create a new AST and copy types as needed. It would be great if the AST importer only copy over forward declarations of types that can be completed later and can also complete types only as needed when asked.

If I understand correctly that is what this patch is trying to do. Seems like we have a code path that is copying over the type and also completing it sometimes without being asked which should be fixed. If we do fix this, complex expressions become a lot faster. To do this right we should always import forward declarations from the source, and be able to complete the new types in the destination as needed. As teemperor said, the source AST should not be mutated in any way. We should track all of this in the importer and know where we should try to complete the type from.

When using expression AST contexts it **is** ok to try and import "Foo" from bbb.so since that where is where we first saw the type, and if we aren't successful, we can grab the definition from anywhere else in the debug session. Since each expression has its own AST, it **is** ok to get the type from anywhere. When searching for this type we should start in the current lldb_private::Block, their pareent blocks, then the file, then the module and then all modules. I think that works today already, but I am not sure if this works for a type "Foo" that is mentioned in a type from a file that doesn't have a complete definition. for example if bbb.so contains:

  struct Bar : public Foo {...};

Due to "-flimit-debug-info" the definition for Foo might be forward declared (if the vtable for Foo isn't in the current binary) and not included in this binary. This won't happen on darwin since the default is -fno-limit-debug-info". The DWARF parser knows how to work around this issue when creating the type in the AST for bbb.so, but when we run an expression with this type, we want to be able to have an AST type from bbb.so with an incomplete definition for "Foo" that we complete during AST import. To do this, we will need to use metadata in the bbb.so AST to indicate we have no definition for this type when we normally would require one and be able to complete the type from another source.

So quick things to stick to:

- no modification of the source AST context
- importer can track anything it needs to in order to complete types in complex situations as mentioned above
  - need metadata that tracks types that need to be complete but aren't in the debug info so they can be properly imported for expressions ("struct Bar: public Foo {}", not ok for Foo to not be complete but we allow it for object file AST contexts otherwise clang crashes us)
  - legal forward declarations should be able to be imported as needed even if the AST form the original source doesn't have a complete type ("struct Bar { Foo *foo_ptr; }", ok for Foo to be forward declared here)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69933/new/

https://reviews.llvm.org/D69933