Skip to content

Commit edb4f6e

Browse files
vedantkvedantk
committed
Require SwiftASTContextReader to be valid at all times
Prior to this patch, it was possible to construct an invalid SwiftASTContextReader (e.g. via default-init). This led to subtle bugs: in rdar://65276251 for example, an invalid SwiftASTContextReader was assigned into an llvm::Optional, and subsequent code crashed because it assumed that the loaded llvm::Optional contained a valid instance. Make it so that SwiftASTContextReader is valid at all times. rdar://65276251
1 parent 639cc50 commit edb4f6e

File tree

13 files changed

+95
-57
lines changed

13 files changed

+95
-57
lines changed

‎lldb/include/lldb/Core/SwiftASTContextReader.h

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,26 @@ struct ScopedSharedMutexReader {
9191
/// or even separate scratch contexts for each lldb::Module. But it
9292
/// will only do this if no client holds on to a read lock on \b
9393
/// m_scratch_typesystem_lock.
94-
struct SwiftASTContextReader : ScopedSharedMutexReader {
95-
SwiftASTContextForExpressions *m_ptr = nullptr;
96-
SwiftASTContextReader() : ScopedSharedMutexReader(nullptr) {}
97-
SwiftASTContextReader(SharedMutex &mutex, SwiftASTContextForExpressions *ctx)
98-
: ScopedSharedMutexReader(&mutex), m_ptr(ctx) {}
94+
class SwiftASTContextReader : ScopedSharedMutexReader {
95+
SwiftASTContextForExpressions *m_ptr;
96+
97+
public:
98+
SwiftASTContextReader(SharedMutex &mutex, SwiftASTContextForExpressions &ctx)
99+
: ScopedSharedMutexReader(&mutex), m_ptr(&ctx) {
100+
assert(m_ptr && "invalid context");
101+
}
102+
99103
SwiftASTContextReader(const SwiftASTContextReader &copy)
100104
: ScopedSharedMutexReader(copy.m_mutex), m_ptr(copy.m_ptr) {}
101-
SwiftASTContextForExpressions *get() { return m_ptr; }
102-
operator bool() const { return m_ptr; }
103-
SwiftASTContextForExpressions *operator->() { return m_ptr; }
104-
SwiftASTContextForExpressions &operator*() { return *m_ptr; }
105+
106+
SwiftASTContextForExpressions *get() {
107+
assert(m_ptr && "invalid context");
108+
return m_ptr;
109+
}
110+
111+
SwiftASTContextForExpressions *operator->() { return get(); }
112+
113+
SwiftASTContextForExpressions &operator*() { return *get(); }
105114
};
106115

107116
/// An RAII object that just acquires the reader lock.

‎lldb/include/lldb/Core/ValueObject.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,7 @@ class ValueObject : public UserID {
584584

585585
virtual bool HasSyntheticValue();
586586

587-
SwiftASTContextReader GetScratchSwiftASTContext();
587+
llvm::Optional<SwiftASTContextReader> GetScratchSwiftASTContext();
588588

589589
virtual bool IsSynthetic() { return false; }
590590

‎lldb/include/lldb/Target/Target.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1127,7 +1127,7 @@ class Target : public std::enable_shared_from_this<Target>,
11271127
return m_scratch_typesystem_lock;
11281128
}
11291129

1130-
SwiftASTContextReader
1130+
llvm::Optional<SwiftASTContextReader>
11311131
GetScratchSwiftASTContext(Status &error, ExecutionContextScope &exe_scope,
11321132
bool create_on_demand = true);
11331133

‎lldb/source/Core/ValueObject.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1697,10 +1697,10 @@ LanguageType ValueObject::GetObjectRuntimeLanguage() {
16971697
return lldb::eLanguageTypeUnknown;
16981698
}
16991699

1700-
SwiftASTContextReader ValueObject::GetScratchSwiftASTContext() {
1700+
llvm::Optional<SwiftASTContextReader> ValueObject::GetScratchSwiftASTContext() {
17011701
lldb::TargetSP target_sp(GetTargetSP());
17021702
if (!target_sp)
1703-
return {};
1703+
return llvm::None;
17041704
Status error;
17051705
ExecutionContext ctx = GetExecutionContextRef().Lock(false);
17061706
auto *exe_scope = ctx.GetBestExecutionContextScope();

‎lldb/source/Core/ValueObjectDynamicValue.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,5 +433,7 @@ bool ValueObjectDynamicValue::DynamicValueTypeInfoNeedsUpdate() {
433433
return false;
434434

435435
auto *cached_ctx = m_value.GetCompilerType().GetTypeSystem();
436-
return cached_ctx == GetScratchSwiftASTContext().get();
436+
llvm::Optional<SwiftASTContextReader> scratch_ctx =
437+
GetScratchSwiftASTContext();
438+
return scratch_ctx ? (cached_ctx == scratch_ctx->get()) : !cached_ctx;
437439
}

‎lldb/source/Expression/Materializer.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -927,9 +927,9 @@ class EntityResultVariable : public Materializer::Entity {
927927

928928
if (m_type.GetMinimumLanguage() == lldb::eLanguageTypeSwift) {
929929
Status status;
930-
auto type_system =
931-
target_sp->GetScratchSwiftASTContext(status, *exe_scope).get();
932-
if (type_system == nullptr) {
930+
llvm::Optional<SwiftASTContextReader> maybe_type_system =
931+
target_sp->GetScratchSwiftASTContext(status, *exe_scope);
932+
if (!maybe_type_system) {
933933
err.SetErrorStringWithFormat("Couldn't dematerialize a result variable: "
934934
"couldn't get the corresponding type "
935935
"system: %s", status.AsCString());

‎lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,11 @@ SwiftExpressionParser::SwiftExpressionParser(
128128

129129
if (target_sp) {
130130
Status error;
131-
m_swift_ast_context = std::make_unique<SwiftASTContextReader>(
132-
target_sp->GetScratchSwiftASTContext(error, *exe_scope, true));
131+
llvm::Optional<SwiftASTContextReader> scratch_ctx =
132+
target_sp->GetScratchSwiftASTContext(error, *exe_scope, true);
133+
if (scratch_ctx)
134+
m_swift_ast_context =
135+
std::make_unique<SwiftASTContextReader>(scratch_ctx.getValue());
133136
}
134137
}
135138

‎lldb/source/Plugins/ExpressionParser/Swift/SwiftUserExpression.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,11 +195,13 @@ void SwiftUserExpression::ScanContext(ExecutionContext &exe_ctx, Status &err) {
195195

196196
// Make sure the target's SwiftASTContext has been setup before doing any
197197
// Swift name lookups.
198-
auto swift_ast_ctx = m_target->GetScratchSwiftASTContext(err, *frame);
199-
if (!swift_ast_ctx) {
198+
llvm::Optional<SwiftASTContextReader> maybe_swift_ast_ctx =
199+
m_target->GetScratchSwiftASTContext(err, *frame);
200+
if (!maybe_swift_ast_ctx) {
200201
LLDB_LOG(log, " [SUE::SC] NULL Swift AST Context");
201202
return;
202203
}
204+
SwiftASTContext *swift_ast_ctx = maybe_swift_ast_ctx->get();
203205

204206
if (!swift_ast_ctx->GetClangImporter()) {
205207
LLDB_LOG(log, " [SUE::SC] Swift AST Context has no Clang importer");

‎lldb/source/Plugins/Language/Swift/SwiftHashedContainer.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -294,13 +294,13 @@ HashedCollectionConfig::StorageObjectAtAddress(
294294
// same address.
295295
Status error;
296296
ExecutionContextScope *exe_scope = exe_ctx.GetBestExecutionContextScope();
297-
auto reader =
297+
llvm::Optional<SwiftASTContextReader> reader =
298298
process_sp->GetTarget().GetScratchSwiftASTContext(error, *exe_scope);
299-
SwiftASTContext *ast_ctx = reader.get();
300-
if (!ast_ctx)
299+
if (!reader)
301300
return nullptr;
302301
if (error.Fail())
303302
return nullptr;
303+
SwiftASTContext *ast_ctx = reader->get();
304304

305305
CompilerType rawStorage_type =
306306
ast_ctx->GetTypeFromMangledTypename(m_nativeStorageRoot_mangled);
@@ -445,10 +445,10 @@ NativeHashedStorageHandler::NativeHashedStorageHandler(
445445
m_value_stride = value_type_stride ? *value_type_stride : 0;
446446
if (TypeSystemSwift *swift_ast =
447447
llvm::dyn_cast_or_null<TypeSystemSwift>(key_type.GetTypeSystem())) {
448-
auto scratch_ctx_reader = nativeStorage_sp->GetScratchSwiftASTContext();
449-
auto scratch_ctx = scratch_ctx_reader.get();
450-
if (!scratch_ctx)
448+
llvm::Optional<SwiftASTContextReader> scratch_ctx_reader = nativeStorage_sp->GetScratchSwiftASTContext();
449+
if (!scratch_ctx_reader)
451450
return;
451+
SwiftASTContext *scratch_ctx = scratch_ctx_reader->get();
452452
auto *runtime = SwiftLanguageRuntime::Get(m_process);
453453
if (!runtime)
454454
return;

‎lldb/source/Plugins/Language/Swift/SwiftLanguage.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,9 +1013,11 @@ std::unique_ptr<Language::TypeScavenger> SwiftLanguage::GetTypeScavenger() {
10131013
if (target) {
10141014
const bool create_on_demand = false;
10151015
Status error;
1016-
auto ast_ctx = target->GetScratchSwiftASTContext(
1017-
error, *exe_scope, create_on_demand);
1018-
if (ast_ctx) {
1016+
llvm::Optional<SwiftASTContextReader> maybe_ast_ctx =
1017+
target->GetScratchSwiftASTContext(error, *exe_scope,
1018+
create_on_demand);
1019+
if (maybe_ast_ctx) {
1020+
SwiftASTContext *ast_ctx = maybe_ast_ctx->get();
10191021
ConstString cs_input{input};
10201022
Mangled mangled(cs_input);
10211023
if (mangled.GuessLanguage() == eLanguageTypeSwift) {
@@ -1071,9 +1073,11 @@ std::unique_ptr<Language::TypeScavenger> SwiftLanguage::GetTypeScavenger() {
10711073
Target *target = exe_scope->CalculateTarget().get();
10721074
const bool create_on_demand = false;
10731075
Status error;
1074-
auto ast_ctx = target->GetScratchSwiftASTContext(error, *exe_scope,
1075-
create_on_demand);
1076-
if (ast_ctx) {
1076+
llvm::Optional<SwiftASTContextReader> maybe_ast_ctx =
1077+
target->GetScratchSwiftASTContext(error, *exe_scope,
1078+
create_on_demand);
1079+
if (maybe_ast_ctx) {
1080+
SwiftASTContext *ast_ctx = maybe_ast_ctx->get();
10771081
auto iter = ast_ctx->GetModuleCache().begin(),
10781082
end = ast_ctx->GetModuleCache().end();
10791083

‎lldb/source/Target/SwiftLanguageRuntime.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -945,8 +945,10 @@ void SwiftLanguageRuntime::FindFunctionPointersInCall(
945945
Status error;
946946
Target &target = frame.GetThread()->GetProcess()->GetTarget();
947947
ExecutionContext exe_ctx(frame);
948-
auto swift_ast = target.GetScratchSwiftASTContext(error, frame);
949-
if (swift_ast) {
948+
llvm::Optional<SwiftASTContextReader> maybe_swift_ast =
949+
target.GetScratchSwiftASTContext(error, frame);
950+
if (maybe_swift_ast) {
951+
SwiftASTContext *swift_ast = maybe_swift_ast->get();
950952
CompilerType function_type = swift_ast->GetTypeFromMangledTypename(
951953
mangled_name.GetMangledName());
952954
if (error.Success()) {
@@ -1152,9 +1154,11 @@ SwiftLanguageRuntime::CalculateErrorValue(StackFrameSP frame_sp,
11521154
if (!exe_scope)
11531155
return error_valobj_sp;
11541156

1155-
auto ast_context = target->GetScratchSwiftASTContext(error, *frame_sp);
1156-
if (!ast_context || error.Fail())
1157+
llvm::Optional<SwiftASTContextReader> maybe_ast_context =
1158+
target->GetScratchSwiftASTContext(error, *frame_sp);
1159+
if (!maybe_ast_context || error.Fail())
11571160
return error_valobj_sp;
1161+
SwiftASTContext *ast_context = maybe_ast_context->get();
11581162

11591163
lldb::DataBufferSP buffer(new lldb_private::DataBufferHeap(
11601164
arg0->GetScalar().GetBytes(), arg0->GetScalar().GetByteSize()));
@@ -1483,9 +1487,9 @@ SwiftLanguageRuntimeImpl::GetBridgedSyntheticChildProvider(
14831487
ProjectionSyntheticChildren::TypeProjectionUP type_projection(
14841488
new ProjectionSyntheticChildren::TypeProjectionUP::element_type());
14851489

1486-
if (auto swift_ast_ctx = valobj.GetScratchSwiftASTContext()) {
1490+
if (auto maybe_swift_ast_ctx = valobj.GetScratchSwiftASTContext()) {
14871491
CompilerType swift_type =
1488-
swift_ast_ctx->GetTypeFromMangledTypename(type_name);
1492+
maybe_swift_ast_ctx->get()->GetTypeFromMangledTypename(type_name);
14891493

14901494
if (swift_type.IsValid()) {
14911495
ExecutionContext exe_ctx(m_process);

‎lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -513,18 +513,20 @@ SwiftLanguageRuntime::MetadataPromise::FulfillTypePromise(Status *error) {
513513
if (m_compiler_type.hasValue())
514514
return m_compiler_type.getValue();
515515

516-
auto swift_ast_ctx = m_for_object_sp->GetScratchSwiftASTContext();
517-
if (!swift_ast_ctx) {
516+
llvm::Optional<SwiftASTContextReader> maybe_swift_ast_ctx =
517+
m_for_object_sp->GetScratchSwiftASTContext();
518+
if (!maybe_swift_ast_ctx) {
518519
error->SetErrorString("couldn't get Swift scratch context");
519520
return CompilerType();
520521
}
522+
SwiftASTContext *swift_ast_ctx = maybe_swift_ast_ctx->get();
521523
auto &remote_ast = m_swift_runtime.GetRemoteASTContext(*swift_ast_ctx);
522524
swift::remoteAST::Result<swift::Type> result =
523525
remote_ast.getTypeForRemoteTypeMetadata(
524526
swift::remote::RemoteAddress(m_metadata_location));
525527

526528
if (result) {
527-
m_compiler_type = {swift_ast_ctx.get(), result.getValue().getPointer()};
529+
m_compiler_type = {swift_ast_ctx, result.getValue().getPointer()};
528530
if (log)
529531
log->Printf("[MetadataPromise] result is type %s",
530532
m_compiler_type->GetTypeName().AsCString());
@@ -543,10 +545,13 @@ SwiftLanguageRuntime::MetadataPromise::FulfillTypePromise(Status *error) {
543545
SwiftLanguageRuntime::MetadataPromiseSP
544546
SwiftLanguageRuntimeImpl::GetMetadataPromise(lldb::addr_t addr,
545547
ValueObject &for_object) {
546-
auto swift_ast_ctx = for_object.GetScratchSwiftASTContext();
547-
if (!swift_ast_ctx || swift_ast_ctx->HasFatalErrors())
548+
llvm::Optional<SwiftASTContextReader> maybe_swift_ast_ctx =
549+
for_object.GetScratchSwiftASTContext();
550+
if (!maybe_swift_ast_ctx)
551+
return nullptr;
552+
SwiftASTContext *swift_ast_ctx = maybe_swift_ast_ctx->get();
553+
if (swift_ast_ctx->HasFatalErrors())
548554
return nullptr;
549-
550555
if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
551556
return nullptr;
552557

@@ -1038,9 +1043,11 @@ SwiftLanguageRuntimeImpl::DoArchetypeBindingForType(StackFrame &stack_frame,
10381043
auto &target = m_process.GetTarget();
10391044
assert(IsScratchContextLocked(target) &&
10401045
"Swift scratch context not locked ahead of archetype binding");
1041-
auto scratch_ctx = target.GetScratchSwiftASTContext(error, stack_frame);
1042-
if (!scratch_ctx)
1046+
llvm::Optional<SwiftASTContextReader> maybe_scratch_ctx =
1047+
target.GetScratchSwiftASTContext(error, stack_frame);
1048+
if (!maybe_scratch_ctx)
10431049
return base_type;
1050+
SwiftASTContext *scratch_ctx = maybe_scratch_ctx->get();
10441051
base_type = scratch_ctx->ImportType(base_type, error);
10451052

10461053
if (base_type.GetTypeInfo() & lldb::eTypeIsSwift) {
@@ -1425,11 +1432,13 @@ bool SwiftLanguageRuntimeImpl::GetDynamicTypeAndAddress_ClangType(
14251432
// Import the remangled dynamic name into the scratch context.
14261433
assert(IsScratchContextLocked(in_value.GetTargetSP()) &&
14271434
"Swift scratch context not locked ahead of dynamic type resolution");
1428-
auto scratch_ctx = in_value.GetScratchSwiftASTContext();
1429-
if (!scratch_ctx)
1435+
llvm::Optional<SwiftASTContextReader> maybe_scratch_ctx =
1436+
in_value.GetScratchSwiftASTContext();
1437+
if (!maybe_scratch_ctx)
14301438
return false;
14311439
CompilerType swift_type =
1432-
scratch_ctx->GetTypeFromMangledTypename(ConstString(remangled));
1440+
maybe_scratch_ctx->get()->GetTypeFromMangledTypename(
1441+
ConstString(remangled));
14331442

14341443
// Roll back the ObjC dynamic type resolution.
14351444
if (!swift_type)
@@ -1483,9 +1492,11 @@ bool SwiftLanguageRuntimeImpl::GetDynamicTypeAndAddress(
14831492
// use the scratch context where such operations are legal and safe.
14841493
assert(IsScratchContextLocked(in_value.GetTargetSP()) &&
14851494
"Swift scratch context not locked ahead of dynamic type resolution");
1486-
auto scratch_ctx = in_value.GetScratchSwiftASTContext();
1487-
if (!scratch_ctx)
1495+
llvm::Optional<SwiftASTContextReader> maybe_scratch_ctx =
1496+
in_value.GetScratchSwiftASTContext();
1497+
if (!maybe_scratch_ctx)
14881498
return false;
1499+
SwiftASTContextForExpressions *scratch_ctx = maybe_scratch_ctx->get();
14891500

14901501
auto retry_once = [&]() {
14911502
// Retry exactly once using the per-module fallback scratch context.

‎lldb/source/Target/Target.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2336,11 +2336,12 @@ Target::GetPersistentExpressionStateForLanguage(lldb::LanguageType language) {
23362336
SwiftPersistentExpressionState *
23372337
Target::GetSwiftPersistentExpressionState(ExecutionContextScope &exe_scope) {
23382338
Status error;
2339-
auto swift_ast_context = GetScratchSwiftASTContext(error, exe_scope, true);
2340-
if (!swift_ast_context)
2339+
auto maybe_swift_ast_context =
2340+
GetScratchSwiftASTContext(error, exe_scope, true);
2341+
if (!maybe_swift_ast_context)
23412342
return nullptr;
23422343
return (SwiftPersistentExpressionState *)
2343-
swift_ast_context->GetPersistentExpressionState();
2344+
maybe_swift_ast_context->get()->GetPersistentExpressionState();
23442345
}
23452346

23462347
UserExpression *Target::GetUserExpressionForLanguage(
@@ -2423,7 +2424,7 @@ ClangASTImporterSP Target::GetClangASTImporter() {
24232424
return ClangASTImporterSP();
24242425
}
24252426

2426-
SwiftASTContextReader Target::GetScratchSwiftASTContext(
2427+
llvm::Optional<SwiftASTContextReader> Target::GetScratchSwiftASTContext(
24272428
Status &error, ExecutionContextScope &exe_scope, bool create_on_demand) {
24282429
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
24292430

@@ -2504,7 +2505,9 @@ SwiftASTContextReader Target::GetScratchSwiftASTContext(
25042505
error);
25052506
}
25062507

2507-
return SwiftASTContextReader(GetSwiftScratchContextLock(), swift_ast_ctx);
2508+
if (!swift_ast_ctx)
2509+
return llvm::None;
2510+
return SwiftASTContextReader(GetSwiftScratchContextLock(), *swift_ast_ctx);
25082511
}
25092512

25102513
static SharedMutex *

0 commit comments

Comments
 (0)