[Lldb-commits] [lldb] 5ab9fa4 - [lldb][NFC] Make metadata tracking type safe
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Fri Dec 13 03:05:26 PST 2019
Author: Raphael Isemann
Date: 2019-12-13T12:04:42+01:00
New Revision: 5ab9fa44cd60d5bca7b6d809a86bf10be416eb5d
URL: https://github.com/llvm/llvm-project/commit/5ab9fa44cd60d5bca7b6d809a86bf10be416eb5d
DIFF: https://github.com/llvm/llvm-project/commit/5ab9fa44cd60d5bca7b6d809a86bf10be416eb5d.diff
LOG: [lldb][NFC] Make metadata tracking type safe
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:
```lang=cpp
// 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.
Reviewers: labath, shafik, aprantl
Reviewed By: labath
Subscribers: JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D71409
Added:
Modified:
lldb/include/lldb/Symbol/ClangASTContext.h
lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Symbol/ClangASTContext.cpp
lldb/source/Symbol/ClangExternalASTSourceCommon.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Symbol/ClangASTContext.h b/lldb/include/lldb/Symbol/ClangASTContext.h
index 43d42105c936..9537f33b3386 100644
--- a/lldb/include/lldb/Symbol/ClangASTContext.h
+++ b/lldb/include/lldb/Symbol/ClangASTContext.h
@@ -145,16 +145,24 @@ class ClangASTContext : public TypeSystem {
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,
@@ -487,7 +495,7 @@ class ClangASTContext : public TypeSystem {
DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc);
static ClangASTMetadata *DeclContextGetMetaData(const CompilerDeclContext &dc,
- const void *object);
+ const clang::Decl *object);
static clang::ASTContext *
DeclContextGetClangASTContext(const CompilerDeclContext &dc);
diff --git a/lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h b/lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h
index 1d40080033a1..e7dd94d28286 100644
--- a/lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h
+++ b/lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h
@@ -126,15 +126,26 @@ class ClangExternalASTSourceCommon : public clang::ExternalASTSource {
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
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 002d373a5d99..0e16153a2387 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1336,7 +1336,8 @@ TypeSP DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
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;
}
diff --git a/lldb/source/Symbol/ClangASTContext.cpp b/lldb/source/Symbol/ClangASTContext.cpp
index 33fdee0dad76..2576a372076c 100644
--- a/lldb/source/Symbol/ClangASTContext.cpp
+++ b/lldb/source/Symbol/ClangASTContext.cpp
@@ -2398,14 +2398,31 @@ bool ClangASTContext::GetCompleteDecl(clang::ASTContext *ast,
}
}
-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(
@@ -2416,14 +2433,23 @@ void ClangASTContext::SetMetadata(const void *object,
}
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);
- else
- return nullptr;
+ 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);
+ return nullptr;
}
bool ClangASTContext::SetTagTypeKind(clang::QualType tag_qual_type,
@@ -5096,7 +5122,7 @@ 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;
@@ -8856,8 +8882,11 @@ void ClangASTContext::DumpSummary(lldb::opaque_compiler_type_t type,
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);
}
@@ -9485,7 +9514,7 @@ ClangASTContext::DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc) {
ClangASTMetadata *
ClangASTContext::DeclContextGetMetaData(const CompilerDeclContext &dc,
- const void *object) {
+ const Decl *object) {
clang::ASTContext *ast = DeclContextGetClangASTContext(dc);
if (ast)
return ClangASTContext::GetMetadata(ast, object);
diff --git a/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp b/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp
index b60b3791aae1..66fb4f3b3f05 100644
--- a/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp
+++ b/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp
@@ -52,17 +52,19 @@ ClangExternalASTSourceCommon::~ClangExternalASTSourceCommon() {
}
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;
+ 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) {
More information about the lldb-commits
mailing list