[lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

[augusto2112]

Summary

Storing raw pointers in DieToTypePtr may cause use-after-frees to occur,
since there are no guarantees that the shared pointers that owns the
underlying pointer to the type are kept around as long as the map.
Change the map to store a shared pointer instead.

[augusto2112]

I changed the value of the map to a shared pointer, but could change it to a weak pointer instead, not sure which is more appropriate in this case.

[JDevlieghere]

In D141318#4037541, @augusto2112 wrote:

I changed the value of the map to a shared pointer, but could change it to a weak pointer instead, not sure which is more appropriate in this case.

I was about to ask the same thing. What currently determines the lifetime of the stored objects?

[augusto2112]

In D141318#4037557, @JDevlieghere wrote:

In D141318#4037541, @augusto2112 wrote:

I changed the value of the map to a shared pointer, but could change it to a weak pointer instead, not sure which is more appropriate in this case.

I was about to ask the same thing. What currently determines the lifetime of the stored objects?

Currently, a bunch of functions in DWARFASTParserClang and SymbolFileDWARF will create a new Type SP, store the underlying pointer in the map, and return the SP. The lifetime of the stored objects varies by whatever the caller does with the returned SP. Looking at some of the callers, I don't see a very clear pattern, most seem to use it and immediately discard it (use after free when we look up the same type again and the underlying pointer is still in the map), but at least one caller (SymbolFileDWARF::ResolveType) returns the underlying pointer to its callers, which would still be UB even if we changed the map to store weak pointers. Based on that I'm thinking we should store them as shared pointers instead of weak ones.

[JDevlieghere]

In D141318#4037640, @augusto2112 wrote:

In D141318#4037557, @JDevlieghere wrote:

In D141318#4037541, @augusto2112 wrote:

I changed the value of the map to a shared pointer, but could change it to a weak pointer instead, not sure which is more appropriate in this case.

I was about to ask the same thing. What currently determines the lifetime of the stored objects?

Currently, a bunch of functions in DWARFASTParserClang and SymbolFileDWARF will create a new Type SP, store the underlying pointer in the map, and return the SP. The lifetime of the stored objects varies by whatever the caller does with the returned SP. Looking at some of the callers, I don't see a very clear pattern, most seem to use it and immediately discard it (use after free when we look up the same type again and the underlying pointer is still in the map), but at least one caller (SymbolFileDWARF::ResolveType) returns the underlying pointer to its callers, which would still be UB even if we changed the map to store weak pointers. Based on that I'm thinking we should store them as shared pointers instead of weak ones.

Then maybe the solution is to store the type as unique pointers in the map and hand out raw pointers, relying on the map to keep the object alive? I feel pretty uncomfortable with handing out shared pointers without clearly understanding its lifetime cycle.

[augusto2112]

In D141318#4037687, @JDevlieghere wrote:

In D141318#4037640, @augusto2112 wrote:

In D141318#4037557, @JDevlieghere wrote:

In D141318#4037541, @augusto2112 wrote:

I changed the value of the map to a shared pointer, but could change it to a weak pointer instead, not sure which is more appropriate in this case.

I was about to ask the same thing. What currently determines the lifetime of the stored objects?

Currently, a bunch of functions in DWARFASTParserClang and SymbolFileDWARF will create a new Type SP, store the underlying pointer in the map, and return the SP. The lifetime of the stored objects varies by whatever the caller does with the returned SP. Looking at some of the callers, I don't see a very clear pattern, most seem to use it and immediately discard it (use after free when we look up the same type again and the underlying pointer is still in the map), but at least one caller (SymbolFileDWARF::ResolveType) returns the underlying pointer to its callers, which would still be UB even if we changed the map to store weak pointers. Based on that I'm thinking we should store them as shared pointers instead of weak ones.

Then maybe the solution is to store the type as unique pointers in the map and hand out raw pointers, relying on the map to keep the object alive? I feel pretty uncomfortable with handing out shared pointers without clearly understanding its lifetime cycle.

Yeah that seems like a better solution. I'm checking how hard implementing that would be. TypeSP is used in a bunch of places in LLDB.

[clayborg]

Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:

TypeSP type_sp(... /* create type here*/ ...);
dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list

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

201–206

Above the "type_sp" is immediately stored in the SymbolFile's type list. So we don't need to do anything here and it _is_ safe to just store a "Type *".

[clayborg]

Storing raw pointers in DieToTypePtr may cause use-after-frees to occur, since there are no guarantees that the shared pointers that owns the underlying pointer to the type are kept around as long as the map. Change the map to store a shared pointer instead.

This isn't true. SymbolFile has a m_type_list that is used to store all shared pointers to any types that are created. Since SymbolFileDWARF is an instance of SymbolFile, it is safe to store "Type *" anywhere inside this class because as along as the SymbolFileDWARF is around, so will the SymbolFile that owns all of the types. If there are any places where someone is creating a type and _not_ storing it in the SymbolFile::m_type_list, then this is the bug and that should be fixed.

[augusto2112]

In D141318#4045372, @clayborg wrote:

Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:

TypeSP type_sp(... /* create type here*/ ...);
dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list

What triggered the use after free for me was this snippet in DWARFASTParserClang::ParseTypeModifier:

  ...
  TypeSP lldb_function_type_sp = ParseTypeFromDWARF(
      sc, function_type, &function_type_is_new_pointer);

  if (lldb_function_type_sp) {
    clang_type = m_ast.CreateBlockPointerType(
        lldb_function_type_sp->GetForwardCompilerType());
    encoding_data_type = Type::eEncodingIsUID;
    attrs.type.Clear();
    resolve_state = Type::ResolveState::Full;
  }
}

lldb_function_type_sp is created, the underlying pointer is placed in the map, the SP is destroyed at the end of the scope, and any lookups for that type will get back the dangling pointer.

I could update this particular instance, but this just seems like a really easy mistake to make... There's nothing to suggest to callers that they must update the m_type_list when they ask for a new type. Wouldn't it be better we prevent this somehow, by updating the raw pointers of the map by being either unique, shared or weak pointers?

[clayborg]

In D141318#4045459, @augusto2112 wrote:

In D141318#4045372, @clayborg wrote:

Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:

TypeSP type_sp(... /* create type here*/ ...);
dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list

What triggered the use after free for me was this snippet in DWARFASTParserClang::ParseTypeModifier:

  ...
  TypeSP lldb_function_type_sp = ParseTypeFromDWARF(
      sc, function_type, &function_type_is_new_pointer);

  if (lldb_function_type_sp) {
    clang_type = m_ast.CreateBlockPointerType(
        lldb_function_type_sp->GetForwardCompilerType());
    encoding_data_type = Type::eEncodingIsUID;
    attrs.type.Clear();
    resolve_state = Type::ResolveState::Full;
  }
}

lldb_function_type_sp is created, the underlying pointer is placed in the map, the SP is destroyed at the end of the scope, and any lookups for that type will get back the dangling pointer.

I could update this particular instance, but this just seems like a really easy mistake to make... There's nothing to suggest to callers that they must update the m_type_list when they ask for a new type. Wouldn't it be better we prevent this somehow, by updating the raw pointers of the map by being either unique, shared or weak pointers?

We really need the SymbolFile to own (have a strong reference to) all types that each SymbolFile subclass creates. One idea would be to make the lldb_private::Type's constructor protected and then friend class the SymbolFile class only, and then add an accessor where all types must be created via a SymbolFile API. Right now anyone can create a type with:

TypeSP type_sp(new Type(...));

This could turn into:

TypeSP type_sp(symbol_file->CreateType(...));

The main issue is shared pointer and weak pointer objects are more expensive objects as they are two pointers each and we need to keep these extra maps, like the the dwarf->GetDIEToType() as small as they can be. We also need to the type to not disappear as it needs to be owned by the SymbolFile classes. Not sure if anyone is creating types outside of the SymbolFile or subclasses.

So my requirement is that all types created by SymbolFile objects must have a single strong reference to the type in SymbolFile::m_type_list. Right now this isn't enforced, but it would be great if it was.

[augusto2112]

Make Type constructor private

[augusto2112]

@clayborg I went with your suggestion and made Type's constructor private, and any new Type created is automatically added to the TypeList. I think later on we should make TypeList keep a vector of unique pointers instead of shared ones.

[clayborg]

New changes looks great, thanks for tackling this. Just a few comments that need to be updated and this will be good to go as long as the test suite passes just fine!

In D141318#4052934, @augusto2112 wrote:

@clayborg I went with your suggestion and made Type's constructor private, and any new Type created is automatically added to the TypeList. I think later on we should make TypeList keep a vector of unique pointers instead of shared ones.

If we did switch the unique pointers, then we would always need to hand out "Type *" and this could cause crashes and use after free errors. So I would hold off on that change.

lldb/include/lldb/Symbol/SymbolFile.h

414

Just add a comment here saying something lie "This function is used to create types that belong to a SymbolFile. The symbol file will own a strong reference to the type in an internal type list."

lldb/include/lldb/Symbol/Type.h

229–230

add a \see SymbolFileCommon::MakeType() reference in the header documentation here so users will know what function to use if the get a compile error.

lldb/include/lldb/Symbol/SymbolFile.h

public:
   virtual void SetDebugInfoIndexWasSavedToCache() = 0;
   /// \}
 
   /// Accessors for the bool that indicates if there was debug info, but errors
   /// stopped variables from being able to be displayed correctly. See
   /// GetFrameVariableError() for details on what are considered errors.
   virtual bool GetDebugInfoHadFrameVariableErrors() const = 0;
   virtual void SetDebugInfoHadFrameVariableErrors() = 0;
 
+  virtual lldb::TypeSP
+  MakeType(lldb::user_id_t uid, ConstString name,
+           std::optional<uint64_t> byte_size, SymbolContextScope *context,
+           lldb::user_id_t encoding_uid,
+           Type::EncodingDataType encoding_uid_type, const Declaration &decl,
+           const CompilerType &compiler_qual_type,
+           Type::ResolveState compiler_type_resolve_state,
+           uint32_t opaque_payload = 0) = 0;
+
 protected:
   void AssertModuleLock();
 
 private:
   SymbolFile(const SymbolFile &) = delete;
   const SymbolFile &operator=(const SymbolFile &) = delete;
 };
 
public:
   }
   bool GetDebugInfoHadFrameVariableErrors() const override {
     return m_debug_info_had_variable_errors;
   }
   void SetDebugInfoHadFrameVariableErrors() override {
      m_debug_info_had_variable_errors = true;
   }
 
+  lldb::TypeSP MakeType(lldb::user_id_t uid, 
+                        ConstString name, std::optional<uint64_t> byte_size,
+                        SymbolContextScope *context,
+                        lldb::user_id_t encoding_uid,
+                        Type::EncodingDataType encoding_uid_type,
+                        const Declaration &decl,
+                        const CompilerType &compiler_qual_type,
+                        Type::ResolveState compiler_type_resolve_state,
+                        uint32_t opaque_payload = 0) override {
+     lldb::TypeSP type_sp (new Type(
+         uid, this, name, byte_size, context, encoding_uid,
+         encoding_uid_type, decl, compiler_qual_type,
+         compiler_type_resolve_state, opaque_payload));
+     m_type_list.Insert(type_sp);
+     return type_sp;
+  }
+
 protected:
   virtual uint32_t CalculateNumCompileUnits() = 0;
   virtual lldb::CompUnitSP ParseCompileUnitAtIndex(uint32_t idx) = 0;
   virtual TypeList &GetTypeList() { return m_type_list; }
   void SetCompileUnitAtIndex(uint32_t idx, const lldb::CompUnitSP &cu_sp);
 
   lldb::ObjectFileSP m_objfile_sp; // Keep a reference to the object file in
                                    // case it isn't the same as the module

lldb/include/lldb/Symbol/SymbolFileOnDemand.h

public:
   }
   bool GetDebugInfoHadFrameVariableErrors() const override {
     return m_sym_file_impl->GetDebugInfoHadFrameVariableErrors();
   }
   void SetDebugInfoHadFrameVariableErrors() override {
     return m_sym_file_impl->SetDebugInfoHadFrameVariableErrors();
   }
 
+  lldb::TypeSP MakeType(lldb::user_id_t uid, ConstString name,
+                        std::optional<uint64_t> byte_size,
+                        SymbolContextScope *context,
+                        lldb::user_id_t encoding_uid,
+                        Type::EncodingDataType encoding_uid_type,
+                        const Declaration &decl,
+                        const CompilerType &compiler_qual_type,
+                        Type::ResolveState compiler_type_resolve_state,
+                        uint32_t opaque_payload = 0) override {
+    return m_sym_file_impl->MakeType(
+        uid, name, byte_size, context, encoding_uid, encoding_uid_type, decl,
+        compiler_qual_type, compiler_type_resolve_state, opaque_payload);
+  }
+
 private:
   Log *GetLog() const { return ::lldb_private::GetLog(LLDBLog::OnDemand); }
 
   ConstString GetSymbolFileName() {
     return GetObjectFile()->GetFileSpec().GetFilename();
   }
 
 private:
   bool m_debug_info_enabled = false;
   bool m_preload_symbols = false;
   std::unique_ptr<SymbolFile> m_sym_file_impl;
 };
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_SYMBOLFILEONDEMAND_H

lldb/include/lldb/Symbol/Type.h

 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/APSInt.h"
 
 #include <optional>
 #include <set>
 
 namespace lldb_private {
+class SymbolFileCommon;
 
 /// CompilerContext allows an array of these items to be passed to perform
 /// detailed lookups in SymbolVendor and SymbolFile functions.
 struct CompilerContext {
   CompilerContext(CompilerContextKind t, ConstString n) : kind(t), name(n) {}
 
   bool operator==(const CompilerContext &rhs) const {
     return kind == rhs.kind && name == rhs.name;
public:
 
   enum class ResolveState : unsigned char {
     Unresolved = 0,
     Forward = 1,
     Layout = 2,
     Full = 3
   };
 
-  Type(lldb::user_id_t uid, SymbolFile *symbol_file, ConstString name,
-       std::optional<uint64_t> byte_size, SymbolContextScope *context,
-       lldb::user_id_t encoding_uid, EncodingDataType encoding_uid_type,
-       const Declaration &decl, const CompilerType &compiler_qual_type,
-       ResolveState compiler_type_resolve_state, uint32_t opaque_payload = 0);
-
-  // This makes an invalid type.  Used for functions that return a Type when
-  // they get an error.
-  Type();
-
   void Dump(Stream *s, bool show_context,
             lldb::DescriptionLevel level = lldb::eDescriptionLevelFull);
 
   void DumpTypeName(Stream *s);
 
   /// Since Type instances only keep a "SymbolFile *" internally, other classes
   /// like TypeImpl need make sure the module is still around before playing
   /// with
protected:
   CompilerType m_compiler_type;
   ResolveState m_compiler_type_resolve_state = ResolveState::Unresolved;
   /// Language-specific flags.
   Payload m_payload;
 
   Type *GetEncodingType();
 
   bool ResolveCompilerType(ResolveState compiler_type_resolve_state);
+private:
+  /// Only allow Symbol File to create types, as they should own them by keeping
+  /// them in their TypeList.
+  friend class lldb_private::SymbolFileCommon;
+
+  Type(lldb::user_id_t uid, SymbolFile *symbol_file, ConstString name,
+       std::optional<uint64_t> byte_size, SymbolContextScope *context,
+       lldb::user_id_t encoding_uid, EncodingDataType encoding_uid_type,
+       const Declaration &decl, const CompilerType &compiler_qual_type,
+       ResolveState compiler_type_resolve_state, uint32_t opaque_payload = 0);
+
+  // This makes an invalid type.  Used for functions that return a Type when
+  // they get an error.
+  Type();
 };
 
 // the two classes here are used by the public API as a backend to the SBType
 // and SBTypeList classes
 
 class TypeImpl {
 public:
   TypeImpl() = default;

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

TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
   // inside type_sp and only looks at IsDefined(), so it never calls
   // ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(),
   // which does extra work for Objective-C classes. This would result
   // in only the forward declaration to be visible.
   if (pcm_type.IsDefined())
     GetClangASTImporter().RequireCompleteType(ClangUtil::GetQualType(type));
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
-  TypeSP type_sp(new Type(die.GetID(), dwarf, pcm_type_sp->GetName(),
-                          pcm_type_sp->GetByteSize(nullptr), nullptr,
-                          LLDB_INVALID_UID, Type::eEncodingInvalid,
-                          &pcm_type_sp->GetDeclaration(), type,
-                          Type::ResolveState::Forward,
-                          TypePayloadClang(GetOwningClangModule(die))));
-
-  dwarf->GetTypeList().Insert(type_sp);
+  auto type_sp = dwarf->MakeType(
+      die.GetID(), pcm_type_sp->GetName(), pcm_type_sp->GetByteSize(nullptr),
+      nullptr, LLDB_INVALID_UID, Type::eEncodingInvalid,
+      &pcm_type_sp->GetDeclaration(), type, Type::ResolveState::Forward,
+      TypePayloadClang(GetOwningClangModule(die)));
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   clang::TagDecl *tag_decl = TypeSystemClang::GetAsTagDecl(type);
   if (tag_decl) {
     LinkDeclContextToDIE(tag_decl, die);
   } else {
     clang::DeclContext *defn_decl_ctx = GetCachedClangDeclContextForDIE(die);
     if (defn_decl_ctx)
       LinkDeclContextToDIE(defn_decl_ctx, die);
   }
if (cu_language == eLanguageTypeObjC ||
             attrs.type.Clear();
             resolve_state = Type::ResolveState::Full;
           }
         }
       }
     }
   }
 
-  type_sp = std::make_shared<Type>(
-      die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
+  type_sp = dwarf->MakeType(
+      die.GetID(), attrs.name, attrs.byte_size, nullptr,
       dwarf->GetUID(attrs.type.Reference()), encoding_data_type, &attrs.decl,
       clang_type, resolve_state, TypePayloadClang(GetOwningClangModule(die)));
 
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   return type_sp;
 }
 
 ConstString
clang_type = m_ast.CreateEnumerationType(
         GetOwningClangModule(die), attrs.decl, enumerator_clang_type,
         attrs.is_scoped_enum);
   } else {
     enumerator_clang_type = m_ast.GetEnumerationIntegerType(clang_type);
   }
 
   LinkDeclContextToDIE(TypeSystemClang::GetDeclContextForType(clang_type), die);
 
-  type_sp = std::make_shared<Type>(
-      die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
-      dwarf->GetUID(attrs.type.Reference()), Type::eEncodingIsUID, &attrs.decl,
-      clang_type, Type::ResolveState::Forward,
+  type_sp = dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr,
+                            dwarf->GetUID(attrs.type.Reference()),
+                            Type::eEncodingIsUID, &attrs.decl, clang_type,
+                            Type::ResolveState::Forward,
                             TypePayloadClang(GetOwningClangModule(die)));
 
   if (TypeSystemClang::StartTagDeclarationDefinition(clang_type)) {
     if (die.HasChildren()) {
       bool is_signed = false;
       enumerator_clang_type.IsIntegerType(is_signed);
       ParseChildEnumerators(clang_type, is_signed,
                             type_sp->GetByteSize(nullptr).value_or(0), die);
     }
if (!type_handled) {
                       object_pointer_name.c_str(),
                       static_cast<void *>(function_decl));
           }
           m_ast.SetMetadata(function_decl, metadata);
         }
       }
     }
   }
-  return std::make_shared<Type>(
-      die.GetID(), dwarf, attrs.name, std::nullopt, nullptr, LLDB_INVALID_UID,
+  return dwarf->MakeType(
+      die.GetID(), attrs.name, std::nullopt, nullptr, LLDB_INVALID_UID,
       Type::eEncodingIsUID, &attrs.decl, clang_type, Type::ResolveState::Full);
 }
 
 TypeSP
 DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
                                     const ParsedDWARFTypeAttributes &attrs) {
   SymbolFileDWARF *dwarf = die.GetDWARF();
 
for (auto pos = array_info->element_orders.rbegin(); pos != end; ++pos) {
                                      ? array_element_bit_stride * num_elements
                                      : array_element_bit_stride;
     }
   } else {
     clang_type =
         m_ast.CreateArrayType(array_element_type, 0, attrs.is_vector);
   }
   ConstString empty_name;
-  TypeSP type_sp = std::make_shared<Type>(
-      die.GetID(), dwarf, empty_name, array_element_bit_stride / 8, nullptr,
-      dwarf->GetUID(type_die), Type::eEncodingIsUID, &attrs.decl, clang_type,
-      Type::ResolveState::Full);
+  TypeSP type_sp =
+      dwarf->MakeType(die.GetID(), empty_name, array_element_bit_stride / 8,
+                      nullptr, dwarf->GetUID(type_die), Type::eEncodingIsUID,
+                      &attrs.decl, clang_type, Type::ResolveState::Full);
   type_sp->SetEncodingType(element_type);
   const clang::Type *type = ClangUtil::GetQualType(clang_type).getTypePtr();
   m_ast.SetMetadataAsUserID(type, die.GetID());
   return type_sp;
 }
 
 TypeSP DWARFASTParserClang::ParsePointerToMemberType(
     const DWARFDIE &die, const ParsedDWARFTypeAttributes &attrs) {
   SymbolFileDWARF *dwarf = die.GetDWARF();
   Type *pointee_type = dwarf->ResolveTypeUID(attrs.type.Reference(), true);
   Type *class_type =
       dwarf->ResolveTypeUID(attrs.containing_type.Reference(), true);
 
   CompilerType pointee_clang_type = pointee_type->GetForwardCompilerType();
   CompilerType class_clang_type = class_type->GetForwardCompilerType();
 
   CompilerType clang_type = TypeSystemClang::CreateMemberPointerType(
       class_clang_type, pointee_clang_type);
 
   if (std::optional<uint64_t> clang_type_size =
           clang_type.GetByteSize(nullptr)) {
-    return std::make_shared<Type>(die.GetID(), dwarf, attrs.name,
-                                  *clang_type_size, nullptr, LLDB_INVALID_UID,
-                                  Type::eEncodingIsUID, nullptr, clang_type,
-                                  Type::ResolveState::Forward);
+    return dwarf->MakeType(die.GetID(), attrs.name, *clang_type_size, nullptr,
+                           LLDB_INVALID_UID, Type::eEncodingIsUID, nullptr,
+                           clang_type, Type::ResolveState::Forward);
   }
   return nullptr;
 }
 
 void DWARFASTParserClang::ParseInheritance(
     const DWARFDIE &die, const DWARFDIE &parent_die,
     const CompilerType class_clang_type, const AccessType default_accessibility,
     const lldb::ModuleSP &module_sp,
if (!clang_type_was_created) {
           &metadata, attrs.exports_symbols);
     }
   }
 
   // Store a forward declaration to this class type in case any
   // parameters in any class methods need it for the clang types for
   // function prototypes.
   LinkDeclContextToDIE(m_ast.GetDeclContextForType(clang_type), die);
-  type_sp = std::make_shared<Type>(
-      die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
-      LLDB_INVALID_UID, Type::eEncodingIsUID, &attrs.decl, clang_type,
+  type_sp = dwarf->MakeType(
+      die.GetID(), attrs.name, attrs.byte_size, nullptr, LLDB_INVALID_UID,
+      Type::eEncodingIsUID, &attrs.decl, clang_type,
       Type::ResolveState::Forward,
       TypePayloadClang(OptionalClangModuleID(), attrs.is_complete_objc_class));
 
   // Add our type to the unique type map so we don't end up creating many
   // copies of the same type over and over in the ASTContext for our
   // module
   unique_ast_entry_up->m_type_sp = type_sp;
   unique_ast_entry_up->m_die = die;

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

if (!ts)
     return {};
 
   DWARFASTParser *dwarf_ast = ts->GetDWARFParser();
   if (!dwarf_ast)
     return {};
 
   TypeSP type_sp = dwarf_ast->ParseTypeFromDWARF(sc, die, type_is_new_ptr);
   if (type_sp) {
-    GetTypeList().Insert(type_sp);
-
     if (die.Tag() == DW_TAG_subprogram) {
       std::string scope_qualified_name(GetDeclContextForUID(die.GetID())
                                            .GetScopeQualifiedName()
                                            .AsCString(""));
       if (scope_qualified_name.size()) {
         m_function_scope_qualified_name_map[scope_qualified_name].insert(
             *die.GetDIERef());
       }

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

lldb::TypeSP SymbolFileNativePDB::CreateModifierType(PdbTypeSymId type_id,
   std::string name;
   if (mr.ModifiedType.isSimple())
     name = std::string(GetSimpleTypeName(mr.ModifiedType.getSimpleKind()));
   else
     name = computeTypeName(stream.typeCollection(), mr.ModifiedType);
   Declaration decl;
   lldb::TypeSP modified_type = GetOrCreateType(mr.ModifiedType);
 
-  return std::make_shared<Type>(toOpaqueUid(type_id), this, ConstString(name),
-                                modified_type->GetByteSize(nullptr), nullptr,
-                                LLDB_INVALID_UID, Type::eEncodingIsUID, decl,
-                                ct, Type::ResolveState::Full);
+  return MakeType(toOpaqueUid(type_id), ConstString(name),
+                  LLDB_INVALID_UID, Type::eEncodingIsUID, decl, ct,
+                  Type::ResolveState::Full);
 }
 
 lldb::TypeSP
 SymbolFileNativePDB::CreatePointerType(PdbTypeSymId type_id,
                                        const llvm::codeview::PointerRecord &pr,
                                        CompilerType ct) {
   TypeSP pointee = GetOrCreateType(pr.ReferentType);
   if (!pointee)
     return nullptr;
 
   if (pr.isPointerToMember()) {
     MemberPointerInfo mpi = pr.getMemberInfo();
     GetOrCreateType(mpi.ContainingType);
   }
 
   Declaration decl;
-  return std::make_shared<Type>(toOpaqueUid(type_id), this, ConstString(),
-                                pr.getSize(), nullptr, LLDB_INVALID_UID,
-                                Type::eEncodingIsUID, decl, ct,
-                                Type::ResolveState::Full);
+  return MakeType(toOpaqueUid(type_id), ConstString(), pr.getSize(), nullptr,
+                  LLDB_INVALID_UID, Type::eEncodingIsUID, decl, ct,
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateSimpleType(TypeIndex ti,
                                                    CompilerType ct) {
   uint64_t uid = toOpaqueUid(PdbTypeSymId(ti, false));
   if (ti == TypeIndex::NullptrT()) {
     Declaration decl;
-    return std::make_shared<Type>(
-        uid, this, ConstString("std::nullptr_t"), 0, nullptr, LLDB_INVALID_UID,
-        Type::eEncodingIsUID, decl, ct, Type::ResolveState::Full);
+    return MakeType(uid, ConstString("std::nullptr_t"), 0, nullptr,
+                    LLDB_INVALID_UID, Type::eEncodingIsUID, decl, ct,
+                    Type::ResolveState::Full);
   }
 
   if (ti.getSimpleMode() != SimpleTypeMode::Direct) {
     TypeSP direct_sp = GetOrCreateType(ti.makeDirect());
     uint32_t pointer_size = 0;
     switch (ti.getSimpleMode()) {
     case SimpleTypeMode::FarPointer32:
     case SimpleTypeMode::NearPointer32:
       pointer_size = 4;
       break;
     case SimpleTypeMode::NearPointer64:
       pointer_size = 8;
       break;
     default:
       // 128-bit and 16-bit pointers unsupported.
       return nullptr;
     }
     Declaration decl;
-    return std::make_shared<Type>(
-        uid, this, ConstString(), pointer_size, nullptr, LLDB_INVALID_UID,
+    return MakeType(uid, ConstString(), pointer_size, nullptr, LLDB_INVALID_UID,
                     Type::eEncodingIsUID, decl, ct, Type::ResolveState::Full);
   }
 
   if (ti.getSimpleKind() == SimpleTypeKind::NotTranslated)
     return nullptr;
 
   size_t size = GetTypeSizeForSimpleKind(ti.getSimpleKind());
   llvm::StringRef type_name = GetSimpleTypeName(ti.getSimpleKind());
 
   Declaration decl;
-  return std::make_shared<Type>(uid, this, ConstString(type_name), size,
-                                nullptr, LLDB_INVALID_UID, Type::eEncodingIsUID,
-                                decl, ct, Type::ResolveState::Full);
+  return MakeType(uid, ConstString(type_name), size, nullptr, LLDB_INVALID_UID,
+                  Type::eEncodingIsUID, decl, ct, Type::ResolveState::Full);
 }
 
 static std::string GetUnqualifiedTypeName(const TagRecord &record) {
   if (!record.hasUniqueName()) {
     MSVCUndecoratedNameParser parser(record.Name);
     llvm::ArrayRef<MSVCUndecoratedNameSpecifier> specs = parser.GetSpecifiers();
 
     return std::string(specs.back().GetBaseName());

 SymbolFileNativePDB::CreateClassStructUnion(PdbTypeSymId type_id,
                                             const TagRecord &record,
                                             size_t size, CompilerType ct) {
 
   std::string uname = GetUnqualifiedTypeName(record);
 
   // FIXME: Search IPI stream for LF_UDT_MOD_SRC_LINE.
   Declaration decl;
-  return std::make_shared<Type>(toOpaqueUid(type_id), this, ConstString(uname),
-                                size, nullptr, LLDB_INVALID_UID,
-                                Type::eEncodingIsUID, decl, ct,
-                                Type::ResolveState::Forward);
+  return MakeType(toOpaqueUid(type_id), ConstString(uname), size, nullptr,
+                  LLDB_INVALID_UID, Type::eEncodingIsUID, decl, ct,
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateTagType(PdbTypeSymId type_id,
                                                 const ClassRecord &cr,
                                                 CompilerType ct) {
   return CreateClassStructUnion(type_id, cr, cr.getSize(), ct);
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateTagType(PdbTypeSymId type_id,
                                                 const UnionRecord &ur,
                                                 CompilerType ct) {
   return CreateClassStructUnion(type_id, ur, ur.getSize(), ct);
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateTagType(PdbTypeSymId type_id,
                                                 const EnumRecord &er,
                                                 CompilerType ct) {
   std::string uname = GetUnqualifiedTypeName(er);
 
   Declaration decl;
   TypeSP underlying_type = GetOrCreateType(er.UnderlyingType);
 
-  return std::make_shared<lldb_private::Type>(
-      toOpaqueUid(type_id), this, ConstString(uname),
-      underlying_type->GetByteSize(nullptr), nullptr, LLDB_INVALID_UID,
-      lldb_private::Type::eEncodingIsUID, decl, ct,
-      lldb_private::Type::ResolveState::Forward);
+  return MakeType(toOpaqueUid(type_id), ConstString(uname),
+                  underlying_type->GetByteSize(nullptr), nullptr,
+                  LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+                  ct, lldb_private::Type::ResolveState::Forward);
 }
 
 TypeSP SymbolFileNativePDB::CreateArrayType(PdbTypeSymId type_id,
                                             const ArrayRecord &ar,
                                             CompilerType ct) {
   TypeSP element_type = GetOrCreateType(ar.ElementType);
 
   Declaration decl;
-  TypeSP array_sp = std::make_shared<lldb_private::Type>(
-      toOpaqueUid(type_id), this, ConstString(), ar.Size, nullptr,
+  TypeSP array_sp =
+      MakeType(toOpaqueUid(type_id), ConstString(), ar.Size, nullptr,
                LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl, ct,
                lldb_private::Type::ResolveState::Full);
   array_sp->SetEncodingType(element_type.get());
   return array_sp;
 }
 
-
 TypeSP SymbolFileNativePDB::CreateFunctionType(PdbTypeSymId type_id,
                                                const MemberFunctionRecord &mfr,
                                                CompilerType ct) {
   Declaration decl;
-  return std::make_shared<lldb_private::Type>(
-      toOpaqueUid(type_id), this, ConstString(), 0, nullptr, LLDB_INVALID_UID,
-      lldb_private::Type::eEncodingIsUID, decl, ct,
-      lldb_private::Type::ResolveState::Full);
+  return MakeType(toOpaqueUid(type_id), ConstString(), 0, nullptr,
+                  LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+                  ct, lldb_private::Type::ResolveState::Full);
 }
 
 TypeSP SymbolFileNativePDB::CreateProcedureType(PdbTypeSymId type_id,
                                                 const ProcedureRecord &pr,
                                                 CompilerType ct) {
   Declaration decl;
-  return std::make_shared<lldb_private::Type>(
-      toOpaqueUid(type_id), this, ConstString(), 0, nullptr, LLDB_INVALID_UID,
-      lldb_private::Type::eEncodingIsUID, decl, ct,
-      lldb_private::Type::ResolveState::Full);
+  return MakeType(toOpaqueUid(type_id), ConstString(), 0, nullptr,
+                  LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+                  ct, lldb_private::Type::ResolveState::Full);
 }
 
 TypeSP SymbolFileNativePDB::CreateType(PdbTypeSymId type_id, CompilerType ct) {
   if (type_id.index.isSimple())
     return CreateSimpleType(type_id.index, ct);
 
   TpiStream &stream = type_id.is_ipi ? m_index->ipi() : m_index->tpi();
   CVType cvt = stream.getType(type_id.index);
if (auto err = ts_or_err.takeError())
     return nullptr;
   auto ts = *ts_or_err;
   if (!ts)
     return nullptr;
 
   ts->GetNativePDBParser()->GetOrCreateTypedefDecl(id);
 
   Declaration decl;
-  return std::make_shared<lldb_private::Type>(
-      toOpaqueUid(id), this, ConstString(udt.Name),
-      target_type->GetByteSize(nullptr), nullptr, target_type->GetID(),
-      lldb_private::Type::eEncodingIsTypedefUID, decl,
-      target_type->GetForwardCompilerType(),
+  return MakeType(
+      toOpaqueUid(id), ConstString(udt.Name), target_type->GetByteSize(nullptr),
+      nullptr, target_type->GetID(), lldb_private::Type::eEncodingIsTypedefUID,
+      decl, target_type->GetForwardCompilerType(),
       lldb_private::Type::ResolveState::Forward);
 }
 
 TypeSP SymbolFileNativePDB::GetOrCreateTypedef(PdbGlobalSymId id) {
   auto iter = m_types.find(toOpaqueUid(id));
   if (iter != m_types.end())
     return iter->second;
 

lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

case PDB_SymType::UDT: {
 
     if (udt->isConstType())
       clang_type = clang_type.AddConstModifier();
 
     if (udt->isVolatileType())
       clang_type = clang_type.AddVolatileModifier();
 
     GetDeclarationForSymbol(type, decl);
-    return std::make_shared<lldb_private::Type>(
-        type.getSymIndexId(), m_ast.GetSymbolFile(), ConstString(name),
-        udt->getLength(), nullptr, LLDB_INVALID_UID,
-        lldb_private::Type::eEncodingIsUID, decl, clang_type,
+    return m_ast.GetSymbolFile()->MakeType(
+        type.getSymIndexId(), ConstString(name), udt->getLength(), nullptr,
+        LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl, clang_type,
         type_resolve_state);
   } break;
   case PDB_SymType::Enum: {
     auto enum_type = llvm::dyn_cast<PDBSymbolTypeEnum>(&type);
     assert(enum_type);
 
     std::string name =
         std::string(MSVCUndecoratedNameParser::DropScope(enum_type->getName()));
case PDB_SymType::Enum: {
 
     if (enum_type->isConstType())
       ast_enum = ast_enum.AddConstModifier();
 
     if (enum_type->isVolatileType())
       ast_enum = ast_enum.AddVolatileModifier();
 
     GetDeclarationForSymbol(type, decl);
-    return std::make_shared<lldb_private::Type>(
-        type.getSymIndexId(), m_ast.GetSymbolFile(), ConstString(name), bytes,
-        nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
-        ast_enum, lldb_private::Type::ResolveState::Full);
+    return m_ast.GetSymbolFile()->MakeType(
+        type.getSymIndexId(), ConstString(name), bytes, nullptr,
+        LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl, ast_enum,
+        lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::Typedef: {
     auto type_def = llvm::dyn_cast<PDBSymbolTypeTypedef>(&type);
     assert(type_def);
 
     SymbolFile *symbol_file = m_ast.GetSymbolFile();
     if (!symbol_file)
       return nullptr;
case PDB_SymType::Typedef: {
 
     if (type_def->isVolatileType())
       ast_typedef = ast_typedef.AddVolatileModifier();
 
     GetDeclarationForSymbol(type, decl);
     std::optional<uint64_t> size;
     if (type_def->getLength())
       size = type_def->getLength();
-    return std::make_shared<lldb_private::Type>(
-        type_def->getSymIndexId(), m_ast.GetSymbolFile(), ConstString(name),
-        size, nullptr, target_type->GetID(),
-        lldb_private::Type::eEncodingIsTypedefUID, decl, ast_typedef,
-        lldb_private::Type::ResolveState::Full);
+    return m_ast.GetSymbolFile()->MakeType(
+        type_def->getSymIndexId(), ConstString(name), size, nullptr,
+        target_type->GetID(), lldb_private::Type::eEncodingIsTypedefUID, decl,
+        ast_typedef, lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::Function:
   case PDB_SymType::FunctionSig: {
     std::string name;
     PDBSymbolTypeFunctionSig *func_sig = nullptr;
     if (auto pdb_func = llvm::dyn_cast<PDBSymbolFunc>(&type)) {
       if (pdb_func->isCompilerGenerated())
         return nullptr;
case PDB_SymType::FunctionSig: {
     if (func_sig->isVolatileType())
       type_quals |= clang::Qualifiers::Volatile;
     auto cc = TranslateCallingConvention(func_sig->getCallingConvention());
     CompilerType func_sig_ast_type =
         m_ast.CreateFunctionType(return_ast_type, arg_list.data(),
                                  arg_list.size(), is_variadic, type_quals, cc);
 
     GetDeclarationForSymbol(type, decl);
-    return std::make_shared<lldb_private::Type>(
-        type.getSymIndexId(), m_ast.GetSymbolFile(), ConstString(name),
-        std::nullopt, nullptr, LLDB_INVALID_UID,
-        lldb_private::Type::eEncodingIsUID, decl, func_sig_ast_type,
-        lldb_private::Type::ResolveState::Full);
+    return m_ast.GetSymbolFile()->MakeType(
+        type.getSymIndexId(), ConstString(name), std::nullopt, nullptr,
+        LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+        func_sig_ast_type, lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::ArrayType: {
     auto array_type = llvm::dyn_cast<PDBSymbolTypeArray>(&type);
     assert(array_type);
     uint32_t num_elements = array_type->getCount();
     uint32_t element_uid = array_type->getElementTypeId();
     std::optional<uint64_t> bytes;
     if (uint64_t size = array_type->getLength())
if (TypeSystemClang::IsCXXClassType(element_ast_type) &&
         TypeSystemClang::CompleteTagDeclarationDefinition(element_ast_type);
       } else {
         // We are not able to start definition.
         return nullptr;
       }
     }
     CompilerType array_ast_type = m_ast.CreateArrayType(
         element_ast_type, num_elements, /*is_gnu_vector*/ false);
-    TypeSP type_sp = std::make_shared<lldb_private::Type>(
-        array_type->getSymIndexId(), m_ast.GetSymbolFile(), ConstString(),
-        bytes, nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID,
-        decl, array_ast_type, lldb_private::Type::ResolveState::Full);
+    TypeSP type_sp = m_ast.GetSymbolFile()->MakeType(
+        array_type->getSymIndexId(), ConstString(), bytes, nullptr,
+        LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+        array_ast_type, lldb_private::Type::ResolveState::Full);
     type_sp->SetEncodingType(element_type);
     return type_sp;
   } break;
   case PDB_SymType::BuiltinType: {
     auto *builtin_type = llvm::dyn_cast<PDBSymbolTypeBuiltin>(&type);
     assert(builtin_type);
     PDB_BuiltinType builtin_kind = builtin_type->getBuiltinType();
     if (builtin_kind == PDB_BuiltinType::None)
case PDB_SymType::BuiltinType: {
     if (builtin_type->isConstType())
       builtin_ast_type = builtin_ast_type.AddConstModifier();
 
     if (builtin_type->isVolatileType())
       builtin_ast_type = builtin_ast_type.AddVolatileModifier();
 
     auto type_name = GetPDBBuiltinTypeName(*builtin_type, builtin_ast_type);
 
-    return std::make_shared<lldb_private::Type>(
-        builtin_type->getSymIndexId(), m_ast.GetSymbolFile(), type_name, bytes,
-        nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+    return m_ast.GetSymbolFile()->MakeType(
+        builtin_type->getSymIndexId(), type_name, bytes, nullptr,
+        LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
         builtin_ast_type, lldb_private::Type::ResolveState::Full);
   } break;
   case PDB_SymType::PointerType: {
     auto *pointer_type = llvm::dyn_cast<PDBSymbolTypePointer>(&type);
     assert(pointer_type);
 
     SymbolFile *symbol_file = m_ast.GetSymbolFile();
     if (!symbol_file)
if (pointer_type->isPointerToDataMember() ||
       assert(class_parent_type);
 
       CompilerType pointer_ast_type;
       pointer_ast_type = TypeSystemClang::CreateMemberPointerType(
           class_parent_type->GetLayoutCompilerType(),
           pointee_type->GetForwardCompilerType());
       assert(pointer_ast_type);
 
-      return std::make_shared<lldb_private::Type>(
-          pointer_type->getSymIndexId(), m_ast.GetSymbolFile(), ConstString(),
+      return m_ast.GetSymbolFile()->MakeType(
+          pointer_type->getSymIndexId(), ConstString(),
           pointer_type->getLength(), nullptr, LLDB_INVALID_UID,
           lldb_private::Type::eEncodingIsUID, decl, pointer_ast_type,
           lldb_private::Type::ResolveState::Forward);
     }
 
     CompilerType pointer_ast_type;
     pointer_ast_type = pointee_type->GetFullCompilerType();
     if (pointer_type->isReference())
       pointer_ast_type = pointer_ast_type.GetLValueReferenceType();
     else if (pointer_type->isRValueReference())
       pointer_ast_type = pointer_ast_type.GetRValueReferenceType();
     else
       pointer_ast_type = pointer_ast_type.GetPointerType();
 
     if (pointer_type->isConstType())
       pointer_ast_type = pointer_ast_type.AddConstModifier();
 
     if (pointer_type->isVolatileType())
       pointer_ast_type = pointer_ast_type.AddVolatileModifier();
 
     if (pointer_type->isRestrictedType())
       pointer_ast_type = pointer_ast_type.AddRestrictModifier();
 
-    return std::make_shared<lldb_private::Type>(
-        pointer_type->getSymIndexId(), m_ast.GetSymbolFile(), ConstString(),
-        pointer_type->getLength(), nullptr, LLDB_INVALID_UID,
-        lldb_private::Type::eEncodingIsUID, decl, pointer_ast_type,
-        lldb_private::Type::ResolveState::Full);
+    return m_ast.GetSymbolFile()->MakeType(
+        pointer_type->getSymIndexId(), ConstString(), pointer_type->getLength(),
+        nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+        pointer_ast_type, lldb_private::Type::ResolveState::Full);
   } break;
   default:
     break;
   }
   return nullptr;
 }
 
 bool PDBASTParser::CompleteTypeFromPDB(

lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

lldb_private::Type *SymbolFilePDB::ResolveTypeUID(lldb::user_id_t type_uid) {
 
   auto pdb_type = m_session_up->getSymbolById(type_uid);
   if (pdb_type == nullptr)
     return nullptr;
 
   lldb::TypeSP result = pdb->CreateLLDBTypeFromPDBType(*pdb_type);
   if (result) {
     m_types.insert(std::make_pair(type_uid, result));
-    GetTypeList().Insert(result);
   }
   return result.get();
 }
 
 std::optional<SymbolFile::ArrayInfo> SymbolFilePDB::GetDynamicArrayInfoForUID(
     lldb::user_id_t type_uid, const lldb_private::ExecutionContext *exe_ctx) {
   return std::nullopt;
 }

lldb/unittests/Symbol/TestTypeSystemClang.cpp

EXPECT_EQ(
       ScratchTypeSystemClang::InferIsolatedASTKindFromLangOpts(lang_opts));
 
   lang_opts.Modules = true;
   EXPECT_EQ(
       ScratchTypeSystemClang::IsolatedASTKind::CppModules,
       ScratchTypeSystemClang::InferIsolatedASTKindFromLangOpts(lang_opts));
 }
 
-TEST_F(TestTypeSystemClang, GetExeModuleWhenMissingSymbolFile) {
-  CompilerType compiler_type = m_ast->GetBasicTypeFromAST(lldb::eBasicTypeInt);
-  lldb_private::Type t(0, nullptr, ConstString("MyType"), std::nullopt, nullptr,
-                       0, {}, {}, compiler_type,
-                       lldb_private::Type::ResolveState::Full);
-  // Test that getting the execution module when no type system is present
-  // is handled gracefully.
-  ModuleSP module = t.GetExeModule();
-  EXPECT_EQ(module.get(), nullptr);
+TEST_F(TestTypeSystemClang, GetDeclContextByNameWhenMissingSymbolFile) {
+  // Test that a type system without a symbol file is handled gracefully.
+  std::vector<CompilerDecl> decls =
+      m_ast->DeclContextFindDeclByName(nullptr, ConstString("SomeName"), true);
+
+  EXPECT_TRUE(decls.empty());
 }