Skip to content

Commit 5ab9fa4

Browse files
TeemperorTeemperor
committed
[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
1 parent 4194ca8 commit 5ab9fa4

File tree

5 files changed

+78
-27
lines changed

5 files changed

+78
-27
lines changed

‎lldb/include/lldb/Symbol/ClangASTContext.h

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,16 +145,24 @@ class ClangASTContext : public TypeSystem {
145145

146146
static bool GetCompleteDecl(clang::ASTContext *ast, clang::Decl *decl);
147147

148-
void SetMetadataAsUserID(const void *object, lldb::user_id_t user_id);
148+
void SetMetadataAsUserID(const clang::Decl *decl, lldb::user_id_t user_id);
149+
void SetMetadataAsUserID(const clang::Type *type, lldb::user_id_t user_id);
149150

150-
void SetMetadata(const void *object, ClangASTMetadata &meta_data);
151+
void SetMetadata(const clang::Decl *object, ClangASTMetadata &meta_data);
152+
void SetMetadata(const clang::Type *object, ClangASTMetadata &meta_data);
153+
ClangASTMetadata *GetMetadata(const clang::Decl *object) {
154+
return GetMetadata(getASTContext(), object);
155+
}
156+
157+
static ClangASTMetadata *GetMetadata(clang::ASTContext *ast,
158+
const clang::Decl *object);
151159

152-
ClangASTMetadata *GetMetadata(const void *object) {
160+
ClangASTMetadata *GetMetadata(const clang::Type *object) {
153161
return GetMetadata(getASTContext(), object);
154162
}
155163

156164
static ClangASTMetadata *GetMetadata(clang::ASTContext *ast,
157-
const void *object);
165+
const clang::Type *object);
158166

159167
// Basic Types
160168
CompilerType GetBuiltinTypeForEncodingAndBitSize(lldb::Encoding encoding,
@@ -487,7 +495,7 @@ class ClangASTContext : public TypeSystem {
487495
DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc);
488496

489497
static ClangASTMetadata *DeclContextGetMetaData(const CompilerDeclContext &dc,
490-
const void *object);
498+
const clang::Decl *object);
491499

492500
static clang::ASTContext *
493501
DeclContextGetClangASTContext(const CompilerDeclContext &dc);

‎lldb/include/lldb/Symbol/ClangExternalASTSourceCommon.h

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,15 +126,26 @@ class ClangExternalASTSourceCommon : public clang::ExternalASTSource {
126126
ClangExternalASTSourceCommon();
127127
~ClangExternalASTSourceCommon() override;
128128

129-
ClangASTMetadata *GetMetadata(const void *object);
130-
void SetMetadata(const void *object, ClangASTMetadata &metadata);
129+
ClangASTMetadata *GetMetadata(const clang::Decl *object);
130+
void SetMetadata(const clang::Decl *object,
131+
const ClangASTMetadata &metadata) {
132+
m_decl_metadata[object] = metadata;
133+
}
134+
135+
ClangASTMetadata *GetMetadata(const clang::Type *object);
136+
void SetMetadata(const clang::Type *object,
137+
const ClangASTMetadata &metadata) {
138+
m_type_metadata[object] = metadata;
139+
}
131140

132141
static ClangExternalASTSourceCommon *Lookup(clang::ExternalASTSource *source);
133142

134143
private:
135-
typedef llvm::DenseMap<const void *, ClangASTMetadata> MetadataMap;
144+
typedef llvm::DenseMap<const clang::Decl *, ClangASTMetadata> DeclMetadataMap;
145+
typedef llvm::DenseMap<const clang::Type *, ClangASTMetadata> TypeMetadataMap;
136146

137-
MetadataMap m_metadata;
147+
DeclMetadataMap m_decl_metadata;
148+
TypeMetadataMap m_type_metadata;
138149
};
139150

140151
} // namespace lldb_private

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1336,7 +1336,8 @@ TypeSP DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
13361336
dwarf->GetUID(type_die), Type::eEncodingIsUID, &attrs.decl, clang_type,
13371337
Type::ResolveState::Full);
13381338
type_sp->SetEncodingType(element_type);
1339-
m_ast.SetMetadataAsUserID(clang_type.GetOpaqueQualType(), die.GetID());
1339+
const clang::Type *type = ClangUtil::GetQualType(clang_type).getTypePtr();
1340+
m_ast.SetMetadataAsUserID(type, die.GetID());
13401341
return type_sp;
13411342
}
13421343

‎lldb/source/Symbol/ClangASTContext.cpp

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2398,14 +2398,31 @@ bool ClangASTContext::GetCompleteDecl(clang::ASTContext *ast,
23982398
}
23992399
}
24002400

2401-
void ClangASTContext::SetMetadataAsUserID(const void *object,
2401+
void ClangASTContext::SetMetadataAsUserID(const clang::Decl *decl,
24022402
user_id_t user_id) {
24032403
ClangASTMetadata meta_data;
24042404
meta_data.SetUserID(user_id);
2405-
SetMetadata(object, meta_data);
2405+
SetMetadata(decl, meta_data);
24062406
}
24072407

2408-
void ClangASTContext::SetMetadata(const void *object,
2408+
void ClangASTContext::SetMetadataAsUserID(const clang::Type *type,
2409+
user_id_t user_id) {
2410+
ClangASTMetadata meta_data;
2411+
meta_data.SetUserID(user_id);
2412+
SetMetadata(type, meta_data);
2413+
}
2414+
2415+
void ClangASTContext::SetMetadata(const clang::Decl *object,
2416+
ClangASTMetadata &metadata) {
2417+
ClangExternalASTSourceCommon *external_source =
2418+
ClangExternalASTSourceCommon::Lookup(
2419+
getASTContext()->getExternalSource());
2420+
2421+
if (external_source)
2422+
external_source->SetMetadata(object, metadata);
2423+
}
2424+
2425+
void ClangASTContext::SetMetadata(const clang::Type *object,
24092426
ClangASTMetadata &metadata) {
24102427
ClangExternalASTSourceCommon *external_source =
24112428
ClangExternalASTSourceCommon::Lookup(
@@ -2416,14 +2433,23 @@ void ClangASTContext::SetMetadata(const void *object,
24162433
}
24172434

24182435
ClangASTMetadata *ClangASTContext::GetMetadata(clang::ASTContext *ast,
2419-
const void *object) {
2436+
const clang::Decl *object) {
24202437
ClangExternalASTSourceCommon *external_source =
24212438
ClangExternalASTSourceCommon::Lookup(ast->getExternalSource());
24222439

24232440
if (external_source)
24242441
return external_source->GetMetadata(object);
2425-
else
2426-
return nullptr;
2442+
return nullptr;
2443+
}
2444+
2445+
ClangASTMetadata *ClangASTContext::GetMetadata(clang::ASTContext *ast,
2446+
const clang::Type *object) {
2447+
ClangExternalASTSourceCommon *external_source =
2448+
ClangExternalASTSourceCommon::Lookup(ast->getExternalSource());
2449+
2450+
if (external_source)
2451+
return external_source->GetMetadata(object);
2452+
return nullptr;
24272453
}
24282454

24292455
bool ClangASTContext::SetTagTypeKind(clang::QualType tag_qual_type,
@@ -5096,7 +5122,7 @@ GetDynamicArrayInfo(ClangASTContext &ast, SymbolFile *sym_file,
50965122
clang::QualType qual_type,
50975123
const ExecutionContext *exe_ctx) {
50985124
if (qual_type->isIncompleteArrayType())
5099-
if (auto *metadata = ast.GetMetadata(qual_type.getAsOpaquePtr()))
5125+
if (auto *metadata = ast.GetMetadata(qual_type.getTypePtr()))
51005126
return sym_file->GetDynamicArrayInfoForUID(metadata->GetUserID(),
51015127
exe_ctx);
51025128
return llvm::None;
@@ -8856,8 +8882,11 @@ void ClangASTContext::DumpSummary(lldb::opaque_compiler_type_t type,
88568882
void ClangASTContext::DumpTypeDescription(lldb::opaque_compiler_type_t type) {
88578883
StreamFile s(stdout, false);
88588884
DumpTypeDescription(type, &s);
8885+
8886+
CompilerType ct(this, type);
8887+
const clang::Type *clang_type = ClangUtil::GetQualType(ct).getTypePtr();
88598888
ClangASTMetadata *metadata =
8860-
ClangASTContext::GetMetadata(getASTContext(), type);
8889+
ClangASTContext::GetMetadata(getASTContext(), clang_type);
88618890
if (metadata) {
88628891
metadata->Dump(&s);
88638892
}
@@ -9485,7 +9514,7 @@ ClangASTContext::DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc) {
94859514

94869515
ClangASTMetadata *
94879516
ClangASTContext::DeclContextGetMetaData(const CompilerDeclContext &dc,
9488-
const void *object) {
9517+
const Decl *object) {
94899518
clang::ASTContext *ast = DeclContextGetClangASTContext(dc);
94909519
if (ast)
94919520
return ClangASTContext::GetMetadata(ast, object);

‎lldb/source/Symbol/ClangExternalASTSourceCommon.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,19 @@ ClangExternalASTSourceCommon::~ClangExternalASTSourceCommon() {
5252
}
5353

5454
ClangASTMetadata *
55-
ClangExternalASTSourceCommon::GetMetadata(const void *object) {
56-
auto It = m_metadata.find(object);
57-
if (It != m_metadata.end())
55+
ClangExternalASTSourceCommon::GetMetadata(const clang::Decl *object) {
56+
auto It = m_decl_metadata.find(object);
57+
if (It != m_decl_metadata.end())
5858
return &It->second;
59-
else
60-
return nullptr;
59+
return nullptr;
6160
}
6261

63-
void ClangExternalASTSourceCommon::SetMetadata(const void *object,
64-
ClangASTMetadata &metadata) {
65-
m_metadata[object] = metadata;
62+
ClangASTMetadata *
63+
ClangExternalASTSourceCommon::GetMetadata(const clang::Type *object) {
64+
auto It = m_type_metadata.find(object);
65+
if (It != m_type_metadata.end())
66+
return &It->second;
67+
return nullptr;
6668
}
6769

6870
void ClangASTMetadata::Dump(Stream *s) {

0 commit comments

Comments
 (0)