Skip to content
This repository was archived by the owner on Apr 2, 2020. It is now read-only.

[Target] Centralize creation of scratch SwiftASTContexts #1772

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from
Open

[Target] Centralize creation of scratch SwiftASTContexts #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 0 additions & 3 deletions include/lldb/Core/ValueObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#ifndef liblldb_ValueObject_h_
#define liblldb_ValueObject_h_

#include "lldb/Core/SwiftASTContextReader.h"
#include "lldb/Core/Value.h"
#include "lldb/Symbol/CompilerType.h"
#include "lldb/Symbol/Type.h"
Expand Down Expand Up @@ -614,8 +613,6 @@ class ValueObject : public UserID {

virtual bool HasSyntheticValue();

SwiftASTContextReader GetScratchSwiftASTContext();

virtual bool IsSynthetic() { return false; }

lldb::ValueObjectSP
Expand Down
5 changes: 5 additions & 0 deletions include/lldb/Target/Target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "lldb/Core/Architecture.h"
#include "lldb/Core/Disassembler.h"
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/SwiftASTContextReader.h"
#include "lldb/Core/UserSettingsController.h"
#include "lldb/Expression/Expression.h"
#include "lldb/Interpreter/OptionValueBoolean.h"
Expand Down Expand Up @@ -1211,6 +1212,10 @@ class Target : public std::enable_shared_from_this<Target>,
GetScratchSwiftASTContext(Status &error, ExecutionContextScope &exe_scope,
bool create_on_demand = true);

SwiftASTContextReader GetScratchSwiftASTContext(Status &error,
ValueObject &valobj,
bool create_on_demand = true);

private:
void DisplayFallbackSwiftContextErrors(SwiftASTContext *swift_ast_ctx);
public:
Expand Down
12 changes: 1 addition & 11 deletions source/Core/ValueObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "lldb/Core/Address.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/SwiftASTContextReader.h"
#include "lldb/Core/ValueObjectCast.h"
#include "lldb/Core/ValueObjectChild.h"
#include "lldb/Core/ValueObjectConstResult.h"
Expand All @@ -28,7 +29,6 @@
#include "lldb/Symbol/ClangASTContext.h"
#include "lldb/Symbol/CompileUnit.h"
#include "lldb/Symbol/CompilerType.h"
#include "lldb/Symbol/SwiftASTContext.h"
#include "lldb/Symbol/Declaration.h"
#include "lldb/Symbol/SymbolContext.h"
#include "lldb/Symbol/Type.h"
Expand Down Expand Up @@ -1708,16 +1708,6 @@ LanguageType ValueObject::GetObjectRuntimeLanguage() {
return lldb::eLanguageTypeUnknown;
}

SwiftASTContextReader ValueObject::GetScratchSwiftASTContext() {
lldb::TargetSP target_sp(GetTargetSP());
if (!target_sp)
return {};
Status error;
ExecutionContext ctx = GetExecutionContextRef().Lock(false);
auto *exe_scope = ctx.GetBestExecutionContextScope();
return target_sp->GetScratchSwiftASTContext(error, *exe_scope);
}

void ValueObject::AddSyntheticChild(ConstString key,
ValueObject *valobj) {
m_synthetic_children[key] = valobj;
Expand Down
11 changes: 8 additions & 3 deletions source/Core/ValueObjectDynamicValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//

#include "lldb/Core/SwiftASTContextReader.h"
#include "lldb/Core/ValueObjectDynamicValue.h"
#include "lldb/Core/Value.h"
#include "lldb/Core/ValueObject.h"
Expand Down Expand Up @@ -420,7 +421,11 @@ bool ValueObjectDynamicValue::DynamicValueTypeInfoNeedsUpdate() {
if (!m_dynamic_type_info.HasType())
return false;

auto *cached_ctx =
static_cast<SwiftASTContext *>(m_value.GetCompilerType().GetTypeSystem());
return cached_ctx == GetScratchSwiftASTContext().get();
Status error;
auto *scratch_ctx = GetTargetSP()->GetScratchSwiftASTContext(error, *this).get();
if (!error.Success())
return false;

auto *cached_ctx = m_value.GetCompilerType().GetTypeSystem();
return cached_ctx == scratch_ctx;
}
6 changes: 5 additions & 1 deletion source/Plugins/Language/Swift/SwiftHashedContainer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,11 @@ NativeHashedStorageHandler::NativeHashedStorageHandler(
m_value_stride = value_type.GetByteStride();
if (SwiftASTContext *swift_ast =
llvm::dyn_cast_or_null<SwiftASTContext>(key_type.GetTypeSystem())) {
auto scratch_ctx_reader = nativeStorage_sp->GetScratchSwiftASTContext();
Status error;
auto scratch_ctx_reader =
m_process->GetTarget().GetScratchSwiftASTContext(error, *nativeStorage_sp.get());
if (!error.Success())
return;
auto scratch_ctx = scratch_ctx_reader.get();
if (!scratch_ctx)
return;
Expand Down
37 changes: 26 additions & 11 deletions source/Target/SwiftLanguageRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,9 +1444,13 @@ SwiftLanguageRuntime::MetadataPromise::FulfillTypePromise(Status *error) {
if (m_compiler_type.hasValue())
return m_compiler_type.getValue();

auto swift_ast_ctx = m_for_object_sp->GetScratchSwiftASTContext();
TargetSP target_sp = m_for_object_sp->GetTargetSP();
Status type_system_error;
auto swift_ast_ctx =
target_sp->GetScratchSwiftASTContext(type_system_error, *m_for_object_sp);
if (!swift_ast_ctx) {
error->SetErrorString("couldn't get Swift scratch context");
error->SetErrorStringWithFormat("couldn't get Swift scratch context: %s",
type_system_error.AsCString());
return CompilerType();
}
auto &remote_ast = m_swift_runtime.GetRemoteASTContext(*swift_ast_ctx);
Expand Down Expand Up @@ -1486,9 +1490,13 @@ SwiftLanguageRuntime::MetadataPromise::FulfillKindPromise(Status *error) {
if (m_metadata_kind.hasValue())
return m_metadata_kind;

auto swift_ast_ctx = m_for_object_sp->GetScratchSwiftASTContext();
TargetSP target_sp = m_for_object_sp->GetTargetSP();
Status type_system_error;
auto swift_ast_ctx =
target_sp->GetScratchSwiftASTContext(type_system_error, *m_for_object_sp);
if (!swift_ast_ctx) {
error->SetErrorString("couldn't get Swift scratch context");
error->SetErrorStringWithFormat("couldn't get Swift scratch context: %s",
type_system_error.AsCString());
return llvm::None;
}
auto &remote_ast = m_swift_runtime.GetRemoteASTContext(*swift_ast_ctx);
Expand Down Expand Up @@ -1530,8 +1538,10 @@ bool SwiftLanguageRuntime::MetadataPromise::IsStaticallyDetermined() {
SwiftLanguageRuntime::MetadataPromiseSP
SwiftLanguageRuntime::GetMetadataPromise(lldb::addr_t addr,
ValueObject &for_object) {
auto swift_ast_ctx = for_object.GetScratchSwiftASTContext();
if (!swift_ast_ctx || swift_ast_ctx->HasFatalErrors())
Status error;
auto swift_ast_ctx =
m_process->GetTarget().GetScratchSwiftASTContext(error, for_object);
if (!error.Success() || swift_ast_ctx->HasFatalErrors())
return nullptr;

if (addr == 0 || addr == LLDB_INVALID_ADDRESS)
Expand Down Expand Up @@ -1619,8 +1629,9 @@ SwiftLanguageRuntime::GetMemberVariableOffset(CompilerType instance_type,

llvm::Optional<SwiftASTContextReader> scratch_ctx;
if (instance) {
scratch_ctx = instance->GetScratchSwiftASTContext();
if (!scratch_ctx)
Status error;
scratch_ctx = m_process->GetTarget().GetScratchSwiftASTContext(error, *instance);
if (!error.Success())
return llvm::None;
}

Expand Down Expand Up @@ -2350,8 +2361,10 @@ bool SwiftLanguageRuntime::GetDynamicTypeAndAddress(
// use the scratch context where such operations are legal and safe.
assert(IsScratchContextLocked(in_value.GetTargetSP()) &&
"Swift scratch context not locked ahead of dynamic type resolution");
auto scratch_ctx = in_value.GetScratchSwiftASTContext();
if (!scratch_ctx)
Status error;
auto scratch_ctx =
m_process->GetTarget().GetScratchSwiftASTContext(error, in_value);
if (!error.Success())
return false;

auto retry_once = [&]() {
Expand Down Expand Up @@ -3748,7 +3761,9 @@ SwiftLanguageRuntime::GetBridgedSyntheticChildProvider(ValueObject &valobj) {
ProjectionSyntheticChildren::TypeProjectionUP type_projection(
new ProjectionSyntheticChildren::TypeProjectionUP::element_type());

if (auto swift_ast_ctx = valobj.GetScratchSwiftASTContext()) {
Status type_system_error;
auto swift_ast_ctx = m_process->GetTarget().GetScratchSwiftASTContext(type_system_error, valobj);
if (type_system_error.Success()) {
Status error;
CompilerType swift_type =
swift_ast_ctx->GetTypeFromMangledTypename(type_name, error);
Expand Down
8 changes: 8 additions & 0 deletions source/Target/Target.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2534,6 +2534,14 @@ SwiftASTContextReader Target::GetScratchSwiftASTContext(
return SwiftASTContextReader(GetSwiftScratchContextLock(), swift_ast_ctx);
}

SwiftASTContextReader Target::GetScratchSwiftASTContext(Status &error,
ValueObject &valobj,
bool create_on_demand) {
ExecutionContext ctx = valobj.GetExecutionContextRef().Lock(false);
auto *exe_scope = ctx.GetBestExecutionContextScope();
return GetScratchSwiftASTContext(error, *exe_scope);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this return an llvm::Expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can do that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I just looked into it. Looks like We always return a SwiftASTContextReader no matter what, so returning an llvm::Expected doesn't make much sense here. What I did find is that GetScratchSwiftASTContext will sometimes fail to build a scratch SwiftASTContext and instead return the project-wide scratch SwiftASTContext. Even if we fail to build a SwiftASTContext, we still return a SwiftASTContextReader.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong preferences, so I'm happy with this once Jonas feels his comments have been addressed.
Maybe longer term we can enforce that the SwiftASTContextReader returned is always valid [and if it's not, assert], given we expect it in such state..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever need to differentiate between the two? This code is saying that the error doesn't matter. Is that true for this function, or is that true always? In the latter case, maybe we should remove the Status as the first argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the looks of it, Target::GetScratchSwiftASTContext will return a Status and the callsites generally use them. It looks like error handling is just not taken into account with the ValueObject version. So even though it's true just for this function, I think that we can probably do a better job and improve error handling here by making the ValueObject version take a Status and then have the clients actually use that Status for error handling.

}

static SharedMutex *
GetSwiftScratchContextMutex(const ExecutionContext *exe_ctx) {
if (!exe_ctx)
Expand Down