[lldb][NFC] Make metadata tracking type safe

[teemperor]

Summary

LLDB associates additional information with Types and Declarations which it calls ClangASTMetadata.
ClangASTMetadata is stored by the ClangASTSourceCommon which is implemented by having a large map of
void * keys to associated ClangASTMetadata values. To make this whole mechanism even unsafer
we also decided to use clang::Decl * as one of pointers we throw in there (beside clang::Type *).

The Decl class hierarchy uses multiple inheritance which means that not all pointers have the
same address when they are implicitly converted to pointers of their parent classes. For example
clang::Decl * and clang::DeclContext * won't end up being the same address when they
are implicitly converted from one of the many Decl-subclasses that inherit from both.

As we use the addresses as the keys in our Metadata map, this means that any implicit type
conversions to parent classes (or anything else that changes the addresses) will break our metadata tracking
in obscure ways.

Just to illustrate how broken this whole mechanism currently is:

// m_ast is our ClangASTContext. Let's double check that from GetTranslationUnitDecl
// in ClangASTContext and ASTContext return the same thing (one method just calls the other).
assert(m_ast->GetTranslationUnitDecl() == m_ast->getASTContext()->getTranslationUnitDecl());
// Ok, both methods have the same TU*. Let's store metadata with the result of one method call.
m_ast->SetMetadataAsUserID(m_ast->GetTranslationUnitDecl(), 1234U);
// Retrieve the same Metadata for the TU by using the TU* from the other method... which fails?
EXPECT_EQ(m_ast->GetMetadata(m_ast->getASTContext()->getTranslationUnitDecl())->GetUserID(), 1234U);
// Turns out that getTranslationUnitDecl one time returns a TranslationUnitDecl* but the other time
// we return one of the parent classes of TranslationUnitDecl (DeclContext).

This patch splits up the void * API into two where one does the clang::Type * tracking and one the clang::Decl * mapping.
Type and Decl are disjoint class hierarchies so there is no implicit conversion possible that could influence
the address values.

I had to change the storing of clang::QualType opaque pointers to their clang::Type * equivalents as
opaque pointers are already void * pointers to begin with. We don't seem to ever set any qualifier in any of these
QualTypes to this conversion should be NFC.

[teemperor]

(I'm aware that a lot of the touched code here could be improved, but I would prefer that I at least can trust my compiler before I refactor the remaining Metadata stuff...)

[labath]

Yes this is definitely better. If storing metadata for QualTypes is important (my impression is it's the opposite), then you could do that (while maintaining a decent amount of type safety) by having the [GS]etMetadata functions take a QualType, and converting it to void* only as an implementation detail..

[aprantl]

lldb/source/Symbol/ClangExternalASTSourceCommon.cpp

64

.lookup(), assuming this is a DenseMap?

[teemperor]

lldb/source/Symbol/ClangExternalASTSourceCommon.cpp

64

We can't use it here as we return a pointer to the internally stored value in the map and not a value :(

lldb/include/lldb/Symbol/ClangASTContext.h

public:
   static void DumpDeclHiearchy(clang::Decl *decl);
 
   static void DumpDeclContextHiearchy(clang::DeclContext *decl_ctx);
 
   static bool DeclsAreEquivalent(clang::Decl *lhs_decl, clang::Decl *rhs_decl);
 
   static bool GetCompleteDecl(clang::ASTContext *ast, clang::Decl *decl);
 
-  void SetMetadataAsUserID(const void *object, lldb::user_id_t user_id);
+  void SetMetadataAsUserID(const clang::Decl *decl, lldb::user_id_t user_id);
+  void SetMetadataAsUserID(const clang::Type *type, lldb::user_id_t user_id);
 
-  void SetMetadata(const void *object, ClangASTMetadata &meta_data);
+  void SetMetadata(const clang::Decl *object, ClangASTMetadata &meta_data);
+  void SetMetadata(const clang::Type *object, ClangASTMetadata &meta_data);
+  ClangASTMetadata *GetMetadata(const clang::Decl *object) {
+    return GetMetadata(getASTContext(), object);
+  }
+
+  static ClangASTMetadata *GetMetadata(clang::ASTContext *ast,
+                                       const clang::Decl *object);
 
-  ClangASTMetadata *GetMetadata(const void *object) {
+  ClangASTMetadata *GetMetadata(const clang::Type *object) {
     return GetMetadata(getASTContext(), object);
   }
 
   static ClangASTMetadata *GetMetadata(clang::ASTContext *ast,
-                                       const void *object);
+                                       const clang::Type *object);
 
   // Basic Types
   CompilerType GetBuiltinTypeForEncodingAndBitSize(lldb::Encoding encoding,
                                                    size_t bit_size) override;
 
   CompilerType GetBasicType(lldb::BasicType type);
 
   static lldb::BasicType GetBasicTypeEnumeration(ConstString name);
public:
 
   static clang::FunctionDecl *
   DeclContextGetAsFunctionDecl(const CompilerDeclContext &dc);
 
   static clang::NamespaceDecl *
   DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc);
 
   static ClangASTMetadata *DeclContextGetMetaData(const CompilerDeclContext &dc,
-                                                  const void *object);
+                                                  const clang::Decl *object);
 
   static clang::ASTContext *
   DeclContextGetClangASTContext(const CompilerDeclContext &dc);
 
   // Tests
 
   bool IsArrayType(lldb::opaque_compiler_type_t type,
                    CompilerType *element_type, uint64_t *size,

lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h

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;
 };
 
 class ClangExternalASTSourceCommon : public clang::ExternalASTSource {
 public:
   ClangExternalASTSourceCommon();
   ~ClangExternalASTSourceCommon() override;
 
-  ClangASTMetadata *GetMetadata(const void *object);
-  void SetMetadata(const void *object, ClangASTMetadata &metadata);
+  ClangASTMetadata *GetMetadata(const clang::Decl *object);
+  void SetMetadata(const clang::Decl *object,
+                   const ClangASTMetadata &metadata) {
+    m_decl_metadata[object] = metadata;
+  }
+
+  ClangASTMetadata *GetMetadata(const clang::Type *object);
+  void SetMetadata(const clang::Type *object,
+                   const ClangASTMetadata &metadata) {
+    m_type_metadata[object] = metadata;
+  }
 
   static ClangExternalASTSourceCommon *Lookup(clang::ExternalASTSource *source);
 
 private:
-  typedef llvm::DenseMap<const void *, ClangASTMetadata> MetadataMap;
+  typedef llvm::DenseMap<const clang::Decl *, ClangASTMetadata> DeclMetadataMap;
+  typedef llvm::DenseMap<const clang::Type *, ClangASTMetadata> TypeMetadataMap;
 
-  MetadataMap m_metadata;
+  DeclMetadataMap m_decl_metadata;
+  TypeMetadataMap m_type_metadata;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_ClangExternalASTSourceCommon_h

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

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);
   type_sp->SetEncodingType(element_type);
-  m_ast.SetMetadataAsUserID(clang_type.GetOpaqueQualType(), die.GetID());
+  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 =

lldb/source/Symbol/ClangASTContext.cpp

if (clang::TagDecl *tag_decl = llvm::dyn_cast<clang::TagDecl>(decl)) {
     ast_source->CompleteType(objc_interface_decl);
 
     return !objc_interface_decl->getTypeForDecl()->isIncompleteType();
   } else {
     return false;
   }
 }
 
-void ClangASTContext::SetMetadataAsUserID(const void *object,
+void ClangASTContext::SetMetadataAsUserID(const clang::Decl *decl,
                                           user_id_t user_id) {
   ClangASTMetadata meta_data;
   meta_data.SetUserID(user_id);
-  SetMetadata(object, meta_data);
+  SetMetadata(decl, meta_data);
 }
 
-void ClangASTContext::SetMetadata(const void *object,
+void ClangASTContext::SetMetadataAsUserID(const clang::Type *type,
+                                          user_id_t user_id) {
+  ClangASTMetadata meta_data;
+  meta_data.SetUserID(user_id);
+  SetMetadata(type, meta_data);
+}
+
+void ClangASTContext::SetMetadata(const clang::Decl *object,
+                                  ClangASTMetadata &metadata) {
+  ClangExternalASTSourceCommon *external_source =
+      ClangExternalASTSourceCommon::Lookup(
+          getASTContext()->getExternalSource());
+
+  if (external_source)
+    external_source->SetMetadata(object, metadata);
+}
+
+void ClangASTContext::SetMetadata(const clang::Type *object,
                                   ClangASTMetadata &metadata) {
   ClangExternalASTSourceCommon *external_source =
       ClangExternalASTSourceCommon::Lookup(
           getASTContext()->getExternalSource());
 
   if (external_source)
     external_source->SetMetadata(object, metadata);
 }
 
 ClangASTMetadata *ClangASTContext::GetMetadata(clang::ASTContext *ast,
-                                               const void *object) {
+                                               const clang::Decl *object) {
+  ClangExternalASTSourceCommon *external_source =
+      ClangExternalASTSourceCommon::Lookup(ast->getExternalSource());
+
+  if (external_source)
+    return external_source->GetMetadata(object);
+  return nullptr;
+}
+
+ClangASTMetadata *ClangASTContext::GetMetadata(clang::ASTContext *ast,
+                                               const clang::Type *object) {
   ClangExternalASTSourceCommon *external_source =
       ClangExternalASTSourceCommon::Lookup(ast->getExternalSource());
 
   if (external_source)
     return external_source->GetMetadata(object);
-  else
-    return nullptr;
 }
 
 bool ClangASTContext::SetTagTypeKind(clang::QualType tag_qual_type,
                                      int kind) const {
   const clang::Type *clang_type = tag_qual_type.getTypePtr();
   if (clang_type) {
     const clang::TagType *tag_type = llvm::dyn_cast<clang::TagType>(clang_type);
     if (tag_type) {
static bool ObjCDeclHasIVars(clang::ObjCInterfaceDecl *class_interface_decl,
   return false;
 }
 
 static Optional<SymbolFile::ArrayInfo>
 GetDynamicArrayInfo(ClangASTContext &ast, SymbolFile *sym_file,
                     clang::QualType qual_type,
                     const ExecutionContext *exe_ctx) {
   if (qual_type->isIncompleteArrayType())
-    if (auto *metadata = ast.GetMetadata(qual_type.getAsOpaquePtr()))
+    if (auto *metadata = ast.GetMetadata(qual_type.getTypePtr()))
       return sym_file->GetDynamicArrayInfoForUID(metadata->GetUserID(),
                                                  exe_ctx);
   return llvm::None;
 }
 
 uint32_t ClangASTContext::GetNumChildren(lldb::opaque_compiler_type_t type,
                                          bool omit_empty_base_classes,
                                          const ExecutionContext *exe_ctx) {
if (exe_ctx) {
       }
     }
   }
 }
 
 void ClangASTContext::DumpTypeDescription(lldb::opaque_compiler_type_t type) {
   StreamFile s(stdout, false);
   DumpTypeDescription(type, &s);
+
+  CompilerType ct(this, type);
+  const clang::Type *clang_type = ClangUtil::GetQualType(ct).getTypePtr();
   ClangASTMetadata *metadata =
-      ClangASTContext::GetMetadata(getASTContext(), type);
+      ClangASTContext::GetMetadata(getASTContext(), clang_type);
   if (metadata) {
     metadata->Dump(&s);
   }
 }
 
 void ClangASTContext::DumpTypeDescription(lldb::opaque_compiler_type_t type,
                                           Stream *s) {
   if (type) {
ClangASTContext::DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc) {
   if (IsClangDeclContext(dc))
     return llvm::dyn_cast<clang::NamespaceDecl>(
         (clang::DeclContext *)dc.GetOpaqueDeclContext());
   return nullptr;
 }
 
 ClangASTMetadata *
 ClangASTContext::DeclContextGetMetaData(const CompilerDeclContext &dc,
-                                        const void *object) {
+                                        const Decl *object) {
   clang::ASTContext *ast = DeclContextGetClangASTContext(dc);
   if (ast)
     return ClangASTContext::GetMetadata(ast, object);
   return nullptr;
 }
 
 clang::ASTContext *
 ClangASTContext::DeclContextGetClangASTContext(const CompilerDeclContext &dc) {

lldb/source/Symbol/ClangExternalASTSourceCommon.cpp

 }
 
 ClangExternalASTSourceCommon::~ClangExternalASTSourceCommon() {
   std::unique_lock<std::mutex> guard;
   GetSourceMap(guard).erase(this);
 }
 
 ClangASTMetadata *
-ClangExternalASTSourceCommon::GetMetadata(const void *object) {
-  auto It = m_metadata.find(object);
-  if (It != m_metadata.end())
+ClangExternalASTSourceCommon::GetMetadata(const clang::Decl *object) {
+  auto It = m_decl_metadata.find(object);
+  if (It != m_decl_metadata.end())
     return &It->second;
-  else
-    return nullptr;
 }
 
-void ClangExternalASTSourceCommon::SetMetadata(const void *object,
-                                               ClangASTMetadata &metadata) {
-  m_metadata[object] = metadata;
+ClangASTMetadata *
+ClangExternalASTSourceCommon::GetMetadata(const clang::Type *object) {
+  auto It = m_type_metadata.find(object);
+  if (It != m_type_metadata.end())
+    return &It->second;
+  return nullptr;
 }
 
 void ClangASTMetadata::Dump(Stream *s) {
   lldb::user_id_t uid = GetUserID();
 
   if (uid != LLDB_INVALID_UID) {
     s->Printf("uid=0x%" PRIx64, uid);
   }