[lldb] Add basic -flimit-debug-info support to expression evaluator

[labath]

Summary

This patch adds support for evaluation of expressions referring to types
which were compiled in -flimit-debug-info (a.k.a -fno-standalone-debug)
in clang. In this mode it's possible that the debug information needed
to fully describe a c++ type is not present in a single shared library

• for example debug info for a base class or a member of a type can

only be found in another shared library. This situation is not
currently handled well within lldb as we are limited to searching within
a single shared library (lldb_private::Module) when searching for the
definition of these types.

The way that this patch gets around this limitation is by doing the
search at a later stage -- during the construction of the expression ast
context. This works by having the parser (currently SymbolFileDWARF, but
a similar approach is probably needed for PDBs too) mark a type as
"forcefully completed". What this means is that the parser has marked
the type as "complete" in the module ast context (as this is necessary
to e.g. derive classes from it), but its definition is not really there.
This is done via a new field on the ClangASTMetadata struct.

Later, when we are importing such a type into the expression ast, we
check this flag. If the flag is set, we try to find a better definition
for the type in other shared libraries. This part reuses the same code
path (ClangASTSource::CompleteType) that we are using to find
definitions of types that are incomplete in some module (the difference
between this situation and -flimit-debug-info is that in the former
case, the type is only ever used opaquely in the module -- e.g. FILE *

• whereas in the latter the type was fully defined in the source code,

but the compiler chose not to emit its definition in the debug info).

This required a small refactor of the ClangASTSource::CompleteType
function -- I split off the part which searches for the equivalent
complete type into a separate function. This was needed because the
import process operates slightly differently here. Previously, we were
being asked to complete an already imported forward declaration. Now we
are searching for an equivalent declaration before importing anything --
that's because importing an "forcefully completed" type would mark the
type as complete in the destination AST and we would not be asked to
complete it anymore.

This patch only implements this handling for base classes, but other
cases (members, array element types, etc.). The changes for that should
be fairly simple and mostly revolve around marking these types as
"forcefully completed" at an approriate time -- the importing logic is
generic already.

Another aspect, which is also not handled by this patch is viewing these
types via the "frame variable" command. This does not use the AST
importer and so it will need to handle these types on its own -- that
will be the subject of another patch.

[clayborg]

The overall approach is what we had talked about previously and looks good as far as I can tell. I don't do enough expression parser work to give the accept on this, but I have no problems with what I see. If any expression parser experts are not on the reviewers, please add them!

This will be great to have, thanks for starting this off!!!

[clayborg]

If we run into a class in the AST importer that was forcefully completed and we can't find a better definition, what do we do? Error out? Do what we did. I would like there to be a nice log line in the "log enable lldb expr" to tell us when this happens so we can know why an expression is failing if we don't emit an error and stop the expression.

Also, for "frame variable" and the any ValueObject objects, we can easily see what a BaseClass is marked as forcefully completed and just do a simple type search for the type given a full decl context as context and substitute the type. That won't use any of this code, it will just need to know to grab the the full type from the target by doing a search. We could make a function similar to CompilerType::GetCompleteType() that takes a target so that it can search all of the modules in a target for a type.

[aprantl]

It's great to see this being addressed! I have a high-level question: When completing types across lldb::Modules — in which ASTContext is the complete type created? Since a per-module TypeSystem can be shared by many debuggers, I want to make sure that types from another module don't pollute another module's ASTContext, and that they are created in the/a scratch context instead.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

882

Can you document what case exactly is being handled here?

lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h

87

It would be important to document somewhere what the semantics of this attribute are exactly, since the name is not self-explanatory.

[labath]

add more comments and logging

[labath]

In D81561#2086435, @clayborg wrote:

If we run into a class in the AST importer that was forcefully completed and we can't find a better definition, what do we do? Error out? Do what we did. I would like there to be a nice log line in the "log enable lldb expr" to tell us when this happens so we can know why an expression is failing if we don't emit an error and stop the expression.

We do what we did previously -- import the empty class. The included test case exercises this scenario as well.

I'll print a log message when that happens.

Also, for "frame variable" and the any ValueObject objects, we can easily see what a BaseClass is marked as forcefully completed and just do a simple type search for the type given a full decl context as context and substitute the type. That won't use any of this code, it will just need to know to grab the the full type from the target by doing a search. We could make a function similar to CompilerType::GetCompleteType() that takes a target so that it can search all of the modules in a target for a type.

Yes, that's the rough idea, but I haven't tried implementing it yet.

In D81561#2086580, @aprantl wrote:

It's great to see this being addressed! I have a high-level question: When completing types across lldb::Modules — in which ASTContext is the complete type created? Since a per-module TypeSystem can be shared by many debuggers, I want to make sure that types from another module don't pollute another module's ASTContext, and that they are created in the/a scratch context instead.

Apart from the adding the "forcefully completed" flag, this solution does not change the contents of module ASTs in any way. If we take the example in the test case, the individual modules ASTs would look something like:
libone:

struct One {
  int one;
};

libtwo:

struct [[forcefully_completed]] One { };
struct Two : One {
  int two;
};

a.out:

struct [[forcefully_completed]] One { };
struct [[forcefully_completed]] Two { };
struct InheritsFromOne: One {
  int member;
};
...

When importing into the expression AST, we pick the best representation for each of the types and create a merged class. I'm not entirely sure what happens when importing this into the AST for persistent expression results (what's the name for that?). Ideally the merged type would be imported there too, but I am not sure if that actually happens right now -- expr inherits_from_one.one works, but a plain expr inherits_from_one just displays the member member. I'm not yet sure if this is simply due to lack of support in the "frame variable" machinery which is supposed to display the result, or if there is some more importing that needs to happen.

[teemperor]

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).

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

891

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:

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.

lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp

11 ↗

(On Diff #269811)

I guess that's WIP code

lldb/test/API/functionalities/limit-debug-info/Makefile

6

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.

[labath]

• address review comments

[labath]

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

891

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.)

lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp

11 ↗

(On Diff #269811)

Yep. removed.

lldb/test/API/functionalities/limit-debug-info/Makefile

6

sounds good.

[labath]

So, what do you make of this patch now, Raphael?

[teemperor]

LGTM beside the _check_type check always passing as discussed on IRC. Thanks!

[labath]

• redo the sanity check in the test (get_field_array does not include inherited types)
• skip gmodules test flavours due to pr46284

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/ExpressionParser/Clang/ClangASTMetadata.h"
+#include "Plugins/ExpressionParser/Clang/ClangASTSource.h"
 #include "Plugins/ExpressionParser/Clang/ClangExternalASTSourceCallbacks.h"
 #include "Plugins/ExpressionParser/Clang/ClangUtil.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 
 #include <memory>
 
 using namespace lldb_private;
 using namespace clang;
ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
   if (origin.Valid()) {
     auto R = m_master.CopyDecl(&getToContext(), origin.decl);
     if (R) {
       RegisterImportedDecl(From, R);
       return R;
     }
   }
 
+  // If we have a forcefully completed type, try to find an actual definition
+  // for it in other modules.
+  const ClangASTMetadata *md = m_master.GetDeclMetadata(From);
+  auto *td = dyn_cast<TagDecl>(From);
+  if (td && md && md->IsForcefullyCompleted()) {
+    Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
+    LLDB_LOG(log,
+             "[ClangASTImporter] Searching for a complete definition of {0} in "
+             "other modules",
+             td->getName());
+    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;
+    DeclContext::lookup_result lr = dc->lookup(*dn_or_err);
+    if (lr.size()) {
+      clang::Decl *lookup_found = lr.front();
+      RegisterImportedDecl(From, lookup_found);
+      m_decls_to_ignore.insert(lookup_found);
+      return lookup_found;
+    } else
+      LLDB_LOG(log, "[ClangASTImporter] Complete definition not found");
+  }
+
   return ASTImporter::ImportImpl(From);
 }
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
     clang::Decl *to, clang::Decl *from) {
   // We might have a forward declaration from a shared library that we
   // gave external lexical storage so that Clang asks us about the full
   // definition when it needs it. In this case the ASTImporter isn't aware

lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h

 #include "lldb/lldb-enumerations.h"
 
 namespace lldb_private {
 
 class ClangASTMetadata {
 public:
   ClangASTMetadata()
       : m_user_id(0), m_union_is_user_id(false), m_union_is_isa_ptr(false),
-        m_has_object_ptr(false), m_is_self(false), m_is_dynamic_cxx(true) {}
+        m_has_object_ptr(false), m_is_self(false), m_is_dynamic_cxx(true),
+        m_is_forcefully_completed(false) {}
 
   bool GetIsDynamicCXXType() const { return m_is_dynamic_cxx; }
 
   void SetIsDynamicCXXType(bool b) { m_is_dynamic_cxx = b; }
 
   void SetUserID(lldb::user_id_t user_id) {
     m_user_id = user_id;
     m_union_is_user_id = true;
if (m_has_object_ptr) {
       else
         return "this";
     } else
       return nullptr;
   }
 
   bool HasObjectPtr() const { return m_has_object_ptr; }
 
+  /// A type is "forcefully completed" if it was declared complete to satisfy an
+  /// AST invariant (e.g. base classes must be complete types), but in fact we
+  /// were not able to find a actual definition for it.
+  bool IsForcefullyCompleted() const { return m_is_forcefully_completed; }
+
+  void SetIsForcefullyCompleted(bool value = true) {
+    m_is_forcefully_completed = true;
+  }
+
   void Dump(Stream *s);
 
 private:
   union {
     lldb::user_id_t m_user_id;
     uint64_t m_isa_ptr;
   };
 
   bool m_union_is_user_id : 1, m_union_is_isa_ptr : 1, m_has_object_ptr : 1,
-      m_is_self : 1, m_is_dynamic_cxx : 1;
+      m_is_self : 1, m_is_dynamic_cxx : 1, m_is_forcefully_completed : 1;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_CLANGASTMETADATA_H

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (!bases.empty()) {
       // assertion in the call to clang_type.TransferBaseClasses()
       for (const auto &base_class : bases) {
         clang::TypeSourceInfo *type_source_info =
             base_class->getTypeSourceInfo();
         if (type_source_info) {
           CompilerType base_class_type =
               m_ast.GetType(type_source_info->getType());
           if (!base_class_type.GetCompleteType()) {
-            auto module = dwarf->GetObjectFile()->GetModule();
-            module->ReportError(":: Class '%s' has a base class '%s' which "
-                                "does not have a complete definition.",
-                                die.GetName(),
-                                base_class_type.GetTypeName().GetCString());
-            if (die.GetCU()->GetProducer() == eProducerClang)
-              module->ReportError(":: Try compiling the source file with "
-                                  "-fstandalone-debug.");
-
-            // We have no choice other than to pretend that the base class
-            // is complete. If we don't do this, clang will crash when we
-            // call setBases() inside of
-            // "clang_type.TransferBaseClasses()" below. Since we
-            // provide layout assistance, all ivars in this class and other
-            // classes will be fine, this is the best we can do short of
-            // crashing.
+            // We mark the class as complete to allow the TransferBaseClasses
+            // call to succeed. But we make a note of the fact that this class
+            // is not _really_ complete so we can later search for a definition
+            // in a different module.
             if (TypeSystemClang::StartTagDeclarationDefinition(
                     base_class_type)) {
               TypeSystemClang::CompleteTagDeclarationDefinition(
                   base_class_type);
+              const auto *td = TypeSystemClang::GetQualType(
+                                   base_class_type.GetOpaqueQualType())
+                                   .getTypePtr()
+                                   ->getAsTagDecl();
+              m_ast.GetMetadata(td)->SetIsForcefullyCompleted();
             }
           }
         }
       }
 
       m_ast.TransferBaseClasses(clang_type.GetOpaqueQualType(),
                                 std::move(bases));
     }

lldb/test/API/functionalities/limit-debug-info/Makefile

+CFLAGS_EXTRAS = $(LIMIT_DEBUG_INFO_FLAGS)
+LD_EXTRAS = -L. -lone -ltwo
+CXX_SOURCES = main.cpp
+
+ONE_CXXFLAGS = $(LIMIT_DEBUG_INFO_FLAGS)
+ifdef STRIP_ONE
+  ONE_CXXFLAGS += -g0
+endif
+
+TWO_CXXFLAGS = $(LIMIT_DEBUG_INFO_FLAGS)
+ifdef STRIP_TWO
+  TWO_CXXFLAGS += -g0
+endif
+
+include Makefile.rules
+
+a.out: libone libtwo
+
+libone:
+  $(MAKE) -f $(MAKEFILE_RULES) \
+    DYLIB_ONLY=YES DYLIB_CXX_SOURCES=one.cpp DYLIB_NAME=one \
+    CFLAGS_EXTRAS="$(ONE_CXXFLAGS)"
+
+libtwo: libone
+  $(MAKE) -f $(MAKEFILE_RULES) \
+    DYLIB_ONLY=YES DYLIB_CXX_SOURCES=two.cpp DYLIB_NAME=two \
+    CFLAGS_EXTRAS="$(TWO_CXXFLAGS)" LD_EXTRAS="-L. -lone"

lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py

+"""
+Test completing types using information from other shared libraries.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LimitDebugInfoTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def _check_type(self, target, name):
+        exe = target.FindModule(lldb.SBFileSpec("a.out"))
+        type_ = exe.FindFirstType(name)
+        self.trace("type_: %s"%type_)
+        self.assertTrue(type_)
+        base = type_.GetDirectBaseClassAtIndex(0).GetType()
+        self.trace("base:%s"%base)
+        self.assertTrue(base)
+        self.assertEquals(base.GetNumberOfFields(), 0)
+
+    def _check_debug_info_is_limited(self, target):
+        # Without other shared libraries we should only see the member declared
+        # in the derived class. This serves as a sanity check that we are truly
+        # building with limited debug info.
+        self._check_type(target, "InheritsFromOne")
+        self._check_type(target, "InheritsFromTwo")
+
+    @skipIf(bugnumber="pr46284", debug_info="gmodules")
+    def test_one_and_two_debug(self):
+        self.build()
+        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+        self._check_debug_info_is_limited(target)
+
+        self.registerSharedLibrariesWithTarget(target, ["one", "two"])
+
+        # But when other shared libraries are loaded, we should be able to see
+        # all members.
+        self.expect_expr("inherits_from_one.member", result_value="47")
+        self.expect_expr("inherits_from_one.one", result_value="142")
+
+        self.expect_expr("inherits_from_two.member", result_value="47")
+        self.expect_expr("inherits_from_two.one", result_value="142")
+        self.expect_expr("inherits_from_two.two", result_value="242")
+
+    @skipIf(bugnumber="pr46284", debug_info="gmodules")
+    def test_two_debug(self):
+        self.build(dictionary=dict(STRIP_ONE="1"))
+        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+        self._check_debug_info_is_limited(target)
+
+        self.registerSharedLibrariesWithTarget(target, ["one", "two"])
+
+        # This time, we should only see the members from the second library.
+        self.expect_expr("inherits_from_one.member", result_value="47")
+        self.expect("expr inherits_from_one.one", error=True,
+            substrs=["no member named 'one' in 'InheritsFromOne'"])
+
+        self.expect_expr("inherits_from_two.member", result_value="47")
+        self.expect("expr inherits_from_two.one", error=True,
+            substrs=["no member named 'one' in 'InheritsFromTwo'"])
+        self.expect_expr("inherits_from_two.two", result_value="242")
+
+    @skipIf(bugnumber="pr46284", debug_info="gmodules")
+    def test_one_debug(self):
+        self.build(dictionary=dict(STRIP_TWO="1"))
+        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+        self._check_debug_info_is_limited(target)
+
+        self.registerSharedLibrariesWithTarget(target, ["one", "two"])
+
+        # In this case we should only see the members from the second library.
+        # Note that we cannot see inherits_from_two.one because without debug
+        # info for "Two", we cannot determine that it in fact inherits from
+        # "One".
+        self.expect_expr("inherits_from_one.member", result_value="47")
+        self.expect_expr("inherits_from_one.one", result_value="142")
+
+        self.expect_expr("inherits_from_two.member", result_value="47")
+        self.expect("expr inherits_from_two.one", error=True,
+            substrs=["no member named 'one' in 'InheritsFromTwo'"])
+        self.expect("expr inherits_from_two.two", error=True,
+            substrs=["no member named 'two' in 'InheritsFromTwo'"])

lldb/test/API/functionalities/limit-debug-info/foo.cpp

+struct A {
+  int a = 47;
+  virtual ~A();
+};
+
+A::~A() = default;

lldb/test/API/functionalities/limit-debug-info/main.cpp

+#include "onetwo.h"
+
+struct InheritsFromOne : One {
+  constexpr InheritsFromOne() = default;
+  int member = 47;
+} inherits_from_one;
+
+struct InheritsFromTwo : Two {
+  constexpr InheritsFromTwo() = default;
+  int member = 47;
+} inherits_from_two;
+
+int main() { return 0; }

lldb/test/API/functionalities/limit-debug-info/one.cpp

+#include "onetwo.h"
+
+One::~One() = default;

lldb/test/API/functionalities/limit-debug-info/onetwo.h

+struct One {
+  int one = 142;
+  constexpr One() = default;
+  virtual ~One();
+};
+
+struct Two : One {
+  int two = 242;
+  constexpr Two() = default;
+  ~Two() override;
+};

lldb/test/API/functionalities/limit-debug-info/two.cpp

+#include "onetwo.h"
+
+Two::~Two() = default;