Skip to content

[lldb] Retire the per-module Swift scratch context fallback #9607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions lldb/include/lldb/Target/Target.h
Original file line number Diff line number Diff line change
Expand Up @@ -1587,14 +1587,6 @@ class Target : public std::enable_shared_from_this<Target>,

void SetREPL(lldb::LanguageType language, lldb::REPLSP repl_sp);

/// Enable the use of a separate sscratch type system per lldb::Module.
void SetUseScratchTypesystemPerModule(bool value) {
m_use_scratch_typesystem_per_module = value;
}
bool UseScratchTypesystemPerModule() const {
return m_use_scratch_typesystem_per_module;
}

public:
StackFrameRecognizerManager &GetFrameRecognizerManager() {
return *m_frame_recognizer_manager_up;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,6 @@ struct ModuleImportError : public llvm::ErrorInfo<ModuleImportError> {
ModuleImportError(llvm::Twine message, bool is_new_dylib = false)
: msg(message.str()), is_new_dylib(is_new_dylib) {}
void log(llvm::raw_ostream &OS) const override {
OS << "error while processing module import: ";
OS << msg;
}
std::error_code convertToErrorCode() const override {
Expand Down Expand Up @@ -1453,7 +1452,8 @@ SwiftExpressionParser::ParseAndImport(
// fatal error state. One way this can happen is if the import
// triggered a dylib import, in which case the context is
// purposefully poisoned.
msg = "import may have triggered a dylib import";
msg = "import may have triggered a dylib import, "
"resetting compiler state";
}
return make_error<ModuleImportError>(msg, /*is_new_dylib=*/true);
}
Expand Down Expand Up @@ -1722,14 +1722,8 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
// There are no fallback contexts in REPL and playgrounds.
if (repl || playground)
return;
if (!m_sc.target_sp->UseScratchTypesystemPerModule()) {
// This, together with the fatal error forces
// a per-module scratch to be instantiated on
// retry.
m_sc.target_sp->SetUseScratchTypesystemPerModule(true);
m_swift_ast_ctx.RaiseFatalError(MIE.message());
retry = true;
}
// The fatal error causes a new compiler to be instantiated on retry.
m_swift_ast_ctx.RaiseFatalError(MIE.message());
},
[&](const SwiftASTContextError &SACE) {
DiagnoseSwiftASTContextError();
Expand All @@ -1745,9 +1739,7 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
});

// Signal that we want to retry the expression exactly once with a
// fresh SwiftASTContext initialized with the flags from the
// current lldb::Module / Swift dylib to avoid header search
// mismatches.
// fresh SwiftASTContext.
if (retry)
return ParseResult::retry_fresh_context;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class SwiftExpressionParser : public ExpressionParser {
public:
enum class ParseResult {
success,
retry_fresh_context,
retry_fresh_context,
retry_no_bind_generic_params,
unrecoverable_error
};
Expand Down
21 changes: 6 additions & 15 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2347,8 +2347,7 @@ ProcessModule(Module &module, std::string m_description,

lldb::TypeSystemSP
SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
TypeSystemSwiftTypeRef &typeref_typesystem,
bool fallback) {
TypeSystemSwiftTypeRef &typeref_typesystem) {
TargetSP target = typeref_typesystem.GetTargetWP().lock();
if (!SwiftASTContextSupportsLanguage(language))
return lldb::TypeSystemSP();
Expand All @@ -2361,10 +2360,7 @@ SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
{
llvm::raw_string_ostream ss(m_description);
ss << "SwiftASTContext";
if (fallback)
ss << "ForExpressions";
else
ss << "ForModule";
ss << "ForModule";
ss << '(' << '"';
module.GetDescription(ss, eDescriptionLevelBrief);
ss << '"' << ')';
Expand Down Expand Up @@ -2424,20 +2420,15 @@ SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
}

// If there is a target this may be a fallback scratch context.
assert((!fallback || target) && "fallback context must specify a target");
std::shared_ptr<SwiftASTContext> swift_ast_sp(
fallback
? static_cast<SwiftASTContext *>(new SwiftASTContextForExpressions(
m_description, typeref_typesystem))
: static_cast<SwiftASTContext *>(new SwiftASTContextForModule(
m_description, typeref_typesystem)));
std::shared_ptr<SwiftASTContext> swift_ast_sp(static_cast<SwiftASTContext *>(
new SwiftASTContextForModule(m_description, typeref_typesystem)));
bool suppress_config_log = false;
auto defer_log =
llvm::make_scope_exit([swift_ast_sp, &suppress_config_log, fallback] {
llvm::make_scope_exit([swift_ast_sp, &suppress_config_log] {
// To avoid spamming the log with useless info, we don't log the
// configuration if everything went fine and the current module
// doesn't have any Swift contents (i.e., the shared cache dylibs).
if (!suppress_config_log || fallback)
if (!suppress_config_log)
swift_ast_sp->LogConfiguration();
});

Expand Down
6 changes: 2 additions & 4 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,10 @@ class SwiftASTContext : public TypeSystemSwift {
/// Create a SwiftASTContext from a Module. This context is used
/// for frame variable and uses ClangImporter options specific to
/// this lldb::Module. The optional target is necessary when
/// creating a module-specific scratch context. If \p fallback is
/// true, then a SwiftASTContextForExpressions is created.
/// creating a module-specific scratch context.
static lldb::TypeSystemSP
CreateInstance(lldb::LanguageType language, Module &module,
TypeSystemSwiftTypeRef &typeref_typesystem,
bool fallback = false);
TypeSystemSwiftTypeRef &typeref_typesystem);
/// Create a SwiftASTContextForExpressions taylored to a specific symbol
/// context.
static lldb::TypeSystemSP
Expand Down
15 changes: 0 additions & 15 deletions lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1659,21 +1659,6 @@ TypeSystemSwiftTypeRef::TypeSystemSwiftTypeRef(Module &module) {
m_description.c_str());
}

TypeSystemSwiftTypeRefForExpressions::TypeSystemSwiftTypeRefForExpressions(
lldb::LanguageType language, Target &target, Module &module)
: TypeSystemSwiftTypeRef(module), m_target_wp(target.shared_from_this()),
m_persistent_state_up(new SwiftPersistentExpressionState) {
m_description = "TypeSystemSwiftTypeRefForExpressions(PerModuleFallback)";
LLDB_LOGF(GetLog(LLDBLog::Types),
"%s::TypeSystemSwiftTypeRefForExpressions()",
m_description.c_str());
m_swift_ast_context_map.insert(
{nullptr,
SwiftASTContext::CreateInstance(
LanguageType::eLanguageTypeSwift, module,
*const_cast<TypeSystemSwiftTypeRefForExpressions *>(this), true)});
}

TypeSystemSwiftTypeRefForExpressions::TypeSystemSwiftTypeRefForExpressions(
lldb::LanguageType language, Target &target, const char *extra_options)
: m_target_wp(target.shared_from_this()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -520,10 +520,6 @@ class TypeSystemSwiftTypeRefForExpressions : public TypeSystemSwiftTypeRef {
Target &target,
const char *extra_options);

/// For per-module fallback contexts.
TypeSystemSwiftTypeRefForExpressions(lldb::LanguageType language,
Target &target, Module &module);

SwiftASTContext *GetSwiftASTContext(const SymbolContext &sc) const override;
SwiftASTContext *
GetSwiftASTContextOrNull(const SymbolContext &sc) const override;
Expand Down
89 changes: 0 additions & 89 deletions lldb/source/Target/Target.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2932,96 +2932,7 @@ Target::GetSwiftScratchContext(Status &error, ExecutionContextScope &exe_scope,
return nullptr;
};

auto maybe_create_fallback_context = [&]() {
ModuleLanguage key = {lldb_module, lldb::eLanguageTypeSwift};
if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
auto *cached_ast_ctx =
llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
cached_ts->GetSwiftASTContext(sc));
if (cached_ast_ctx && cached_ast_ctx->HasFatalErrors() &&
!m_cant_make_scratch_type_system.count(lldb::eLanguageTypeSwift)) {
DisplayFallbackSwiftContextErrors(cached_ast_ctx);
// Try again.
// FIXME: Shouldn't this continue rather than return?
auto &lock = GetSwiftScratchContextLock();
if (!lock.try_lock()) {
if (log)
log->Printf("module scratch context has errors but couldn't "
"acquire scratch context lock\n");
return;
}
std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
m_scratch_typesystem_for_module.erase(key);
if (log)
log->Printf("erased module-wide scratch context with errors\n");
return;
}
if (log)
log->PutCString("returned cached module-wide scratch context");
return;
}

if (!create_on_demand) {
if (log)
log->PutCString("not allowed to create a new context");
return;
}

// Call for its side effects of establishing the Swift scratch type
// system.
auto type_system_or_err =
GetScratchTypeSystemForLanguage(eLanguageTypeSwift, false);
if (!type_system_or_err) {
llvm::consumeError(type_system_or_err.takeError());
return;
}

auto &lock = GetSwiftScratchContextLock();
if (!lock.try_lock()) {
if (log)
log->PutCString("couldn't acquire scratch context lock");
return;
}
std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);

// With the lock held, get the current scratch type system. This ensures
// the current instance is used even in the unlikely event it was changed
// during the brief window between the call to
// `GetScratchTypeSystemForLanguage` and taking the lock.
type_system_or_err = m_scratch_type_system_map.GetTypeSystemForLanguage(
eLanguageTypeSwift, this, false);
if (!type_system_or_err) {
llvm::consumeError(type_system_or_err.takeError());
return;
}

if (auto *global_scratch_ctx =
llvm::cast_or_null<TypeSystemSwiftTypeRefForExpressions>(
type_system_or_err->get()))
if (auto *swift_ast_ctx =
llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
global_scratch_ctx->GetSwiftASTContext(sc)))
DisplayFallbackSwiftContextErrors(swift_ast_ctx);

auto typesystem_sp = std::make_shared<TypeSystemSwiftTypeRefForExpressions>(
lldb::eLanguageTypeSwift, *this, *lldb_module);
typesystem_sp->GetSwiftASTContext(sc);
m_scratch_typesystem_for_module.insert({key, typesystem_sp});
if (log)
log->PutCString("created module-wide scratch context");
return;
};

std::optional<SwiftScratchContextReader> reader;
if (lldb_module && m_use_scratch_typesystem_per_module) {
maybe_create_fallback_context();
std::shared_lock<std::shared_mutex> lock(GetSwiftScratchContextLock());
if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
reader = SwiftScratchContextReader(std::move(lock), *cached_ts);
if (log)
log->PutCString("returned project-wide scratch context");
}
}
// FIXME: Don't return the project-wide context after requesting the
// module-wide one.
if (!reader) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,3 @@ def test(self):
threads = lldbutil.get_threads_stopped_at_breakpoint(
process, b_breakpoint)
self.expect("expression foo", error=True)

per_module_fallback = 0
import io
with open(log, "r", encoding='utf-8') as logfile:
for line in logfile:
if 'SwiftASTContextForExpressions("Framework")' in line:
per_module_fallback += 1
self.assertGreater(per_module_fallback, 0,
"failed to create per-module scratch context")