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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added inline comments.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:891
+
+    if (auto *proxy = llvm::dyn_cast<ClangASTSource::ClangASTSourceProxy>(
+            getToContext().getExternalSource())) {
----------------
teemperor wrote:
> 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.
I wasn't really sure what that flag is guarding against (i.e. how reliable recursive lookups are), so I was trying to go beneath it. But if you think deleting it is ok, then the new version is definitely better. (There are ways we could mitigate the RTTI problem, but this sidesteps it completely.)


================
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"
----------------
teemperor wrote:
> I guess that's WIP code
Yep. removed.


================
Comment at: lldb/test/API/functionalities/limit-debug-info/Makefile:5
+
+ONE_CXXFLAGS := -flimit-debug-info
+ifdef STRIP_ONE
----------------
teemperor wrote:
> 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.
sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81561