Re: [PATCH] Add a 'starti' command.

[Keith Seitz <keiths@redhat.com>]

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 (&current_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