[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<const CTFRecord *>(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