[Lldb-commits] [PATCH] D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D141318#4045459 <https://reviews.llvm.org/D141318#4045459>, @augusto2112 wrote:

> In D141318#4045372 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141318