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

[Adrian Vogelsgesang via lldb-commits]

Thank you for that hint, Jim! This infrastructure indeed looks very
interesting for my use case.
I will most likely use the HistoryThread class to represent the backtrace
of the logical coroutine threads.

Is my understanding correct, that `GetExtendedBacktraceThreads` only allows
me to append additional stack trace frames to existing, physical
threads, though? I want to expose completely new threads instead. In a
sense, I want to display each pending request inside my asynchronous
multiplexer as its own thread, such that I can see not only the requests
which an OS-level thread is currently working on, but also the onse which
are currently queued/not yet processed/blocked on IO

On Fri, Aug 26, 2022 at 6:38 PM Jim Ingham  wrote:

>
>
> > On Aug 26, 2022, at 7:05 AM, Adrian Vogelsgesang via Phabricator via
> lldb-commits  wrote:
> >
> > 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)
>
> lldb has the notion of "extended backtrace threads" - backed by lldb's
> "History" threads - that it uses in a similar circumstance handling Darwin
> dispatch queues.  If you have a thread that is serving a Darwin "dispatch
> queue" SBThread.GetExtendedBacktraceThreads will return the backtrace of
> the thread that enqueued the work, at the point where the enqueuing is
> done.  I bet you could make the same setup work for these coroutines.
>
> Jim
>
>
> >
> >
> > 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 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`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D132624#3951525 , @avogelsgesang 
wrote:

> @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?

Ah sigh. :(   I reverted both https://reviews.llvm.org/D132624 and 
https://reviews.llvm.org/D132735 -- failed to comment here in 
https://reviews.llvm.org/D132624 that I reverted the change, and put the 
comment about https://reviews.llvm.org/D132735 in the wrong phabracator.  I 
don't know how I got the two mixed up in so many ways ;) but they are both 
backed out right now and we have a green status on the macOS Incremental CI bot 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/

The mixup I can see, but I don't know how I failed to comment in this one about 
it being reverted.  I must not have hit a Submit button, apologies.


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.

@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] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I temporarily reverted this via

  commit 2b2f2f66141d52dc0d3082ddd12805d36872a189
  Author: Jason Molenda 
  Date:   Fri Nov 25 12:28:28 2022 -0800
  
  Revert "[LLDB] Recognize `std::noop_coroutine()` in 
`std::coroutine_handle` pretty printer"
  
  This reverts commit 4346318f5c700f4e85f866610fb8328fc429319b.
  
  This test case is failing on macOS, reverting until it can be
  looked at more closely to unblock the macOS CI bots.
  
File 
"/Volumes/work/llvm/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py",
 line 121, in test_libcpp
  self.do_test(USE_LIBCPP)
File 
"/Volumes/work/llvm/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py",
 line 45, in do_test
  self.expect_expr("noop_hdl",
File 
"/Volumes/work/llvm/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 2441, in expect_expr
  value_check.check_value(self, eval_result, str(eval_result))
File 
"/Volumes/work/llvm/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py",
 line 306, in check_value
  test_base.assertEqual(self.expect_summary, val.GetSummary(),
  AssertionError: 'noop_coroutine' != 'coro frame = 0x14058'
  - noop_coroutine+ coro frame = 0x14058 : 
(std::coroutine_handle) $1 = coro frame = 0x14058 {
resume = 0x00013344 (a.out`___lldb_unnamed_symbol223)
destroy = 0x00013344 (a.out`___lldb_unnamed_symbol223)
  }
  Checking SBValue: (std::coroutine_handle) $1 = coro frame = 
0x14058 {
resume = 0x00013344 (a.out`___lldb_unnamed_symbol223)
destroy = 0x00013344 (a.out`___lldb_unnamed_symbol223)
  }
  
  Those lldb_unnamed_symbols are synthetic names that ObjectFileMachO
  adds to the symbol table, most often seen with stripped binaries,
  based off of the function start addresses for all the functions -
  if a function has no symbol name, lldb adds one of these names.
  This change was originally landed via https://reviews.llvm.org/D132624


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]

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=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",
+

[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=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=[
+

[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=[
+

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Mechanically this looks good to me.


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.

@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] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Chuanqi Xu via Phabricator via lldb-commits]

ChuanqiXu added a comment.

In D132624#3754299 , @avogelsgesang 
wrote:

> @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 
> .

You could sort these revisions by 'Edit Related Revisions' automatically.


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.

@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

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

[Jim Ingham via lldb-commits]

> On Aug 26, 2022, at 5:08 PM, Adrian Vogelsgesang 
>  wrote:
> 
> Thank you for that hint, Jim! This infrastructure indeed looks very 
> interesting for my use case.
> I will most likely use the HistoryThread class to represent the backtrace of 
> the logical coroutine threads.
> 
> Is my understanding correct, that `GetExtendedBacktraceThreads` only allows 
> me to append additional stack trace frames to existing, physical threads, 
> though? I want to expose completely new threads instead. In a sense, I want 
> to display each pending request inside my asynchronous multiplexer as its own 
> thread, such that I can see not only the requests which an OS-level thread is 
> currently working on, but also the onse which are currently queued/not yet 
> processed/blocked on IO

I don't know how the std::coroutine stuff works...  In what sense is a "pending 
request" a Thread?  Isn't it a bunch of state and a function pointer that's 
waiting around to be run?  In which case, maybe we need another entity to 
describe those?  This is something that we don't handle at all in the "dispatch 
queue" interface in lldb.  We don't have a way to say "show me all queued but 
not being serviced" work.  When we get around to supporting that we'd need to 
add something to describe "queue-able work".

OTOH, if you are really dealing something like a scheduler so that the "pending 
work" is threads that were moved off of "system threads" and left in memory 
till they are re-enqueued, then you do need a way to tell lldb about those 
threads.  There are two ways to do that in lldb.  One is the OperatingSystem 
Plugin, which requires only that you provide a "Register Context's" for threads 
that you know about but the system doesn't.  The stack pointer in the context 
you return should point to the stored stack, and then lldb will do the work of 
producing the rest of the stack frame for that thread.  The other way is using 
the ScriptedProcess feature, where you can just make up threads and their stack 
frames out of whole cloth.  If your thread is still backed by the stack memory 
it was using, then the OperatingSystem plugin way is the easiest way to go for 
this.

Jim

> 
> On Fri, Aug 26, 2022 at 6:38 PM Jim Ingham  > wrote:
>> 
>> 
>> > On Aug 26, 2022, at 7:05 AM, Adrian Vogelsgesang via Phabricator via 
>> > lldb-commits > > > wrote:
>> > 
>> > 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)
>> 
>> lldb has the notion of "extended backtrace threads" - backed by lldb's 
>> "History" threads - that it uses in a similar circumstance handling Darwin 
>> dispatch queues.  If you have a thread that is serving a Darwin "dispatch 
>> queue" SBThread.GetExtendedBacktraceThreads will return the backtrace of the 
>> thread that enqueued the work, at the point where the enqueuing is done.  I 
>> bet you could make the same setup work for these coroutines.
>> 
>> Jim
>> 
>> 
>> > 
>> > 
>> > 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 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 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 = 

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

[Jim Ingham via lldb-commits]

> On Aug 26, 2022, at 7:05 AM, Adrian Vogelsgesang via Phabricator via 
> lldb-commits  wrote:
> 
> 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)

lldb has the notion of "extended backtrace threads" - backed by lldb's 
"History" threads - that it uses in a similar circumstance handling Darwin 
dispatch queues.  If you have a thread that is serving a Darwin "dispatch 
queue" SBThread.GetExtendedBacktraceThreads will return the backtrace of the 
thread that enqueued the work, at the point where the enqueuing is done.  I bet 
you could make the same setup work for these coroutines.

Jim


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

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] D132624: [LLDB] Devirtualize coroutine promise types for `std::coroutine_handle`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D132624#3749263 , @avogelsgesang 
wrote:

>> 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 = 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)
>   }

The question isn't so much about the dereferencing of actual physical pointers, 
but more of a any "logical" pointers in the data structures and how they're 
being presented by the formatters. Most of our formatters dereference some 
kinds of pointers. For example, a std::set formatter may need to dereference 
hundreds of pointers to display all of the members. That is OK(*), because 
those elements are logically "inside" that set. Just like you cannot have a 
`struct S` hold itself as a member (only a pointer to `S`), you also cannot 
have a `std::set` contain itself (it would either have to be a pointer, or a 
set of a different type). And, I think, neither should a coroutine "contain" 
another coroutine. I don't know much about coroutines, but it seems like your 
goal is to format them like a linked list, so may 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 *`  ?

(*) However, the formatter should be wary of loops due to corrupted data 
structures. That's why our std::list formatter has an internal loop detection 
to prevent it from getting stuck on corrupted lists. AFAIK, our map formatter 
does not have that, probably because noone ran into that problem yet.


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

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

[Jim Ingham via lldb-commits]

> On Aug 25, 2022, at 8:11 AM, Pavel Labath via Phabricator via lldb-commits 
>  wrote:
> 
> labath added a comment.
> 
> In D132624#3748434 , @avogelsgesang 
> wrote:
> 
>>> 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
> 
> The best (I think) mechanism we have is the "pointer depth" limit (defaulting 
> to 1). It works fine on regular objects, but requires some care with 
> synthetic children. The fact that it does not kick in here 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. That is a 
> problem. Not only it creates excessively large outputs, it can even cause 
> lldb to hang (looping endlessly while trying to print "all" children) if the 
> data structures it is trying to print are circular (e.g. due to corruption).

There are actually two separate controls for the depth of child traversal:

(lldb) help v
...
   -D  ( --depth  )
Set the max recurse depth when dumping aggregate types (default is 
infinity).
...
   -P  ( --ptr-depth  )
The number of pointers to be traversed when dumping values (default 
is zero).

There have to be two, because aggregate types can't have cycles, so it's safe 
to set those to a high value, but pointer following can lead to cycles and we 
don't currently do cycle detection so you have to be more careful with this 
setting.

Jim


> 
> 
> 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 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.

> 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`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D132624#3748434 , @avogelsgesang 
wrote:

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

The best (I think) mechanism we have is the "pointer depth" limit (defaulting 
to 1). It works fine on regular objects, but requires some care with synthetic 
children. The fact that it does not kick in here 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. That is a problem. Not only it creates 
excessively large outputs, it can even cause lldb to hang (looping endlessly 
while trying to print "all" children) if the data structures it is trying to 
print are circular (e.g. due to corruption).


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`

[Chuanqi Xu via Phabricator via lldb-commits]

ChuanqiXu added a comment.

In D132624#3748434 , @avogelsgesang 
wrote:

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

Yeah, I am not objecting this. I understand it is bad to make perfect the enemy 
of better.


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`

[Chuanqi Xu via Phabricator via lldb-commits]

ChuanqiXu added a comment.

The only concern is that if it would be not so easy to read if there is 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?


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`

[Chuanqi Xu via Phabricator via lldb-commits]

ChuanqiXu added a comment.

LGTM basically. Thanks!


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