[PATCH 2/2] Fix advance/until and multiple locations (PR gdb/26524)
Simon Marchi
simark@simark.ca
Wed Aug 26 21:01:35 GMT 2020
On 2020-08-22 4:38 p.m., Pedro Alves wrote:
> If you do "advance LINESPEC", and LINESPEC expands to more than one
> location, GDB just errors out:
>
> if (sals.size () != 1)
> error (_("Couldn't get information on specified line."));
>
> For example, advancing to a line in an inlined function, inlined three
> times:
>
> (gdb) b 21
> Breakpoint 1 at 0x55555555516f: advance.cc:21. (3 locations)
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 breakpoint keep y <MULTIPLE>
> 1.1 y 0x000055555555516f in inline_func at advance.cc:21
> 1.2 y 0x000055555555517e in inline_func at advance.cc:21
> 1.3 y 0x000055555555518d in inline_func at advance.cc:21
> (gdb) advance 21
> Couldn't get information on specified line.
> (gdb)
>
> Similar issue with the "until" command, as it shares the
> implementation with "advance".
>
> Since, as the comment in gdb.base/advance.exp says, "advance <location>"
> is really just syntactic sugar for "tbreak <location>;continue",
> fix this by making GDB insert a breakpoint at all the resolved
> locations.
>
> A new testcase is included, which exercises both "advance" and
> "until", in two different cases expanding to multiple locations:
>
> - inlined functions
> - C++ overloads
>
> This also exercises the inline frames issue fixed by the previous
> patch.
>
> gdb/ChangeLog:
>
> PR gdb/26524
> * breakpoint.c (until_break_fsm) <location_breakpoint,
> caller_breakpoint>: Delete fields.
> <breakpoints>: New field.
> <until_break_fsm>: Adjust to save a breakpoint vector instead of
> two individual breakpoints.
> (until_break_fsm::should_stop): Loop over breakpoints in the
> breakpoint vector.
> (until_break_fsm::clean_up): Adjust to clear the breakpoints
> vector.
> (until_break_command): Handle location expanding into multiple
> sals.
>
> gdb/testsuite/ChangeLog:
>
> PR gdb/26523
> PR gdb/26524
> * gdb.base/advance-until-multiple-locations.cc: New.
> * gdb.base/advance-until-multiple-locations.exp: New.
> ---
> gdb/breakpoint.c | 77 ++++++-----
> .../gdb.base/advance-until-multiple-locations.cc | 61 +++++++++
> .../gdb.base/advance-until-multiple-locations.exp | 142 +++++++++++++++++++++
> 3 files changed, 239 insertions(+), 41 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
> create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 977599db1db..4f94a0acbac 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10950,20 +10950,15 @@ struct until_break_fsm : public thread_fsm
> /* The thread that was current when the command was executed. */
> int thread;
>
> - /* The breakpoint set at the destination location. */
> - breakpoint_up location_breakpoint;
> -
> - /* Breakpoint set at the return address in the caller frame. May be
> - NULL. */
> - breakpoint_up caller_breakpoint;
> + /* The breakpoint set at the return address in the caller frame,
> + plus breakpoints at the destination location. */
I think it would make sense to use the plural "locations" in the comment, if we are
talking about multiple destinations locations in terms of machine code. Though it
could be ok at the singular if you meant the source location, ofwhich there is only
one.
> + std::vector<breakpoint_up> breakpoints;
>
> until_break_fsm (struct interp *cmd_interp, int thread,
> - breakpoint_up &&location_breakpoint,
> - breakpoint_up &&caller_breakpoint)
> + std::vector<breakpoint_up> &&breakpoints)
> : thread_fsm (cmd_interp),
> thread (thread),
> - location_breakpoint (std::move (location_breakpoint)),
> - caller_breakpoint (std::move (caller_breakpoint))
> + breakpoints (std::move (breakpoints))
> {
> }
>
> @@ -10978,12 +10973,13 @@ struct until_break_fsm : public thread_fsm
> bool
> until_break_fsm::should_stop (struct thread_info *tp)
> {
> - if (bpstat_find_breakpoint (tp->control.stop_bpstat,
> - location_breakpoint.get ()) != NULL
> - || (caller_breakpoint != NULL
> - && bpstat_find_breakpoint (tp->control.stop_bpstat,
> - caller_breakpoint.get ()) != NULL))
> - set_finished ();
> + for (const breakpoint_up &bp : breakpoints)
> + if (bpstat_find_breakpoint (tp->control.stop_bpstat,
> + bp.get ()) != NULL)
> + {
> + set_finished ();
> + break;
> + }
When using nested control structures, I thought we required braces for the
outer ones, like this (as mentioned in the GNU coding standards):
for (...)
{
if (...)
...
}
If that's not the case, I have been misleading people for a while!
>
> return true;
> }
> @@ -10995,8 +10991,7 @@ void
> until_break_fsm::clean_up (struct thread_info *)
> {
> /* Clean up our temporary breakpoints. */
> - location_breakpoint.reset ();
> - caller_breakpoint.reset ();
> + breakpoints.clear ();
> delete_longjmp_breakpoint (thread);
> }
>
> @@ -11034,16 +11029,12 @@ until_break_command (const char *arg, int from_tty, int anywhere)
> : decode_line_1 (location.get (), DECODE_LINE_FUNFIRSTLINE,
> NULL, NULL, 0));
>
> - if (sals.size () != 1)
> + if (sals.empty ())
> error (_("Couldn't get information on specified line."));
>
> - symtab_and_line &sal = sals[0];
> -
> if (*arg)
> error (_("Junk at end of arguments."));
>
> - resolve_sal_pc (&sal);
> -
> tp = inferior_thread ();
> thread = tp->global_num;
>
> @@ -11060,7 +11051,7 @@ until_break_command (const char *arg, int from_tty, int anywhere)
> /* Keep within the current frame, or in frames called by the current
> one. */
>
> - breakpoint_up caller_breakpoint;
> + std::vector<breakpoint_up> breakpoints;
>
> gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
>
> @@ -11072,10 +11063,11 @@ until_break_command (const char *arg, int from_tty, int anywhere)
> sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);
> sal2.pc = frame_unwind_caller_pc (frame);
> caller_gdbarch = frame_unwind_caller_arch (frame);
> - caller_breakpoint = set_momentary_breakpoint (caller_gdbarch,
> - sal2,
> - caller_frame_id,
> - bp_until);
> +
> + breakpoint_up caller_breakpoint
> + = set_momentary_breakpoint (caller_gdbarch, sal2,
> + caller_frame_id, bp_until);
> + breakpoints.emplace_back (std::move (caller_breakpoint));
>
> set_longjmp_breakpoint (tp, caller_frame_id);
> lj_deleter.emplace (thread);
> @@ -11084,21 +11076,24 @@ until_break_command (const char *arg, int from_tty, int anywhere)
> /* set_momentary_breakpoint could invalidate FRAME. */
> frame = NULL;
>
> - breakpoint_up location_breakpoint;
> - if (anywhere)
> - /* If the user told us to continue until a specified location,
> - we don't specify a frame at which we need to stop. */
> - location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,
> - null_frame_id, bp_until);
> - else
> - /* Otherwise, specify the selected frame, because we want to stop
> - only at the very same frame. */
> - location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,
> - stack_frame_id, bp_until);
> + /* If the user told us to continue until a specified location, we
> + don't specify a frame at which we need to stop. Otherwise,
> + specify the selected frame, because we want to stop only at the
> + very same frame. */
> + frame_id stop_frame_id = anywhere ? null_frame_id : stack_frame_id;
> +
> + for (symtab_and_line &sal : sals)
> + {
> + resolve_sal_pc (&sal);
> +
> + breakpoint_up location_breakpoint
> + = set_momentary_breakpoint (frame_gdbarch, sal,
> + stop_frame_id, bp_until);
> + breakpoints.emplace_back (std::move (location_breakpoint));
> + }
I was a bit surprised to see multiple breakpoints being created here. Since the
parallel is that advance/until is like "tbreak <loc>; continue", I would have
expected a single breakpoint with multiple locations.
I attributed that to the fact this chain of functions take a single sal:
- set_momentary_breakpoint, which calls
- set_raw_breakpoint, which calls
- init_raw_breakpoint
I presume that they could all be made to accept a list of sals instead of a single
sal, and init_raw_breakpoint would add all of them to the breakpoint.
But since that is all internal things never seen by the user, I don't think it would
be worth it to do this change.
So, the two patches LGTM. I think they are safe enough to merge right now, before the
gdb 10 branch is created. They address new scenarios without risking too much breaking
old ones.
Simon
More information about the Gdb-patches
mailing list