[LLDB] Do not dereference promise pointer in coroutine_handle pretty printer

So far, the pretty printer for `std::coroutine_handle` internally
dereferenced the contained frame pointer displayed the `promise`
as a sub-value. As noticed in https://reviews.llvm.org/D132624
by @labath, this can lead to an endless loop in lldb during printing
if the coroutine frame pointers form a cycle.

This commit breaks the cycle by exposing the `promise` as a pointer
type instead of a value type. The depth to which the `frame variable`
and the `expression` commands dereference those pointers can be
controlled using the `--ptr-depth` argument.

Differential Revision: https://reviews.llvm.org/D132815

Author: vogelsgesang

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):
+                 children=None, dereference=None):
         """
         :param name: The name that the SBValue should have. None if the summary
                      should not be checked.
@@ -261,12 +261,15 @@ 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):
         """
@@ -308,6 +311,9 @@ 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

lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp

@@ -17,34 +17,35 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
-static ValueObjectSP GetCoroFramePtrFromHandle(ValueObject &valobj) {
-  ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
+static lldb::addr_t GetCoroFramePtrFromHandle(ValueObjectSP valobj_sp) {
   if (!valobj_sp)
-    return nullptr;
+    return LLDB_INVALID_ADDRESS;
 
   // We expect a single pointer in the `coroutine_handle` class.
   // We don't care about its name.
   if (valobj_sp->GetNumChildren() != 1)
-    return nullptr;
+    return LLDB_INVALID_ADDRESS;
   ValueObjectSP ptr_sp(valobj_sp->GetChildAtIndex(0, true));
   if (!ptr_sp)
-    return nullptr;
+    return LLDB_INVALID_ADDRESS;
   if (!ptr_sp->GetCompilerType().IsPointerType())
-    return nullptr;
-
-  return ptr_sp;
-}
-
-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();
+    return LLDB_INVALID_ADDRESS;
 
   AddressType addr_type;
-  lldb::addr_t frame_ptr_addr = frame_ptr_sp->GetPointerValue(&addr_type);
+  lldb::addr_t frame_ptr_addr = ptr_sp->GetPointerValue(&addr_type);
   if (!frame_ptr_addr || frame_ptr_addr == LLDB_INVALID_ADDRESS)
-    return nullptr;
+    return LLDB_INVALID_ADDRESS;
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
+  if (addr_type != AddressType::eAddressTypeLoad)
+    return LLDB_INVALID_ADDRESS;
+
+  return frame_ptr_addr;
+}
+
+static Function *ExtractFunction(lldb::TargetSP target_sp,
+                                 lldb::addr_t frame_ptr_addr, int offset) {
+  lldb::ProcessSP process_sp = target_sp->GetProcessSP();
+  auto ptr_size = process_sp->GetAddressByteSize();
 
   Status error;
   auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
@@ -60,12 +61,14 @@ static Function *ExtractFunction(ValueObjectSP &frame_ptr_sp, int offset) {
   return func_address.CalculateSymbolContextFunction();
 }
 
-static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
-  return ExtractFunction(frame_ptr_sp, 0);
+static Function *ExtractResumeFunction(lldb::TargetSP target_sp,
+                                       lldb::addr_t frame_ptr_addr) {
+  return ExtractFunction(target_sp, frame_ptr_addr, 0);
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
-  return ExtractFunction(frame_ptr_sp, 1);
+static Function *ExtractDestroyFunction(lldb::TargetSP target_sp,
+                                        lldb::addr_t frame_ptr_addr) {
+  return ExtractFunction(target_sp, frame_ptr_addr, 1);
 }
 
 static bool IsNoopCoroFunction(Function *f) {
@@ -125,43 +128,26 @@ 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) {
-  ValueObjectSP ptr_sp(GetCoroFramePtrFromHandle(valobj));
-  if (!ptr_sp)
+  lldb::addr_t frame_ptr_addr =
+      GetCoroFramePtrFromHandle(valobj.GetNonSyntheticValue());
+  if (frame_ptr_addr == LLDB_INVALID_ADDRESS)
     return false;
 
-  if (!ptr_sp->GetValueAsUnsigned(0)) {
+  if (frame_ptr_addr == 0) {
     stream << "nullptr";
     return true;
   }
-  if (IsNoopCoroFunction(ExtractResumeFunction(ptr_sp)) &&
-      IsNoopCoroFunction(ExtractDestroyFunction(ptr_sp))) {
+
+  lldb::TargetSP target_sp = valobj.GetTargetSP();
+  if (IsNoopCoroFunction(ExtractResumeFunction(target_sp, frame_ptr_addr)) &&
+      IsNoopCoroFunction(ExtractDestroyFunction(target_sp, frame_ptr_addr))) {
     stream << "noop_coroutine";
     return true;
   }
 
-  stream.Printf("coro frame = 0x%" PRIx64, ptr_sp->GetValueAsUnsigned(0));
+  stream.Printf("coro frame = 0x%" PRIx64, frame_ptr_addr);
   return true;
 }
 
@@ -178,39 +164,67 @@ lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 
 size_t lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
     CalculateNumChildren() {
-  if (!m_frame_ptr_sp)
+  if (!m_resume_ptr_sp || !m_destroy_ptr_sp)
     return 0;
 
-  return m_frame_ptr_sp->GetNumChildren();
+  return m_promise_ptr_sp ? 3 : 2;
 }
 
 lldb::ValueObjectSP lldb_private::formatters::
     StdlibCoroutineHandleSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
-  if (!m_frame_ptr_sp)
-    return lldb::ValueObjectSP();
-
-  return m_frame_ptr_sp->GetChildAtIndex(idx, true);
+  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();
 }
 
 bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
     Update() {
-  m_frame_ptr_sp.reset();
+  m_resume_ptr_sp.reset();
+  m_destroy_ptr_sp.reset();
+  m_promise_ptr_sp.reset();
 
-  ValueObjectSP valobj_sp = m_backend.GetSP();
+  ValueObjectSP valobj_sp = m_backend.GetNonSyntheticValue();
   if (!valobj_sp)
     return false;
 
-  ValueObjectSP ptr_sp(GetCoroFramePtrFromHandle(m_backend));
-  if (!ptr_sp)
+  lldb::addr_t frame_ptr_addr = GetCoroFramePtrFromHandle(valobj_sp);
+  if (frame_ptr_addr == 0 || frame_ptr_addr == LLDB_INVALID_ADDRESS)
     return false;
 
-  Function *resume_func = ExtractResumeFunction(ptr_sp);
-  Function *destroy_func = ExtractDestroyFunction(ptr_sp);
+  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);
 
-  if (IsNoopCoroFunction(resume_func) && IsNoopCoroFunction(destroy_func)) {
-    // For `std::noop_coroutine()`, we don't want to display any child nodes.
+  // For `std::noop_coroutine()`, we don't want to display any child nodes.
+  if (IsNoopCoroFunction(resume_func) && IsNoopCoroFunction(destroy_func))
     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(
@@ -219,21 +233,23 @@ 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
       promise_type = m_ast_importer->CopyType(*ast_ctx, inferred_type);
     }
   }
 
-  // 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());
+  // 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"));
 
   return false;
 }
@@ -245,10 +261,17 @@ bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 
 size_t StdlibCoroutineHandleSyntheticFrontEnd::GetIndexOfChildWithName(
     ConstString name) {
-  if (!m_frame_ptr_sp)
+  if (!m_resume_ptr_sp || !m_destroy_ptr_sp)
     return UINT32_MAX;
 
-  return m_frame_ptr_sp->GetIndexOfChildWithName(name);
+  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;
 }
 
 SyntheticChildrenFrontEnd *

lldb/source/Plugins/Language/CPlusPlus/Coroutines.h

@@ -47,7 +47,9 @@ class StdlibCoroutineHandleSyntheticFrontEnd
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
-  lldb::ValueObjectSP m_frame_ptr_sp;
+  lldb::ValueObjectSP m_resume_ptr_sp;
+  lldb::ValueObjectSP m_destroy_ptr_sp;
+  lldb::ValueObjectSP m_promise_ptr_sp;
   std::unique_ptr<lldb_private::ClangASTImporter> m_ast_importer;
 };
 

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", value="-1")
+                    ValueCheck(name="promise", dereference=ValueCheck(value="-1"))
                 ])
 
         # Run until after the `co_yield`