[lldb] Fix dynamic type resolution for types parsed from declarations (llvm#137974)

This fixes a regression caused by us starting to parse types from
declarations. The code in TypeSystemClang was assuming that the value
held in ClangASTMetadata was authoritative, but this isn't (and cannot)
be the case when the type is parsed from a forward-declaration.

For the fix, I add a new "don't know" state to ClangASTMetadata, and
make sure DWARFASTParserClang sets it only when it encounters a forward
declaration. In this case, the type system will fall back to completing
the type.

This does mean that we will be completing more types than before, but
I'm hoping this will offset by the fact that we don't search for
definition DIEs eagerly. In particular, I don't expect it to make a
difference in -fstandalone-debug scenarios, since types will nearly
always be present as definitions.

To avoid this cost, we'd need to create some sort of a back channel to
query the DWARFASTParser about the dynamicness of the type without
actually completing it. I'd like to avoid that if it is not necessary.

Author: labath

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

@@ -11,6 +11,22 @@
 
 using namespace lldb_private;
 
+std::optional<bool> ClangASTMetadata::GetIsDynamicCXXType() const {
+  switch (m_is_dynamic_cxx) {
+  case 0:
+    return std::nullopt;
+  case 1:
+    return false;
+  case 2:
+    return true;
+  }
+  llvm_unreachable("Invalid m_is_dynamic_cxx value");
+}
+
+void ClangASTMetadata::SetIsDynamicCXXType(std::optional<bool> b) {
+  m_is_dynamic_cxx = b ? *b + 1 : 0;
+}
+
 void ClangASTMetadata::Dump(Stream *s) {
   lldb::user_id_t uid = GetUserID();
 

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

@@ -12,19 +12,22 @@
 #include "lldb/Core/dwarf.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-private-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_is_forcefully_completed(false) {}
+        m_has_object_ptr(false), m_is_self(false),
+        m_is_forcefully_completed(false) {
+    SetIsDynamicCXXType(std::nullopt);
+  }
 
-  bool GetIsDynamicCXXType() const { return m_is_dynamic_cxx; }
+  std::optional<bool> GetIsDynamicCXXType() const;
 
-  void SetIsDynamicCXXType(bool b) { m_is_dynamic_cxx = b; }
+  void SetIsDynamicCXXType(std::optional<bool> b);
 
   void SetUserID(lldb::user_id_t user_id) {
     m_user_id = user_id;
@@ -101,8 +104,8 @@ class ClangASTMetadata {
     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_forcefully_completed : 1;
+  unsigned 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 : 2, m_is_forcefully_completed : 1;
 };
 
 } // namespace lldb_private

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

@@ -1837,7 +1837,8 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
 
   ClangASTMetadata metadata;
   metadata.SetUserID(die.GetID());
-  metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
+  if (!attrs.is_forward_declaration)
+    metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
 
   TypeSystemClang::TemplateParameterInfos template_param_infos;
   if (ParseTemplateParameterInfos(die, template_param_infos)) {

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

@@ -3657,13 +3657,17 @@ bool TypeSystemClang::IsPossibleDynamicType(lldb::opaque_compiler_type_t type,
     bool success;
     if (cxx_record_decl->isCompleteDefinition())
       success = cxx_record_decl->isDynamicClass();
-    else if (std::optional<ClangASTMetadata> metadata =
-                 GetMetadata(cxx_record_decl))
-      success = metadata->GetIsDynamicCXXType();
-    else if (GetType(pointee_qual_type).GetCompleteType())
-      success = cxx_record_decl->isDynamicClass();
-    else
-      success = false;
+    else {
+      std::optional<ClangASTMetadata> metadata = GetMetadata(cxx_record_decl);
+      std::optional<bool> is_dynamic =
+          metadata ? metadata->GetIsDynamicCXXType() : std::nullopt;
+      if (is_dynamic)
+        success = *is_dynamic;
+      else if (GetType(pointee_qual_type).GetCompleteType())
+        success = cxx_record_decl->isDynamicClass();
+      else
+        success = false;
+    }
 
     if (success)
       set_dynamic_pointee_type(pointee_qual_type);

lldb/test/API/lang/cpp/dynamic-value/Makefile

@@ -1,3 +1,3 @@
-CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp
+CXX_SOURCES := pass-to-base.cpp anonymous-b.cpp forward-a.cpp
 
 include Makefile.rules

lldb/test/API/lang/cpp/dynamic-value/TestDynamicValue.py

@@ -266,3 +266,15 @@ def examine_value_object_of_this_ptr(
         contained_b_static_addr = int(contained_b_static.GetValue(), 16)
 
         self.assertLess(contained_b_addr, contained_b_static_addr)
+
+    @no_debug_info_test
+    def test_from_forward_decl(self):
+        """Test fetching C++ dynamic values forward-declared types. It's
+        imperative that this is a separate test so that we don't end up parsing
+        a definition of A from somewhere else."""
+        self.build()
+        lldbutil.run_to_name_breakpoint(self, "take_A")
+        self.expect(
+            "frame var -d run-target --ptr-depth=1 --show-types a",
+            substrs=["(B *) a", "m_b_value = 10"],
+        )

lldb/test/API/lang/cpp/dynamic-value/a.h

@@ -22,4 +22,6 @@ class A {
 
 A *make_anonymous_B();
 
+A *take_A(A *a);
+
 #endif

lldb/test/API/lang/cpp/dynamic-value/forward-a.cpp

@@ -0,0 +1,3 @@
+class A;
+
+A *take_A(A *a) { return a; }

lldb/test/API/lang/cpp/dynamic-value/pass-to-base.cpp

@@ -45,5 +45,7 @@ main (int argc, char **argv)
 
   myB.doSomething(*make_anonymous_B());
 
+  take_A(&myB);
+
   return 0;
 }