This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add a 'starti' command.
- From: Keith Seitz <keiths at redhat dot com>
- To: John Baldwin <jhb at FreeBSD dot org>, gdb-patches at sourceware dot org
- Date: Wed, 30 Aug 2017 15:31:51 -0700
- Subject: Re: [PATCH] Add a 'starti' command.
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=keiths at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CD76A356C8
- References: <20170829225457.66096-1-jhb@FreeBSD.org>
Hi,
I've looked over this patch, and I have a few small requests. [I am not a global maintainer and cannot officially approve your patch.]
On 08/29/2017 03:54 PM, John Baldwin wrote:
> This works like 'start' but it stops at the first instruction rather than
> the first line in main(). This is useful if one wants to single step
> through runtime linker startup.
I like this!
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index bd9ead8a45..5e9173ff44 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -520,12 +520,14 @@ prepare_execution_command (struct target_ops *target, int background)
> }
> }
>
> +enum run_break { NONE, MAIN, FIRST_INSTR };
> +
This does not conform to our coding standard. We typically require a comment before the enum explaining its usage/purpose and additional comments explaining what each member means. Something like:
/* Determines how run_command_1 will behave. */
enum run_break
{
/* Run the inferior. */
NONE,
/* Run the inferior, stopping at the first instruction of main. */
MAIN,
// ...
};
While I would rather see these options be a little more succinctly named, such as RUN_NONE, RUN_STOP_MAIN, RUN_STOP_MAIN_FIRST_INSN, I'm not going to ask for that change. But please do consider it.
> /* Implement the "run" command. If TBREAK_AT_MAIN is set, then insert
> a temporary breakpoint at the begining of the main program before
> running the program. */
>
> static void
> -run_command_1 (char *args, int from_tty, int tbreak_at_main)
> +run_command_1 (char *args, int from_tty, enum run_break run_break)
The comment to this function needs updating: it still refers to the (removed) argument TBREAK_AT_MAIN and does not mention RUN_BREAK.
> {
> const char *exec_file;
> struct cleanup *old_chain;
> @@ -632,6 +634,14 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main)
> has done its thing; now we are setting up the running program. */
> post_create_inferior (¤t_target, 0);
>
> + if (run_break == FIRST_INSTR)
> + {
> + gdb::unique_xmalloc_ptr<char> loc
> + (xstrdup (("*" + std::to_string (regcache_read_pc
> + (get_current_regcache ()))).c_str ()));
> + tbreak_command (loc.get (), 0);
> + }
> +
I understand why you did it this way, but I really think using create_breakpoint directly is a better option instead of routing through an existing CLI command (tbreak_command).
Something along the lines of:
CORE_ADDR insn = regcache_read_pc (get_current_regcache ());
event_location_up loc = new_address_location (insn, NULL, 0);
create_breakpoint (get_current_arch (), loc.get (),
NULL /* cond_string */, -1 /* thread */,
NULL /* extra_string */, 0 /* parse_extra */,
1 /* tempflag */, bp_breakpoint,
0 /* ignore_count */,
AUTO_BOOLEAN_FALSE /* pending_break_support */,
&bkpt_breakpoint_ops, from_tty, 1 /* enabled */,
0 /* internal */, 0 /* flags */);
> /* Start the target running. Do not use -1 continuation as it would skip
> breakpoint right at the entry point. */
> proceed (regcache_read_pc (get_current_regcache ()), GDB_SIGNAL_0);
> @@ -660,7 +670,17 @@ start_command (char *args, int from_tty)
> error (_("No symbol table loaded. Use the \"file\" command."));
>
> /* Run the program until reaching the main procedure... */
> - run_command_1 (args, from_tty, 1);
> + run_command_1 (args, from_tty, MAIN);
> +}
> +
> +/* Start the execution of the program until the first instruction. */
> +
> +static void
> +starti_command (char *args, int from_tty)
> +{
> +
> + /* Run the program until reaching the first instruction... */
> + run_command_1 (args, from_tty, FIRST_INSTR);
> }
I'm all for commenting code, but the comment describing the function is sufficient. :-)
> static int
> @@ -3415,6 +3435,12 @@ You may specify arguments to give to your program, just as with the\n\
> \"run\" command."));
> set_cmd_completer (c, filename_completer);
>
> + c = add_com ("starti", class_run, starti_command, _("\
> +Start the debugged program stopping at the first instruction.\n\
> +You may specify arguments to give to your program, just as with the\n\
> +\"run\" command."));
> + set_cmd_completer (c, filename_completer);
> +
> add_com ("interrupt", class_run, interrupt_command,
> _("Interrupt the execution of the debugged program.\n\
> If non-stop mode is enabled, interrupt only the current thread,\n\
>
I would take the common start/starti text and put it into a macro similar to the way the breakpoint commands are setup.
Alas the last thing missing is a test case for this. It doesn't have to be anything fancy, just check that with no breakpoints installed, starti will actually cause the inferior to stop somewhere/anywhere. [I don't think we can generically do much better than this, e.g., testing if we actually do stop at the first insn.]
Keith