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` _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
