[2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c
Commit Message
On 2020-01-16 4:01 a.m., Aktemur, Tankut Baris wrote:
>> @@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
>> /* The initial inferior is created before this function is
>> called, so we need to report it explicitly. Use iteration in
>> case future version of GDB creates more than one inferior
>> - up-front. */
>> - iterate_over_inferiors (report_initial_inferior, mi);
>> + up-front.
>> +
>> + This function is called from mi_interpreter_init, and since
>
> There is an 8-spaces/tab issue above. And also, the comment piece "This
> function is called from mi_interpreter_init, and" seems obsolete now.
Thanks for spotting these.
That made me realize there are many issues with that comment.
- The part about not being able to use mi_inferior_added because top_level_interpreter_data
is not set is stale. That was true before the interps were made into C++ classes. Back
then, the mi_interpreter_init function would return the pointer to be used as interpreter
data, which the caller (the interp_set function) would assign to interp->data. So it was
indeed not possible to call mi_inferior_added, because it required interp->data to be set.
- It is still not possible today to use mi_inferior_added, because it iterates over all MI UIs
and to print the event on all of them. So if you do:
1. Start GDB with `-i mi`
2. Add some inferiors
3. Create a new MI UI, with `new-ui mi /dev/pts/X`
When initializing the new MI UI, it will print the inferior-added events on both MI UIs.
- It is necessary to use iteration, not only in case GDB creates more than one inferior
up front, but also because there might be multiple inferiors when initializing a secondary
MI UI, as shown above.
I don't know if it is by design, that the =inferior-added events are sent to a new MI UI added
with new-ui, but in any case I don't want to change the behavior with this patch.
Also, when testing this, I realized it was wrong to put the gdb_flush out of the loop. It's not
a simple flush as in "make sure the data is all sent to the underlying device", it's also what
adds the final \n! So with the gdb_flush outside the loop, all the events were printed on the same
line. I've put the terminal setting calls and the gdb_flush back into the loop, to minimize the
changes.
The patch would now look like this:
From cb845a4f28d1761feb463914c13b0a45c97d9974 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 15 Jan 2020 14:12:20 -0500
Subject: [PATCH] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c
Replace it with a range-based for. I've updated the comment in
mi_interp::init, which was a bit stale.
gdb/ChangeLog:
* mi/mi-interp.c (report_initial_inferior): Remove.
(mi_interp::init): Use range-based for to iterate over inferiors.
---
gdb/mi/mi-interp.c | 49 ++++++++++++++++++++--------------------------
1 file changed, 21 insertions(+), 28 deletions(-)
Comments
On Thursday, January 16, 2020 4:24 PM, Simon Marchi wrote:
> On 2020-01-16 4:01 a.m., Aktemur, Tankut Baris wrote:
> >> @@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
> >> /* The initial inferior is created before this function is
> >> called, so we need to report it explicitly. Use iteration in
> >> case future version of GDB creates more than one inferior
> >> - up-front. */
> >> - iterate_over_inferiors (report_initial_inferior, mi);
> >> + up-front.
> >> +
> >> + This function is called from mi_interpreter_init, and since
> >
> > There is an 8-spaces/tab issue above. And also, the comment piece "This
> > function is called from mi_interpreter_init, and" seems obsolete now.
>
> Thanks for spotting these.
>
> That made me realize there are many issues with that comment.
>
> - The part about not being able to use mi_inferior_added because top_level_interpreter_data
> is not set is stale. That was true before the interps were made into C++ classes. Back
> then, the mi_interpreter_init function would return the pointer to be used as interpreter
> data, which the caller (the interp_set function) would assign to interp->data. So it was
> indeed not possible to call mi_inferior_added, because it required interp->data to be set.
>
> - It is still not possible today to use mi_inferior_added, because it iterates over all MI UIs
> and to print the event on all of them. So if you do:
>
> 1. Start GDB with `-i mi`
> 2. Add some inferiors
> 3. Create a new MI UI, with `new-ui mi /dev/pts/X`
>
> When initializing the new MI UI, it will print the inferior-added events on both MI UIs.
>
> - It is necessary to use iteration, not only in case GDB creates more than one inferior
> up front, but also because there might be multiple inferiors when initializing a secondary
> MI UI, as shown above.
>
> I don't know if it is by design, that the =inferior-added events are sent to a new MI UI added
> with new-ui, but in any case I don't want to change the behavior with this patch.
>
> Also, when testing this, I realized it was wrong to put the gdb_flush out of the loop. It's not
> a simple flush as in "make sure the data is all sent to the underlying device", it's also what
> adds the final \n! So with the gdb_flush outside the loop, all the events were printed on the same
> line. I've put the terminal setting calls and the gdb_flush back into the loop, to minimize the
> changes.
>
> The patch would now look like this:
Thank you for the detailed explanation. I think the revised patch is very clear.
-Baris
>
> From cb845a4f28d1761feb463914c13b0a45c97d9974 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Wed, 15 Jan 2020 14:12:20 -0500
> Subject: [PATCH] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c
>
> Replace it with a range-based for. I've updated the comment in
> mi_interp::init, which was a bit stale.
>
> gdb/ChangeLog:
>
> * mi/mi-interp.c (report_initial_inferior): Remove.
> (mi_interp::init): Use range-based for to iterate over inferiors.
> ---
> gdb/mi/mi-interp.c | 49 ++++++++++++++++++++--------------------------
> 1 file changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 2ac4c119961c..e77093cfa282 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
> ssize_t len, const bfd_byte *myaddr);
> static void mi_on_sync_execution_done (void);
>
> -static int report_initial_inferior (struct inferior *inf, void *closure);
> -
> /* Display the MI prompt. */
>
> static void
> @@ -137,12 +135,27 @@ mi_interp::init (bool top_level)
>
> if (top_level)
> {
> - /* The initial inferior is created before this function is
> - called, so we need to report it explicitly. Use iteration in
> - case future version of GDB creates more than one inferior
> - up-front. */
> - iterate_over_inferiors (report_initial_inferior, mi);
> - }
> + /* The initial inferior is created before this function is called, so we
> + need to report it explicitly when initializing the top-level MI
> + interpreter.
> +
> + This is also called when additional MI interpreters are added (using
> + the new-ui command), when multiple inferiors possibly exist, so we need
> + to use iteration to report all the inferiors. mi_inferior_added can't
> + be used, because it would print the event on all the other MI UIs. */
> +
> + for (inferior *inf : all_inferiors ())
> + {
> + target_terminal::scoped_restore_terminal_state term_state;
> + target_terminal::ours_for_output ();
> +
> + fprintf_unfiltered (mi->event_channel,
> + "thread-group-added,id=\"i%d\"",
> + inf->num);
> +
> + gdb_flush (mi->event_channel);
> + }
> + }
> }
>
> void
> @@ -1253,26 +1266,6 @@ mi_user_selected_context_changed (user_selected_what selection)
> }
> }
>
> -static int
> -report_initial_inferior (struct inferior *inf, void *closure)
> -{
> - /* This function is called from mi_interpreter_init, and since
> - mi_inferior_added assumes that inferior is fully initialized
> - and top_level_interpreter_data is set, we cannot call
> - it here. */
> - struct mi_interp *mi = (struct mi_interp *) closure;
> -
> - target_terminal::scoped_restore_terminal_state term_state;
> - target_terminal::ours_for_output ();
> -
> - fprintf_unfiltered (mi->event_channel,
> - "thread-group-added,id=\"i%d\"",
> - inf->num);
> - gdb_flush (mi->event_channel);
> -
> - return 0;
> -}
> -
> ui_out *
> mi_interp::interp_ui_out ()
> {
> --
> 2.24.1
>
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
On 2020-01-17 2:24 a.m., Aktemur, Tankut Baris wrote:
> Thank you for the detailed explanation. I think the revised patch is very clear.
>
> -Baris
Ok thanks for the review, I'll push the series shortly.
Simon
@@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
ssize_t len, const bfd_byte *myaddr);
static void mi_on_sync_execution_done (void);
-static int report_initial_inferior (struct inferior *inf, void *closure);
-
/* Display the MI prompt. */
static void
@@ -137,12 +135,27 @@ mi_interp::init (bool top_level)
if (top_level)
{
- /* The initial inferior is created before this function is
- called, so we need to report it explicitly. Use iteration in
- case future version of GDB creates more than one inferior
- up-front. */
- iterate_over_inferiors (report_initial_inferior, mi);
- }
+ /* The initial inferior is created before this function is called, so we
+ need to report it explicitly when initializing the top-level MI
+ interpreter.
+
+ This is also called when additional MI interpreters are added (using
+ the new-ui command), when multiple inferiors possibly exist, so we need
+ to use iteration to report all the inferiors. mi_inferior_added can't
+ be used, because it would print the event on all the other MI UIs. */
+
+ for (inferior *inf : all_inferiors ())
+ {
+ target_terminal::scoped_restore_terminal_state term_state;
+ target_terminal::ours_for_output ();
+
+ fprintf_unfiltered (mi->event_channel,
+ "thread-group-added,id=\"i%d\"",
+ inf->num);
+
+ gdb_flush (mi->event_channel);
+ }
+ }
}
void
@@ -1253,26 +1266,6 @@ mi_user_selected_context_changed (user_selected_what selection)
}
}
-static int
-report_initial_inferior (struct inferior *inf, void *closure)
-{
- /* This function is called from mi_interpreter_init, and since
- mi_inferior_added assumes that inferior is fully initialized
- and top_level_interpreter_data is set, we cannot call
- it here. */
- struct mi_interp *mi = (struct mi_interp *) closure;
-
- target_terminal::scoped_restore_terminal_state term_state;
- target_terminal::ours_for_output ();
-
- fprintf_unfiltered (mi->event_channel,
- "thread-group-added,id=\"i%d\"",
- inf->num);
- gdb_flush (mi->event_channel);
-
- return 0;
-}
-
ui_out *
mi_interp::interp_ui_out ()
{