[Lldb-commits] [lldb] f6d4e68 - Revert "[LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer"
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Fri Nov 25 12:23:04 PST 2022
Author: Jason Molenda
Date: 2022-11-25T12:22:58-08:00
New Revision: f6d4e687172ab14728c569a0c1d3eb1c76021250
URL: https://github.com/llvm/llvm-project/commit/f6d4e687172ab14728c569a0c1d3eb1c76021250
DIFF: https://github.com/llvm/llvm-project/commit/f6d4e687172ab14728c569a0c1d3eb1c76021250.diff
LOG: Revert "[LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer"
This reverts commit cd3091a88f7c55c90d9b5fff372ce1cdfc71948d.
This change crashes on macOS systems in
formatters::StdlibCoroutineHandleSyntheticFrontEnd when
it fails to create the `ValueObjectSP promise` and calls
a method on it. The failure causes a segfault while running
TestCoroutineHandle.py on the "LLDB Incremental" CI bot,
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/
This change originally landed via https://reviews.llvm.org/D132815
Added:
Modified:
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
Removed:
################################################################################
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index f579a0160b2b7..63bad9d0241de 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -246,7 +246,7 @@ def which(program):
class ValueCheck:
def __init__(self, name=None, value=None, type=None, summary=None,
- children=None, dereference=None):
+ children=None):
"""
:param name: The name that the SBValue should have. None if the summary
should not be checked.
@@ -261,15 +261,12 @@ def __init__(self, name=None, value=None, type=None, summary=None,
The order of checks is the order of the checks in the
list. The number of checks has to match the number of
children.
- :param dereference: A ValueCheck for the SBValue returned by the
- `Dereference` function.
"""
self.expect_name = name
self.expect_value = value
self.expect_type = type
self.expect_summary = summary
self.children = children
- self.dereference = dereference
def check_value(self, test_base, val, error_msg=None):
"""
@@ -311,9 +308,6 @@ def check_value(self, test_base, val, error_msg=None):
if self.children is not None:
self.check_value_children(test_base, val, error_msg)
- if self.dereference is not None:
- self.dereference.check_value(test_base, val.Dereference(), error_msg)
-
def check_value_children(self, test_base, val, error_msg=None):
"""
Checks that the children of a SBValue match a certain structure and
diff --git a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
index 7e1302c7eb785..aa6a6ef7e56ae 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -17,36 +17,35 @@ using namespace lldb;
using namespace lldb_private;
using namespace lldb_private::formatters;
-static lldb::addr_t GetCoroFramePtrFromHandle(ValueObjectSP valobj_sp) {
+static ValueObjectSP GetCoroFramePtrFromHandle(ValueObject &valobj) {
+ ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
if (!valobj_sp)
- return LLDB_INVALID_ADDRESS;
+ return nullptr;
// We expect a single pointer in the `coroutine_handle` class.
// We don't care about its name.
if (valobj_sp->GetNumChildren() != 1)
- return LLDB_INVALID_ADDRESS;
+ return nullptr;
ValueObjectSP ptr_sp(valobj_sp->GetChildAtIndex(0, true));
if (!ptr_sp)
- return LLDB_INVALID_ADDRESS;
+ return nullptr;
if (!ptr_sp->GetCompilerType().IsPointerType())
- return LLDB_INVALID_ADDRESS;
-
- AddressType addr_type;
- lldb::addr_t frame_ptr_addr = ptr_sp->GetPointerValue(&addr_type);
- if (!frame_ptr_addr || frame_ptr_addr == LLDB_INVALID_ADDRESS)
- return LLDB_INVALID_ADDRESS;
- lldbassert(addr_type == AddressType::eAddressTypeLoad);
- if (addr_type != AddressType::eAddressTypeLoad)
- return LLDB_INVALID_ADDRESS;
+ return nullptr;
- return frame_ptr_addr;
+ return ptr_sp;
}
-static Function *ExtractFunction(lldb::TargetSP target_sp,
- lldb::addr_t frame_ptr_addr, int offset) {
- lldb::ProcessSP process_sp = target_sp->GetProcessSP();
+static Function *ExtractFunction(ValueObjectSP &frame_ptr_sp, int offset) {
+ lldb::TargetSP target_sp = frame_ptr_sp->GetTargetSP();
+ lldb::ProcessSP process_sp = frame_ptr_sp->GetProcessSP();
auto ptr_size = process_sp->GetAddressByteSize();
+ AddressType addr_type;
+ lldb::addr_t frame_ptr_addr = frame_ptr_sp->GetPointerValue(&addr_type);
+ if (!frame_ptr_addr || frame_ptr_addr == LLDB_INVALID_ADDRESS)
+ return nullptr;
+ lldbassert(addr_type == AddressType::eAddressTypeLoad);
+
Status error;
auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
lldb::addr_t func_addr =
@@ -61,14 +60,12 @@ static Function *ExtractFunction(lldb::TargetSP target_sp,
return func_address.CalculateSymbolContextFunction();
}
-static Function *ExtractResumeFunction(lldb::TargetSP target_sp,
- lldb::addr_t frame_ptr_addr) {
- return ExtractFunction(target_sp, frame_ptr_addr, 0);
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+ return ExtractFunction(frame_ptr_sp, 0);
}
-static Function *ExtractDestroyFunction(lldb::TargetSP target_sp,
- lldb::addr_t frame_ptr_addr) {
- return ExtractFunction(target_sp, frame_ptr_addr, 1);
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+ return ExtractFunction(frame_ptr_sp, 1);
}
static bool IsNoopCoroFunction(Function *f) {
@@ -128,26 +125,43 @@ static CompilerType InferPromiseType(Function &destroy_func) {
return promise_type->GetForwardCompilerType();
}
+static CompilerType GetCoroutineFrameType(TypeSystemClang &ast_ctx,
+ CompilerType promise_type) {
+ CompilerType void_type = ast_ctx.GetBasicType(lldb::eBasicTypeVoid);
+ CompilerType coro_func_type = ast_ctx.CreateFunctionType(
+ /*result_type=*/void_type, /*args=*/&void_type, /*num_args=*/1,
+ /*is_variadic=*/false, /*qualifiers=*/0);
+ CompilerType coro_abi_type;
+ if (promise_type.IsVoidType()) {
+ coro_abi_type = ast_ctx.CreateStructForIdentifier(
+ ConstString(), {{"resume", coro_func_type.GetPointerType()},
+ {"destroy", coro_func_type.GetPointerType()}});
+ } else {
+ coro_abi_type = ast_ctx.CreateStructForIdentifier(
+ ConstString(), {{"resume", coro_func_type.GetPointerType()},
+ {"destroy", coro_func_type.GetPointerType()},
+ {"promise", promise_type}});
+ }
+ return coro_abi_type;
+}
+
bool lldb_private::formatters::StdlibCoroutineHandleSummaryProvider(
ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
- lldb::addr_t frame_ptr_addr =
- GetCoroFramePtrFromHandle(valobj.GetNonSyntheticValue());
- if (frame_ptr_addr == LLDB_INVALID_ADDRESS)
+ ValueObjectSP ptr_sp(GetCoroFramePtrFromHandle(valobj));
+ if (!ptr_sp)
return false;
- if (frame_ptr_addr == 0) {
+ if (!ptr_sp->GetValueAsUnsigned(0)) {
stream << "nullptr";
return true;
}
-
- lldb::TargetSP target_sp = valobj.GetTargetSP();
- if (IsNoopCoroFunction(ExtractResumeFunction(target_sp, frame_ptr_addr)) &&
- IsNoopCoroFunction(ExtractDestroyFunction(target_sp, frame_ptr_addr))) {
+ if (IsNoopCoroFunction(ExtractResumeFunction(ptr_sp)) &&
+ IsNoopCoroFunction(ExtractDestroyFunction(ptr_sp))) {
stream << "noop_coroutine";
return true;
}
- stream.Printf("coro frame = 0x%" PRIx64, frame_ptr_addr);
+ stream.Printf("coro frame = 0x%" PRIx64, ptr_sp->GetValueAsUnsigned(0));
return true;
}
@@ -164,67 +178,39 @@ lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
size_t lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
CalculateNumChildren() {
- if (!m_resume_ptr_sp || !m_destroy_ptr_sp)
+ if (!m_frame_ptr_sp)
return 0;
- return m_promise_ptr_sp ? 3 : 2;
+ return m_frame_ptr_sp->GetNumChildren();
}
lldb::ValueObjectSP lldb_private::formatters::
StdlibCoroutineHandleSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
- switch (idx) {
- case 0:
- return m_resume_ptr_sp;
- case 1:
- return m_destroy_ptr_sp;
- case 2:
- return m_promise_ptr_sp;
- }
- return lldb::ValueObjectSP();
+ if (!m_frame_ptr_sp)
+ return lldb::ValueObjectSP();
+
+ return m_frame_ptr_sp->GetChildAtIndex(idx, true);
}
bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
Update() {
- m_resume_ptr_sp.reset();
- m_destroy_ptr_sp.reset();
- m_promise_ptr_sp.reset();
+ m_frame_ptr_sp.reset();
- ValueObjectSP valobj_sp = m_backend.GetNonSyntheticValue();
+ ValueObjectSP valobj_sp = m_backend.GetSP();
if (!valobj_sp)
return false;
- lldb::addr_t frame_ptr_addr = GetCoroFramePtrFromHandle(valobj_sp);
- if (frame_ptr_addr == 0 || frame_ptr_addr == LLDB_INVALID_ADDRESS)
+ ValueObjectSP ptr_sp(GetCoroFramePtrFromHandle(m_backend));
+ if (!ptr_sp)
return false;
- lldb::TargetSP target_sp = m_backend.GetTargetSP();
- Function *resume_func = ExtractResumeFunction(target_sp, frame_ptr_addr);
- Function *destroy_func = ExtractDestroyFunction(target_sp, frame_ptr_addr);
+ Function *resume_func = ExtractResumeFunction(ptr_sp);
+ Function *destroy_func = ExtractDestroyFunction(ptr_sp);
- // For `std::noop_coroutine()`, we don't want to display any child nodes.
- if (IsNoopCoroFunction(resume_func) && IsNoopCoroFunction(destroy_func))
+ if (IsNoopCoroFunction(resume_func) && IsNoopCoroFunction(destroy_func)) {
+ // For `std::noop_coroutine()`, we don't want to display any child nodes.
return false;
-
- auto ts = valobj_sp->GetCompilerType().GetTypeSystem();
- auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
- if (!ast_ctx)
- return {};
-
- // Create the `resume` and `destroy` children
- auto &exe_ctx = m_backend.GetExecutionContextRef();
- lldb::ProcessSP process_sp = target_sp->GetProcessSP();
- auto ptr_size = process_sp->GetAddressByteSize();
- CompilerType void_type = ast_ctx->GetBasicType(lldb::eBasicTypeVoid);
- CompilerType coro_func_type = ast_ctx->CreateFunctionType(
- /*result_type=*/void_type, /*args=*/&void_type, /*num_args=*/1,
- /*is_variadic=*/false, /*qualifiers=*/0);
- CompilerType coro_func_ptr_type = coro_func_type.GetPointerType();
- m_resume_ptr_sp = CreateValueObjectFromAddress(
- "resume", frame_ptr_addr + 0 * ptr_size, exe_ctx, coro_func_ptr_type);
- lldbassert(m_resume_ptr_sp);
- m_destroy_ptr_sp = CreateValueObjectFromAddress(
- "destroy", frame_ptr_addr + 1 * ptr_size, exe_ctx, coro_func_ptr_type);
- lldbassert(m_destroy_ptr_sp);
+ }
// Get the `promise_type` from the template argument
CompilerType promise_type(
@@ -233,6 +219,10 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
return false;
// Try to infer the promise_type if it was type-erased
+ auto ts = valobj_sp->GetCompilerType().GetTypeSystem();
+ auto ast_ctx = ts.dyn_cast_or_null<TypeSystemClang>();
+ if (!ast_ctx)
+ return false;
if (promise_type.IsVoidType() && destroy_func) {
if (CompilerType inferred_type = InferPromiseType(*destroy_func)) {
// Copy the type over to the correct `TypeSystemClang` instance
@@ -240,16 +230,10 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
}
}
- // Add the `promise` member. We intentionally add `promise` as a pointer type
- // instead of a value type, and don't automatically dereference this pointer.
- // We do so to avoid potential very deep recursion in case there is a cycle in
- // formed between `std::coroutine_handle`s and their promises.
- lldb::ValueObjectSP promise = CreateValueObjectFromAddress(
- "promise", frame_ptr_addr + 2 * ptr_size, exe_ctx, promise_type);
- Status error;
- lldb::ValueObjectSP promisePtr = promise->AddressOf(error);
- if (error.Success())
- m_promise_ptr_sp = promisePtr->Clone(ConstString("promise"));
+ // Build the coroutine frame type
+ CompilerType coro_frame_type = GetCoroutineFrameType(*ast_ctx, promise_type);
+
+ m_frame_ptr_sp = ptr_sp->Cast(coro_frame_type.GetPointerType());
return false;
}
@@ -261,17 +245,10 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
size_t StdlibCoroutineHandleSyntheticFrontEnd::GetIndexOfChildWithName(
ConstString name) {
- if (!m_resume_ptr_sp || !m_destroy_ptr_sp)
+ if (!m_frame_ptr_sp)
return UINT32_MAX;
- if (name == ConstString("resume"))
- return 0;
- if (name == ConstString("destroy"))
- return 1;
- if (name == ConstString("promise_ptr") && m_promise_ptr_sp)
- return 2;
-
- return UINT32_MAX;
+ return m_frame_ptr_sp->GetIndexOfChildWithName(name);
}
SyntheticChildrenFrontEnd *
diff --git a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
index e2e640ab48430..c052bd23f4ca9 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -47,9 +47,7 @@ class StdlibCoroutineHandleSyntheticFrontEnd
size_t GetIndexOfChildWithName(ConstString name) override;
private:
- lldb::ValueObjectSP m_resume_ptr_sp;
- lldb::ValueObjectSP m_destroy_ptr_sp;
- lldb::ValueObjectSP m_promise_ptr_sp;
+ lldb::ValueObjectSP m_frame_ptr_sp;
std::unique_ptr<lldb_private::ClangASTImporter> m_ast_importer;
};
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
index 60f2c707e866a..d1b4d59de5f5f 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -68,7 +68,7 @@ def do_test(self, stdlib_type):
result_children=[
ValueCheck(name="resume", summary = test_generator_func_ptr_re),
ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
- ValueCheck(name="promise", dereference=ValueCheck(value="-1"))
+ ValueCheck(name="promise", value="-1")
])
# Run until after the `co_yield`
More information about the lldb-commits
mailing list