[v3] Pass thread_info pointer to various inferior control functions
Commit Message
From: Simon Marchi <simon.marchi@polymtl.ca>
[ Migrating this from Gerrit: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/321 ]
I noticed that some functions in infcmd and infrun call each other and
all call inferior_thread, while they could just get the thread_info
pointer from their caller. That means less calls to inferior_thread, so
less reliance on global state, since inferior_thread reads
inferior_ptid.
The paths I am unsure about are:
- fetch_inferior_event calls...
- step_command_fsm::should_stop calls...
- prepare_one_step
and
- process_event_stop_test calls...
- set_step_info
Before this patch, prepare_one_step gets the thread pointer using
inferior_thread. After this patch, it gets it from the
execution_control_state structure in fetch_inferior_event. Are we sure
that the thread from the execution_control_state structure is the same
as the one inferior_thread would return? This code path is used when a
thread completes a step, but the user had specified a step count (e.g.
"step 5") so we decide to do one more step. It would be strange (and
even a bug I suppose) if the thread in the ecs structure in
fetch_inferior_event was not the same thread that is prepared to stepped
by prepare_one_step. So I believe passing the ecs thread is fine.
The same logic applies to process_event_stop_test calling
set_step_info.
gdb/ChangeLog:
* infrun.h: Forward-declare thread_info.
(set_step_info): Add thread_info parameter, add doc.
* infrun.c (set_step_info): Add thread_info parameter, move doc
to header.
* infrun.c (process_event_stop_test): Pass thread to
set_step_info call.
* infcmd.c (set_step_frame): Add thread_info pointer, pass it to
set_step_info.
(prepare_one_step): Add thread_info parameter, pass it to
set_step_frame and prepare_one_step (recursive) call.
(step_1): Pass thread to prepare_one_step call.
(step_command_fsm::should_stop): Pass thread to
prepare_one_step.
(until_next_fsm): Pass thread to set_step_frame call.
(finish_command): Pass thread to set_step_info call.
---
gdb/infcmd.c | 29 +++++++++++------------------
gdb/infrun.c | 10 +++++-----
gdb/infrun.h | 5 ++++-
3 files changed, 20 insertions(+), 24 deletions(-)
Comments
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
Simon> I noticed that some functions in infcmd and infrun call each other and
Simon> all call inferior_thread, while they could just get the thread_info
Simon> pointer from their caller. That means less calls to inferior_thread, so
Simon> less reliance on global state, since inferior_thread reads
Simon> inferior_ptid.
...
Simon> -set_step_frame (void)
Simon> +set_step_frame (thread_info *tp)
Simon> {
Simon> frame_info *frame = get_current_frame ();
I like the idea of passing parameters rather than relying on global
state. However, in its current form, I think this patch may lull the
reader into a false sense of security.
That is, it makes it seems like these functions operate on a thread that
you pass in. However, they don't actually. For example, in this one, I
think get_current_frame must be relying on the global inferior_ptid.
So, this would change the function from something that obviously has to
be relying on globals to something that un-obviously is.
I don't know whether that's a reason to reject it or not.
It just seemed like maybe a future source of bugs.
I do welcome changes to reduce our reliance on globals.
Perhaps there's an argument that we can only achieve this incrementally
like this. Though, the follow-on question there is whether this is
something that will actually happen.
Let me know what you think of this.
thanks,
Tom
On 2020-02-18 3:52 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
>
> Simon> I noticed that some functions in infcmd and infrun call each other and
> Simon> all call inferior_thread, while they could just get the thread_info
> Simon> pointer from their caller. That means less calls to inferior_thread, so
> Simon> less reliance on global state, since inferior_thread reads
> Simon> inferior_ptid.
>
> ...
>
> Simon> -set_step_frame (void)
> Simon> +set_step_frame (thread_info *tp)
> Simon> {
> Simon> frame_info *frame = get_current_frame ();
>
> I like the idea of passing parameters rather than relying on global
> state. However, in its current form, I think this patch may lull the
> reader into a false sense of security.
>
> That is, it makes it seems like these functions operate on a thread that
> you pass in. However, they don't actually. For example, in this one, I
> think get_current_frame must be relying on the global inferior_ptid.
> So, this would change the function from something that obviously has to
> be relying on globals to something that un-obviously is.
I agree.
> I don't know whether that's a reason to reject it or not.
> It just seemed like maybe a future source of bugs.
>
> I do welcome changes to reduce our reliance on globals.
> Perhaps there's an argument that we can only achieve this incrementally
> like this.
That's what I think, we have to start somewhere, and for some time be in this
weird state where we pass in things by parameter but also rely on globals.
Perhaps a good way would be to add this parameter, but also assert at the
entry of the function that the passed in parameter matches the global (of
course, with a comment that says why). When we are convinced the code does
not rely on globals anymore, we can remove the assertion.
There's a precedent for such a thing here (altough I added it, so it might
not count):
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/target.c;h=470ef51d69ef5d712fc51a54f426c1cd3b79c977;hb=HEAD#l2020
So here, I would add this at the beginning of step_step_info, set_step_frame
and prepare_one_step:
/* This can be removed once this function does not rely on global state
anymore. */
gdb_assert (inferior_ptid == tp->ptid);
>Though, the follow-on question there is whether this is
> something that will actually happen.
I want to believe.
>
> Let me know what you think of this.
Simon
On 2020-02-18 4:12 p.m., Simon Marchi wrote:
> On 2020-02-18 3:52 p.m., Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
>>
>> Simon> I noticed that some functions in infcmd and infrun call each other and
>> Simon> all call inferior_thread, while they could just get the thread_info
>> Simon> pointer from their caller. That means less calls to inferior_thread, so
>> Simon> less reliance on global state, since inferior_thread reads
>> Simon> inferior_ptid.
>>
>> ...
>>
>> Simon> -set_step_frame (void)
>> Simon> +set_step_frame (thread_info *tp)
>> Simon> {
>> Simon> frame_info *frame = get_current_frame ();
>>
>> I like the idea of passing parameters rather than relying on global
>> state. However, in its current form, I think this patch may lull the
>> reader into a false sense of security.
>>
>> That is, it makes it seems like these functions operate on a thread that
>> you pass in. However, they don't actually. For example, in this one, I
>> think get_current_frame must be relying on the global inferior_ptid.
>> So, this would change the function from something that obviously has to
>> be relying on globals to something that un-obviously is.
>
> I agree.
>
>> I don't know whether that's a reason to reject it or not.
>> It just seemed like maybe a future source of bugs.
>>
>> I do welcome changes to reduce our reliance on globals.
>> Perhaps there's an argument that we can only achieve this incrementally
>> like this.
>
> That's what I think, we have to start somewhere, and for some time be in this
> weird state where we pass in things by parameter but also rely on globals.
> Perhaps a good way would be to add this parameter, but also assert at the
> entry of the function that the passed in parameter matches the global (of
> course, with a comment that says why). When we are convinced the code does
> not rely on globals anymore, we can remove the assertion.
>
> There's a precedent for such a thing here (altough I added it, so it might
> not count):
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/target.c;h=470ef51d69ef5d712fc51a54f426c1cd3b79c977;hb=HEAD#l2020
>
> So here, I would add this at the beginning of step_step_info, set_step_frame
> and prepare_one_step:
>
> /* This can be removed once this function does not rely on global state
> anymore. */
> gdb_assert (inferior_ptid == tp->ptid);
>
>> Though, the follow-on question there is whether this is
>> something that will actually happen.
>
> I want to believe.
>
>>
>> Let me know what you think of this.
>
> Simon
>
I've pushed the patch after adding these assertions.
Simon
@@ -914,18 +914,17 @@ continue_command (const char *args, int from_tty)
continue_1 (all_threads_p);
}
-/* Record the starting point of a "step" or "next" command. */
+/* Record in TP the starting point of a "step" or "next" command. */
static void
-set_step_frame (void)
+set_step_frame (thread_info *tp)
{
frame_info *frame = get_current_frame ();
symtab_and_line sal = find_frame_sal (frame);
- set_step_info (frame, sal);
+ set_step_info (tp, frame, sal);
CORE_ADDR pc = get_frame_pc (frame);
- thread_info *tp = inferior_thread ();
tp->control.step_start_function = find_pc_function (pc);
}
@@ -1002,7 +1001,7 @@ step_command_fsm_prepare (struct step_command_fsm *sm,
thread->control.stepping_command = 1;
}
-static int prepare_one_step (struct step_command_fsm *sm);
+static int prepare_one_step (thread_info *, struct step_command_fsm *sm);
static void
step_1 (int skip_subroutines, int single_inst, const char *count_string)
@@ -1040,7 +1039,7 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string)
loop. Let the continuation figure out how many other steps we
need to do, and handle them one at the time, through
step_once. */
- if (!prepare_one_step (step_sm))
+ if (!prepare_one_step (thr, step_sm))
proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
else
{
@@ -1070,7 +1069,7 @@ step_command_fsm::should_stop (struct thread_info *tp)
/* There are more steps to make, and we did stop due to
ending a stepping range. Do another step. */
if (--count > 0)
- return prepare_one_step (this);
+ return prepare_one_step (tp, this);
set_finished ();
}
@@ -1102,19 +1101,13 @@ step_command_fsm::do_async_reply_reason ()
resumed. */
static int
-prepare_one_step (struct step_command_fsm *sm)
+prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
{
if (sm->count > 0)
{
struct frame_info *frame = get_current_frame ();
- /* Don't assume THREAD is a valid thread id. It is set to -1 if
- the longjmp breakpoint was not required. Use the
- INFERIOR_PTID thread instead, which is the same thread when
- THREAD is set. */
- struct thread_info *tp = inferior_thread ();
-
- set_step_frame ();
+ set_step_frame (tp);
if (!sm->single_inst)
{
@@ -1146,7 +1139,7 @@ prepare_one_step (struct step_command_fsm *sm)
|| !function_name_is_marked_for_skip (fn, sal))
{
sm->count--;
- return prepare_one_step (sm);
+ return prepare_one_step (tp, sm);
}
}
@@ -1488,7 +1481,7 @@ until_next_command (int from_tty)
struct until_next_fsm *sm;
clear_proceed_status (0);
- set_step_frame ();
+ set_step_frame (tp);
frame = get_current_frame ();
@@ -1945,7 +1938,7 @@ finish_command (const char *arg, int from_tty)
called by that frame. We don't use the magic "1" value for
step_range_end, because then infrun will think this is nexti,
and not step over the rest of this inlined function call. */
- set_step_info (frame, {});
+ set_step_info (tp, frame, {});
tp->control.step_range_start = get_frame_pc (frame);
tp->control.step_range_end = tp->control.step_range_start;
tp->control.step_over_calls = STEP_OVER_ALL;
@@ -4073,12 +4073,12 @@ fetch_inferior_event (void *client_data)
printf_unfiltered (_("completed.\n"));
}
-/* Record the frame and location we're currently stepping through. */
+/* See infrun.h. */
+
void
-set_step_info (struct frame_info *frame, struct symtab_and_line sal)
+set_step_info (thread_info *tp, struct frame_info *frame,
+ struct symtab_and_line sal)
{
- struct thread_info *tp = inferior_thread ();
-
tp->control.step_frame_id = get_frame_id (frame);
tp->control.step_stack_frame_id = get_stack_frame_id (frame);
@@ -7171,7 +7171,7 @@ process_event_stop_test (struct execution_control_state *ecs)
ecs->event_thread->control.step_range_start = stop_pc_sal.pc;
ecs->event_thread->control.step_range_end = stop_pc_sal.end;
ecs->event_thread->control.may_range_step = 1;
- set_step_info (frame, stop_pc_sal);
+ set_step_info (ecs->event_thread, frame, stop_pc_sal);
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: keep going\n");
@@ -26,6 +26,7 @@ struct frame_info;
struct address_space;
struct return_value_info;
struct process_stratum_target;
+struct thread_info;
/* True if we are debugging run control. */
extern unsigned int debug_infrun;
@@ -150,7 +151,9 @@ extern int thread_is_stepping_over_breakpoint (int thread);
triggers a non-steppable watchpoint. */
extern int stepping_past_nonsteppable_watchpoint (void);
-extern void set_step_info (struct frame_info *frame,
+/* Record in TP the frame and location we're currently stepping through. */
+extern void set_step_info (thread_info *tp,
+ struct frame_info *frame,
struct symtab_and_line sal);
/* Several print_*_reason helper functions to print why the inferior