[RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Dennis Lambe Jr.]

When frame filters are enabled and a stack unwinder does something to
cause an async record to be emitted, -stack-list-frames output is
incorrect. A comma is missing from the first async record, and a
spurious comma is inserted before the first frame= of the frame list.

Example:

-stack-list-frames
=cmd-param-changedparam="language",value="c++"
=cmd-param-changed,param="language",value="auto"
^done,stack=[,frame={level="0",addr="0x0000555555555148",func="main",file="/home/malsyned/projects/gdb-comma-bug/main.c",line="4",arch="i386:x86-64"}]

This happens because async record emitters like
mi_interp::on_param_changed() are written as though calling
mi_out::do_redirect() ensures that a comma will be emitted by the next
following mi_ui_out::field_string() call. It's not guaranteed, though.
It usually works out by coincidence because the emitter methods are
called when no other output record is mid-creation.

Emitting async records from a Python unwinder definitely breaks that
assumption, but it wouldn't surprise me if there were other ways to
violate it. The way async record emitter methods are written implies to
me that they are meant to be able to operate correctly in this context,
but when emitted by stack unwinders, they don't always.

This patch set works by adding an optional `bool async` parameter to
ui_out::redirect() and related methods. Most classes derived from ui_out
ignore this parameter, but mi_ui_out uses it to decide whether to push a
new m_suppress_field_separator onto a stack.

RFC: This version of the patch set only changes
mi_interp::on_param_changed(), because that's the async record my code
was emitting when I found this bug and that I have good test cases for.
I'm interested in revving this patch set to change other async record
emitters in mi-interp.c based on comments on this version.

Dennis Lambe Jr. (5):
  [PR gdb/33469] ui_out: add optional async param to redirect()
  [PR gdb/33469] ui_out_redirect_pop: add optional async param
  [PR gdb/33469] mi_ui_out: redirect: push separator state when async =
    true
  [PR gdb/33469] mi-interp: on_param_changed: pass async = true to
    redirect()
  [PR gdb/33469] testsuite: framefilters-mi: async records from unwinder

 gdb/mi/mi-interp.c                            |  3 +-
 gdb/mi/mi-out.c                               | 28 ++++++++++++++-----
 gdb/mi/mi-out.h                               |  4 ++-
 .../gdb.python/py-framefilter-mi.exp          |  6 ++++
 gdb/testsuite/gdb.python/py-framefilter.py    | 15 ++++++++++
 gdb/ui-out.h                                  | 14 ++++++----
 6 files changed, 56 insertions(+), 14 deletions(-)

-- 
2.51.0

[RFC][PATCH 1/5] [PR gdb/33469] ui_out: add optional async param to redirect()

[Dennis Lambe Jr.]

This adds a new overload of the protected do_redirect(), rather than
adding an extra optional parameter, so that it can provide a default
implementation of the two-parameter form without disturbing
implementations that don't care about this distinction (which is all but
one of them).

Most uses of redirect(), such as in buffered_streams, assume that this
sort of state won't change during the redirect, so the default for the
one-parameter overload is to forward to the two-parameter form with
async = false.
---
 gdb/ui-out.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 1796e9c9eb3..f7d6be2b026 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -263,7 +263,8 @@ class ui_out
   void flush () { do_flush (); }
 
   /* Redirect the output of a ui_out object temporarily.  */
-  void redirect (ui_file *outstream) { do_redirect (outstream); }
+  void redirect (ui_file *outstream, bool async = false)
+  { do_redirect (outstream, async); }
 
   ui_out_flags test_flags (ui_out_flags mask)
   { return m_flags & mask; }
@@ -368,6 +369,8 @@ class ui_out
   virtual void do_wrap_hint (int indent) = 0;
   virtual void do_flush () = 0;
   virtual void do_redirect (struct ui_file *outstream) = 0;
+  virtual void do_redirect (struct ui_file *outstream, bool async)
+  { do_redirect (outstream); }
 
   virtual void do_progress_start () = 0;
   virtual void do_progress_notify (const std::string &, const char *,
-- 
2.51.0

[RFC][PATCH 2/5] [PR gdb/33469] ui_out_redirect_pop: add optional async param

[Dennis Lambe Jr.]

This is the actual interface used by the mi_interp methods that emit
async records. If called with an async = true, ui_out state related to
field output can be saved and restored along with the output stream.
---
 gdb/ui-out.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index f7d6be2b026..6997895be02 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -463,15 +463,15 @@ class ui_out_redirect_pop
 {
 public:
 
-  ui_out_redirect_pop (ui_out *uiout, ui_file *stream)
-    : m_uiout (uiout)
+  ui_out_redirect_pop (ui_out *uiout, ui_file *stream, bool async = false)
+    : m_uiout (uiout), m_async (async)
   {
-    m_uiout->redirect (stream);
+    m_uiout->redirect (stream, m_async);
   }
 
   ~ui_out_redirect_pop ()
   {
-    m_uiout->redirect (NULL);
+    m_uiout->redirect (NULL, m_async);
   }
 
   ui_out_redirect_pop (const ui_out_redirect_pop &) = delete;
@@ -479,6 +479,7 @@ class ui_out_redirect_pop
 
 private:
   struct ui_out *m_uiout;
+  bool m_async;
 };
 
 struct buffered_streams;
-- 
2.51.0

[RFC][PATCH 3/5] [PR gdb/33469] mi_ui_out: redirect: push separator state when async = true

[Dennis Lambe Jr.]

* Convert m_suppress_field_separator from a bool to a std::vector<bool>.
* When do_redirect() is called with async = true, push a new value onto
  m_suppress_field_separator.
---
 gdb/mi/mi-out.c | 28 +++++++++++++++++++++-------
 gdb/mi/mi-out.h |  4 +++-
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index 6adf108b1cc..1aafbb39f32 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -187,18 +187,32 @@ mi_ui_out::do_flush ()
 
 void
 mi_ui_out::do_redirect (ui_file *outstream)
+{
+  do_redirect (outstream, false);
+}
+
+void
+mi_ui_out::do_redirect (ui_file *outstream, bool async)
 {
   if (outstream != NULL)
-    m_streams.push_back (outstream);
+    {
+      m_streams.push_back (outstream);
+      if (async)
+	m_suppress_field_separator.push_back (false);
+    }
   else
-    m_streams.pop_back ();
+    {
+      m_streams.pop_back ();
+      if (async)
+	m_suppress_field_separator.pop_back ();
+    }
 }
 
 void
 mi_ui_out::field_separator ()
 {
-  if (m_suppress_field_separator)
-    m_suppress_field_separator = false;
+  if (m_suppress_field_separator.back ())
+    m_suppress_field_separator.back () = false;
   else
     gdb_putc (',', m_streams.back ());
 }
@@ -209,7 +223,7 @@ mi_ui_out::open (const char *name, ui_out_type type)
   ui_file *stream = m_streams.back ();
 
   field_separator ();
-  m_suppress_field_separator = true;
+  m_suppress_field_separator.back () = true;
 
   if (name)
     gdb_printf (stream, "%s=", name);
@@ -248,7 +262,7 @@ mi_ui_out::close (ui_out_type type)
       internal_error (_("bad switch"));
     }
 
-  m_suppress_field_separator = false;
+  m_suppress_field_separator.back () = false;
 }
 
 string_file *
@@ -322,11 +336,11 @@ mi_ui_out::version ()
 
 mi_ui_out::mi_ui_out (int mi_version)
 : ui_out (make_flags (mi_version)),
-  m_suppress_field_separator (false),
   m_mi_version (mi_version)
 {
   string_file *stream = new string_file ();
   m_streams.push_back (stream);
+  m_suppress_field_separator.push_back (false);
 }
 
 mi_ui_out::~mi_ui_out ()
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index ec3de74ab60..2a64d8f08d1 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -84,6 +84,8 @@ class mi_ui_out : public ui_out
   virtual void do_wrap_hint (int indent) override;
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
+  virtual void do_redirect (struct ui_file *outstream,
+			    bool push_state) override;
 
   virtual bool do_is_mi_like_p () const override
   { return true; }
@@ -138,7 +140,7 @@ class mi_ui_out : public ui_out
     return flags;
   }
 
-  bool m_suppress_field_separator;
+  std::vector<bool> m_suppress_field_separator;
   int m_mi_version;
   std::vector<ui_file *> m_streams;
 };
-- 
2.51.0

[RFC][PATCH 4/5] [PR gdb/33469] mi-interp: on_param_changed: pass async = true to redirect()

[Dennis Lambe Jr.]

When frame filters are enabled and a stack unwinder does something to
cause an async record to be emitted, -stack-list-frames output is
incorrect. A comma is missing from the first async record, and a
spurious comma is inserted before the first frame= of the frame list.

This happens because async record emitters like
mi_interp::on_param_changed() are written as though calling
mi_out::do_redirect() ensures that a comma will be emitted by the next
following mi_ui_out::field_string() call. It's not guaranteed, though.
It usually works out by coincidence because the emitter methods are
called when no other output record is mid-creation.

Pass async = true to the ui_out_redirect_pop constructor to back up and
reset ui_out state related to list generation.

RFC: This version of this patch only changes
mi_interp::on_param_changed(), because that's the async record my code
was emitting when I found this bug and that I have good test cases for.
I'm interested in revving this patch set to change other async record
emitters in mi-interp.c based on comments on this version.
---
 gdb/mi/mi-interp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 8373e1a9ea8..10c1e70ce1e 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -800,7 +800,8 @@ mi_interp::on_param_changed (const char *param, const char *value)
 
   gdb_printf (this->event_channel, "cmd-param-changed");
 
-  ui_out_redirect_pop redir (mi_uiout, this->event_channel);
+  ui_out_redirect_pop redir (mi_uiout, this->event_channel,
+			     /* async = */ true);
 
   mi_uiout->field_string ("param", param);
   mi_uiout->field_string ("value", value);
-- 
2.51.0

[RFC][PATCH 5/5] [PR gdb/33469] testsuite: framefilters-mi: async records from unwinder

[Dennis Lambe Jr.]

Add a testsuite case to detect regressions of PR gdb/33469.
---
 gdb/testsuite/gdb.python/py-framefilter-mi.exp |  6 ++++++
 gdb/testsuite/gdb.python/py-framefilter.py     | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/gdb/testsuite/gdb.python/py-framefilter-mi.exp b/gdb/testsuite/gdb.python/py-framefilter-mi.exp
index 03e5f90580c..df4b246f37a 100644
--- a/gdb/testsuite/gdb.python/py-framefilter-mi.exp
+++ b/gdb/testsuite/gdb.python/py-framefilter-mi.exp
@@ -71,6 +71,12 @@ mi_gdb_test "-stack-list-frames 22 24" \
     "\\^done,stack=\\\[frame={level=\"22\",addr=\"$hex\",func=\"1cnuf\".*,arch=\"$any\",children=\\\[frame={level=\"23\",addr=\"$hex\",func=\"func2\".*,arch=\"$any\"}\\\]},frame={level=\"24\",addr=\"$hex\",func=\"3cnuf\".*,arch=\"$any\"}\\\]" \
     "filtered stack list 22 24"
 
+mi_gdb_test "enable unwinder global InterruptingUnwinder" ".*1 unwinder enabled.*\\^done." "enable InterruptingUnwinder"
+mi_gdb_test "-stack-list-frames" \
+    "(=cmd-param-changed,param=\"$any\",value=\"$any\"\r?\n)+\\^done,stack=\\\[frame={level=.*" \
+    "list stack frames when unwinder emits async records"
+mi_gdb_test "disable unwinder global InterruptingUnwinder" ".*1 unwinder disabled.*\\^done." "disable InterruptingUnwinder"
+
 #stack list arguments
 
 
diff --git a/gdb/testsuite/gdb.python/py-framefilter.py b/gdb/testsuite/gdb.python/py-framefilter.py
index 5c3790d33f2..5ed95681687 100644
--- a/gdb/testsuite/gdb.python/py-framefilter.py
+++ b/gdb/testsuite/gdb.python/py-framefilter.py
@@ -20,6 +20,7 @@ import itertools
 # frame-filters.
 import gdb
 from gdb.FrameDecorator import FrameDecorator
+from gdb.unwinder import Unwinder, register_unwinder
 
 
 class Reverse_Function(FrameDecorator):
@@ -167,6 +168,20 @@ class ErrorFilter:
             return map(ErrorInName, frame_iter)
 
 
+# An unwinder that causes some async record MI traffic
+class InterruptingUnwinder(Unwinder):
+    def __init__(self):
+        super().__init__("InterruptingUnwinder")
+        self.enabled = False
+
+    def __call__(self, pending_frame):
+        language = gdb.parameter("language")
+        gdb.set_parameter("language", "c++")
+        gdb.set_parameter("language", language)
+        return None
+
+
 FrameFilter()
 FrameElider()
 ErrorFilter()
+register_unwinder(None, InterruptingUnwinder())
-- 
2.51.0

Re: [RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Tom Tromey]

Dennis> RFC: This version of the patch set only changes
Dennis> mi_interp::on_param_changed(), because that's the async record my code
Dennis> was emitting when I found this bug and that I have good test cases for.
Dennis> I'm interested in revving this patch set to change other async record
Dennis> emitters in mi-interp.c based on comments on this version.

Thanks for the patches.

Before reviewing this -- what is your copyright assignment status?  A
series this size will need an assignment.

If you don't have one, email "assign@gnu.org" and say that you have some
gdb patches; they will get you started.

If you do have one, let us know.

thanks,
Tom

Re: [RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Dennis Lambe Jr.]

I filled out the copyright assignment paperwork in 2018 to contribute to 
diffutils, does that carry over to binutils as well or do I need to sign 
separate paperwork for that?

On 2025-10-03 14:39, Tom Tromey wrote:
> Dennis> RFC: This version of the patch set only changes
> Dennis> mi_interp::on_param_changed(), because that's the async record 
> my code
> Dennis> was emitting when I found this bug and that I have good test 
> cases for.
> Dennis> I'm interested in revving this patch set to change other async 
> record
> Dennis> emitters in mi-interp.c based on comments on this version.
> 
> Thanks for the patches.
> 
> Before reviewing this -- what is your copyright assignment status?  A
> series this size will need an assignment.
> 
> If you don't have one, email "assign@gnu.org" and say that you have 
> some
> gdb patches; they will get you started.
> 
> If you do have one, let us know.
> 
> thanks,
> Tom

Re: [RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Tom Tromey]

>>>>> "Dennis" == Dennis Lambe <malsyned@malsyned.net> writes:

Dennis> I filled out the copyright assignment paperwork in 2018 to contribute
Dennis> to diffutils, does that carry over to binutils as well or do I need to
Dennis> sign separate paperwork for that?

Unfortunately you do need separate paperwork for gdb.

Tom

Re: [RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Dennis" == Dennis Lambe <malsyned@malsyned.net> writes:
Dennis> I filled out the copyright assignment paperwork in 2018 to contribute
Dennis> to diffutils, does that carry over to binutils as well or do I need to
Dennis> sign separate paperwork for that?

Tom> Unfortunately you do need separate paperwork for gdb.

Hi, did this ever get sorted out?

thanks,
Tom

Re: [RFC][PATCH 0/5] [PR gdb/33469] mi, python: Fix comma location when unwinder emits async records

[Dennis Lambe Jr.]

It did, yup! I sent my GDB copyright assignment in about a month and a 
half ago, and gave your email address as someone to notify about it. 
Sorry for the delay on it, thanks for checking back in.

On 2026-02-19 13:27, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
>>>>>> "Dennis" == Dennis Lambe <malsyned@malsyned.net> writes:
> Dennis> I filled out the copyright assignment paperwork in 2018 to 
> contribute
> Dennis> to diffutils, does that carry over to binutils as well or do I 
> need to
> Dennis> sign separate paperwork for that?
> 
> Tom> Unfortunately you do need separate paperwork for gdb.
> 
> Hi, did this ever get sorted out?
> 
> thanks,
> Tom