[Lldb-commits] [PATCH] D81561: [lldb] Add basic -flimit-debug-info support to expression evaluator

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

I think the approach here makes sense. The only issue I see is how we get the complete replacement type (see the inline comment).

There is the issue if having an artificial empty class in an expression is an error, but we anyway already do this so I don't think this discussion should block this patch.

Also the FindCompleteType refactoring should IMHO land as it's own NFC commit just because it's such a huge part of the patch (I don't think that needs a separate review, it LGTM).



================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:891
+
+    if (auto *proxy = llvm::dyn_cast<ClangASTSource::ClangASTSourceProxy>(
+            getToContext().getExternalSource())) {
----------------
This check can really easily break. We should always be able to replace the ExternalASTSource with some kind of multiplexer (like MultiplexExternalSemaSource) without breaking functionality, so doing an RTTI check for a specific class here will break that concept. For example activating the import-std-module setting will make Clang add its own ASTReader source to the AST, so the ClangASTSourceProxy is hidden in some Multiplex* class (that contains ClangASTSourceProxy and the ASTReader) and this check will fail.

I think can simplified to this logic which doesn't require any RTTI support or ClangASTSource refactoring (even though the FindCompleteType refactor is IMHO something we should do even if we don't need it for this patch). I also removed that we forcibly complete the class which I don't think is necessary:

```
lang=c++
  if (td && md && md->IsForcefullyCompleted()) {
    Expected<DeclContext *> dc_or_err = ImportContext(td->getDeclContext());
    if (!dc_or_err)
      return dc_or_err.takeError();
    Expected<DeclarationName> dn_or_err = Import(td->getDeclName());
    if (!dn_or_err)
      return dn_or_err.takeError();
    DeclContext *dc = *dc_or_err;
    DeclarationName dn = *dn_or_err;
    DeclContext::lookup_result lr = dc->lookup(dn);
    if (lr.size()) {
      clang::Decl *lookup_found = lr.front();
      RegisterImportedDecl(From, lookup_found);
      m_decls_to_ignore.insert(lookup_found);
      return lookup_found;
    }
  }
```

The only required change for getting this to work is to get rid of the `ImportInProgress` flag that completely disables all lookups when we copy a type. I don't think there is a good reason for even having this flag as it's anyway usually not correctly set when we do some kind of import, so I think deleting it is fine.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp:11
+#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
+#include "Plugins/ExpressionParser/Clang/ClangASTMetadata.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
----------------
I guess that's WIP code


================
Comment at: lldb/test/API/functionalities/limit-debug-info/Makefile:5
+
+ONE_CXXFLAGS := -flimit-debug-info
+ifdef STRIP_ONE
----------------
The in-tree limit-debug-info test already uses the LIMIT_DEBUG_INFO_FLAGS variable instead of hardcoding the flag, so I think we might as well do it here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81561