[Lldb-commits] [lldb] b9867df - [lldb] Fix CTF parsing of large structs

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-07-29T19:37:08-07:00
New Revision: b9867df64a312b2fe248d536d943733b8817ed4f

URL: 
https://github.com/llvm/llvm-project/commit/b9867df64a312b2fe248d536d943733b8817ed4f
DIFF: 
https://github.com/llvm/llvm-project/commit/b9867df64a312b2fe248d536d943733b8817ed4f.diff

LOG: [lldb] Fix CTF parsing of large structs

Fix parsing of large structs. If the size of a struct exceeds a certain
threshold, the offset is encoded using two 32-bit integers instead of
one.

Differential revision: https://reviews.llvm.org/D156490

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
lldb/test/API/macosx/ctf/Makefile
lldb/test/API/macosx/ctf/TestCTF.py
lldb/test/API/macosx/ctf/test.c

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h 
b/lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
index b2cf5cf3191b64..99a74dbe674aea 100644
--- a/lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
+++ b/lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
@@ -131,14 +131,12 @@ struct CTFFunction : public CTFType {
 struct CTFRecord : public CTFType {
 public:
   struct Field {
-Field(llvm::StringRef name, uint32_t type, uint16_t offset,
-  uint16_t padding)
-: name(name), type(type), offset(offset), padding(padding) {}
+Field(llvm::StringRef name, uint32_t type, uint64_t offset)
+: name(name), type(type), offset(offset) {}
 
 llvm::StringRef name;
 uint32_t type;
-uint16_t offset;
-uint16_t padding;
+uint64_t offset;
   };
 
   CTFRecord(Kind kind, lldb::user_id_t uid, llvm::StringRef name,

diff  --git a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp 
b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
index f737db3ed4e4b2..1c4de94e06d7b8 100644
--- a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
+++ b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
@@ -642,9 +642,16 @@ SymbolFileCTF::ParseType(lldb::offset_t &offset, 
lldb::user_id_t uid) {
 for (uint32_t i = 0; i < variable_length; ++i) {
   const uint32_t field_name = m_data.GetU32(&offset);
   const uint32_t type = m_data.GetU32(&offset);
-  const uint16_t field_offset = m_data.GetU16(&offset);
-  const uint16_t padding = m_data.GetU16(&offset);
-  fields.emplace_back(ReadString(field_name), type, field_offset, padding);
+  uint64_t field_offset = 0;
+  if (size < g_ctf_field_threshold) {
+field_offset = m_data.GetU16(&offset);
+m_data.GetU16(&offset); // Padding
+  } else {
+const uint32_t offset_hi = m_data.GetU32(&offset);
+const uint32_t offset_lo = m_data.GetU32(&offset);
+field_offset = (((uint64_t)offset_hi) << 32) | ((uint64_t)offset_lo);
+  }
+  fields.emplace_back(ReadString(field_name), type, field_offset);
 }
 return std::make_unique(static_cast(kind), uid,
name, variable_length, size, fields);
@@ -850,7 +857,6 @@ size_t SymbolFileCTF::ParseObjects(CompileUnit &comp_unit) {
 if (Symbol *symbol =
 symtab->FindSymbolWithType(eSymbolTypeData, Symtab::eDebugYes,
Symtab::eVisibilityAny, symbol_idx)) {
-
   Variable::RangeList ranges;
   ranges.Append(symbol->GetFileAddress(), symbol->GetByteSize());
 

diff  --git a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h 
b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
index f5a78e5b59a908..170a6d8700516a 100644
--- a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
+++ b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
@@ -254,6 +254,7 @@ class SymbolFileCTF : public lldb_private::SymbolFileCommon 
{
 
   static constexpr uint16_t g_ctf_magic = 0xcff1;
   static constexpr uint8_t g_ctf_version = 4;
+  static constexpr uint16_t g_ctf_field_threshold = 0x2000;
 };
 } // namespace lldb_private
 

diff  --git a/lldb/test/API/macosx/ctf/Makefile 
b/lldb/test/API/macosx/ctf/Makefile
index efe1043cc584ab..afe6ab1b5db06b 100644
--- a/lldb/test/API/macosx/ctf/Makefile
+++ b/lldb/test/API/macosx/ctf/Makefile
@@ -28,3 +28,4 @@ a.ctf: a.out.dSYM
-R __DWARF,__apple_objc \
a.ctf a.ctf
rm -rf a.out.dSYM
+   rm -rf test.o

diff  --git a/lldb/test/API/macosx/ctf/TestCTF.py 
b/lldb/test/API/macosx/ctf/TestCTF.py
index 9f8583bccb7a5b..470d35f74d1d92 100644
--- a/lldb/test/API/macosx/ctf/TestCTF.py
+++ b/lldb/test/API/macosx/ctf/TestCTF.py
@@ -37,6 +37,9 @@ def do_test(self):
 
 symbol_file = self.getBuildArtifact("a.ctf")
 
+if self.TraceOn():
+self.runCmd("log enable -v lldb symbol")
+
 self.runCmd("target symbols add {}".format(symbol_file))
 self.expect(
 "target variable foo",
@@ -53,7 +56,6 @@ def do_tes

[Lldb-commits] [PATCH] D156490: [lldb] Fix CTF parsing of large structs

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb9867df64a31: [lldb] Fix CTF parsing of large structs 
(authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D156490?vs=544922&id=545396#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156490

Files:
  lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
  lldb/test/API/macosx/ctf/Makefile
  lldb/test/API/macosx/ctf/TestCTF.py
  lldb/test/API/macosx/ctf/test.c

Index: lldb/test/API/macosx/ctf/test.c
===
--- lldb/test/API/macosx/ctf/test.c
+++ lldb/test/API/macosx/ctf/test.c
@@ -31,8 +31,14 @@
   void (*f)(int);
 } MyStructT;
 
+struct LargeStruct {
+  char buffer[9000];
+  int b;
+};
+
 MyStructT foo;
 struct ForwardDecl *forward;
+struct LargeStruct bar;
 
 void populate(MyInt i) {
   foo.n.i = i;
@@ -45,6 +51,7 @@
   foo.n.e = eOne;
   foo.f = NULL;
   forward = NULL;
+  bar.b = i;
 }
 
 int main(int argc, char** argv) {
Index: lldb/test/API/macosx/ctf/TestCTF.py
===
--- lldb/test/API/macosx/ctf/TestCTF.py
+++ lldb/test/API/macosx/ctf/TestCTF.py
@@ -37,6 +37,9 @@
 
 symbol_file = self.getBuildArtifact("a.ctf")
 
+if self.TraceOn():
+self.runCmd("log enable -v lldb symbol")
+
 self.runCmd("target symbols add {}".format(symbol_file))
 self.expect(
 "target variable foo",
@@ -53,7 +56,6 @@
 "f = 0x",
 ],
 )
-
 self.expect("target variable foo.n.i", substrs=["(MyInt) foo.n.i = 1"])
 self.expect(
 "target variable foo.n.s", substrs=["(const char *) foo.n.s", '"foo"']
Index: lldb/test/API/macosx/ctf/Makefile
===
--- lldb/test/API/macosx/ctf/Makefile
+++ lldb/test/API/macosx/ctf/Makefile
@@ -28,3 +28,4 @@
 		-R __DWARF,__apple_objc \
 		a.ctf a.ctf
 	rm -rf a.out.dSYM
+	rm -rf test.o
Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
@@ -254,6 +254,7 @@
 
   static constexpr uint16_t g_ctf_magic = 0xcff1;
   static constexpr uint8_t g_ctf_version = 4;
+  static constexpr uint16_t g_ctf_field_threshold = 0x2000;
 };
 } // namespace lldb_private
 
Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
@@ -642,9 +642,16 @@
 for (uint32_t i = 0; i < variable_length; ++i) {
   const uint32_t field_name = m_data.GetU32(&offset);
   const uint32_t type = m_data.GetU32(&offset);
-  const uint16_t field_offset = m_data.GetU16(&offset);
-  const uint16_t padding = m_data.GetU16(&offset);
-  fields.emplace_back(ReadString(field_name), type, field_offset, padding);
+  uint64_t field_offset = 0;
+  if (size < g_ctf_field_threshold) {
+field_offset = m_data.GetU16(&offset);
+m_data.GetU16(&offset); // Padding
+  } else {
+const uint32_t offset_hi = m_data.GetU32(&offset);
+const uint32_t offset_lo = m_data.GetU32(&offset);
+field_offset = (((uint64_t)offset_hi) << 32) | ((uint64_t)offset_lo);
+  }
+  fields.emplace_back(ReadString(field_name), type, field_offset);
 }
 return std::make_unique(static_cast(kind), uid,
name, variable_length, size, fields);
@@ -850,7 +857,6 @@
 if (Symbol *symbol =
 symtab->FindSymbolWithType(eSymbolTypeData, Symtab::eDebugYes,
Symtab::eVisibilityAny, symbol_idx)) {
-
   Variable::RangeList ranges;
   ranges.Append(symbol->GetFileAddress(), symbol->GetByteSize());
 
Index: lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
===
--- lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
+++ lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
@@ -131,14 +131,12 @@
 struct CTFRecord : public CTFType {
 public:
   struct Field {
-Field(llvm::StringRef name, uint32_t type, uint16_t offset,
-  uint16_t padding)
-: name(name), type(type), offset(offset), padding(padding) {}
+Field(llvm::StringRef name, uint32_t type, uint64_t offset)
+: name(name), type(type), offset(offset) {}
 
 llvm::StringRef name;
 uint32_t type;
-uint16_t offset;
-uint16_t p

[Lldb-commits] [PATCH] D156498: [lldb] Support recursive record types in CTF

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp:508
   ctf_record.name.data(), tag_kind, 
eLanguageTypeC);
+  m_compiler_types[record_type.GetOpaqueQualType()] = &ctf_record;
+  Declaration decl;

Michael137 wrote:
> Just to clarify the lifetimes. This `ctf_record` lives on `m_ast` while the 
> `m_compiler_types` on the SymbolFile, so we're guaranteed that the 
> `ctf_record` lives long enough?
1. Yes, the `ctf_record` is owned by the `m_ctf_types;` whose lifetime is the 
same as the SymbolFile. We only need the CTF types until they're converted to 
LLDB types so we can be more memory efficient. I'll tackle that in a follow up. 
2. Once we've completed a type, we should remove the compiler type from 
`m_compiler_types`. I'll include that in this patch.



Comment at: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp:524-529
+  // We only support resolving record types.
+  assert(ctf_type->kind == CTFType::Kind::eStruct ||
+ ctf_type->kind == CTFType::Kind::eUnion);
 
-  m_ast->StartTagDeclarationDefinition(record_type);
-  for (const CTFRecord::Field &field : ctf_record.fields) {
-if (Type *field_type = ResolveTypeUID(field.type)) {
-  const uint32_t field_size = field_type->GetByteSize(nullptr).value_or(0);
-  TypeSystemClang::AddFieldToRecordType(record_type, field.name,
-field_type->GetFullCompilerType(),
-eAccessPublic, field_size);
+  // Cast to the appropriate CTF type.
+  const CTFRecord *ctf_record = static_cast(ctf_type);

Michael137 wrote:
> Nit: would it make sense to just add `classof` methods to `CTFType` and use 
> the llvm cast facilities?
> 
> Feel free to ignore since there's just one instance of such cast afaict
I considered it too, but at this point I don't think it's worth it. If we need 
other casts that's definitely the way to go. 


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

https://reviews.llvm.org/D156498

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156606: [lldb] Improve memory usage by freeing CTF types (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: Michael137.
Herald added a project: All.
JDevlieghere requested review of this revision.

Improve memory usage by reducing the lifetime of CTF types. Once a CTF type has 
been converted to a (complete) LLDB type, there's no need to keep it in memory 
anymore. For most types, we can free them right after creating the 
corresponding LLDB types. The only exception is record types, which are only 
completed lazily.

This patch also adds LLVM RTTI support to CTF type. This was suggested in 
D156498  but with just one caller that didn't 
seem worth it yet. This patch introduced a second call site which tipped the 
scales.


https://reviews.llvm.org/D156606

Files:
  lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h

Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
@@ -245,15 +245,17 @@
 
   std::optional m_header;
 
-  std::vector> m_ctf_types;
+  /// Parsed CTF types.
+  llvm::DenseMap> m_ctf_types;
+
+  /// Parsed LLDB types.
+  llvm::DenseMap m_types;
 
   /// To complete types, we need a way to map (imcomplete) compiler types back
   /// to parsed CTF types.
   llvm::DenseMap
   m_compiler_types;
 
-  llvm::DenseMap m_types;
-
   std::vector m_functions;
   std::vector m_variables;
 
Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
@@ -523,8 +523,7 @@
   assert(ctf_type && "m_compiler_types should only contain valid CTF types");
 
   // We only support resolving record types.
-  assert(ctf_type->kind == CTFType::Kind::eStruct ||
- ctf_type->kind == CTFType::Kind::eUnion);
+  assert(llvm::isa(ctf_type));
 
   // Cast to the appropriate CTF type.
   const CTFRecord *ctf_record = static_cast(ctf_type);
@@ -551,9 +550,10 @@
   }
   m_ast->CompleteTagDeclarationDefinition(compiler_type);
 
-  // Now that the compiler type is no longer incomplete we don't need to
-  // remember it anymore.
+  // Now that the compiler type is complete, we don't need to remember it
+  // anymore and can remove the CTF record type.
   m_compiler_types.erase(compiler_type.GetOpaqueQualType());
+  m_ctf_types.erase(ctf_type->uid);
 
   return true;
 }
@@ -727,9 +727,8 @@
 llvm::Expected> type_or_error =
 ParseType(type_offset, type_uid);
 if (type_or_error) {
-  m_ctf_types.emplace_back(std::move(*type_or_error));
+  m_ctf_types[(*type_or_error)->uid] = std::move(*type_or_error);
 } else {
-  m_ctf_types.emplace_back(std::unique_ptr());
   LLDB_LOG_ERROR(log, type_or_error.takeError(),
  "Failed to parse type {1} at offset {2}: {0}", type_uid,
  type_offset);
@@ -982,16 +981,16 @@
 }
 
 lldb_private::Type *SymbolFileCTF::ResolveTypeUID(lldb::user_id_t type_uid) {
-  auto find_result = m_types.find(type_uid);
-  if (find_result != m_types.end())
-return find_result->second.get();
+  auto type_it = m_types.find(type_uid);
+  if (type_it != m_types.end())
+return type_it->second.get();
 
-  if (type_uid == 0 || type_uid > m_ctf_types.size())
+  auto ctf_type_it = m_ctf_types.find(type_uid);
+  if (ctf_type_it == m_ctf_types.end())
 return nullptr;
 
-  CTFType *ctf_type = m_ctf_types[type_uid - 1].get();
-  if (!ctf_type)
-return nullptr;
+  CTFType *ctf_type = ctf_type_it->second.get();
+  assert(ctf_type && "m_ctf_types should only contain valid CTF types");
 
   Log *log = GetLog(LLDBLog::Symbols);
 
@@ -1013,6 +1012,11 @@
 
   m_types[type_uid] = type_sp;
 
+  // Except for record types which we'll need to complete later, we don't need
+  // the CTF type anymore.
+  if (!isa(ctf_type))
+m_ctf_types.erase(type_uid);
+
   return type_sp.get();
 }
 
Index: lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
===
--- lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
+++ lldb/source/Plugins/SymbolFile/CTF/CTFTypes.h
@@ -46,6 +46,8 @@
  uint32_t encoding)
   : CTFType(eInteger, uid, name), bits(bits), encoding(encoding) {}
 
+  static bool classof(const CTFType *T) { return T->kind == eInteger; }
+
   uint32_t bits;
   uint32_t encoding;
 };
@@ -55,6 +57,11 @@
   CTFModifier(Kind kind, lldb::user_id_t uid, uint32_t type)
   : CTFType(kind, uid, ""), type(type) {}
 
+  static bool classof(const CTFType *T) {
+return T->kind == ePointer || T->kind == eConst || T->kind == eVolatile ||
+   T->kind == eRestrict;
+  }
+
 public:
   uint32_t type;
 };
@@ -62,27 +69,36 @@
 struct CTFPo

[Lldb-commits] [lldb] 12f3d97 - [lldb] Support recursive record types in CTF

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-07-29T22:32:24-07:00
New Revision: 12f3d97fc68b304e0efbe183665c0183d9a372b3

URL: 
https://github.com/llvm/llvm-project/commit/12f3d97fc68b304e0efbe183665c0183d9a372b3
DIFF: 
https://github.com/llvm/llvm-project/commit/12f3d97fc68b304e0efbe183665c0183d9a372b3.diff

LOG: [lldb] Support recursive record types in CTF

Support recursive record types in CTF, for example a struct that
contains a pointer to itself:

  struct S {
struct S *n;
  };

We are now more lazy when creating LLDB types. When encountering a
record type (struct or union) we create a forward declaration and only
complete it when requested.

Differential revision: https://reviews.llvm.org/D156498

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
lldb/test/API/macosx/ctf/TestCTF.py
lldb/test/API/macosx/ctf/test.c

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp 
b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
index 1c4de94e06d7b8..55e3a1ddb96165 100644
--- a/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
+++ b/lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
@@ -503,26 +503,59 @@ SymbolFileCTF::CreateFunction(const CTFFunction 
&ctf_function) {
 llvm::Expected
 SymbolFileCTF::CreateRecord(const CTFRecord &ctf_record) {
   const clang::TagTypeKind tag_kind = TranslateRecordKind(ctf_record.kind);
-
   CompilerType record_type =
   m_ast->CreateRecordType(nullptr, OptionalClangModuleID(), eAccessPublic,
   ctf_record.name.data(), tag_kind, 
eLanguageTypeC);
+  m_compiler_types[record_type.GetOpaqueQualType()] = &ctf_record;
+  Declaration decl;
+  return MakeType(ctf_record.uid, ConstString(ctf_record.name), 
ctf_record.size,
+  nullptr, LLDB_INVALID_UID, 
lldb_private::Type::eEncodingIsUID,
+  decl, record_type, 
lldb_private::Type::ResolveState::Forward);
+}
+
+bool SymbolFileCTF::CompleteType(CompilerType &compiler_type) {
+  // Check if we have a CTF type for the given incomplete compiler type.
+  auto it = m_compiler_types.find(compiler_type.GetOpaqueQualType());
+  if (it == m_compiler_types.end())
+return false;
+
+  const CTFType *ctf_type = it->second;
+  assert(ctf_type && "m_compiler_types should only contain valid CTF types");
+
+  // We only support resolving record types.
+  assert(ctf_type->kind == CTFType::Kind::eStruct ||
+ ctf_type->kind == CTFType::Kind::eUnion);
 
-  m_ast->StartTagDeclarationDefinition(record_type);
-  for (const CTFRecord::Field &field : ctf_record.fields) {
-if (Type *field_type = ResolveTypeUID(field.type)) {
-  const uint32_t field_size = field_type->GetByteSize(nullptr).value_or(0);
-  TypeSystemClang::AddFieldToRecordType(record_type, field.name,
-field_type->GetFullCompilerType(),
-eAccessPublic, field_size);
+  // Cast to the appropriate CTF type.
+  const CTFRecord *ctf_record = static_cast(ctf_type);
+
+  // If any of the fields are incomplete, we cannot complete the type.
+  for (const CTFRecord::Field &field : ctf_record->fields) {
+if (!ResolveTypeUID(field.type)) {
+  LLDB_LOG(GetLog(LLDBLog::Symbols),
+   "Cannot complete type {0} because field {1} is incomplete",
+   ctf_type->uid, field.type);
+  return false;
 }
   }
-  m_ast->CompleteTagDeclarationDefinition(record_type);
 
-  Declaration decl;
-  return MakeType(ctf_record.uid, ConstString(ctf_record.name), 
ctf_record.size,
-  nullptr, LLDB_INVALID_UID, 
lldb_private::Type::eEncodingIsUID,
-  decl, record_type, lldb_private::Type::ResolveState::Full);
+  // Complete the record type.
+  m_ast->StartTagDeclarationDefinition(compiler_type);
+  for (const CTFRecord::Field &field : ctf_record->fields) {
+Type *field_type = ResolveTypeUID(field.type);
+assert(field_type && "field must be complete");
+const uint32_t field_size = field_type->GetByteSize(nullptr).value_or(0);
+TypeSystemClang::AddFieldToRecordType(compiler_type, field.name,
+  field_type->GetFullCompilerType(),
+  eAccessPublic, field_size);
+  }
+  m_ast->CompleteTagDeclarationDefinition(compiler_type);
+
+  // Now that the compiler type is no longer incomplete we don't need to
+  // remember it anymore.
+  m_compiler_types.erase(compiler_type.GetOpaqueQualType());
+
+  return true;
 }
 
 llvm::Expected
@@ -960,7 +993,6 @@ lldb_private::Type 
*SymbolFileCTF::ResolveTypeUID(lldb::user_id_t type_uid) {
   if (!ctf_type)
 return nullptr;
 
-  m_types[type_uid] = TypeSP();
   Log *log = GetLog(LLDBLog::Symbols);
 
   llvm::Expected type_or_error = CreateType(ctf_type);

diff  

[Lldb-commits] [PATCH] D156498: [lldb] Support recursive record types in CTF

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 2 inline comments as done.
Closed by commit rG12f3d97fc68b: [lldb] Support recursive record types in CTF 
(authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D156498?vs=544989&id=545404#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156498

Files:
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
  lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
  lldb/test/API/macosx/ctf/TestCTF.py
  lldb/test/API/macosx/ctf/test.c

Index: lldb/test/API/macosx/ctf/test.c
===
--- lldb/test/API/macosx/ctf/test.c
+++ lldb/test/API/macosx/ctf/test.c
@@ -36,9 +36,14 @@
   int b;
 };
 
+struct RecursiveStruct {
+  struct RecursiveStruct *n;
+};
+
 MyStructT foo;
 struct ForwardDecl *forward;
 struct LargeStruct bar;
+struct RecursiveStruct ke;
 
 void populate(MyInt i) {
   foo.n.i = i;
@@ -52,6 +57,7 @@
   foo.f = NULL;
   forward = NULL;
   bar.b = i;
+  ke.n = NULL;
 }
 
 int main(int argc, char** argv) {
Index: lldb/test/API/macosx/ctf/TestCTF.py
===
--- lldb/test/API/macosx/ctf/TestCTF.py
+++ lldb/test/API/macosx/ctf/TestCTF.py
@@ -91,3 +91,5 @@
 "}",
 ],
 )
+
+self.expect("type lookup RecursiveStruct", substrs=["RecursiveStruct *n;"])
Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.h
@@ -93,7 +93,7 @@
 return std::nullopt;
   }
 
-  bool CompleteType(CompilerType &compiler_type) override { return false; }
+  bool CompleteType(CompilerType &compiler_type) override;
 
   uint32_t ResolveSymbolContext(const lldb_private::Address &so_addr,
 lldb::SymbolContextItem resolve_scope,
@@ -247,6 +247,11 @@
 
   std::vector> m_ctf_types;
 
+  /// To complete types, we need a way to map (imcomplete) compiler types back
+  /// to parsed CTF types.
+  llvm::DenseMap
+  m_compiler_types;
+
   llvm::DenseMap m_types;
 
   std::vector m_functions;
Index: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
===
--- lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
+++ lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp
@@ -503,26 +503,59 @@
 llvm::Expected
 SymbolFileCTF::CreateRecord(const CTFRecord &ctf_record) {
   const clang::TagTypeKind tag_kind = TranslateRecordKind(ctf_record.kind);
-
   CompilerType record_type =
   m_ast->CreateRecordType(nullptr, OptionalClangModuleID(), eAccessPublic,
   ctf_record.name.data(), tag_kind, eLanguageTypeC);
+  m_compiler_types[record_type.GetOpaqueQualType()] = &ctf_record;
+  Declaration decl;
+  return MakeType(ctf_record.uid, ConstString(ctf_record.name), ctf_record.size,
+  nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID,
+  decl, record_type, lldb_private::Type::ResolveState::Forward);
+}
+
+bool SymbolFileCTF::CompleteType(CompilerType &compiler_type) {
+  // Check if we have a CTF type for the given incomplete compiler type.
+  auto it = m_compiler_types.find(compiler_type.GetOpaqueQualType());
+  if (it == m_compiler_types.end())
+return false;
+
+  const CTFType *ctf_type = it->second;
+  assert(ctf_type && "m_compiler_types should only contain valid CTF types");
+
+  // We only support resolving record types.
+  assert(ctf_type->kind == CTFType::Kind::eStruct ||
+ ctf_type->kind == CTFType::Kind::eUnion);
 
-  m_ast->StartTagDeclarationDefinition(record_type);
-  for (const CTFRecord::Field &field : ctf_record.fields) {
-if (Type *field_type = ResolveTypeUID(field.type)) {
-  const uint32_t field_size = field_type->GetByteSize(nullptr).value_or(0);
-  TypeSystemClang::AddFieldToRecordType(record_type, field.name,
-field_type->GetFullCompilerType(),
-eAccessPublic, field_size);
+  // Cast to the appropriate CTF type.
+  const CTFRecord *ctf_record = static_cast(ctf_type);
+
+  // If any of the fields are incomplete, we cannot complete the type.
+  for (const CTFRecord::Field &field : ctf_record->fields) {
+if (!ResolveTypeUID(field.type)) {
+  LLDB_LOG(GetLog(LLDBLog::Symbols),
+   "Cannot complete type {0} because field {1} is incomplete",
+   ctf_type->uid, field.type);
+  return false;
 }
   }
-  m_ast->CompleteTagDeclara