• Made test stricter.
• Moved code to ASTImporterDelegate.

In D72946#1827372, @clayborg wrote:

Is an AST importer specific to a target? Can we just put it into the Clang AST type system subclass and create it lazily?

LGTM modulo some minor points regarding the test. The refactoring of the parsing code and using expect_expr can be done as NFC follow-ups. Thanks for the patch, great work!

I just went back to the radar we had about the CG crash and this *does* fix that issue (rdar://53932023). I could also not reproduce the CG crash again so I assume that was something else that fiddled around in that area. So LGTM minus the thing with the last_bitfield_info last_field_info duality which I would prefer we could get rid of before landing (but it's not a must and can be done as a follow-up commit).

I can land it, seems to pass all LLDB tests too.

I really don't think the ASTImporter should ever manipulate records in the source context (effectively the source context should be considered immutable). It also seems *very* wrong that what we import depends in any way on a previous expression so I agree we should fix that. In theory the ImportDefinition call in the ASTImporter shouldn't do any real work as we have the MinimalImport mode on in LLDB so it should only load some bare bone record with external storage IIUC. So I think the original version of the patch seems like a better approach to me from a quick glance.

This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this SystemInitializerMostPlugins ?

I moved the single non-plugin call back to the original Full/Test subclasses so the name is now correct. Also I removed all the duplicated linking flags that I forgot to remove before.

• Removed all the duplicated linking flags too.

Thanks for the review!

• Added a simple expr evaluation as an additional way to trigger the error.

Actually that seems to be more complicated in case we get the method decl by asking the ASTImporter for a definition (which will skip this code).

(Jim pointed out that we land this without expect_expr to make back porting easier but somehow Phabricator didn't add his comment to the review here. expect_expr is not yet in the downstream Github branch but it is in the 10 release branch, so that makes sense to me).

How does this deal with linking flags etc.? Your cmake cache disabled LLDB_PLUGIN_SYMBOLFILE_NATIVEPDB_BUILD (well, it would without the LDB typo) but that just gives me a build that fails to link with ld: error: unable to find library -llldbPluginSymbolFileNativePDB.

(Just some quick comments, will review this properly during normal working hours)

I wish we could do this without a global map. Also the ClangASTImporter shouldn't have a dependency on Target (I'm actually surprised this compiles without an additional include).

Are there any objections against this? Otherwise I'll land this tomorrow.

• Removed substr functionality.
• Using frame() now.

• Removed everything that is not summary or value.

• Replaced dummy contents with actual patch

I also got rid of the expr->frame var->expr flow and it's not just expr->frame var. I don't want to remove them as we found two formatter bugs by testing both but once these thing don't break constantly then we can remove the 'expr' variant from the simple expression function.

• Moved to using the SBAPI.

In D72684#1820486, @clayborg wrote:

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

• Readd test files.

• Rebase.

• Revert to original dummy patch.

Huh, seems I uploaded to the wrong review

• Rebased to get rid of shady StringRef -> C-String conversion.

FWIW this patch makes me realise that we really should have a Sema around when we create the module AST as this would prevent issues like this and saves us from simulating Sema behavior. That's probably yet another large refactoring so I don't have time for that soon-ish but if anyone wants to do this then be my guest.

• Upload correct revision of the test file.

LGTM, thanks!

Both TypeSystemClang and ClangTypeSystem works for me.

We are currently after a fresh rebranch downstream so now is a good time to rename this without causing merge-conflict hell later on. It would be useful if could get consensus if and to what we should rename this class soon-ish so that I can make this move as painless as possible.

FWIW we solved the LLDB problems.

In D72447#1817559, @stella.stamenova wrote:

This introduced a failure on the Windows LLDB bot (there were already other failing tests though, so maybe you missed it):

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/12554