[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

@labath @aprantl Thanks for your reviews! I would like to backport this fix 
together with the fix from https://reviews.llvm.org/D132815 to the 16.0 release 
branch now. For that I would need your approval on 
https://github.com/llvm/llvm-project-release-prs/pull/254


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143127/new/

https://reviews.llvm.org/D143127

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG54d4a2550d31: [LLDB] Fix assertion failure by removing 
`CopyType` in `std::coroutine_handle`… (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143127/new/

https://reviews.llvm.org/D143127

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h


Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -16,8 +16,6 @@
 
 namespace lldb_private {
 
-class ClangASTImporter;
-
 namespace formatters {
 
 /// Summary provider for `std::coroutine_handle` from  libc++, libstdc++ and
@@ -50,7 +48,6 @@
   lldb::ValueObjectSP m_resume_ptr_sp;
   lldb::ValueObjectSP m_destroy_ptr_sp;
   lldb::ValueObjectSP m_promise_ptr_sp;
-  std::unique_ptr m_ast_importer;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -8,7 +8,6 @@
 
 #include "Coroutines.h"
 
-#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/VariableList.h"
@@ -97,8 +96,7 @@
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 StdlibCoroutineHandleSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp),
-  m_ast_importer(std::make_unique()) {
+: SyntheticChildrenFrontEnd(*valobj_sp) {
   if (valobj_sp)
 Update();
 }
@@ -174,8 +172,7 @@
 if (Function *destroy_func =
 ExtractDestroyFunction(target_sp, frame_ptr_addr)) {
   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);
+promise_type = inferred_type;
   }
 }
   }


Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -16,8 +16,6 @@
 
 namespace lldb_private {
 
-class ClangASTImporter;
-
 namespace formatters {
 
 /// Summary provider for `std::coroutine_handle` from  libc++, libstdc++ and
@@ -50,7 +48,6 @@
   lldb::ValueObjectSP m_resume_ptr_sp;
   lldb::ValueObjectSP m_destroy_ptr_sp;
   lldb::ValueObjectSP m_promise_ptr_sp;
-  std::unique_ptr m_ast_importer;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -8,7 +8,6 @@
 
 #include "Coroutines.h"
 
-#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/VariableList.h"
@@ -97,8 +96,7 @@
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 StdlibCoroutineHandleSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp),
-  m_ast_importer(std::make_unique()) {
+: SyntheticChildrenFrontEnd(*valobj_sp) {
   if (valobj_sp)
 Update();
 }
@@ -174,8 +172,7 @@
 if (Function *destroy_func =
 ExtractDestroyFunction(target_sp, frame_ptr_addr)) {
   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);
+promise_type = inferred_type;
   }
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

Agree, other pretty-printers also don't use `ASTImporter`. I think the call was 
still needed originally in 
https://github.com/llvm/llvm-project/commit/01f4c305fae9ff2f165ce0f635a90f8e2292308c,
 because the promise_type was combined with newly created AST types which were 
created in a different AST-instance (see `GetCoroutineFrameType` in this old 
version of the code). My interpretation now is: Building struct types which 
refer to types from other AST instances is not supported. Returning synthetic 
children from different with types from different ASTs is supported, thoguh.

I will wait until Wednesday morning before merging this fix, to give other 
potentially interested reviewers some time. After merging this to `main`, I 
will open a backport request for LLDB-16.

I am still a bit concerned that the `lldbassert` was not caught by the existing 
tests. It seems the tests are ignoring `lldbassert` given that those asserts 
only print some information to `stderr` but don't actually terminate lldb? I 
assume this is something which should be fixed globally for all API test? Do 
you have an idea how I could fix this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143127/new/

https://reviews.llvm.org/D143127

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> This change makes me a little nervous, but mostly because I don't quite 
> understand the design here.
> So ASTImporter does not have a check whether src context and dst context are 
> identical?

I am completely with you. I also don't understand the design of `ASTImporter`.
I had to learn the hard way that there are multiple ASTs around at the same 
time (one per object file + one for the "debugging session" in which all 
temporary types etc. are created).
But I don't quite understand their lifetimes, and which types of cross-AST 
linking is supported and in which cases types need to be copied over.

In that sense, I see this commit mostly as a basis for discussions, but I am 
not completely confident that this is the right solution. I would hope to find 
someone more experienced with lldb who can point me in the right direction :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143127/new/

https://reviews.llvm.org/D143127

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: labath, aprantl.
Herald added a subscriber: ChuanqiXu.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The pretty printer for `std::coroutine_handle` was running into

> Assertion failed: (target_ctx != source_ctx && "Can't import into itself")

from ClangASTImporter.h, line 270.

This commit fixes the issue by removing the `CopyType` call from the
pretty printer. While this call was necessary in the past, it seems to
be no longer required, at least all test cases are still passing. Maybe
something changed in the meantime around the handling of `TypesystemClang`
instances. I don't quite understand why `CopyType` was necessary earlier.

I am not sure how to add a regression test for this, though. It seems
the issue is already triggered by the exising `TestCoroutineHandle.py`,
but API tests seem to ignore all violations of `lldbassert` and still
report the test as "passed", even if assertions were triggered


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143127

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h


Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -16,8 +16,6 @@
 
 namespace lldb_private {
 
-class ClangASTImporter;
-
 namespace formatters {
 
 /// Summary provider for `std::coroutine_handle` from  libc++, libstdc++ and
@@ -50,7 +48,6 @@
   lldb::ValueObjectSP m_resume_ptr_sp;
   lldb::ValueObjectSP m_destroy_ptr_sp;
   lldb::ValueObjectSP m_promise_ptr_sp;
-  std::unique_ptr m_ast_importer;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -8,7 +8,6 @@
 
 #include "Coroutines.h"
 
-#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/VariableList.h"
@@ -97,8 +96,7 @@
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 StdlibCoroutineHandleSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp),
-  m_ast_importer(std::make_unique()) {
+: SyntheticChildrenFrontEnd(*valobj_sp) {
   if (valobj_sp)
 Update();
 }
@@ -174,8 +172,7 @@
 if (Function *destroy_func =
 ExtractDestroyFunction(target_sp, frame_ptr_addr)) {
   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);
+promise_type = inferred_type;
   }
 }
   }


Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -16,8 +16,6 @@
 
 namespace lldb_private {
 
-class ClangASTImporter;
-
 namespace formatters {
 
 /// Summary provider for `std::coroutine_handle` from  libc++, libstdc++ and
@@ -50,7 +48,6 @@
   lldb::ValueObjectSP m_resume_ptr_sp;
   lldb::ValueObjectSP m_destroy_ptr_sp;
   lldb::ValueObjectSP m_promise_ptr_sp;
-  std::unique_ptr m_ast_importer;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -8,7 +8,6 @@
 
 #include "Coroutines.h"
 
-#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/VariableList.h"
@@ -97,8 +96,7 @@
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
 StdlibCoroutineHandleSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-: SyntheticChildrenFrontEnd(*valobj_sp),
-  m_ast_importer(std::make_unique()) {
+: SyntheticChildrenFrontEnd(*valobj_sp) {
   if (valobj_sp)
 Update();
 }
@@ -174,8 +172,7 @@
 if (Function *destroy_func =
 ExtractDestroyFunction(target_sp, frame_ptr_addr)) {
   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);
+promise_type = inferred_type;
   }
 }
   }
__

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8aa313755118: [LLDB] Do not dereference promise pointer in 
`coroutine_handle` pretty printer (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

Files:
  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

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -61,7 +61,7 @@
 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`
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -47,7 +47,9 @@
   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 m_ast_importer;
 };
 
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -17,38 +17,37 @@
 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 *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
-  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 *ExtractDestroyFunction(lldb::TargetSP target_sp,
+lldb::addr_t frame_ptr_addr) {
+  lldb::ProcessSP process_sp = target_sp->GetProcessSP();
+  auto ptr_size = process_sp->GetAddressByteSize();
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
   auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
   lldb::addr_t destroy_func_addr =
   process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
@@ -59,12 +58,7 @@
   if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
-
-  return destroy_func;
+  return destroy_func_address.CalculateSymbolContextFunction();
 }
 
 static CompilerType InferPromiseType(Function &destroy_func) {
@@ -85,37 +79,19 @@
   return promise_type->GetForwardCompilerType();
 }
 
-static CompilerType GetCoroutineFrameType(TypeSystemClang &ast_ctx,
-  CompilerType promise_type) {
-  CompilerType

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

Since I am still blocked on solving the Apple-specific issues in 
https://reviews.llvm.org/D132735 and couldn't solve them in time for LLVM-16, I 
now rebased this change directly on `main`, so that it no longer depends on the 
refactorings from the other commit.

My idea is to backport this change to LLVM-16 after it landed successfully on 
`main`. I think including this commit in LLVM-16 makes sense, because I see it 
as a "bug fix": it fixes a potential infinite loop in the debugger.

https://reviews.llvm.org/D132735 on the other hand can wait until LLVM-17, 
given it's only a usability improvement/new feature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 493583.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

fix crash in cases where the promise type is unknown even after an attempt at 
devirtualization


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

Files:
  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

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -61,7 +61,7 @@
 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`
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -47,7 +47,9 @@
   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 m_ast_importer;
 };
 
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -17,38 +17,37 @@
 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 *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
-  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 *ExtractDestroyFunction(lldb::TargetSP target_sp,
+lldb::addr_t frame_ptr_addr) {
+  lldb::ProcessSP process_sp = target_sp->GetProcessSP();
+  auto ptr_size = process_sp->GetAddressByteSize();
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
   auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
   lldb::addr_t destroy_func_addr =
   process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
@@ -59,12 +58,7 @@
   if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
-
-  return destroy_func;
+  return destroy_func_address.CalculateSymbolContextFunction();
 }
 
 static CompilerType InferPromiseType(Function &destroy_func) {
@@ -85,37 +79,19 @@
   return promise_type->GetForwardCompilerType();
 }
 
-static CompilerType GetCoroutineFrameType(TypeSystemClang &ast_ctx,
-  CompilerType promise_type) {
- 

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

I reapplied this patch locally on top of the latest `main` and tried to 
reproduce the issue on my Mac using

> cmake -B build -G Ninja -C lldb/cmake/caches/Apple-lldb-macOS.cmake 
> -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" -DLLVM_ENABLE_PROJECTS="clang;lldb" 
> llvm
> ninja check-lldb

This gives me 10 failing tests, but none of them are related to the 
coroutine_handle pretty printer.

If I run

> ./bin/llvm-lit 
> ../lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle

I get

> UNSUPPORTED: lldb-api :: 
> functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
>  (1 of 1)

No idea why this test is marked as unsupportd.
Any idea how I can run this test case locally on my mac?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

The issue with this change was that if devirtualization is failing, then in the 
line

  lldb::ValueObjectSP promise = CreateValueObjectFromAddress(
"promise", frame_ptr_addr + 2 * ptr_size, exe_ctx, promise_type);

the `promise_type` is `void`. Creating an object of type void is obviously not 
possible.

Adding a simple additional `!promise_type.isVoid()` check will fix this. 
Waiting for resolution of the test failure on https://reviews.llvm.org/D132735 
first, though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a subscriber: jasonmolenda.
avogelsgesang added a comment.

As discussed via email, the reason for the test failures on green dragon is, 
that green dragon seems to be using some compiler which passes the test

  if not (is_clang and self.expectedCompilerVersion(["<", "16"])):

but does not actually provide the expected debug informations. @jasonmolenda Do 
you know which exact compiler is used on green-dragon?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

@jasonmolenda I am a bit confused about your revert.
Did you indeed revert this change here? Afaict, you reverted 
https://reviews.llvm.org/D132735 and https://reviews.llvm.org/D132815, but left 
this commit in place. I did not actually read through your code changes, 
though, I only looked at the commit messages so far. Is my understanding of 
which commits you actually reverted correct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
avogelsgesang marked an inline comment as done.
Closed by commit rGcd3091a88f7c: [LLDB] Do not dereference promise pointer in 
`coroutine_handle` pretty printer (authored by avogelsgesang).

Changed prior to commit:
  https://reviews.llvm.org/D132815?vs=461232&id=476774#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

Files:
  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

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -68,7 +68,7 @@
 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`
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -47,7 +47,9 @@
   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 m_ast_importer;
 };
 
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -17,34 +17,35 @@
 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 @@
   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) {

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4346318f5c70: [LLDB] Recognize `std::noop_coroutine()` in 
`std::coroutine_handle` pretty… (authored by avogelsgesang).

Changed prior to commit:
  https://reviews.llvm.org/D132735?vs=461235&id=476765#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -38,6 +38,13 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"])):
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
 if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -35,7 +35,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -47,24 +47,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
+
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 0);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopCoroFunction(Function *f) {
+  if (!f)
+return false;
 
-  return destroy_func;
+  // clang's `__builtin_coro_noop` gets lowered to
+  // `_NoopCoro_ResumeDestroy`. This is used by libc++
+  // on clang.
+  auto mangledName = f->GetMangled().GetMangledName();
+  if (mangledName == "__NoopCoro_ResumeDestroy")
+return true;
+
+  // libc++ uses the following name as a fallba

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG01f4c305fae9: Reapply "[LLDB] Devirtualize coroutine 
promise types for `std… (authored by avogelsgesang).

Changed prior to commit:
  https://reviews.llvm.org/D132624?vs=475327&id=476743#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

Files:
  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
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -43,6 +43,8 @@
   bool is_supported = is_implementation_supported();
   int_generator gen = my_generator_func();
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  std::coroutine_handle incorrectly_typed_hdl =
+  std::coroutine_handle::from_address(gen.hdl.address());
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -16,6 +16,7 @@
 def do_test(self, stdlib_type):
 """Test std::coroutine_handle is displayed correctly."""
 self.build(dictionary={stdlib_type: "1"})
+is_clang = self.expectedCompiler(["clang"])
 
 test_generator_func_ptr_re = re.compile(
 r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
@@ -37,14 +38,31 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-# For type-erased `coroutine_handle<>` we are missing the `promise`
-# but still show `resume` and `destroy`.
-self.expect_expr("type_erased_hdl",
-result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
-result_children=[
-ValueCheck(name="resume", summary = test_generator_func_ptr_re),
-ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
-])
+if is_clang:
+# For a type-erased `coroutine_handle<>`, we can still devirtualize
+# the promise call and display the correctly typed promise.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For an incorrectly typed `coroutine_handle`, we use the user-supplied
+# incorrect type instead of inferring the correct type. Strictly speaking,
+# incorrectly typed coroutine handles are undefined behavior. However,
+# it provides probably a better debugging experience if we display the
+# promise as seen by the program instead of fixing this bug based on
+# the available debug info.
+self.expect_expr("incorrectly_typed_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+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")
+])
 
 # Run until after the `co_yield`
 process = self.process()
@@ -73,6 +91,18 @@
 ValueCheck(name="current_value", value = "42"),
 ])
 ])
+if is_clang:
+# Devirtualization still works, also at the final suspension point, despite
+# the `resume` pointer being reset to a nullptr
+self.expect_expr("type_erased_hdl",
+resul

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

@aprantl I still had to adjust the code slightly to copy the type from the 
`ASTContext for '.../a.out'` to the `scratch ASTContext` (without this, my code 
was triggering an assert).
See https://reviews.llvm.org/D132624?vs=474793&id=475327 for the related 
changes.
Because I have no prior experience with `ClangASTImporter` and the usage of 
multiple different `TypeSystemClang` instances, I would appreciate if you could 
take another quick look


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 475327.
avogelsgesang added a comment.

Fix ownership of CompilerType between "scratch typesystem" and "module type 
system"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

Files:
  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
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -43,6 +43,8 @@
   bool is_supported = is_implementation_supported();
   int_generator gen = my_generator_func();
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  std::coroutine_handle incorrectly_typed_hdl =
+  std::coroutine_handle::from_address(gen.hdl.address());
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -16,6 +16,7 @@
 def do_test(self, stdlib_type):
 """Test std::coroutine_handle is displayed correctly."""
 self.build(dictionary={stdlib_type: "1"})
+is_clang = self.expectedCompiler(["clang"])
 
 test_generator_func_ptr_re = re.compile(
 r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
@@ -37,14 +38,31 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-# For type-erased `coroutine_handle<>` we are missing the `promise`
-# but still show `resume` and `destroy`.
-self.expect_expr("type_erased_hdl",
-result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
-result_children=[
-ValueCheck(name="resume", summary = test_generator_func_ptr_re),
-ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
-])
+if is_clang:
+# For a type-erased `coroutine_handle<>`, we can still devirtualize
+# the promise call and display the correctly typed promise.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For an incorrectly typed `coroutine_handle`, we use the user-supplied
+# incorrect type instead of inferring the correct type. Strictly speaking,
+# incorrectly typed coroutine handles are undefined behavior. However,
+# it provides probably a better debugging experience if we display the
+# promise as seen by the program instead of fixing this bug based on
+# the available debug info.
+self.expect_expr("incorrectly_typed_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+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")
+])
 
 # Run until after the `co_yield`
 process = self.process()
@@ -73,6 +91,18 @@
 ValueCheck(name="current_value", value = "42"),
 ])
 ])
+if is_clang:
+# Devirtualization still works, also at the final suspension point, despite
+# the `resume` pointer being reset to a nullptr
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCh

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG558db7787005: [LLDB] Devirtualize coroutine promise types 
for `std::coroutine_handle` (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -43,6 +43,8 @@
   bool is_supported = is_implementation_supported();
   int_generator gen = my_generator_func();
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  std::coroutine_handle incorrectly_typed_hdl =
+  std::coroutine_handle::from_address(gen.hdl.address());
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -16,6 +16,7 @@
 def do_test(self, stdlib_type):
 """Test std::coroutine_handle is displayed correctly."""
 self.build(dictionary={stdlib_type: "1"})
+is_clang = self.expectedCompiler(["clang"])
 
 test_generator_func_ptr_re = re.compile(
 r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
@@ -37,14 +38,31 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-# For type-erased `coroutine_handle<>` we are missing the `promise`
-# but still show `resume` and `destroy`.
-self.expect_expr("type_erased_hdl",
-result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
-result_children=[
-ValueCheck(name="resume", summary = test_generator_func_ptr_re),
-ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
-])
+if is_clang:
+# For a type-erased `coroutine_handle<>`, we can still devirtualize
+# the promise call and display the correctly typed promise.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For an incorrectly typed `coroutine_handle`, we use the user-supplied
+# incorrect type instead of inferring the correct type. Strictly speaking,
+# incorrectly typed coroutine handles are undefined behavior. However,
+# it provides probably a better debugging experience if we display the
+# promise as seen by the program instead of fixing this bug based on
+# the available debug info.
+self.expect_expr("incorrectly_typed_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+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")
+])
 
 # Run until after the `co_yield`
 process = self.process()
@@ -73,6 +91,18 @@
 ValueCheck(name="current_value", value = "42"),
 ])
 ])
+if is_clang:
+# Devirtualization still works, also at the final suspension point, despite
+# the `resume` pointer being reset to a nullptr
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resum

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

@labath @aprantl Thank you for your reviews on https://reviews.llvm.org/D132815 
and https://reviews.llvm.org/D132735
I would appreciate if you also have time to review this change here, as it is a 
pre-requisite for the other two changes :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 461235.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

rename `IsNoopResumeDestroy` -> `IsNoopCoroFunction`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -38,6 +38,13 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"])):
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
 if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return destroy_func;
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 0);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopCoroFunction(Function *f) {
+  if (!f)
+return false;
+
+  // clang's `__builtin_coro_noop` gets lowered to
+  // `_NoopCoro_ResumeDestroy`. This is used by libc++
+  // on clang.
+  auto mangledName = f->GetMangled().GetMangledName();
+  if (mangledName == "__NoopCoro_ResumeDestroy")
+return true;
+
+  // libc++ uses the following name as a fallback on
+  // compilers without `__builtin_coro_noop`.
+  auto name = f->GetNameNoArguments();
+  static RegularExpression libcxxRegex(
+  "^std::coroutine_handle::"

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang marked 4 inline comments as done.
avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:19
 
-static ValueObjectSP GetCoroFramePtrFromHandle(ValueObject &valobj) {
-  ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
+static uint64_t GetCoroFramePtrFromHandle(ValueObjectSP valobj_sp) {
   if (!valobj_sp)

hawkinsw wrote:
> Do we want to return a `lldb::addr_t`? I know that they are likely the same, 
> but do we want it for consistency?
> 
> I see (below) several instances where we are passing around "addresses" and 
> they are plain `uint64_t` so I am obviously being unhelpful. Sorry!
changed to `lldb::addr_t` in this complete file


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> wondering if we couldn't fix this by creating the (non-pointer) object using 
> the CreateValueObjectFromAddress function, as above, but then actually use 
> valobj->AddressOf as the synthetic child

yes, that worked quite nicely


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 461232.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

Files:
  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

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -68,7 +68,7 @@
 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`
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -44,7 +44,9 @@
   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;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -16,34 +16,35 @@
 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;
@@ -59,12 +60,14 @@
   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 IsNoopResumeDestroy(Function *f) {
@@ -125,43 +128,26 @@
   return promise_type->GetForwardCompilerType();
 }
 
-static CompilerType GetCoroutineFrameType(TypeSystemClang &ast_ct

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

labath wrote:
> avogelsgesang wrote:
> > labath wrote:
> > > Have you checked there won't be a use-after-free problem here, given that 
> > > this data extractor will refer to the stack object?
> > > 
> > > To create persistent data, you need to use the DataBufferSP constructor, 
> > > but I'm wondering if we couldn't fix this by creating the (non-pointer) 
> > > object using the `CreateValueObjectFromAddress` function, as above, but 
> > > then actually use valobj->AddressOf as the synthetic child.
> > > 
> > > I am also somewhat surprised that we need to use the GetAddressOf trick 
> > > here, as this seems to indicate that the coroutine contains (in the 
> > > proper C "subobject" kind of way) the promise object. That's not 
> > > necessarily wrong, but it makes me think we may be "breaking the cycle" 
> > > at the wrong place.
> > Thanks for taking a look!
> > 
> > > To create persistent data, you need to use the DataBufferSP constructor
> > 
> > good point, I will keep this in mind as a fallback in case we don't decide 
> > to follow any of the other directions you hinted at.
> > 
> > > wondering if we couldn't fix this by creating the (non-pointer) object 
> > > using the CreateValueObjectFromAddress function, as above, but then 
> > > actually use valobj->AddressOf as the synthetic child
> > 
> > Good idea! I will give it a try
> > 
> > 
> > > [...] as this seems to indicate that the coroutine contains (in the 
> > > proper C "subobject" kind of way) the promise object. That's not 
> > > necessarily wrong, but it makes me think we may be "breaking the cycle" 
> > > at the wrong place.
> > 
> > The physical layout of this is:
> > ```
> > // in the standard library
> > template
> > struct exception_handle {
> > __coro_frame* __hdl; // <--- this is the pointer we read 
> > with `GetCoroFramePtrFromHandle`
> > };
> > 
> > // compiler-generated coroutine frame. Generated ad-hoc per coroutine
> > struct __coro_frame {
> >  // The ABI guaranteees that hose two pointers are always the first two 
> > pointers in the struct.
> >  void (*resume)(void*); // function pointer for type erasure
> >  void (*destroy)(void*); // function pointer for type erasure
> >  // Next comes our promise type. This is under the control of the 
> > program author
> >  promise_type promise;
> >  // Next comes any compiler-generated, internal state which gets 
> > persisted across suspension points. 
> >  // The functions pointed to by `resume`/`destroy` know how to 
> > interpret this part of the coroutine frame.
> >  int __suspension_point_id;
> >  double __some_internal_state;
> >  std::string __some_other_internal_state;
> >  
> > };
> > ```
> > 
> > The programmer (i.e., most likely the user of this pretty-printer), wrote 
> > only the "promise" explicitly in his code. Everything else is 
> > compiler-generated. As such, the lldb-user will usually look for the 
> > "promise" first, and I would like to make it easy to find it, by exposing 
> > it as a top-level children of the `exception_handle` instead of hiding it 
> > inside a sub-child.
> > As such, the lldb-user will usually look for the "promise" first, and I 
> > would like to make it easy to find it, by exposing it as a top-level 
> > children of the `exception_handle` instead of hiding it inside a sub-child.
> 
> That makes sense. And I think that's a good argument for automatically 
> "dereferencing" that object (i.e., status quo). That said, it's not fully 
> clear to me how do we end up looping here. I take it the promise object 
> contains a (compiler-generated ?) pointer to another __coro_frame object? 
> What would happen if we turned *that* into a pointer?
> I take it the promise object contains a [...] pointer to another __coro_frame 
> object?

yes

> (compiler-generated ?)

no

We end up looping if the user explicitly puts an `coroutine_handle` inside 
`promise_type`. You can find an example of this in 
https://clang.llvm.org/docs/DebuggingCoroutines.html#get-the-asynchronous-stack,
 in particular

```
struct promise_type {
// [...] shortened for readability

std::coroutine_handle<> continuation = std::noop_coroutine();
int result = 0;
};
```

In asynchronous programming, it is common to "chain continuations" by putting a 
`coroutine_handle` into the `promise_type`. This coroutine_handle inside the 
promise_type gives my asynchronous coroutine the answer to the question "where 
should I continue next, after finishing the asychronous operation modelled by 
my own coroutine?".

In normal situations (i.e. in the absence of bugs inside the debugged program), 
those coroutine_handle's should not

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

labath wrote:
> Have you checked there won't be a use-after-free problem here, given that 
> this data extractor will refer to the stack object?
> 
> To create persistent data, you need to use the DataBufferSP constructor, but 
> I'm wondering if we couldn't fix this by creating the (non-pointer) object 
> using the `CreateValueObjectFromAddress` function, as above, but then 
> actually use valobj->AddressOf as the synthetic child.
> 
> I am also somewhat surprised that we need to use the GetAddressOf trick here, 
> as this seems to indicate that the coroutine contains (in the proper C 
> "subobject" kind of way) the promise object. That's not necessarily wrong, 
> but it makes me think we may be "breaking the cycle" at the wrong place.
Thanks for taking a look!

> To create persistent data, you need to use the DataBufferSP constructor

good point, I will keep this in mind as a fallback in case we don't decide to 
follow any of the other directions you hinted at.

> wondering if we couldn't fix this by creating the (non-pointer) object using 
> the CreateValueObjectFromAddress function, as above, but then actually use 
> valobj->AddressOf as the synthetic child

Good idea! I will give it a try


> [...] as this seems to indicate that the coroutine contains (in the proper C 
> "subobject" kind of way) the promise object. That's not necessarily wrong, 
> but it makes me think we may be "breaking the cycle" at the wrong place.

The physical layout of this is:
```
// in the standard library
template
struct exception_handle {
__coro_frame* __hdl; // <--- this is the pointer we read with 
`GetCoroFramePtrFromHandle`
};

// compiler-generated coroutine frame. Generated ad-hoc per coroutine
struct __coro_frame {
 // The ABI guaranteees that hose two pointers are always the first two 
pointers in the struct.
 void (*resume)(void*); // function pointer for type erasure
 void (*destroy)(void*); // function pointer for type erasure
 // Next comes our promise type. This is under the control of the program 
author
 promise_type promise;
 // Next comes any compiler-generated, internal state which gets persisted 
across suspension points. 
 // The functions pointed to by `resume`/`destroy` know how to interpret 
this part of the coroutine frame.
 int __suspension_point_id;
 double __some_internal_state;
 std::string __some_other_internal_state;
 
};
```

The programmer (i.e., most likely the user of this pretty-printer), wrote only 
the "promise" explicitly in his code. Everything else is compiler-generated. As 
such, the lldb-user will usually look for the "promise" first, and I would like 
to make it easy to find it, by exposing it as a top-level children of the 
`exception_handle` instead of hiding it inside a sub-child.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132815/new/

https://reviews.llvm.org/D132815

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

@labath D132815  addresses your point: With 
that change, the `promise` member now is typed as a pointer.

I would prefer to land the patches in the order D132624 
 (this review), D132735 
, D132815 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: labath, dblaikie, aprantl, ChuanqiXu, mib.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132815

Files:
  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

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -68,7 +68,7 @@
 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`
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
@@ -44,7 +44,9 @@
   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;
 };
 
 SyntheticChildrenFrontEnd *
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -16,34 +16,35 @@
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
-static ValueObjectSP GetCoroFramePtrFromHandle(ValueObject &valobj) {
-  ValueObjectSP valobj_sp(valobj.GetNonSyntheticValue());
+static uint64_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,
+ uint64_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;
@@ -59,12 +60,14 @@
   return func_address.CalculateSymbolContextFunction();
 }
 
-static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
-  return ExtractFunction(frame_ptr_sp, 0);
+static Function *ExtractResumeFunction(lldb::TargetSP target_sp,
+   uint64_t frame

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 456155.
avogelsgesang added a comment.

fix test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -38,6 +38,13 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"])):
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
 if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return destroy_func;
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 0);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopResumeDestroy(Function *f) {
+  if (!f)
+return false;
+
+  // clang's `__builtin_coro_noop` gets lowered to
+  // `_NoopCoro_ResumeDestroy`. This is used by libc++
+  // on clang.
+  auto mangledName = f->GetMangled().GetMangledName();
+  if (mangledName == "__NoopCoro_ResumeDestroy")
+return true;
+
+  // libc++ uses the following name as a fallback on
+  // compilers without `__builtin_coro_noop`.
+  auto name = f->GetNameNoArguments();
+  static RegularExpression libcxxRegex(
+  "^std::coroutine_handle::"
+  "__noop_coroutine_frame_ty_::__dummy_resume_destroy_func$");
+  lldbassert(libc

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 456062.
avogelsgesang added a comment.

fix offset


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -38,6 +38,14 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"]:
+assert(False)
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
 if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return destroy_func;
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 0);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopResumeDestroy(Function *f) {
+  if (!f)
+return false;
+
+  // clang's `__builtin_coro_noop` gets lowered to
+  // `_NoopCoro_ResumeDestroy`. This is used by libc++
+  // on clang.
+  auto mangledName = f->GetMangled().GetMangledName();
+  if (mangledName == "__NoopCoro_ResumeDestroy")
+return true;
+
+  // libc++ uses the following name as a fallback on
+  // compilers without `__builtin_coro_noop`.
+  auto name = f->GetNameNoArguments();
+  static RegularExpression libcxxRegex(
+  "^std::coroutine_handle::"
+  "__noop_coroutine_frame_ty_::__dummy_resume_destroy_func$

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 456060.
avogelsgesang added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -38,6 +38,14 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"]:
+assert(False)
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
 if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return destroy_func;
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopResumeDestroy(Function *f) {
+  if (!f)
+return false;
+
+  // clang's `__builtin_coro_noop` gets lowered to
+  // `_NoopCoro_ResumeDestroy`. This is used by libc++
+  // on clang.
+  auto mangledName = f->GetMangled().GetMangledName();
+  if (mangledName == "__NoopCoro_ResumeDestroy")
+return true;
+
+  // libc++ uses the following name as a fallback on
+  // compilers without `__builtin_coro_noop`.
+  auto name = f->GetNameNoArguments();
+  static RegularExpression libcxxRegex(
+  "^std::coroutine_handle::"
+  "__noop_coroutine_frame_ty_::__dummy_resume_destroy_func$");

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 456055.
avogelsgesang added a comment.

disable tests on gcc


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -43,6 +43,8 @@
   bool is_supported = is_implementation_supported();
   int_generator gen = my_generator_func();
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  std::coroutine_handle incorrectly_typed_hdl =
+  std::coroutine_handle::from_address(gen.hdl.address());
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -16,6 +16,7 @@
 def do_test(self, stdlib_type):
 """Test std::coroutine_handle is displayed correctly."""
 self.build(dictionary={stdlib_type: "1"})
+is_clang = self.expectedCompiler(["clang"])
 
 test_generator_func_ptr_re = re.compile(
 r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
@@ -37,14 +38,31 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-# For type-erased `coroutine_handle<>` we are missing the `promise`
-# but still show `resume` and `destroy`.
-self.expect_expr("type_erased_hdl",
-result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
-result_children=[
-ValueCheck(name="resume", summary = test_generator_func_ptr_re),
-ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
-])
+if is_clang:
+# For a type-erased `coroutine_handle<>`, we can still devirtualize
+# the promise call and display the correctly typed promise.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For an incorrectly typed `coroutine_handle`, we use the user-supplied
+# incorrect type instead of inferring the correct type. Strictly speaking,
+# incorrectly typed coroutine handles are undefined behavior. However,
+# it provides probably a better debugging experience if we display the
+# promise as seen by the program instead of fixing this bug based on
+# the available debug info.
+self.expect_expr("incorrectly_typed_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+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")
+])
 
 # Run until after the `co_yield`
 process = self.process()
@@ -73,6 +91,18 @@
 ValueCheck(name="current_value", value = "42"),
 ])
 ])
+if is_clang:
+# Devirtualization still works, also at the final suspension point, despite
+# the `resume` pointer being reset to a nullptr
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", value = "0x"),
+ValueCheck(name="destroy", summary = test_gener

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> I don't know much about coroutines, but it seems like your goal is to format 
> them like a linked list

actually, my preferred goal would be to show them as a logical, user-level 
thread. Such that you can type

  thread backtrace cxxcoro:0xb2a0

to get the backtrace of the logical coroutine thread routed at the coroutine at 
address `0xb2a0`, or maybe even

  thread backtrace cxxcoro:hdl

where `hdl` is evaluated as an expression to identify the coroutine handle from 
where to dump the backtrace.

Also, it would be neat if those logical threads show up in `thread list`...

But it seems there is currently no infrastructure yet in LLDB for logical 
threads provided by `LanguageRuntime` plugins.

I guess at some point, I will write an RFC about that on discourse. But before 
that, I will first do some more exploration on how LLDB works and I will first 
grab the low-hanging fruits (like a data formatter for `std::coroutine_handle` 
and patching LLVM to emit the necessary debug info)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> I don't know much about coroutines, but it seems like your goal is to format 
> them like a linked list

Yes, and no. Coroutines are a compiler-provided type-erasure mechanism which 
(among other things) erase the type of the contained promise.
One common use case is that the library types (`std::generator`, the upcoming 
`std::lazy`, user-defined types, ...) store a "next"/"continuation" pointer 
inside that promise.

> so maybe the (synthetic) object which represents the "next" coroutine 
> (promise?) in the list should be of a pointer type, forcing the lldb (and 
> user) to do an actual dereference operation before viewing the next element. 
> Maybe that could be achieved by changing the type of 
> __promise.continuation.promise (I'm using the example from your patch 
> description) from std::...::promise_type to std::...::promise_type * ?

Good idea. That should work!

I will prepare a separate review which uses this approach. I would prefer to 
still first land this patch, then D132735 , 
because I want to reuse helper functions from those two commits for making the 
proposed change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 455893.
avogelsgesang added a comment.

Exclude unsupported compilers from test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132735/new/

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -45,6 +45,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -16,6 +16,7 @@
 def do_test(self, stdlib_type):
 """Test std::coroutine_handle is displayed correctly."""
 self.build(dictionary={stdlib_type: "1"})
+is_clang = self.expectedCompiler(["clang"])
 
 test_generator_func_ptr_re = re.compile(
 r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
@@ -37,7 +38,15 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-if "clang" in self.getCompiler():
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+# clang version < 16 did not yet write debug info for the noop coroutines.
+if not (is_clang and self.expectedCompilerVersion(["<", "16"]:
+assert(False)
+self.expect_expr("noop_hdl",
+result_summary="noop_coroutine",
+result_children=[])
+if is_clang:
 # For a type-erased `coroutine_handle<>`, we can still devirtualize
 # the promise call and display the correctly typed promise.
 self.expect_expr("type_erased_hdl",
@@ -90,7 +99,7 @@
 ValueCheck(name="current_value", value = "42"),
 ])
 ])
-if "clang" in self.getCompiler():
+if is_clang:
 # Devirtualization still works, also at the final suspension point, despite
 # the `resume` pointer being reset to a nullptr
 self.expect_expr("type_erased_hdl",
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`.
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return d

[Lldb-commits] [PATCH] D132735: [LLDB] Recognize `std::noop_coroutine()` in `std::coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: ChuanqiXu, aprantl, dblaikie, ychen, jryans, 
JDevlieghere.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

With this commit, the `std::coroutine_handle` pretty printer now
recognizes `std::noop_coroutine()` handles. For noop coroutine handles,
we identify use the summary string `noop_coroutine` and we don't print
children

Instead of

  (std::coroutine_handle) $3 = coro frame = 0x9058 {
resume = 0x64f0 
(a.out`std::__1::coroutine_handle::__noop_coroutine_frame_ty_::__dummy_resume_destroy_func()
 at noop_coroutine_handle.h:79)
destroy = 0x64f0 
(a.out`std::__1::coroutine_handle::__noop_coroutine_frame_ty_::__dummy_resume_destroy_func()
 at noop_coroutine_handle.h:79)
  }

we now print

  (std::coroutine_handle) $3 = noop_coroutine


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132735

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -36,6 +36,7 @@
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
   std::coroutine_handle incorrectly_typed_hdl =
   std::coroutine_handle::from_address(gen.hdl.address());
+  std::coroutine_handle<> noop_hdl = std::noop_coroutine();
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -57,6 +57,11 @@
 ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
 ValueCheck(name="promise", value="-1")
 ])
+# We recognize and pretty-print `std::noop_coroutine`. We don't display
+# any children as those are irrelevant for the noop coroutine.
+self.expect_expr("noop_hdl",
+result_summary=re.compile("^noop_coroutine$"),
+result_children=[])
 
 # Run until after the `co_yield`
 process = self.process()
Index: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -34,7 +34,7 @@
   return ptr_sp;
 }
 
-static Function *ExtractDestroyFunction(ValueObjectSP &frame_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();
@@ -46,24 +46,64 @@
   lldbassert(addr_type == AddressType::eAddressTypeLoad);
 
   Status error;
-  // The destroy pointer is the 2nd pointer inside the compiler-generated
-  // `pair`. It
-  auto destroy_func_ptr_addr = frame_ptr_addr + ptr_size;
-  lldb::addr_t destroy_func_addr =
-  process_sp->ReadPointerFromMemory(destroy_func_ptr_addr, error);
+  auto func_ptr_addr = frame_ptr_addr + offset * ptr_size;
+  lldb::addr_t func_addr =
+  process_sp->ReadPointerFromMemory(func_ptr_addr, error);
   if (error.Fail())
 return nullptr;
 
-  Address destroy_func_address;
-  if (!target_sp->ResolveLoadAddress(destroy_func_addr, destroy_func_address))
+  Address func_address;
+  if (!target_sp->ResolveLoadAddress(func_addr, func_address))
 return nullptr;
 
-  Function *destroy_func =
-  destroy_func_address.CalculateSymbolContextFunction();
-  if (!destroy_func)
-return nullptr;
+  return func_address.CalculateSymbolContextFunction();
+}
 
-  return destroy_func;
+static Function *ExtractResumeFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static Function *ExtractDestroyFunction(ValueObjectSP &frame_ptr_sp) {
+  return ExtractFunction(frame_ptr_sp, 1);
+}
+
+static bool IsNoopResumeDestroy(Function *f) {

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> leads me to believe that the data formatter is creating the synthetic 
> children as non pointer objects (e.g. by automatically dereferencing any 
> nested pointers), even though the structures clearly contain some pointers 
> inside

Yes, that is correct. The formatter (introduced recently in 
https://reviews.llvm.org/D132415) does dereference the pointer internally. It 
replaces the

  (std::coroutine_handle<>) hdl = {
__handle_ = 0xb2a0
  }

by

  (std::coroutine_handle<>) hdl = {
  resume = 0x5a10 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  destroy = 0x6090 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  }

by dereferencing the pointer and directly exposing its children.

I could not find a good way to not dereference the pointer internally. Maybe 
you can help me find a better way?

I tried to just adjust the pointer type of the `__handle_` member, but then I 
got multiple problems:

1. Unnecessary clutter:

  (std::coroutine_handle<>) hdl = {
  __handle_ = {
  resume = 0x5a10 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  destroy = 0x6090 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  }
  }

The additional `__handle_` in it is just visual clutter.

Not expanded by default:

By default, if I write `p hdl`, LLDB did not expand its children and just 
displayed

  (std::coroutine_handle<>) hdl = {
__handle_ = 0xb2a0
  }

without an indication that there was more to see inside the pointer.

Also, when I write `p hdl.__handle_`, LLDB did not expand the pointer. Because 
now the `hdl.__handle_` access is done with C++ semantics, returning a `void*` 
and hence the data formatter for `std::exception_handle` did not kick in


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added a comment.

> The only concern is that if it would be not so easy to read if there are too 
> many levels? (Consider the 30levels example in D132451 
> ). If it would be better to print ... at 
> certain level? Or this is already handled by lldb?

Agree, it would be better to limit the printing at some point. However, afaict, 
limiting the depth to which children are expanded cannot be done from inside 
the data formatter/synthetic children frontend. I am not sure if lldb already 
has a separate mechanism in place to limit printing in this case. If it does 
not, I think it makes sense to add it, but that would be a separate commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132624/new/

https://reviews.llvm.org/D132624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: Chuanfeng, aprantl, dblaikie, ychen, jryans, 
JDevlieghere.
Herald added subscribers: ChuanqiXu, Prazek.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This commit teaches the `std::coroutine_handle` pretty-printer to
devirtualize type-erased promise types. This is particularly useful to
resonstruct call stacks, either of asynchronous control flow or of
recursive invocations of `std::generator`. For the example recently
introduced by https://reviews.llvm.org/D132451, printing the `__promise`
variable now shows

  (std::__coroutine_traits_sfinae::promise_type) __promise = {
continuation = coro frame = 0x55562430 {
  resume = 0x6310 (a.out`task detail::chain_fn<1>() at 
llvm-nested-example.cpp:66)
  destroy = 0x6700 (a.out`task detail::chain_fn<1>() at 
llvm-nested-example.cpp:66)
  promise = {
continuation = coro frame = 0x555623e0 {
  resume = 0x7070 (a.out`task detail::chain_fn<2>() at 
llvm-nested-example.cpp:66)
  destroy = 0x7460 (a.out`task detail::chain_fn<2>() at 
llvm-nested-example.cpp:66)
  promise = {
...
  }
}
result = 0
  }
}
result = 0
  }

(shortened to keep the commit message readable) instead of

  (std::__coroutine_traits_sfinae::promise_type) __promise = {
continuation = coro frame = 0x55562430 {
  resume = 0x6310 (a.out`task detail::chain_fn<1>() at 
llvm-nested-example.cpp:66)
  destroy = 0x6700 (a.out`task detail::chain_fn<1>() at 
llvm-nested-example.cpp:66)
}
result = 0
  }

Note how the new debug output reveals the complete asynchronous call
stack: our own function resumes `chain_fn<1>` which in turn will resume
`chain_fn<2>` and so on. Thereby this change allows users of lldb to
inspect the logical coroutine call stack without using any custom debug
scripts (although the display is still a bit clumsy. It would be nicer
to also integrate this into lldb's backtrace feature, but I don't know
how to do so)

The devirtualization currently works by introspecting the function
pointed to by the `destroy` pointer. (The `resume` pointer is not worth
much, given that for the final suspend point `resume` is set to a
nullptr. We have to use the `destroy` pointer instead.) We then look
for a `__promise` variable inside the `destroy` function. This
`__promise` variable is synthetically generated by LLVM, and looking at
its type reveals the type-erased promise_type.

This approach only works for clang-generated code, though. While gcc
also adds a `_Coro_promise` variable to the `resume` function, it does
not do so for the `destroy` function. However, we can't use the `resume`
function, as it will be reset to a nullptr at the final suspension
point. For the time being, I am happy with de-virtualization only working
for clang. A follow-up commit will further improve devirtualization and
also expose the variables spilled to the coroutine frame. As part of
this, I will also revisit gcc support.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132624

Files:
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -34,6 +34,8 @@
 int main() {
   int_generator gen = my_generator_func();
   std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  std::coroutine_handle incorrectly_typed_hdl =
+  std::coroutine_handle::from_address(gen.hdl.address());
   gen.hdl.resume();// Break at initial_suspend
   gen.hdl.resume();// Break after co_yield
   empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -33,13 +33,29 @@
 ValueCheck(name="current_value", value = "-1"),
 ])
 ])
-# For type-erased `coroutin

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG91389000abe8: [LLDB] Add data formatter for 
std::coroutine_handle (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated val

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:39
+  CompilerType coro_func_type = ast_ctx.CreateFunctionType(
+  /*result_type*/ void_type, /*args*/ &void_type, /*num_args*/ 1,
+  /*is_variadic*/ false, /*qualifiers*/ 0);

shafik wrote:
> `/*result_type=*/` see 
> https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
> 
> applies to the rest of them as well.
done


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 455111.
avogelsgesang marked an inline comment as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCh

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py:15
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):

jingham wrote:
> aprantl wrote:
> > Nice test!
> Would you mind changing moving this into lldbutil.py?  This seems generally 
> useful.  If you return the thread & breakpoint it would be even more 
> generally useful?
> moving this into lldbutil.py

done


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 455022.
avogelsgesang marked 4 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCh

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang marked an inline comment as done.
avogelsgesang added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py:44
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))

aprantl wrote:
> Does this launch a new process? It might be faster to just set an additional 
> breakpoint and run `process.Continue()`.
somehow, I assumed `run_to_source_breakpoint` would actually continue the 
running process instead of starting a new process. But looking into 
`run_to_source_breakpoint`, you are right, this launches a new process...

I changed this to instead continue execution of the already launched process


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 454911.
avogelsgesang marked 6 inline comments as done.
avogelsgesang added a comment.

addressed comments from @aprantl


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,87 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def continue_to_source_breakpoint(self, bkpt_pattern, source_spec):
+target = self.target()
+breakpoint = target.BreakpointCreateBySourceRegex(
+bkpt_pattern, source_spec, None)
+self.assertTrue(breakpoint, VALID_BREAKPOINT)
+threads = lldbutil.continue_to_breakpoint(self.process(), breakpoint)
+self.assertEqual(len(threads), 1)
+
+
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+   

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 454830.
avogelsgesang added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "42"),
+])
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break at final_suspend',
+lldb.SBFileSpec("ma

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang updated this revision to Diff 454728.
avogelsgesang added a comment.

remove unrelated changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  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/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "42"),
+])
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break at final_suspend',
+lldb.SB

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:1139
 
+  AddCXXSynthetic( // XXX
+  cpp_category_sp,

I need to remove this. This is a left-over from an earlier implementation sketch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132415/new/

https://reviews.llvm.org/D132415

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: ChuanqiXu, aprantl, labath, mib, JDevlieghere.
Herald added a subscriber: mgorny.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

This patch adds a formatter for `std::coroutine_handle`, both for libc++
and libstdc++. For the type-erased `coroutine_handle<>`, it shows the
`resume` and `destroy` function pointers. For a non-type-erased
`coroutine_handle` it also shows the `promise` value.

With this change, executing the `v t` command on the example from
https://clang.llvm.org/docs/DebuggingCoroutines.html now outputs

  (task) t = {
handle = coro frame = 0xb2a0 {
  resume = 0x5a10 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  destroy = 0x6090 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
}
  }

instead of just

  (task) t = {
handle = {
  __handle_ = 0xb2a0
}
  }

Note, how the symbols for the `resume` and `destroy` function pointer
reveals which coroutine is stored inside the `std::coroutine_handle`.
A follow-up commit will use this fact to infer the coroutine's promise
type and the representation of its internal coroutine state based on
the `resume` and`destroy` pointers.

The same formatter is used for both libc++ and libstdc++. It would
also work for MSVC's standard library, however it is not registered
for MSVC, given that lldb does not provide pretty printers for other
MSVC types, either.

The formatter is in a new added  `Coroutines.{h,cpp}` file because there
does not seem to be an already existing place where we could share
formatters across libc++ and libstdc++. Also, I expect this code to grow
as we improve debugging experience for coroutines further.

**Testing**

- Added API test


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+