[RFA 0/8] Various command-related improvements

[RFA 0/8] Various command-related improvements

[Tom Tromey]

This series started as a few command-related bug fixes, but while
doing this I saw that several refactorings were needed, and so I've
added these in as well.

Regression tested by the buildbot.

Tom

[RFA 7/8] Allow breakpoint commands to be set from Python

[Tom Tromey]

This changes the Python API so that breakpoint commands can be set by
writing to the "commands" attribute.

2018-04-19  Tom Tromey  <tom@tromey.com>

	PR python/22731:
	* NEWS: Mention that breakpoint commands are writable.
	* python/py-breakpoint.c (bppy_set_commands): New function.
	(breakpoint_object_getset) <"commands">: Use it.

doc/ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	PR python/22731:
	* python.texi (Breakpoints In Python): Mention that "commands" is
	writable.

testsuite/ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	PR python/22731:
	* gdb.python/py-breakpoint.exp: Test setting breakpoint commands.
---
 gdb/ChangeLog                              |  7 +++++
 gdb/NEWS                                   |  5 ++++
 gdb/doc/ChangeLog                          |  6 ++++
 gdb/doc/python.texi                        |  2 +-
 gdb/python/py-breakpoint.c                 | 45 +++++++++++++++++++++++++++++-
 gdb/testsuite/ChangeLog                    |  5 ++++
 gdb/testsuite/gdb.python/py-breakpoint.exp | 10 ++++++-
 7 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 63fe30d175..14d5fbb7c0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -24,6 +24,11 @@ set|show record btrace cpu
   Controls the processor to be used for enabling errata workarounds for
   branch trace decode.
 
+* Python Scripting
+
+  ** The commands attached to a breakpoint can be set by assigning to
+     the breakpoint's "commands" field.
+
 * New targets
 
 RiscV ELF			riscv*-*-elf
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ebd48fffe7..a2b948a4ca 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5116,7 +5116,7 @@ value is @code{None}.  This attribute is writable.
 This attribute holds the commands attached to the breakpoint.  If
 there are commands, this attribute's value is a string holding all the
 commands, separated by newlines.  If there are no commands, this
-attribute is @code{None}.  This attribute is not writable.
+attribute is @code{None}.  This attribute is writable.
 @end defvar
 
 @node Finish Breakpoints in Python
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d654b92a8c..f386661bfb 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -510,6 +510,49 @@ bppy_get_commands (PyObject *self, void *closure)
   return host_string_to_python_string (stb.c_str ());
 }
 
+/* Set the commands attached to a breakpoint.  Returns 0 on success.
+   Returns -1 on error, with a python exception set.  */
+static int
+bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
+{
+  gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
+  struct breakpoint *bp = self_bp->bp;
+  struct gdb_exception except = exception_none;
+
+  BPPY_SET_REQUIRE_VALID (self_bp);
+
+  gdb::unique_xmalloc_ptr<char> commands
+    (python_string_to_host_string (newvalue));
+  if (commands == nullptr)
+    return -1;
+
+  TRY
+    {
+      bool first = true;
+      char *save_ptr = nullptr;
+      gdb::function_view<const char * ()> reader
+	= [&] ()
+	  {
+	    const char *result = strtok_r (first ? commands.get () : nullptr,
+					   "\n", &save_ptr);
+	    first = false;
+	    return result;
+	  };
+
+      counted_command_line lines = read_command_lines_1 (reader, 1, nullptr);
+      breakpoint_set_commands (self_bp->bp, std::move (lines));
+    }
+  CATCH (ex, RETURN_MASK_ALL)
+    {
+      except = ex;
+    }
+  END_CATCH
+
+  GDB_PY_SET_HANDLE_EXCEPTION (except);
+
+  return 0;
+}
+
 /* Python function to get the breakpoint type.  */
 static PyObject *
 bppy_get_type (PyObject *self, void *closure)
@@ -1185,7 +1228,7 @@ when setting this property.", NULL },
   { "condition", bppy_get_condition, bppy_set_condition,
     "Condition of the breakpoint, as specified by the user,\
 or None if no condition set."},
-  { "commands", bppy_get_commands, NULL,
+  { "commands", bppy_get_commands, bppy_set_commands,
     "Commands of the breakpoint, as specified by the user."},
   { "type", bppy_get_type, NULL,
     "Type of breakpoint."},
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index 6e0ff88f87..3ce0ea11de 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -197,8 +197,16 @@ proc_with_prefix test_bkpt_cond_and_cmds { } {
 
     gdb_py_test_silent_cmd "python blist = gdb.breakpoints()" \
 	"Get Breakpoint List" 0
-    gdb_test "python print (blist\[len(blist)-1\].commands)" \
+    gdb_py_test_silent_cmd "python last_bp = blist\[len(blist)-1\]" \
+	"Find last breakpoint" 0
+    gdb_test "python print (last_bp.commands)" \
 	"print \"Command for breakpoint has been executed.\".*print result"
+
+    gdb_test_no_output "python last_bp.commands = 'echo hi\\necho there'" \
+	"set commands"
+    # Note the length is 3 because the string ends in a \n.
+    gdb_test "python print (len(last_bp.commands.split('\\n')))" "3" \
+	"check number of lines in commands"
 }
 
 proc_with_prefix test_bkpt_invisible { } {
-- 
2.13.6

Re: [RFA 7/8] Allow breakpoint commands to be set from Python

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Thu, 19 Apr 2018 13:15:38 -0600
> 
> This changes the Python API so that breakpoint commands can be set by
> writing to the "commands" attribute.
> 
> 2018-04-19  Tom Tromey  <tom@tromey.com>
> 
> 	PR python/22731:
> 	* NEWS: Mention that breakpoint commands are writable.
> 	* python/py-breakpoint.c (bppy_set_commands): New function.
> 	(breakpoint_object_getset) <"commands">: Use it.
> 
> doc/ChangeLog
> 2018-04-19  Tom Tromey  <tom@tromey.com>
> 
> 	PR python/22731:
> 	* python.texi (Breakpoints In Python): Mention that "commands" is
> 	writable.
> 
> testsuite/ChangeLog
> 2018-04-19  Tom Tromey  <tom@tromey.com>
> 
> 	PR python/22731:
> 	* gdb.python/py-breakpoint.exp: Test setting breakpoint commands.

OK for the documentation parts.

Thanks.

Re: [RFA 7/8] Allow breakpoint commands to be set from Python

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:

> +/* Set the commands attached to a breakpoint.  Returns 0 on success.
> +   Returns -1 on error, with a python exception set.  */
> +static int
> +bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
> +{
> +  gdbpy_breakpoint_object *self_bp = (gdbpy_breakpoint_object *) self;
> +  struct breakpoint *bp = self_bp->bp;
> +  struct gdb_exception except = exception_none;
> +
> +  BPPY_SET_REQUIRE_VALID (self_bp);
> +
> +  gdb::unique_xmalloc_ptr<char> commands
> +    (python_string_to_host_string (newvalue));
> +  if (commands == nullptr)
> +    return -1;
> +
> +  TRY
> +    {
> +      bool first = true;
> +      char *save_ptr = nullptr;
> +      gdb::function_view<const char * ()> reader
> +	= [&] ()

Like in the other patch, binding to a temporary.  Use "auto".

Otherwise looks fine to me.

> +	  {
> +	    const char *result = strtok_r (first ? commands.get () : nullptr,
> +					   "\n", &save_ptr);
> +	    first = false;
> +	    return result;
> +	  };
> +
> +      counted_command_line lines = read_command_lines_1 (reader, 1, nullptr);
> +      breakpoint_set_commands (self_bp->bp, std::move (lines));
> +    }
> +  CATCH (ex, RETURN_MASK_ALL)
> +    {
> +      except = ex;
> +    }
> +  END_CATCH
> +

Thanks,
Pedro Alves

[RFA 8/8] Let gdb.execute handle multi-line commands

[Tom Tromey]

This changes the Python API so that gdb.execute can now handle
multi-line commands, like "commands" or "define".

2018-04-19  Tom Tromey  <tom@tromey.com>

	PR python/22730:
	* NEWS: Mention gdb.execute change.
	* gdbcmd.h (execute_control_command): Don't declare.
	* python/python.c (execute_gdb_command): Use read_command_lines_1,
	execute_control_commands, execute_control_commands_to_string.
	* cli/cli-script.h (execute_control_commands)
	(execute_control_commands_to_string): Declare.
	(execute_control_command): Add from_tty parameter.
	* cli/cli-script.c (execute_control_commands)
	(execute_control_commands_to_string): New functions.
	(execute_user_command): Use execute_control_commands.
	(execute_control_command_1): Add "from_tty" parameter.  Update.
	(execute_control_command): Likewise.

testsuite/ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	PR python/22730:
	* gdb.python/python.exp: Test multi-line execute.
---
 gdb/ChangeLog                       | 16 +++++++
 gdb/NEWS                            |  2 +
 gdb/cli/cli-script.c                | 93 ++++++++++++++++++++++++++-----------
 gdb/cli/cli-script.h                | 15 +++++-
 gdb/gdbcmd.h                        |  2 -
 gdb/python/python.c                 | 19 +++++++-
 gdb/testsuite/ChangeLog             |  5 ++
 gdb/testsuite/gdb.python/python.exp |  3 ++
 8 files changed, 123 insertions(+), 32 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 14d5fbb7c0..a72ee21cf8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -29,6 +29,8 @@ set|show record btrace cpu
   ** The commands attached to a breakpoint can be set by assigning to
      the breakpoint's "commands" field.
 
+  ** gdb.execute can now execute multi-line gdb commands.
+
 * New targets
 
 RiscV ELF			riscv*-*-elf
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 453983b3bd..775fda4b96 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -369,12 +369,69 @@ execute_cmd_post_hook (struct cmd_list_element *c)
     }
 }
 
+/* See cli-script.h.  */
+
+void
+execute_control_commands (struct command_line *cmdlines, int from_tty)
+{
+  /* Set the instream to 0, indicating execution of a
+     user-defined function.  */
+  scoped_restore restore_instream
+    = make_scoped_restore (&current_ui->instream, nullptr);
+  scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
+  scoped_restore save_nesting
+    = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);
+
+  while (cmdlines)
+    {
+      enum command_control_type ret = execute_control_command (cmdlines,
+							       from_tty);
+      if (ret != simple_control && ret != break_control)
+	{
+	  warning (_("Error executing canned sequence of commands."));
+	  break;
+	}
+      cmdlines = cmdlines->next;
+    }
+}
+
+/* See cli-script.h.  */
+
+std::string
+execute_control_commands_to_string (struct command_line *commands,
+				    int from_tty)
+{
+  /* GDB_STDOUT should be better already restored during these
+     restoration callbacks.  */
+  set_batch_flag_and_restore_page_info save_page_info;
+
+  string_file str_file;
+
+  {
+    current_uiout->redirect (&str_file);
+    ui_out_redirect_pop redirect_popper (current_uiout);
+
+    scoped_restore save_stdout
+      = make_scoped_restore (&gdb_stdout, &str_file);
+    scoped_restore save_stderr
+      = make_scoped_restore (&gdb_stderr, &str_file);
+    scoped_restore save_stdlog
+      = make_scoped_restore (&gdb_stdlog, &str_file);
+    scoped_restore save_stdtarg
+      = make_scoped_restore (&gdb_stdtarg, &str_file);
+    scoped_restore save_stdtargerr
+      = make_scoped_restore (&gdb_stdtargerr, &str_file);
+
+    execute_control_commands (commands, from_tty);
+  }
+
+  return std::move (str_file.string ());
+}
+
 void
 execute_user_command (struct cmd_list_element *c, const char *args)
 {
-  struct ui *ui = current_ui;
   counted_command_line cmdlines_copy;
-  enum command_control_type ret;
   extern unsigned int max_user_call_depth;
 
   /* Ensure that the user commands can't be deleted while they are
@@ -390,25 +447,7 @@ execute_user_command (struct cmd_list_element *c, const char *args)
   if (user_args_stack.size () > max_user_call_depth)
     error (_("Max user call depth exceeded -- command aborted."));
 
-  /* Set the instream to 0, indicating execution of a
-     user-defined function.  */
-  scoped_restore restore_instream
-    = make_scoped_restore (&ui->instream, nullptr);
-
-  scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
-
-  scoped_restore save_nesting
-    = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);
-  while (cmdlines)
-    {
-      ret = execute_control_command (cmdlines);
-      if (ret != simple_control && ret != break_control)
-	{
-	  warning (_("Error executing canned sequence of commands."));
-	  break;
-	}
-      cmdlines = cmdlines->next;
-    }
+  execute_control_commands (cmdlines, 0);
 }
 
 /* This function is called every time GDB prints a prompt.  It ensures
@@ -460,7 +499,7 @@ print_command_trace (const char *fmt, ...)
 /* Helper for execute_control_command.  */
 
 static enum command_control_type
-execute_control_command_1 (struct command_line *cmd)
+execute_control_command_1 (struct command_line *cmd, int from_tty)
 {
   struct command_line *current;
   struct value *val;
@@ -478,7 +517,7 @@ execute_control_command_1 (struct command_line *cmd)
       {
 	/* A simple command, execute it and return.  */
 	std::string new_line = insert_user_defined_cmd_args (cmd->line);
-	execute_command (new_line.c_str (), 0);
+	execute_command (new_line.c_str (), from_tty);
 	ret = cmd->control_type;
 	break;
       }
@@ -533,7 +572,7 @@ execute_control_command_1 (struct command_line *cmd)
 	      {
 		scoped_restore save_nesting
 		  = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);
-		ret = execute_control_command_1 (current);
+		ret = execute_control_command_1 (current, from_tty);
 
 		/* If we got an error, or a "break" command, then stop
 		   looping.  */
@@ -588,7 +627,7 @@ execute_control_command_1 (struct command_line *cmd)
 	  {
 	    scoped_restore save_nesting
 	      = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);
-	    ret = execute_control_command_1 (current);
+	    ret = execute_control_command_1 (current, from_tty);
 
 	    /* If we got an error, get out.  */
 	    if (ret != simple_control)
@@ -639,7 +678,7 @@ execute_control_command_1 (struct command_line *cmd)
 }
 
 enum command_control_type
-execute_control_command (struct command_line *cmd)
+execute_control_command (struct command_line *cmd, int from_tty)
 {
   /* Make sure we use the console uiout.  It's possible that we are executing
      breakpoint commands while running the MI interpreter.  */
@@ -647,7 +686,7 @@ execute_control_command (struct command_line *cmd)
   scoped_restore save_uiout
     = make_scoped_restore (&current_uiout, interp_ui_out (console));
 
-  return execute_control_command_1 (cmd);
+  return execute_control_command_1 (cmd, from_tty);
 }
 
 /* Like execute_control_command, but first set
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 3bebd0ed9d..736ebb3a7b 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -122,10 +122,23 @@ extern void show_user_1 (struct cmd_list_element *c,
 			 const char *name,
 			 struct ui_file *stream);
 
+/* Execute the commands in CMDLINES.  */
+
+extern void execute_control_commands (struct command_line *cmdlines,
+				      int from_tty);
+
+/* Run execute_control_commands for COMMANDS.  Capture its output into
+   the returned string, do not display it to the screen.  BATCH_FLAG
+   will be temporarily set to true.  */
+
+extern std::string execute_control_commands_to_string
+    (struct command_line *commands, int from_tty);
+
 /* Exported to gdb/breakpoint.c */
 
 extern enum command_control_type
-	execute_control_command (struct command_line *cmd);
+	execute_control_command (struct command_line *cmd,
+				 int from_tty = 0);
 
 extern enum command_control_type
 	execute_control_command_untraced (struct command_line *cmd);
diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h
index 342c8b2c0e..b675ae8618 100644
--- a/gdb/gdbcmd.h
+++ b/gdb/gdbcmd.h
@@ -135,8 +135,6 @@ extern struct cmd_list_element *save_cmdlist;
 extern void execute_command (const char *, int);
 extern std::string execute_command_to_string (const char *p, int from_tty);
 
-enum command_control_type execute_control_command (struct command_line *);
-
 extern void print_command_line (struct command_line *, unsigned int,
 				struct ui_file *);
 extern void print_command_lines (struct ui_out *,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 45b6516fc2..5438a148b5 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -581,6 +581,20 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
     {
       struct interp *interp;
 
+      std::string arg_copy = arg;
+      bool first = true;
+      char *save_ptr = nullptr;
+      gdb::function_view<const char * ()> reader
+	= [&] ()
+	  {
+	    const char *result = strtok_r (first ? &arg_copy[0] : nullptr,
+					   "\n", &save_ptr);
+	    first = false;
+	    return result;
+	  };
+
+      counted_command_line lines = read_command_lines_1 (reader, 1, nullptr);
+
       scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
 
       scoped_restore save_uiout = make_scoped_restore (&current_uiout);
@@ -592,9 +606,10 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 
       scoped_restore preventer = prevent_dont_repeat ();
       if (to_string)
-	to_string_res = execute_command_to_string (arg, from_tty);
+	to_string_res = execute_control_commands_to_string (lines.get (),
+							    from_tty);
       else
-	execute_command (arg, from_tty);
+	execute_control_commands (lines.get (), from_tty);
     }
   CATCH (except, RETURN_MASK_ALL)
     {
diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
index cee195f315..f6bf93add0 100644
--- a/gdb/testsuite/gdb.python/python.exp
+++ b/gdb/testsuite/gdb.python/python.exp
@@ -119,6 +119,9 @@ gdb_test_no_output \
     "python x = gdb.execute('printf \"%d\", 23', to_string = True)"
 gdb_test "python print (x)" "23"
 
+gdb_test "python gdb.execute('echo 2\\necho 3\\\\n\\n')" "23" \
+    "multi-line execute"
+
 # Test post_event.
 gdb_py_test_multiple "post event insertion" \
   "python" "" \
-- 
2.13.6

Re: [RFA 8/8] Let gdb.execute handle multi-line commands

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Thu, 19 Apr 2018 13:15:39 -0600
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 14d5fbb7c0..a72ee21cf8 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -29,6 +29,8 @@ set|show record btrace cpu
>    ** The commands attached to a breakpoint can be set by assigning to
>       the breakpoint's "commands" field.
>  
> +  ** gdb.execute can now execute multi-line gdb commands.
> +
>  * New targets
>  

OK for this part, thanks.

Re: [RFA 8/8] Let gdb.execute handle multi-line commands

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:

> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -581,6 +581,20 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>      {
>        struct interp *interp;
>  
> +      std::string arg_copy = arg;
> +      bool first = true;
> +      char *save_ptr = nullptr;
> +      gdb::function_view<const char * ()> reader
> +	= [&] ()

"auto reader"

Looks good to me otherwise.

> +	  {
> +	    const char *result = strtok_r (first ? &arg_copy[0] : nullptr,
> +					   "\n", &save_ptr);
> +	    first = false;
> +	    return result;
> +	  };
> +
Thanks,
Pedro Alves

[RFA 4/8] Constify prompt argument to read_command_lines

[Tom Tromey]

The prompt argument to read_command_lines can be const.  This patch
makes this change, and also removes some fixed-sized buffers in favor
of using string_printf.

2018-04-19  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (actions_command): Update.
	* cli/cli-script.h (read_command_lines): Update.
	* cli/cli-script.c (read_command_lines): Constify prompt_arg.
	(MAX_TMPBUF): Remove define.
	(define_command): Use string_printf.
	(document_command): Likewise.
	* breakpoint.c (commands_command_1): Update.
---
 gdb/ChangeLog        | 10 ++++++++++
 gdb/breakpoint.c     |  3 +--
 gdb/cli/cli-script.c | 20 +++++++++-----------
 gdb/cli/cli-script.h |  2 +-
 gdb/tracepoint.c     |  3 ++-
 5 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2cb9c4b657..7d284e9a96 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1256,8 +1256,7 @@ commands_command_1 (const char *arg, int from_tty,
 				    "%s, one per line."),
 				  arg);
 
-	       cmd = read_command_lines (&str[0],
-					 from_tty, 1,
+	       cmd = read_command_lines (str.c_str (), from_tty, 1,
 					 (is_tracepoint (b)
 					  ? check_tracepoint_command : 0),
 					 b);
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index c7d405c0d0..36740b97ad 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -1121,7 +1121,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 #define END_MESSAGE "End with a line saying just \"end\"."
 
 counted_command_line
-read_command_lines (char *prompt_arg, int from_tty, int parse_commands,
+read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
 		    void (*validator)(char *, void *), void *closure)
 {
   if (from_tty && input_interactive_p (current_ui))
@@ -1306,7 +1306,6 @@ user_defined_command (const char *ignore, int from_tty)
 static void
 define_command (const char *comname, int from_tty)
 {
-#define MAX_TMPBUF 128   
   enum cmd_hook_type
     {
       CMD_NO_HOOK = 0,
@@ -1315,7 +1314,6 @@ define_command (const char *comname, int from_tty)
     };
   struct cmd_list_element *c, *newc, *hookc = 0, **list;
   const char *tem, *comfull;
-  char tmpbuf[MAX_TMPBUF];
   int  hook_type      = CMD_NO_HOOK;
   int  hook_name_size = 0;
    
@@ -1379,9 +1377,10 @@ define_command (const char *comname, int from_tty)
 
   comname = xstrdup (comname);
 
-  xsnprintf (tmpbuf, sizeof (tmpbuf),
-	     "Type commands for definition of \"%s\".", comfull);
-  counted_command_line cmds = read_command_lines (tmpbuf, from_tty, 1, 0, 0);
+  std::string prompt
+    = string_printf ("Type commands for definition of \"%s\".", comfull);
+  counted_command_line cmds = read_command_lines (prompt.c_str (), from_tty,
+						  1, 0, 0);
 
   newc = add_cmd (comname, class_user, user_defined_command,
 		  (c && c->theclass == class_user)
@@ -1416,7 +1415,6 @@ document_command (const char *comname, int from_tty)
   struct cmd_list_element *c, **list;
   const char *tem;
   const char *comfull;
-  char tmpbuf[128];
 
   comfull = comname;
   list = validate_comname (&comname);
@@ -1427,10 +1425,10 @@ document_command (const char *comname, int from_tty)
   if (c->theclass != class_user)
     error (_("Command \"%s\" is built-in."), comfull);
 
-  xsnprintf (tmpbuf, sizeof (tmpbuf), "Type documentation for \"%s\".",
-	     comfull);
-  counted_command_line doclines = read_command_lines (tmpbuf, from_tty,
-						      0, 0, 0);
+  std::string prompt = string_printf ("Type documentation for \"%s\".",
+				      comfull);
+  counted_command_line doclines = read_command_lines (prompt.c_str (),
+						      from_tty, 0, 0, 0);
 
   if (c->doc)
     xfree ((char *) c->doc);
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 10b6c17789..7e5f94c0ad 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -105,7 +105,7 @@ private:
   }
 };
 
-extern counted_command_line read_command_lines (char *, int, int,
+extern counted_command_line read_command_lines (const char *, int, int,
 						void (*)(char *, void *),
 						void *);
 extern counted_command_line read_command_lines_1 (char * (*) (void), int,
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 0c3842199f..0c62b957ef 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -578,7 +578,8 @@ actions_command (const char *args, int from_tty)
 	string_printf ("Enter actions for tracepoint %d, one per line.",
 		       t->number);
 
-      counted_command_line l = read_command_lines (&tmpbuf[0], from_tty, 1,
+      counted_command_line l = read_command_lines (tmpbuf.c_str (),
+						   from_tty, 1,
 						   check_tracepoint_command,
 						   t);
       breakpoint_set_commands (t, std::move (l));
-- 
2.13.6

Re: [RFA 4/8] Constify prompt argument to read_command_lines

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:
> The prompt argument to read_command_lines can be const.  This patch
> makes this change, and also removes some fixed-sized buffers in favor
> of using string_printf.
> 
> 2018-04-19  Tom Tromey  <tom@tromey.com>
> 
> 	* tracepoint.c (actions_command): Update.
> 	* cli/cli-script.h (read_command_lines): Update.
> 	* cli/cli-script.c (read_command_lines): Constify prompt_arg.
> 	(MAX_TMPBUF): Remove define.
> 	(define_command): Use string_printf.
> 	(document_command): Likewise.
> 	* breakpoint.c (commands_command_1): Update.

Looks good.

Thanks,
Pedro Alves

[RFA 2/8] Use counted_command_line everywhere

[Tom Tromey]

Currently command lines are reference counted using shared_ptr only
when attached to breakpoints.  This patch changes gdb to use
shared_ptr in commands as well.  This allows for the removal of
copy_command_lines.

Note that the change to execute_user_command explicitly makes a new
reference to the command line.  This will be used in a later patch.

This simplifies struct command_line based on the observation that a
given command can have at most two child bodies: an "if" can have both
"then" and "else" parts.  Perhaps the names I've chosen for the
replacements here are not very good -- your input requested.

ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (all_tracepoint_actions): Rename from
	all_tracepoint_actions_and_cleanup.  Change return type.
	(actions_command, encode_actions_1, encode_actions)
	(trace_dump_actions, tdump_command): Update.
	* remote.c (remote_download_command_source): Update.
	* python/python.c (gdbpy_eval_from_control_command)
	(python_command, python_interactive_command): Update.
	* mi/mi-cmd-break.c (mi_cmd_break_commands): Update.
	* guile/guile.c (guile_command)
	(gdbscm_eval_from_control_command, guile_command): Update.
	* compile/compile.c (compile_code_command)
	(compile_print_command, compile_to_object): Update.
	* cli/cli-script.h (struct command_lines_deleter): New.
	(counted_command_line): New typedef.
	(struct command_line): Add constructor, destructor.
	<body_list>: Remove.
	<body_list_0, body_list_1>: New members.
	(command_line_up): Remove typedef.
	(read_command_lines, read_command_lines_1, get_command_line):
	Update.
	(copy_command_lines): Don't declare.
	* cli/cli-script.c (build_command_line): Use "new".
	(get_command_line): Return counted_command_line.
	(print_command_lines, execute_user_command)
	(execute_control_command_1, while_command, if_command): Update.
	(realloc_body_list): Remove.
	(process_next_line, recurse_read_control_structure): Update.
	(read_command_lines, read_command_lines_1): Return counted_command_line.
	(free_command_lines): Use "delete".
	(copy_command_lines): Remove.
	(define_command, document_command, show_user_1): Update.
	* cli/cli-decode.h (struct cmd_list_element) <user_commands>: Now
	a counted_command_line.
	* breakpoint.h (counted_command_line): Remove typedef.
	(breakpoint_set_commands): Update.
	* breakpoint.c (check_no_tracepoint_commands)
	(validate_commands_for_breakpoint): Update.
	(breakpoint_set_commands): Change commands to be a
	counted_command_line.
	(commands_command_1, update_dprintf_command_list)
	(create_tracepoint_from_upload): Update.
---
 gdb/ChangeLog         |  44 ++++++++++++
 gdb/breakpoint.c      |  30 ++++----
 gdb/breakpoint.h      |   6 +-
 gdb/cli/cli-decode.h  |   3 +-
 gdb/cli/cli-script.c  | 190 ++++++++++++--------------------------------------
 gdb/cli/cli-script.h  |  66 +++++++++++-------
 gdb/compile/compile.c |   6 +-
 gdb/guile/guile.c     |   8 +--
 gdb/mi/mi-cmd-break.c |   2 +-
 gdb/python/python.c   |   8 +--
 gdb/remote.c          |   2 +-
 gdb/tracepoint.c      |  62 +++++++---------
 12 files changed, 184 insertions(+), 243 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 11b89bcf88..2cb9c4b657 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1015,8 +1015,8 @@ check_no_tracepoint_commands (struct command_line *commands)
 	error (_("The 'while-stepping' command can "
 		 "only be used for tracepoints"));
 
-      for (i = 0; i < c->body_count; ++i)
-	check_no_tracepoint_commands ((c->body_list)[i]);
+      check_no_tracepoint_commands (c->body_list_0.get ());
+      check_no_tracepoint_commands (c->body_list_1.get ());
 
       /* Not that command parsing removes leading whitespace and comment
 	 lines and also empty lines.  So, we only need to check for
@@ -1127,8 +1127,8 @@ validate_commands_for_breakpoint (struct breakpoint *b,
 	{
 	  struct command_line *c2;
 
-	  gdb_assert (while_stepping->body_count == 1);
-	  c2 = while_stepping->body_list[0];
+	  gdb_assert (while_stepping->body_list_1 == nullptr);
+	  c2 = while_stepping->body_list_0.get ();
 	  for (; c2; c2 = c2->next)
 	    {
 	      if (c2->control_type == while_stepping_control)
@@ -1168,7 +1168,7 @@ static_tracepoints_here (CORE_ADDR addr)
 
 void
 breakpoint_set_commands (struct breakpoint *b, 
-			 command_line_up &&commands)
+			 counted_command_line &&commands)
 {
   validate_commands_for_breakpoint (b, commands.get ());
 
@@ -1248,7 +1248,7 @@ commands_command_1 (const char *arg, int from_tty,
        if (cmd == NULL)
 	 {
 	   if (control != NULL)
-	     cmd = copy_command_lines (control->body_list[0]);
+	     cmd = control->body_list_0;
 	   else
 	     {
 	       std::string str
@@ -8781,18 +8781,12 @@ update_dprintf_command_list (struct breakpoint *b)
 		    _("Invalid dprintf style."));
 
   gdb_assert (printf_line != NULL);
-  /* Manufacture a printf sequence.  */
-  {
-    struct command_line *printf_cmd_line = XNEW (struct command_line);
-
-    printf_cmd_line->control_type = simple_control;
-    printf_cmd_line->body_count = 0;
-    printf_cmd_line->body_list = NULL;
-    printf_cmd_line->next = NULL;
-    printf_cmd_line->line = printf_line;
 
-    breakpoint_set_commands (b, command_line_up (printf_cmd_line));
-  }
+  /* Manufacture a printf sequence.  */
+  struct command_line *printf_cmd_line
+    = new struct command_line (simple_control, printf_line);
+  breakpoint_set_commands (b, counted_command_line (printf_cmd_line,
+						    command_lines_deleter ()));
 }
 
 /* Update all dprintf commands, making their command lists reflect
@@ -14796,7 +14790,7 @@ create_tracepoint_from_upload (struct uploaded_tp *utp)
      function.  */
   if (!utp->cmd_strings.empty ())
     {
-      command_line_up cmd_list;
+      counted_command_line cmd_list;
 
       this_utp = utp;
       next_cmd = 0;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 062e390469..f3c2599286 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -661,10 +661,6 @@ enum watchpoint_triggered
 typedef struct bp_location *bp_location_p;
 DEF_VEC_P(bp_location_p);
 
-/* A reference-counted struct command_line. This is an implementation
-   detail to the breakpoints module.  */
-typedef std::shared_ptr<command_line> counted_command_line;
-
 /* Some targets (e.g., embedded PowerPC) need two debug registers to set
    a watchpoint over a memory region.  If this flag is true, GDB will use
    only one register per watchpoint, thus assuming that all acesses that
@@ -1479,7 +1475,7 @@ extern void disable_breakpoint (struct breakpoint *);
 extern void enable_breakpoint (struct breakpoint *);
 
 extern void breakpoint_set_commands (struct breakpoint *b, 
-				     command_line_up &&commands);
+				     counted_command_line &&commands);
 
 extern void breakpoint_set_silent (struct breakpoint *b, int silent);
 
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index a3208e1fc6..e18b709767 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -24,6 +24,7 @@
 /* Include the public interfaces.  */
 #include "command.h"
 #include "gdb_regex.h"
+#include "cli-script.h"
 
 #if 0
 /* FIXME: cagney/2002-03-17: Once cmd_type() has been removed, ``enum
@@ -213,7 +214,7 @@ struct cmd_list_element
     const char *const *enums = nullptr;
 
     /* Pointer to command strings of user-defined commands */
-    struct command_line *user_commands = nullptr;
+    counted_command_line user_commands;
 
     /* Pointer to command that is hooked by this one, (by hook_pre)
        so the hook can be removed when this one is deleted.  */
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index cdfda113a7..b066da7d60 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -134,32 +134,23 @@ multi_line_command_p (enum command_control_type type)
 static struct command_line *
 build_command_line (enum command_control_type type, const char *args)
 {
-  struct command_line *cmd;
-
   if ((args == NULL || *args == '\0')
       && (type == if_control || type == while_control))
     error (_("if/while commands require arguments."));
   gdb_assert (args != NULL);
 
-  cmd = XNEW (struct command_line);
-  cmd->next = NULL;
-  cmd->control_type = type;
-
-  cmd->body_count = 1;
-  cmd->body_list = XCNEWVEC (struct command_line *, cmd->body_count);
-  cmd->line = xstrdup (args);
-
-  return cmd;
+  return new struct command_line (type, xstrdup (args));
 }
 
 /* Build and return a new command structure for the control commands
    such as "if" and "while".  */
 
-command_line_up
+counted_command_line
 get_command_line (enum command_control_type type, const char *arg)
 {
   /* Allocate and build a new command line structure.  */
-  command_line_up cmd (build_command_line (type, arg));
+  counted_command_line cmd (build_command_line (type, arg),
+			    command_lines_deleter ());
 
   /* Read in the body of this command.  */
   if (recurse_read_control_structure (read_next_line, cmd.get (), 0, 0)
@@ -228,7 +219,7 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	  else
 	    uiout->field_string (NULL, list->line);
 	  uiout->text ("\n");
-	  print_command_lines (uiout, *list->body_list, depth + 1);
+	  print_command_lines (uiout, list->body_list_0.get (), depth + 1);
 	  if (depth)
 	    uiout->spaces (2 * depth);
 	  uiout->field_string (NULL, "end");
@@ -244,16 +235,16 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	  uiout->field_fmt (NULL, "if %s", list->line);
 	  uiout->text ("\n");
 	  /* The true arm.  */
-	  print_command_lines (uiout, list->body_list[0], depth + 1);
+	  print_command_lines (uiout, list->body_list_0.get (), depth + 1);
 
 	  /* Show the false arm if it exists.  */
-	  if (list->body_count == 2)
+	  if (list->body_list_1 != nullptr)
 	    {
 	      if (depth)
 		uiout->spaces (2 * depth);
 	      uiout->field_string (NULL, "else");
 	      uiout->text ("\n");
-	      print_command_lines (uiout, list->body_list[1], depth + 1);
+	      print_command_lines (uiout, list->body_list_1.get (), depth + 1);
 	    }
 
 	  if (depth)
@@ -273,7 +264,7 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	  else
 	    uiout->field_string (NULL, "commands");
 	  uiout->text ("\n");
-	  print_command_lines (uiout, *list->body_list, depth + 1);
+	  print_command_lines (uiout, list->body_list_0.get (), depth + 1);
 	  if (depth)
 	    uiout->spaces (2 * depth);
 	  uiout->field_string (NULL, "end");
@@ -287,7 +278,7 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	  uiout->field_string (NULL, "python");
 	  uiout->text ("\n");
 	  /* Don't indent python code at all.  */
-	  print_command_lines (uiout, *list->body_list, 0);
+	  print_command_lines (uiout, list->body_list_0.get (), 0);
 	  if (depth)
 	    uiout->spaces (2 * depth);
 	  uiout->field_string (NULL, "end");
@@ -300,7 +291,7 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	{
 	  uiout->field_string (NULL, "compile expression");
 	  uiout->text ("\n");
-	  print_command_lines (uiout, *list->body_list, 0);
+	  print_command_lines (uiout, list->body_list_0.get (), 0);
 	  if (depth)
 	    uiout->spaces (2 * depth);
 	  uiout->field_string (NULL, "end");
@@ -313,7 +304,7 @@ print_command_lines (struct ui_out *uiout, struct command_line *cmd,
 	{
 	  uiout->field_string (NULL, "guile");
 	  uiout->text ("\n");
-	  print_command_lines (uiout, *list->body_list, depth + 1);
+	  print_command_lines (uiout, list->body_list_0.get (), depth + 1);
 	  if (depth)
 	    uiout->spaces (2 * depth);
 	  uiout->field_string (NULL, "end");
@@ -377,14 +368,17 @@ void
 execute_user_command (struct cmd_list_element *c, const char *args)
 {
   struct ui *ui = current_ui;
-  struct command_line *cmdlines;
+  counted_command_line cmdlines_copy;
   enum command_control_type ret;
   extern unsigned int max_user_call_depth;
 
-  cmdlines = c->user_commands;
-  if (cmdlines == 0)
+  /* Ensure that the user commands can't be deleted while they are
+     executing.  */
+  cmdlines_copy = c->user_commands;
+  if (cmdlines_copy == 0)
     /* Null command */
     return;
+  struct command_line *cmdlines = cmdlines_copy.get ();
 
   scoped_user_args_level push_user_args (args);
 
@@ -527,7 +521,7 @@ execute_control_command_1 (struct command_line *cmd)
 	      break;
 
 	    /* Execute the body of the while statement.  */
-	    current = *cmd->body_list;
+	    current = cmd->body_list_0.get ();
 	    while (current)
 	      {
 		scoped_restore save_nesting
@@ -581,9 +575,9 @@ execute_control_command_1 (struct command_line *cmd)
 	/* Choose which arm to take commands from based on the value
 	   of the conditional expression.  */
 	if (value_true (val))
-	  current = *cmd->body_list;
-	else if (cmd->body_count == 2)
-	  current = *(cmd->body_list + 1);
+	  current = cmd->body_list_0.get ();
+	else if (cmd->body_list_1 != nullptr)
+	  current = cmd->body_list_1.get ();
 	value_free_to_mark (val_mark);
 
 	/* Execute commands in the given arm.  */
@@ -665,7 +659,7 @@ static void
 while_command (const char *arg, int from_tty)
 {
   control_level = 1;
-  command_line_up command = get_command_line (while_control, arg);
+  counted_command_line command = get_command_line (while_control, arg);
 
   if (command == NULL)
     return;
@@ -682,7 +676,7 @@ static void
 if_command (const char *arg, int from_tty)
 {
   control_level = 1;
-  command_line_up command = get_command_line (if_control, arg);
+  counted_command_line command = get_command_line (if_control, arg);
 
   if (command == NULL)
     return;
@@ -828,31 +822,6 @@ user_args::insert_args (const char *line) const
 }
 
 \f
-/* Expand the body_list of COMMAND so that it can hold NEW_LENGTH
-   code bodies.  This is typically used when we encounter an "else"
-   clause for an "if" command.  */
-
-static void
-realloc_body_list (struct command_line *command, int new_length)
-{
-  int n;
-  struct command_line **body_list;
-
-  n = command->body_count;
-
-  /* Nothing to do?  */
-  if (new_length <= n)
-    return;
-
-  body_list = XCNEWVEC (struct command_line *, new_length);
-
-  memcpy (body_list, command->body_list, sizeof (struct command_line *) * n);
-
-  xfree (command->body_list);
-  command->body_list = body_list;
-  command->body_count = new_length;
-}
-
 /* Read next line from stdin.  Passed to read_command_line_1 and
    recurse_read_control_structure whenever we need to read commands
    from stdin.  */
@@ -1012,23 +981,9 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
 	  *command = build_command_line (guile_control, "");
 	}
       else if (p_end - p == 10 && startswith (p, "loop_break"))
-	{
-	  *command = XNEW (struct command_line);
-	  (*command)->next = NULL;
-	  (*command)->line = NULL;
-	  (*command)->control_type = break_control;
-	  (*command)->body_count = 0;
-	  (*command)->body_list = NULL;
-	}
+	*command = new struct command_line (break_control);
       else if (p_end - p == 13 && startswith (p, "loop_continue"))
-	{
-	  *command = XNEW (struct command_line);
-	  (*command)->next = NULL;
-	  (*command)->line = NULL;
-	  (*command)->control_type = continue_control;
-	  (*command)->body_count = 0;
-	  (*command)->body_list = NULL;
-	}
+	*command = new struct command_line (continue_control);
       else
 	not_handled = 1;
     }
@@ -1036,12 +991,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
   if (!parse_commands || not_handled)
     {
       /* A normal command.  */
-      *command = XNEW (struct command_line);
-      (*command)->next = NULL;
-      (*command)->line = savestring (p, p_end - p);
-      (*command)->control_type = simple_control;
-      (*command)->body_count = 0;
-      (*command)->body_list = NULL;
+      *command = new struct command_line (simple_control,
+					  savestring (p, p_end - p));
     }
 
   if (validator)
@@ -1053,7 +1004,7 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
 	}
       CATCH (ex, RETURN_MASK_ALL)
 	{
-	  xfree (*command);
+	  free_command_lines (command);
 	  throw_exception (ex);
 	}
       END_CATCH
@@ -1073,21 +1024,17 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 				void (*validator)(char *, void *),
 				void *closure)
 {
-  int current_body, i;
   enum misc_command_type val;
   enum command_control_type ret;
   struct command_line **body_ptr, *child_tail, *next;
+  counted_command_line *current_body = &current_cmd->body_list_0;
 
   child_tail = NULL;
-  current_body = 1;
 
   /* Sanity checks.  */
   if (current_cmd->control_type == simple_control)
     error (_("Recursed on a simple control type."));
 
-  if (current_body > current_cmd->body_count)
-    error (_("Allocated body is smaller than this command type needs."));
-
   /* Read lines from the input stream and build control structures.  */
   while (1)
     {
@@ -1123,10 +1070,9 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
       if (val == else_command)
 	{
 	  if (current_cmd->control_type == if_control
-	      && current_body == 1)
+	      && current_body == &current_cmd->body_list_0)
 	    {
-	      realloc_body_list (current_cmd, 2);
-	      current_body = 2;
+	      current_body = &current_cmd->body_list_1;
 	      child_tail = NULL;
 	      continue;
 	    }
@@ -1142,14 +1088,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 	  child_tail->next = next;
 	}
       else
-	{
-	  body_ptr = current_cmd->body_list;
-	  for (i = 1; i < current_body; i++)
-	    body_ptr++;
-
-	  *body_ptr = next;
-
-	}
+	*current_body = counted_command_line (next, command_lines_deleter ());
 
       child_tail = next;
 
@@ -1183,7 +1122,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 
 #define END_MESSAGE "End with a line saying just \"end\"."
 
-command_line_up
+counted_command_line
 read_command_lines (char *prompt_arg, int from_tty, int parse_commands,
 		    void (*validator)(char *, void *), void *closure)
 {
@@ -1205,7 +1144,7 @@ read_command_lines (char *prompt_arg, int from_tty, int parse_commands,
 
   /* Reading commands assumes the CLI behavior, so temporarily
      override the current interpreter with CLI.  */
-  command_line_up head;
+  counted_command_line head (nullptr, command_lines_deleter ());
   if (current_interp_named_p (INTERP_CONSOLE))
     head = read_command_lines_1 (read_next_line, parse_commands,
 				 validator, closure);
@@ -1228,12 +1167,12 @@ read_command_lines (char *prompt_arg, int from_tty, int parse_commands,
 /* Act the same way as read_command_lines, except that each new line is
    obtained using READ_NEXT_LINE_FUNC.  */
 
-command_line_up
+counted_command_line
 read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
 		      void (*validator)(char *, void *), void *closure)
 {
   struct command_line *tail, *next;
-  command_line_up head;
+  counted_command_line head (nullptr, command_lines_deleter ());
   enum command_control_type ret;
   enum misc_command_type val;
 
@@ -1279,7 +1218,7 @@ read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
 	}
       else
 	{
-	  head.reset (next);
+	  head = counted_command_line (next, command_lines_deleter ());
 	}
       tail = next;
     }
@@ -1299,54 +1238,15 @@ free_command_lines (struct command_line **lptr)
 {
   struct command_line *l = *lptr;
   struct command_line *next;
-  struct command_line **blist;
-  int i;
 
   while (l)
     {
-      if (l->body_count > 0)
-	{
-	  blist = l->body_list;
-	  for (i = 0; i < l->body_count; i++, blist++)
-	    free_command_lines (blist);
-	}
       next = l->next;
-      xfree (l->line);
-      xfree (l);
+      delete l;
       l = next;
     }
   *lptr = NULL;
 }
-
-command_line_up
-copy_command_lines (struct command_line *cmds)
-{
-  struct command_line *result = NULL;
-
-  if (cmds)
-    {
-      result = XNEW (struct command_line);
-
-      result->next = copy_command_lines (cmds->next).release ();
-      result->line = xstrdup (cmds->line);
-      result->control_type = cmds->control_type;
-      result->body_count = cmds->body_count;
-      if (cmds->body_count > 0)
-        {
-          int i;
-
-          result->body_list = XNEWVEC (struct command_line *, cmds->body_count);
-
-          for (i = 0; i < cmds->body_count; i++)
-            result->body_list[i]
-	      = copy_command_lines (cmds->body_list[i]).release ();
-        }
-      else
-        result->body_list = NULL;
-    }
-
-  return command_line_up (result);
-}
 \f
 /* Validate that *COMNAME is a valid name for a command.  Return the
    containing command list, in case it starts with a prefix command.
@@ -1483,15 +1383,12 @@ define_command (const char *comname, int from_tty)
 
   xsnprintf (tmpbuf, sizeof (tmpbuf),
 	     "Type commands for definition of \"%s\".", comfull);
-  command_line_up cmds = read_command_lines (tmpbuf, from_tty, 1, 0, 0);
-
-  if (c && c->theclass == class_user)
-    free_command_lines (&c->user_commands);
+  counted_command_line cmds = read_command_lines (tmpbuf, from_tty, 1, 0, 0);
 
   newc = add_cmd (comname, class_user, user_defined_command,
 		  (c && c->theclass == class_user)
 		  ? c->doc : xstrdup ("User-defined."), list);
-  newc->user_commands = cmds.release ();
+  newc->user_commands = std::move (cmds);
 
   /* If this new command is a hook, then mark both commands as being
      tied.  */
@@ -1534,7 +1431,8 @@ document_command (const char *comname, int from_tty)
 
   xsnprintf (tmpbuf, sizeof (tmpbuf), "Type documentation for \"%s\".",
 	     comfull);
-  command_line_up doclines = read_command_lines (tmpbuf, from_tty, 0, 0, 0);
+  counted_command_line doclines = read_command_lines (tmpbuf, from_tty,
+						      0, 0, 0);
 
   if (c->doc)
     xfree ((char *) c->doc);
@@ -1611,7 +1509,7 @@ show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name,
       return;
     }
 
-  cmdlines = c->user_commands;
+  cmdlines = c->user_commands.get ();
   fprintf_filtered (stream, "User command \"%s%s\":\n", prefix, name);
 
   if (!cmdlines)
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 518c8a80b8..58dede2342 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -45,12 +45,38 @@ enum command_control_type
   invalid_control
 };
 
+struct command_line;
+
+extern void free_command_lines (struct command_line **);
+
+/* A deleter for command_line that calls free_command_lines.  */
+
+struct command_lines_deleter
+{
+  void operator() (command_line *cmd_lines) const
+  {
+    free_command_lines (&cmd_lines);
+  }
+};
+
+/* A reference-counted struct command_line.  */
+typedef std::shared_ptr<command_line> counted_command_line;
+
 /* * Structure for saved commands lines (for breakpoints, defined
    commands, etc).  */
 
 struct command_line
 {
-  struct command_line *next;
+  explicit command_line (command_control_type type_, char *line_ = nullptr)
+    : line (line_),
+      control_type (type_)
+  {
+    memset (&control_u, 0, sizeof (control_u));
+  }
+
+  DISABLE_COPY_AND_ASSIGN (command_line);
+
+  struct command_line *next = nullptr;
   char *line;
   enum command_control_type control_type;
   union
@@ -63,36 +89,28 @@ struct command_line
       compile;
     }
   control_u;
-  /* * The number of elements in body_list.  */
-  int body_count;
   /* * For composite commands, the nested lists of commands.  For
      example, for "if" command this will contain the then branch and
      the else branch, if that is available.  */
-  struct command_line **body_list;
-};
+  counted_command_line body_list_0;
+  counted_command_line body_list_1;
 
-extern void free_command_lines (struct command_line **);
+private:
 
-/* A deleter for command_line that calls free_command_lines.  */
+  friend void free_command_lines (struct command_line **);
 
-struct command_lines_deleter
-{
-  void operator() (command_line *cmd_lines) const
+  ~command_line ()
   {
-    free_command_lines (&cmd_lines);
+    xfree (line);
   }
 };
 
-/* A unique pointer to a command_line.  */
-
-typedef std::unique_ptr<command_line, command_lines_deleter> command_line_up;
-
-extern command_line_up read_command_lines (char *, int, int,
-					   void (*)(char *, void *),
-					   void *);
-extern command_line_up read_command_lines_1 (char * (*) (void), int,
-					     void (*)(char *, void *),
-					     void *);
+extern counted_command_line read_command_lines (char *, int, int,
+						void (*)(char *, void *),
+						void *);
+extern counted_command_line read_command_lines_1 (char * (*) (void), int,
+						  void (*)(char *, void *),
+						  void *);
 
 
 /* Exported to cli/cli-cmds.c */
@@ -112,14 +130,12 @@ extern enum command_control_type
 extern enum command_control_type
 	execute_control_command_untraced (struct command_line *cmd);
 
-extern command_line_up get_command_line (enum command_control_type,
-					 const char *);
+extern counted_command_line get_command_line (enum command_control_type,
+					      const char *);
 
 extern void print_command_lines (struct ui_out *,
 				 struct command_line *, unsigned int);
 
-extern command_line_up copy_command_lines (struct command_line *cmds);
-
 /* Exported to gdb/infrun.c */
 
 extern void execute_user_command (struct cmd_list_element *c, const char *args);
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 7f35272872..01c0bc4b39 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -149,7 +149,7 @@ compile_code_command (const char *arg, int from_tty)
     eval_compile_command (NULL, arg, scope, NULL);
   else
     {
-      command_line_up l = get_command_line (compile_control, "");
+      counted_command_line l = get_command_line (compile_control, "");
 
       l->control_u.compile.scope = scope;
       execute_control_command_untraced (l.get ());
@@ -187,7 +187,7 @@ compile_print_command (const char *arg, int from_tty)
     eval_compile_command (NULL, arg, scope, &fmt);
   else
     {
-      command_line_up l = get_command_line (compile_control, "");
+      counted_command_line l = get_command_line (compile_control, "");
 
       l->control_u.compile.scope = scope;
       l->control_u.compile.scope_data = &fmt;
@@ -512,7 +512,7 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
     {
       struct command_line *iter;
 
-      for (iter = cmd->body_list[0]; iter; iter = iter->next)
+      for (iter = cmd->body_list_0.get (); iter; iter = iter->next)
 	{
 	  input_buf.puts (iter->line);
 	  input_buf.puts ("\n");
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 8fc12dd876..0bbbf6eac1 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -209,7 +209,7 @@ guile_command (const char *arg, int from_tty)
     }
   else
     {
-      command_line_up l = get_command_line (guile_control, "");
+      counted_command_line l = get_command_line (guile_control, "");
 
       execute_control_command_untraced (l.get ());
     }
@@ -256,12 +256,12 @@ gdbscm_eval_from_control_command
   char *script, *msg;
   struct cleanup *cleanup;
 
-  if (cmd->body_count != 1)
+  if (cmd->body_list_1 != nullptr)
     error (_("Invalid \"guile\" block structure."));
 
   cleanup = make_cleanup (null_cleanup, NULL);
 
-  script = compute_scheme_string (cmd->body_list[0]);
+  script = compute_scheme_string (cmd->body_list_0.get ());
   msg = gdbscm_safe_eval_string (script, 0);
   xfree (script);
   if (msg != NULL)
@@ -408,7 +408,7 @@ guile_command (const char *arg, int from_tty)
     {
       /* Even if Guile isn't enabled, we still have to slurp the
 	 command list to the corresponding "end".  */
-      command_line_up l = get_command_line (guile_control, "");
+      counted_command_line l = get_command_line (guile_control, "");
 
       execute_control_command_untraced (l.get ());
     }
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index c1f5e2d924..1772fad43c 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -489,7 +489,7 @@ mi_read_next_line (void)
 void
 mi_cmd_break_commands (const char *command, char **argv, int argc)
 {
-  command_line_up break_command;
+  counted_command_line break_command;
   char *endptr;
   int bnum;
   struct breakpoint *b;
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 9eae8a1aef..45b6516fc2 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -385,12 +385,12 @@ gdbpy_eval_from_control_command (const struct extension_language_defn *extlang,
 {
   int ret;
 
-  if (cmd->body_count != 1)
+  if (cmd->body_list_1 != nullptr)
     error (_("Invalid \"python\" block structure."));
 
   gdbpy_enter enter_py (get_current_arch (), current_language);
 
-  std::string script = compute_python_string (cmd->body_list[0]);
+  std::string script = compute_python_string (cmd->body_list_0.get ());
   ret = PyRun_SimpleString (script.c_str ());
   if (ret)
     error (_("Error while executing Python code."));
@@ -413,7 +413,7 @@ python_command (const char *arg, int from_tty)
     }
   else
     {
-      command_line_up l = get_command_line (python_control, "");
+      counted_command_line l = get_command_line (python_control, "");
 
       execute_control_command_untraced (l.get ());
     }
@@ -1582,7 +1582,7 @@ python_interactive_command (const char *arg, int from_tty)
     error (_("Python scripting is not supported in this copy of GDB."));
   else
     {
-      command_line_up l = get_command_line (python_control, "");
+      counted_command_line l = get_command_line (python_control, "");
 
       execute_control_command_untraced (l.get ());
     }
diff --git a/gdb/remote.c b/gdb/remote.c
index f54a38833b..47c546c3d1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12238,7 +12238,7 @@ remote_download_command_source (int num, ULONGEST addr,
       if (cmd->control_type == while_control
 	  || cmd->control_type == while_stepping_control)
 	{
-	  remote_download_command_source (num, addr, *cmd->body_list);
+	  remote_download_command_source (num, addr, cmd->body_list_0.get ());
 
 	  QUIT;	/* Allow user to bail out with ^C.  */
 	  strcpy (rs->buf, "QTDPsrc:");
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 7e173ce75d..0c3842199f 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -167,8 +167,7 @@ char *trace_stop_notes = NULL;
 struct collection_list;
 static char *mem2hex (gdb_byte *, char *, int);
 
-static struct command_line *
-  all_tracepoint_actions_and_cleanup (struct breakpoint *t);
+static counted_command_line all_tracepoint_actions (struct breakpoint *);
 
 static struct trace_status trace_status;
 
@@ -579,8 +578,9 @@ actions_command (const char *args, int from_tty)
 	string_printf ("Enter actions for tracepoint %d, one per line.",
 		       t->number);
 
-      command_line_up l = read_command_lines (&tmpbuf[0], from_tty, 1,
-					      check_tracepoint_command, t);
+      counted_command_line l = read_command_lines (&tmpbuf[0], from_tty, 1,
+						   check_tracepoint_command,
+						   t);
       breakpoint_set_commands (t, std::move (l));
     }
   /* else just return */
@@ -1437,7 +1437,7 @@ encode_actions_1 (struct command_line *action,
 	     here.  */
 	  gdb_assert (stepping_list);
 
-	  encode_actions_1 (action->body_list[0], tloc, frame_reg,
+	  encode_actions_1 (action->body_list_0.get (), tloc, frame_reg,
 			    frame_offset, stepping_list, NULL);
 	}
       else
@@ -1453,17 +1453,17 @@ encode_actions (struct bp_location *tloc,
 		struct collection_list *tracepoint_list,
 		struct collection_list *stepping_list)
 {
-  struct command_line *actions;
   int frame_reg;
   LONGEST frame_offset;
 
   gdbarch_virtual_frame_pointer (tloc->gdbarch,
 				 tloc->address, &frame_reg, &frame_offset);
 
-  actions = all_tracepoint_actions_and_cleanup (tloc->owner);
-
-  encode_actions_1 (actions, tloc, frame_reg, frame_offset,
+  counted_command_line actions = all_tracepoint_actions (tloc->owner);
+  encode_actions_1 (actions.get (), tloc, frame_reg, frame_offset,
 		    tracepoint_list, stepping_list);
+  encode_actions_1 (breakpoint_commands (tloc->owner), tloc,
+		    frame_reg, frame_offset, tracepoint_list, stepping_list);
 
   tracepoint_list->finish ();
   stepping_list->finish ();
@@ -2663,11 +2663,10 @@ trace_dump_actions (struct command_line *action,
 
       if (cmd_cfunc_eq (cmd, while_stepping_pseudocommand))
 	{
-	  int i;
-
-	  for (i = 0; i < action->body_count; ++i)
-	    trace_dump_actions (action->body_list[i],
-				1, stepping_frame, from_tty);
+	  trace_dump_actions (action->body_list_0.get (),
+			      1, stepping_frame, from_tty);
+	  trace_dump_actions (action->body_list_1.get (),
+			      1, stepping_frame, from_tty);
 	}
       else if (cmd_cfunc_eq (cmd, collect_pseudocommand))
 	{
@@ -2778,16 +2777,12 @@ get_traceframe_location (int *stepping_frame_p)
   return t->loc;
 }
 
-/* Return all the actions, including default collect, of a tracepoint
-   T.  It constructs cleanups into the chain, and leaves the caller to
-   handle them (call do_cleanups).  */
+/* Return the default collect actions of a tracepoint T.  */
 
-static struct command_line *
-all_tracepoint_actions_and_cleanup (struct breakpoint *t)
+static counted_command_line
+all_tracepoint_actions (struct breakpoint *t)
 {
-  struct command_line *actions;
-
-  actions = breakpoint_commands (t);
+  counted_command_line actions (nullptr, command_lines_deleter ());
 
   /* If there are default expressions to collect, make up a collect
      action and prepend to the action list to encode.  Note that since
@@ -2797,17 +2792,13 @@ all_tracepoint_actions_and_cleanup (struct breakpoint *t)
   if (*default_collect)
     {
       struct command_line *default_collect_action;
-      char *default_collect_line;
-
-      default_collect_line = xstrprintf ("collect %s", default_collect);
-      make_cleanup (xfree, default_collect_line);
+      gdb::unique_xmalloc_ptr<char> default_collect_line
+	(xstrprintf ("collect %s", default_collect));
 
-      validate_actionline (default_collect_line, t);
-      default_collect_action = XNEW (struct command_line);
-      make_cleanup (xfree, default_collect_action);
-      default_collect_action->next = actions;
-      default_collect_action->line = default_collect_line;
-      actions = default_collect_action;
+      validate_actionline (default_collect_line.get (), t);
+      actions.reset (new struct command_line (simple_control,
+					      default_collect_line.release ()),
+		     command_lines_deleter ());
     }
 
   return actions;
@@ -2820,7 +2811,6 @@ tdump_command (const char *args, int from_tty)
 {
   int stepping_frame = 0;
   struct bp_location *loc;
-  struct command_line *actions;
 
   /* This throws an error is not inspecting a trace frame.  */
   loc = get_traceframe_location (&stepping_frame);
@@ -2834,9 +2824,11 @@ tdump_command (const char *args, int from_tty)
 
   select_frame (get_current_frame ());
 
-  actions = all_tracepoint_actions_and_cleanup (loc->owner);
+  counted_command_line actions = all_tracepoint_actions (loc->owner);
 
-  trace_dump_actions (actions, 0, stepping_frame, from_tty);
+  trace_dump_actions (actions.get (), 0, stepping_frame, from_tty);
+  trace_dump_actions (breakpoint_commands (loc->owner), 0, stepping_frame,
+		      from_tty);
 }
 
 /* Encode a piece of a tracepoint's source-level definition in a form
-- 
2.13.6

Re: [RFA 2/8] Use counted_command_line everywhere

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:
> Currently command lines are reference counted using shared_ptr only
> when attached to breakpoints.  This patch changes gdb to use
> shared_ptr in commands as well.  This allows for the removal of
> copy_command_lines.
> 
> Note that the change to execute_user_command explicitly makes a new
> reference to the command line.  This will be used in a later patch.
> 
> This simplifies struct command_line based on the observation that a
> given command can have at most two child bodies: an "if" can have both
> "then" and "else" parts.  Perhaps the names I've chosen for the
> replacements here are not very good -- your input requested.
> 

Names look fine to me.

> @@ -2663,11 +2663,10 @@ trace_dump_actions (struct command_line *action,
>  
>        if (cmd_cfunc_eq (cmd, while_stepping_pseudocommand))
>  	{
> -	  int i;
> -
> -	  for (i = 0; i < action->body_count; ++i)
> -	    trace_dump_actions (action->body_list[i],
> -				1, stepping_frame, from_tty);
> +	  trace_dump_actions (action->body_list_0.get (),
> +			      1, stepping_frame, from_tty);
> +	  trace_dump_actions (action->body_list_1.get (),
> +			      1, stepping_frame, from_tty);

Hmm, this looked suspicious.  I'm not seeing why would a
while-stepping action have two body lists.  I guess this
happens to work because trace_dump_actions does nothing
if ACTION is NULL.

> @@ -2797,17 +2792,13 @@ all_tracepoint_actions_and_cleanup (struct breakpoint *t)
>    if (*default_collect)
>      {
>        struct command_line *default_collect_action;
> -      char *default_collect_line;
> -
> -      default_collect_line = xstrprintf ("collect %s", default_collect);
> -      make_cleanup (xfree, default_collect_line);
> +      gdb::unique_xmalloc_ptr<char> default_collect_line
> +	(xstrprintf ("collect %s", default_collect));

string_printf ?

>  
> -      validate_actionline (default_collect_line, t);
> -      default_collect_action = XNEW (struct command_line);
> -      make_cleanup (xfree, default_collect_action);
> -      default_collect_action->next = actions;
> -      default_collect_action->line = default_collect_line;
> -      actions = default_collect_action;
> +      validate_actionline (default_collect_line.get (), t);
> +      actions.reset (new struct command_line (simple_control,
> +					      default_collect_line.release ()),
> +		     command_lines_deleter ());
>      }

LGTM.

It's a shame that we can't use std::make_shared to avoid
the separate control block allocation.

Thanks,
Pedro Alves

Re: [RFA 2/8] Use counted_command_line everywhere

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> -	  for (i = 0; i < action->body_count; ++i)
>> -	    trace_dump_actions (action->body_list[i],
>> -				1, stepping_frame, from_tty);
>> +	  trace_dump_actions (action->body_list_0.get (),
>> +			      1, stepping_frame, from_tty);
>> +	  trace_dump_actions (action->body_list_1.get (),
>> +			      1, stepping_frame, from_tty);

Pedro> Hmm, this looked suspicious.  I'm not seeing why would a
Pedro> while-stepping action have two body lists.  I guess this
Pedro> happens to work because trace_dump_actions does nothing
Pedro> if ACTION is NULL.

Yeah.  If you prefer I can add an assert that body_list_1==nullptr,
which seems like it ought to always be true.

>> +      gdb::unique_xmalloc_ptr<char> default_collect_line
>> +	(xstrprintf ("collect %s", default_collect));

Pedro> string_printf ?

I'll do that.

Pedro> It's a shame that we can't use std::make_shared to avoid
Pedro> the separate control block allocation.

It could be done if we changed the representation a bit, but it looked
like a pain to me, so I didn't.

Tom

Re: [RFA 2/8] Use counted_command_line everywhere

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +      gdb::unique_xmalloc_ptr<char> default_collect_line
>> +	(xstrprintf ("collect %s", default_collect));

Pedro> string_printf ?

Actually, a few lines later:

>> +      actions.reset (new struct command_line (simple_control,
>> +					      default_collect_line.release ()),

So this couldn't use string_printf without additional work.

Tom

[RFA 3/8] Make print_command_trace varargs

[Tom Tromey]

I noticed some code in execute_control_command_1 that could be
simplified by making print_command_trace a printf-like function.  This
patch makes this change.

2018-04-19  Tom Tromey  <tom@tromey.com>

	* top.c (execute_command): Update.
	* cli/cli-script.h (print_command_lines): Now varargs.
	* cli/cli-script.c (print_command_lines): Now varargs.
	(execute_control_command_1) <case while_control, case if_control>:
	Update.
---
 gdb/ChangeLog        |  8 ++++++++
 gdb/cli/cli-script.c | 22 ++++++++++------------
 gdb/cli/cli-script.h |  3 ++-
 gdb/top.c            |  2 +-
 4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index b066da7d60..c7d405c0d0 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -426,8 +426,9 @@ reset_command_nest_depth (void)
    via while_command or if_command.  Inner levels of 'if' and 'while'
    are dealt with directly.  Therefore we can use these functions
    to determine whether the command has been printed already or not.  */
+ATTRIBUTE_PRINTF (1, 2)
 void
-print_command_trace (const char *cmd)
+print_command_trace (const char *fmt, ...)
 {
   int i;
 
@@ -443,7 +444,12 @@ print_command_trace (const char *cmd)
   for (i=0; i < command_nest_depth; i++)
     printf_filtered ("+");
 
-  printf_filtered ("%s\n", cmd);
+  va_list args;
+
+  va_start (args, fmt);
+  vprintf_filtered (fmt, args);
+  va_end (args);
+  puts_filtered ("\n");
 }
 
 /* Helper for execute_control_command.  */
@@ -490,11 +496,7 @@ execute_control_command_1 (struct command_line *cmd)
 
     case while_control:
       {
-	int len = strlen (cmd->line) + 7;
-	char *buffer = (char *) alloca (len);
-
-	xsnprintf (buffer, len, "while %s", cmd->line);
-	print_command_trace (buffer);
+	print_command_trace ("while %s", cmd->line);
 
 	/* Parse the loop control expression for the while statement.  */
 	std::string new_line = insert_user_defined_cmd_args (cmd->line);
@@ -555,11 +557,7 @@ execute_control_command_1 (struct command_line *cmd)
 
     case if_control:
       {
-	int len = strlen (cmd->line) + 4;
-	char *buffer = (char *) alloca (len);
-
-	xsnprintf (buffer, len, "if %s", cmd->line);
-	print_command_trace (buffer);
+	print_command_trace ("if %s", cmd->line);
 
 	/* Parse the conditional for the if statement.  */
 	std::string new_line = insert_user_defined_cmd_args (cmd->line);
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 58dede2342..10b6c17789 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -148,7 +148,8 @@ extern std::string insert_user_defined_cmd_args (const char *line);
 
 /* Exported to top.c */
 
-extern void print_command_trace (const char *cmd);
+extern void print_command_trace (const char *cmd, ...)
+  ATTRIBUTE_PRINTF (1, 2);
 
 /* Exported to event-top.c */
 
diff --git a/gdb/top.c b/gdb/top.c
index 8903a92983..6f5abd5a3d 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -571,7 +571,7 @@ execute_command (const char *p, int from_tty)
       line = p;
 
       /* If trace-commands is set then this will print this command.  */
-      print_command_trace (p);
+      print_command_trace ("%s", p);
 
       c = lookup_cmd (&cmd, cmdlist, "", 0, 1);
       p = cmd;
-- 
2.13.6

Re: [RFA 3/8] Make print_command_trace varargs

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:
> I noticed some code in execute_control_command_1 that could be
> simplified by making print_command_trace a printf-like function.  This
> patch makes this change.
> 
> 2018-04-19  Tom Tromey  <tom@tromey.com>
> 
> 	* top.c (execute_command): Update.
> 	* cli/cli-script.h (print_command_lines): Now varargs.
> 	* cli/cli-script.c (print_command_lines): Now varargs.
> 	(execute_control_command_1) <case while_control, case if_control>:
> 	Update.

Looks fine to me.

Thanks,
Pedro Alves

[RFA 1/8] Allocate cmd_list_element with new

[Tom Tromey]

This adds a constructor and destructor to cmd_list_element and changes
it to be allocated with new.  This will be useful in a subsequent
patch.

ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	* cli/cli-decode.h (cmd_list_element): New constructor.
	(~cmd_list_element): New destructor.
	(struct cmd_list_element): Add initializers.
	* cli/cli-decode.c (do_add_cmd): Use "new".
	(delete_cmd): Use "delete".
---
 gdb/ChangeLog        |  8 ++++++
 gdb/cli/cli-decode.c | 35 +++----------------------
 gdb/cli/cli-decode.h | 74 ++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 62 insertions(+), 55 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 0afe36aa4e..c8dda700ca 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -193,7 +193,8 @@ static struct cmd_list_element *
 do_add_cmd (const char *name, enum command_class theclass,
 	    const char *doc, struct cmd_list_element **list)
 {
-  struct cmd_list_element *c = XNEW (struct cmd_list_element);
+  struct cmd_list_element *c = new struct cmd_list_element (name, theclass,
+							    doc);
   struct cmd_list_element *p, *iter;
 
   /* Turn each alias of the old command into an alias of the new
@@ -227,34 +228,6 @@ do_add_cmd (const char *name, enum command_class theclass,
       p->next = c;
     }
 
-  c->name = name;
-  c->theclass = theclass;
-  set_cmd_context (c, NULL);
-  c->doc = doc;
-  c->cmd_deprecated = 0;
-  c->deprecated_warn_user = 0;
-  c->malloced_replacement = 0;
-  c->doc_allocated = 0;
-  c->replacement = NULL;
-  c->pre_show_hook = NULL;
-  c->hook_in = 0;
-  c->prefixlist = NULL;
-  c->prefixname = NULL;
-  c->allow_unknown = 0;
-  c->prefix = NULL;
-  c->abbrev_flag = 0;
-  set_cmd_completer (c, symbol_completer);
-  c->completer_handle_brkchars = NULL;
-  c->destroyer = NULL;
-  c->type = not_set_cmd;
-  c->var = NULL;
-  c->var_type = var_boolean;
-  c->enums = NULL;
-  c->user_commands = NULL;
-  c->cmd_pointer = NULL;
-  c->alias_chain = NULL;
-  c->suppress_notification = NULL;
-
   return c;
 }
 
@@ -841,8 +814,6 @@ delete_cmd (const char *name, struct cmd_list_element **list,
 	  *prehookee = iter->hookee_pre;
 	  if (iter->hookee_post)
 	    iter->hookee_post->hook_post = 0;
-	  if (iter->doc && iter->doc_allocated)
-	    xfree ((char *) iter->doc);
 	  *posthook = iter->hook_post;
 	  *posthookee = iter->hookee_post;
 
@@ -866,7 +837,7 @@ delete_cmd (const char *name, struct cmd_list_element **list,
 	      *prevp = iter->alias_chain;
 	    }
 
-	  xfree (iter);
+	  delete iter;
 
 	  /* We won't see another command with the same name.  */
 	  break;
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index 30b4fa7ca2..a3208e1fc6 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -45,8 +45,35 @@ cmd_types;
 
 struct cmd_list_element
   {
+    cmd_list_element (const char *name_, enum command_class theclass_,
+		      const char *doc_)
+      : name (name_),
+	theclass (theclass_),
+	cmd_deprecated (0),
+	deprecated_warn_user (0),
+	malloced_replacement (0),
+	doc_allocated (0),
+	hook_in (0),
+	allow_unknown (0),
+	abbrev_flag (0),
+	type (not_set_cmd),
+	var_type (var_boolean),
+	doc (doc_)
+    {
+      memset (&function, 0, sizeof (function));
+    }
+
+    ~cmd_list_element ()
+    {
+      if (doc && doc_allocated)
+	xfree ((char *) doc);
+    }
+
+    DISABLE_COPY_AND_ASSIGN (cmd_list_element);
+
+
     /* Points to next command in this list.  */
-    struct cmd_list_element *next;
+    struct cmd_list_element *next = nullptr;
 
     /* Name of this command.  */
     const char *name;
@@ -107,7 +134,8 @@ struct cmd_list_element
        cagney/2002-02-02: This function signature is evolving.  For
        the moment suggest sticking with either set_cmd_cfunc() or
        set_cmd_sfunc().  */
-    void (*func) (struct cmd_list_element *c, const char *args, int from_tty);
+    void (*func) (struct cmd_list_element *c, const char *args, int from_tty)
+      = nullptr;
     /* The command's real callback.  At present func() bounces through
        to one of the below.  */
     union
@@ -121,7 +149,7 @@ struct cmd_list_element
     function;
 
     /* Local state (context) for this command.  This can be anything.  */
-    void *context;
+    void *context = nullptr;
 
     /* Documentation of this command (or help topic).
        First line is brief documentation; remaining lines form, with it,
@@ -131,37 +159,37 @@ struct cmd_list_element
 
     /* For set/show commands.  A method for printing the output to the
        specified stream.  */
-    show_value_ftype *show_value_func;
+    show_value_ftype *show_value_func = nullptr;
 
     /* If this command is deprecated, this is the replacement name.  */
-    const char *replacement;
+    const char *replacement = nullptr;
 
     /* If this command represents a show command, then this function
        is called before the variable's value is examined.  */
-    void (*pre_show_hook) (struct cmd_list_element *c);
+    void (*pre_show_hook) (struct cmd_list_element *c) = nullptr;
 
     /* Hook for another command to be executed before this command.  */
-    struct cmd_list_element *hook_pre;
+    struct cmd_list_element *hook_pre = nullptr;
 
     /* Hook for another command to be executed after this command.  */
-    struct cmd_list_element *hook_post;
+    struct cmd_list_element *hook_post = nullptr;
 
     /* Nonzero identifies a prefix command.  For them, the address
        of the variable containing the list of subcommands.  */
-    struct cmd_list_element **prefixlist;
+    struct cmd_list_element **prefixlist = nullptr;
 
     /* For prefix commands only:
        String containing prefix commands to get here: this one
        plus any others needed to get to it.  Should end in a space.
        It is used before the word "command" in describing the
        commands reached through this prefix.  */
-    const char *prefixname;
+    const char *prefixname = nullptr;
 
     /* The prefix command of this command.  */
-    struct cmd_list_element *prefix;
+    struct cmd_list_element *prefix = nullptr;
 
     /* Completion routine for this command.  */
-    completer_ftype *completer;
+    completer_ftype *completer = symbol_completer;
 
     /* Handle the word break characters for this completer.  Usually
        this function need not be defined, but for some types of
@@ -169,47 +197,47 @@ struct cmd_list_element
        a class) the word break chars may need to be redefined
        depending on the completer type (e.g., for filename
        completers).  */
-    completer_handle_brkchars_ftype *completer_handle_brkchars;
+    completer_handle_brkchars_ftype *completer_handle_brkchars = nullptr;
 
     /* Destruction routine for this command.  If non-NULL, this is
        called when this command instance is destroyed.  This may be
        used to finalize the CONTEXT field, if needed.  */
-    void (*destroyer) (struct cmd_list_element *self, void *context);
+    void (*destroyer) (struct cmd_list_element *self, void *context) = nullptr;
 
     /* Pointer to variable affected by "set" and "show".  Doesn't
        matter if type is not_set.  */
-    void *var;
+    void *var = nullptr;
 
     /* Pointer to NULL terminated list of enumerated values (like
        argv).  */
-    const char *const *enums;
+    const char *const *enums = nullptr;
 
     /* Pointer to command strings of user-defined commands */
-    struct command_line *user_commands;
+    struct command_line *user_commands = nullptr;
 
     /* Pointer to command that is hooked by this one, (by hook_pre)
        so the hook can be removed when this one is deleted.  */
-    struct cmd_list_element *hookee_pre;
+    struct cmd_list_element *hookee_pre = nullptr;
 
     /* Pointer to command that is hooked by this one, (by hook_post)
        so the hook can be removed when this one is deleted.  */
-    struct cmd_list_element *hookee_post;
+    struct cmd_list_element *hookee_post = nullptr;
 
     /* Pointer to command that is aliased by this one, so the
        aliased command can be located in case it has been hooked.  */
-    struct cmd_list_element *cmd_pointer;
+    struct cmd_list_element *cmd_pointer = nullptr;
 
     /* Start of a linked list of all aliases of this command.  */
-    struct cmd_list_element *aliases;
+    struct cmd_list_element *aliases = nullptr;
 
     /* Link pointer for aliases on an alias list.  */
-    struct cmd_list_element *alias_chain;
+    struct cmd_list_element *alias_chain = nullptr;
 
     /* If non-null, the pointer to a field in 'struct
        cli_suppress_notification', which will be set to true in cmd_func
        when this command is being executed.  It will be set back to false
        when the command has been executed.  */
-    int *suppress_notification;
+    int *suppress_notification = nullptr;
   };
 
 extern void help_cmd_list (struct cmd_list_element *, enum command_class,
-- 
2.13.6

[RFA 5/8] Allow defining a user command inside a user command

[Tom Tromey]

PR gdb/11750 concerns defining a command inside a user commnad, like:

    define outer
      define inner
	echo hi\n
      end
    end

This patch adds this capability to gdb.

2018-04-19  Tom Tromey  <tom@tromey.com>

	PR gdb/11750:
	* cli/cli-script.h (enum command_control_type) <define_control>:
	New constant.
	* cli/cli-script.c (multi_line_command_p): Handle define_control.
	(build_command_line, execute_control_command_1)
	(process_next_line): Likewise.
	(do_define_command): New function, extracted from define_command.
	(define_command): Use it.

testsuite/ChangeLog
2018-04-19  Tom Tromey  <tom@tromey.com>

	PR gdb/11750:
	* gdb.base/define.exp: Test defining a user command inside a user
	command.
	* gdb.base/commands.exp (define_if_without_arg_test): Test "define".
---
 gdb/ChangeLog                       | 11 +++++++++
 gdb/cli/cli-script.c                | 49 ++++++++++++++++++++++++++++++-------
 gdb/cli/cli-script.h                |  1 +
 gdb/testsuite/ChangeLog             |  7 ++++++
 gdb/testsuite/gdb.base/commands.exp |  4 +--
 gdb/testsuite/gdb.base/define.exp   |  8 ++++++
 6 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 36740b97ad..624a3bda68 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -44,6 +44,9 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 				void (*validator)(char *, void *),
 				void *closure);
 
+static void do_define_command (const char *comname, int from_tty,
+			       const counted_command_line *commands);
+
 static char *read_next_line (void);
 
 /* Level of control structure when reading.  */
@@ -122,6 +125,7 @@ multi_line_command_p (enum command_control_type type)
     case compile_control:
     case python_control:
     case guile_control:
+    case define_control:
       return 1;
     default:
       return 0;
@@ -135,8 +139,9 @@ static struct command_line *
 build_command_line (enum command_control_type type, const char *args)
 {
   if ((args == NULL || *args == '\0')
-      && (type == if_control || type == while_control))
-    error (_("if/while commands require arguments."));
+      && (type == if_control || type == while_control
+	  || type == define_control))
+    error (_("if/while/define commands require arguments."));
   gdb_assert (args != NULL);
 
   return new struct command_line (type, xstrdup (args));
@@ -611,6 +616,12 @@ execute_control_command_1 (struct command_line *cmd)
       ret = simple_control;
       break;
 
+    case define_control:
+      print_command_trace ("define %s", cmd->line);
+      do_define_command (cmd->line, 0, &cmd->body_list_0);
+      ret = simple_control;
+      break;
+
     case python_control:
     case guile_control:
       {
@@ -960,6 +971,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
 	{
 	  *command = build_command_line (commands_control, line_first_arg (p));
 	}
+      else if (command_name_equals (cmd, "define"))
+	*command = build_command_line (define_control, line_first_arg (p));
       else if (command_name_equals (cmd, "python") && !inline_cmd)
 	{
 	  /* Note that we ignore the inline "python command" form
@@ -1303,8 +1316,15 @@ user_defined_command (const char *ignore, int from_tty)
 {
 }
 
+/* Define a user-defined command.  If COMMANDS is NULL, then this is
+   an interactive call and the commands will be read from the user.
+   Otherwise, it is a "define" command in a script and the commands
+   are provided.  In the non-interactive case, various prompts and
+   warnings are disabled.  */
+
 static void
-define_command (const char *comname, int from_tty)
+do_define_command (const char *comname, int from_tty,
+		   const counted_command_line *commands)
 {
   enum cmd_hook_type
     {
@@ -1331,7 +1351,7 @@ define_command (const char *comname, int from_tty)
   if (c && strcmp (comname, c->name) != 0)
     c = 0;
 
-  if (c)
+  if (c && commands == nullptr)
     {
       int q;
 
@@ -1365,7 +1385,7 @@ define_command (const char *comname, int from_tty)
       hookc = lookup_cmd (&tem, *list, "", -1, 0);
       if (hookc && strcmp (comname + hook_name_size, hookc->name) != 0)
 	hookc = 0;
-      if (!hookc)
+      if (!hookc && commands == nullptr)
 	{
 	  warning (_("Your new `%s' command does not "
 		     "hook any existing command."),
@@ -1377,10 +1397,15 @@ define_command (const char *comname, int from_tty)
 
   comname = xstrdup (comname);
 
-  std::string prompt
-    = string_printf ("Type commands for definition of \"%s\".", comfull);
-  counted_command_line cmds = read_command_lines (prompt.c_str (), from_tty,
-						  1, 0, 0);
+  counted_command_line cmds;
+  if (commands == nullptr)
+    {
+      std::string prompt
+	= string_printf ("Type commands for definition of \"%s\".", comfull);
+      cmds = read_command_lines (prompt.c_str (), from_tty, 1, 0, 0);
+    }
+  else
+    cmds = *commands;
 
   newc = add_cmd (comname, class_user, user_defined_command,
 		  (c && c->theclass == class_user)
@@ -1410,6 +1435,12 @@ define_command (const char *comname, int from_tty)
 }
 
 static void
+define_command (const char *comname, int from_tty)
+{
+  do_define_command (comname, from_tty, nullptr);
+}
+
+static void
 document_command (const char *comname, int from_tty)
 {
   struct cmd_list_element *c, **list;
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 7e5f94c0ad..0bd0d597ae 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -42,6 +42,7 @@ enum command_control_type
   compile_control,
   guile_control,
   while_stepping_control,
+  define_control,
   invalid_control
 };
 
diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
index b33e5124ec..f2f2b5d87c 100644
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -1014,7 +1014,7 @@ proc_with_prefix redefine_backtrace_test {} {
 # Test using "if" and "while" without args when building a command list.
 
 proc define_if_without_arg_test {} {
-    foreach cmd {if while} {
+    foreach cmd {if while define} {
 	set test "define some_command_$cmd"
 	gdb_test_multiple $test $test {
 	    -re "End with"  {
@@ -1022,7 +1022,7 @@ proc define_if_without_arg_test {} {
 	    }
 	}
 
-	gdb_test "$cmd" "if/while commands require arguments." "type $cmd without args"
+	gdb_test "$cmd" "if/while/define commands require arguments." "type $cmd without args"
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/define.exp b/gdb/testsuite/gdb.base/define.exp
index f82a9efdff..e4064b7afc 100644
--- a/gdb/testsuite/gdb.base/define.exp
+++ b/gdb/testsuite/gdb.base/define.exp
@@ -298,5 +298,13 @@ gdb_test_multiple "set prompt \\(gdb\\) " "reset gdb_prompt" {
     }
 }
 
+gdb_test_multiple "define do-define" "" {
+    -re "Type commands for definition of \"do-define\".\r\nEnd with a line saying just \"end\".\r\n>$" {
+	gdb_test "define do-printit\necho here\\n\nend\nend" "" "define do-define"
+    }
+}
+gdb_test_no_output "do-define" "invoke do-define"
+gdb_test "do-printit" "here" "invoke do-printit"
+
 gdb_exit
 return 0
-- 
2.13.6

Re: [RFA 5/8] Allow defining a user command inside a user command

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:

>  build_command_line (enum command_control_type type, const char *args)
>  {
>    if ((args == NULL || *args == '\0')
> -      && (type == if_control || type == while_control))
> -    error (_("if/while commands require arguments."));
> +      && (type == if_control || type == while_control
> +	  || type == define_control))
> +    error (_("if/while/define commands require arguments."));

I'd vote for splitting those up:

    if (args == NULL || *args == '\0')
      {
        switch (type)
          {
          case if_control:
            error (_("if commands require arguments."));
          case while_control:
            error (_("while commands require arguments."));
          case define_control:
            error (_("define commands require arguments."));
          }
      }

> +/* Define a user-defined command.  If COMMANDS is NULL, then this is
> +   an interactive call and the commands will be read from the user.

Isn't this conflating top-level "define" command, with interactive
input?  I imagine that a top-level "define" command in e.g., gdbinit will
considered "interactive call" according to the above, while in truth,
it's not really interactive, according to FROM_TTY.

> +   Otherwise, it is a "define" command in a script and the commands
> +   are provided.  

Similarly, can't you write a "define" command inside a "define"
command interactively on the command line?

>In the non-interactive case, various prompts and
> +   warnings are disabled.  */
> +
>  static void
> -define_command (const char *comname, int from_tty)
> +do_define_command (const char *comname, int from_tty,
> +		   const counted_command_line *commands)
>  {

Thanks,
Pedro Alves

Re: [RFA 5/8] Allow defining a user command inside a user command

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'd vote for splitting those up:

I agree, I did this.

Pedro> Isn't this conflating top-level "define" command, with interactive
Pedro> input?  I imagine that a top-level "define" command in e.g., gdbinit will
Pedro> considered "interactive call" according to the above, while in truth,
Pedro> it's not really interactive, according to FROM_TTY.

Yes, I've re-worded this.

Tom

[RFA 6/8] Use function_view in cli-script.c

[Tom Tromey]

This changes some functions in cli-script.c to use function_view
rather than a function pointer and closure argument.  This simplifies
the code a bit and is useful in a subsequent patch.

2018-04-19  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (actions_command): Update.
	* mi/mi-cmd-break.c (mi_command_line_array)
	(mi_command_line_array_cnt, mi_command_line_array_ptr)
	(mi_read_next_line): Remove.
	(mi_cmd_break_commands): Update.
	* cli/cli-script.h (read_command_lines, read_command_lines_1): Use
	function_view.
	* cli/cli-script.c (get_command_line): Update.
	(process_next_line): Use function_view.  Constify.
	(recurse_read_control_structure, read_command_lines)
	(read_command_lines_1): Change argument types to function_view.
	(do_define_command, document_command): Update.
	* breakpoint.h (check_tracepoint_command): Don't declare.
	* breakpoint.c (check_tracepoint_command): Remove.
	(commands_command_1, create_tracepoint_from_upload): Update.
---
 gdb/ChangeLog         | 18 ++++++++++++++++++
 gdb/breakpoint.c      | 22 +++++++++-------------
 gdb/breakpoint.h      |  4 ----
 gdb/cli/cli-script.c  | 51 ++++++++++++++++++++++++++-------------------------
 gdb/cli/cli-script.h  | 11 +++++------
 gdb/mi/mi-cmd-break.c | 40 ++++++++++++++++------------------------
 gdb/tracepoint.c      |  6 ++++--
 7 files changed, 78 insertions(+), 74 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7d284e9a96..679494b7b8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1216,14 +1216,6 @@ breakpoint_set_task (struct breakpoint *b, int task)
     gdb::observers::breakpoint_modified.notify (b);
 }
 
-void
-check_tracepoint_command (char *line, void *closure)
-{
-  struct breakpoint *b = (struct breakpoint *) closure;
-
-  validate_actionline (line, b);
-}
-
 static void
 commands_command_1 (const char *arg, int from_tty,
 		    struct command_line *control)
@@ -1256,10 +1248,14 @@ commands_command_1 (const char *arg, int from_tty,
 				    "%s, one per line."),
 				  arg);
 
-	       cmd = read_command_lines (str.c_str (), from_tty, 1,
-					 (is_tracepoint (b)
-					  ? check_tracepoint_command : 0),
-					 b);
+	       gdb::function_view<void (const char *)> validator;
+	       if (is_tracepoint (b))
+		 validator = [=] (const char *line)
+		   {
+		     validate_actionline (line, b);
+		   };
+
+	       cmd = read_command_lines (str.c_str (), from_tty, 1, validator);
 	     }
 	 }
 
@@ -14794,7 +14790,7 @@ create_tracepoint_from_upload (struct uploaded_tp *utp)
       this_utp = utp;
       next_cmd = 0;
 
-      cmd_list = read_command_lines_1 (read_uploaded_action, 1, NULL, NULL);
+      cmd_list = read_command_lines_1 (read_uploaded_action, 1, NULL);
 
       breakpoint_set_commands (tp, std::move (cmd_list));
     }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f3c2599286..2008683a0a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1600,10 +1600,6 @@ extern int is_tracepoint (const struct breakpoint *b);
    it.  */
 extern VEC(breakpoint_p) *static_tracepoints_here (CORE_ADDR addr);
 
-/* Function that can be passed to read_command_line to validate
-   that each command is suitable for tracepoint command list.  */
-extern void check_tracepoint_command (char *line, void *closure);
-
 /* Create an instance of this to start registering breakpoint numbers
    for a later "commands" command.  */
 
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 624a3bda68..453983b3bd 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -39,10 +39,10 @@
 /* Prototypes for local functions.  */
 
 static enum command_control_type
-recurse_read_control_structure (char * (*read_next_line_func) (void),
-				struct command_line *current_cmd,
-				void (*validator)(char *, void *),
-				void *closure);
+recurse_read_control_structure
+    (gdb::function_view<const char * ()> read_next_line_func,
+     struct command_line *current_cmd,
+     gdb::function_view<void (const char *)> validator);
 
 static void do_define_command (const char *comname, int from_tty,
 			       const counted_command_line *commands);
@@ -158,7 +158,7 @@ get_command_line (enum command_control_type type, const char *arg)
 			    command_lines_deleter ());
 
   /* Read in the body of this command.  */
-  if (recurse_read_control_structure (read_next_line, cmd.get (), 0, 0)
+  if (recurse_read_control_structure (read_next_line, cmd.get (), 0)
       == invalid_control)
     {
       warning (_("Error reading in canned sequence of commands."));
@@ -892,11 +892,13 @@ line_first_arg (const char *p)
    Otherwise, only "end" is recognized.  */
 
 static enum misc_command_type
-process_next_line (char *p, struct command_line **command, int parse_commands,
-		   void (*validator)(char *, void *), void *closure)
+process_next_line (const char *p, struct command_line **command,
+		   int parse_commands,
+		   gdb::function_view<void (const char *)> validator)
+
 {
-  char *p_end;
-  char *p_start;
+  const char *p_end;
+  const char *p_start;
   int not_handled = 0;
 
   /* Not sure what to do here.  */
@@ -1008,10 +1010,9 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
 
   if (validator)
     {
-
       TRY
 	{
-	  validator ((*command)->line, closure);
+	  validator ((*command)->line);
 	}
       CATCH (ex, RETURN_MASK_ALL)
 	{
@@ -1030,10 +1031,9 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
    obtain lines of the command.  */
 
 static enum command_control_type
-recurse_read_control_structure (char * (*read_next_line_func) (void),
+recurse_read_control_structure (gdb::function_view<const char * ()> read_next_line_func,
 				struct command_line *current_cmd,
-				void (*validator)(char *, void *),
-				void *closure)
+				gdb::function_view<void (const char *)> validator)
 {
   enum misc_command_type val;
   enum command_control_type ret;
@@ -1056,7 +1056,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 			       current_cmd->control_type != python_control
 			       && current_cmd->control_type != guile_control
 			       && current_cmd->control_type != compile_control,
-			       validator, closure);
+			       validator);
 
       /* Just skip blanks and comments.  */
       if (val == nop_command)
@@ -1109,7 +1109,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 	{
 	  control_level++;
 	  ret = recurse_read_control_structure (read_next_line_func, next,
-						validator, closure);
+						validator);
 	  control_level--;
 
 	  if (ret != simple_control)
@@ -1135,7 +1135,7 @@ recurse_read_control_structure (char * (*read_next_line_func) (void),
 
 counted_command_line
 read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
-		    void (*validator)(char *, void *), void *closure)
+		    gdb::function_view<void (const char *)> validator)
 {
   if (from_tty && input_interactive_p (current_ui))
     {
@@ -1158,13 +1158,13 @@ read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
   counted_command_line head (nullptr, command_lines_deleter ());
   if (current_interp_named_p (INTERP_CONSOLE))
     head = read_command_lines_1 (read_next_line, parse_commands,
-				 validator, closure);
+				 validator);
   else
     {
       scoped_restore_interp interp_restorer (INTERP_CONSOLE);
 
       head = read_command_lines_1 (read_next_line, parse_commands,
-				   validator, closure);
+				   validator);
     }
 
   if (from_tty && input_interactive_p (current_ui)
@@ -1179,8 +1179,9 @@ read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
    obtained using READ_NEXT_LINE_FUNC.  */
 
 counted_command_line
-read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
-		      void (*validator)(char *, void *), void *closure)
+read_command_lines_1 (gdb::function_view<const char * ()> read_next_line_func,
+		      int parse_commands,
+		      gdb::function_view<void (const char *)> validator)
 {
   struct command_line *tail, *next;
   counted_command_line head (nullptr, command_lines_deleter ());
@@ -1194,7 +1195,7 @@ read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
     {
       dont_repeat ();
       val = process_next_line (read_next_line_func (), &next, parse_commands,
-			       validator, closure);
+			       validator);
 
       /* Ignore blank lines or comments.  */
       if (val == nop_command)
@@ -1216,7 +1217,7 @@ read_command_lines_1 (char * (*read_next_line_func) (void), int parse_commands,
 	{
 	  control_level++;
 	  ret = recurse_read_control_structure (read_next_line_func, next,
-						validator, closure);
+						validator);
 	  control_level--;
 
 	  if (ret == invalid_control)
@@ -1402,7 +1403,7 @@ do_define_command (const char *comname, int from_tty,
     {
       std::string prompt
 	= string_printf ("Type commands for definition of \"%s\".", comfull);
-      cmds = read_command_lines (prompt.c_str (), from_tty, 1, 0, 0);
+      cmds = read_command_lines (prompt.c_str (), from_tty, 1, 0);
     }
   else
     cmds = *commands;
@@ -1459,7 +1460,7 @@ document_command (const char *comname, int from_tty)
   std::string prompt = string_printf ("Type documentation for \"%s\".",
 				      comfull);
   counted_command_line doclines = read_command_lines (prompt.c_str (),
-						      from_tty, 0, 0, 0);
+						      from_tty, 0, 0);
 
   if (c->doc)
     xfree ((char *) c->doc);
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 0bd0d597ae..3bebd0ed9d 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -106,12 +106,11 @@ private:
   }
 };
 
-extern counted_command_line read_command_lines (const char *, int, int,
-						void (*)(char *, void *),
-						void *);
-extern counted_command_line read_command_lines_1 (char * (*) (void), int,
-						  void (*)(char *, void *),
-						  void *);
+extern counted_command_line read_command_lines
+    (const char *, int, int, gdb::function_view<void (const char *)>);
+extern counted_command_line read_command_lines_1
+    (gdb::function_view<const char * ()>, int,
+     gdb::function_view<void (const char *)>);
 
 
 /* Exported to cli/cli-cmds.c */
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 1772fad43c..7437c72b85 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -32,6 +32,7 @@
 #include "linespec.h"
 #include "gdb_obstack.h"
 #include <ctype.h>
+#include "tracepoint.h"
 
 enum
   {
@@ -468,24 +469,6 @@ mi_cmd_break_watch (const char *command, char **argv, int argc)
     }
 }
 
-/* The mi_read_next_line consults these variable to return successive
-   command lines.  While it would be clearer to use a closure pointer,
-   it is not expected that any future code will use read_command_lines_1,
-   therefore no point of overengineering.  */
-
-static char **mi_command_line_array;
-static int mi_command_line_array_cnt;
-static int mi_command_line_array_ptr;
-
-static char *
-mi_read_next_line (void)
-{
-  if (mi_command_line_array_ptr == mi_command_line_array_cnt)
-    return NULL;
-  else
-    return mi_command_line_array[mi_command_line_array_ptr++];
-}
-
 void
 mi_cmd_break_commands (const char *command, char **argv, int argc)
 {
@@ -509,15 +492,24 @@ mi_cmd_break_commands (const char *command, char **argv, int argc)
   if (b == NULL)
     error (_("breakpoint %d not found."), bnum);
 
-  mi_command_line_array = argv;
-  mi_command_line_array_ptr = 1;
-  mi_command_line_array_cnt = argc;
+  int count = 1;
+  gdb::function_view<const char * ()> reader
+    = [&] ()
+      {
+	const char *result = nullptr;
+	if (count < argc)
+	  result = argv[count++];
+	return result;
+      };
 
   if (is_tracepoint (b))
-    break_command = read_command_lines_1 (mi_read_next_line, 1,
-					  check_tracepoint_command, b);
+    break_command = read_command_lines_1 (reader, 1,
+					  [=] (const char *line)
+					    {
+					      validate_actionline (line, b);
+					    });
   else
-    break_command = read_command_lines_1 (mi_read_next_line, 1, 0, 0);
+    break_command = read_command_lines_1 (reader, 1, 0);
 
   breakpoint_set_commands (b, std::move (break_command));
 }
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 0c62b957ef..bac8d0d10f 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -580,8 +580,10 @@ actions_command (const char *args, int from_tty)
 
       counted_command_line l = read_command_lines (tmpbuf.c_str (),
 						   from_tty, 1,
-						   check_tracepoint_command,
-						   t);
+						   [=] (const char *line)
+						     {
+						       validate_actionline (line, t);
+						     });
       breakpoint_set_commands (t, std::move (l));
     }
   /* else just return */
-- 
2.13.6

Re: [RFA 6/8] Use function_view in cli-script.c

[Pedro Alves]

On 04/19/2018 08:15 PM, Tom Tromey wrote:

> -/* The mi_read_next_line consults these variable to return successive
> -   command lines.  While it would be clearer to use a closure pointer,
> -   it is not expected that any future code will use read_command_lines_1,
> -   therefore no point of overengineering.  */

Ahahahah :-)


>  void
>  mi_cmd_break_commands (const char *command, char **argv, int argc)
>  {
> @@ -509,15 +492,24 @@ mi_cmd_break_commands (const char *command, char **argv, int argc)
>    if (b == NULL)
>      error (_("breakpoint %d not found."), bnum);
>  
> -  mi_command_line_array = argv;
> -  mi_command_line_array_ptr = 1;
> -  mi_command_line_array_cnt = argc;
> +  int count = 1;
> +  gdb::function_view<const char * ()> reader
> +    = [&] ()
> +      {
> +	const char *result = nullptr;
> +	if (count < argc)
> +	  result = argv[count++];
> +	return result;
> +      };

This is incorrect -- The 'reader' function_view is storing an address
to a closure whose lifetime ends right after the function_view ctor
is run.  This a similar issue with assigning a string_view to
a temporary std::string, like:

  std::string_view view = std::string ("temp");

C++ doesn't yet have a mechanism that allows making string_view and
types like it extend the lifetime of temporaries they're bound to,
like const references can.

You can fix the issue at hand here easily by not using function_view
at all:

  auto reader = [&] ()
    {
      const char *result = nullptr;
      if (count < argc)
        result = argv[count++];
      return result;
    };

Now "reader" is the lambda closure itself.

> -	       cmd = read_command_lines (str.c_str (), from_tty, 1,
> -					 (is_tracepoint (b)
> -					  ? check_tracepoint_command : 0),
> -					 b);
> +	       gdb::function_view<void (const char *)> validator;
> +	       if (is_tracepoint (b))
> +		 validator = [=] (const char *line)
> +		   {
> +		     validate_actionline (line, b);
> +		   };
> +
> +	       cmd = read_command_lines (str.c_str (), from_tty, 1, validator);
>  	     }
>  	 }

Similar problem here.  Something like this would fix it:

               auto validator = [=] (const char *line)
                 {
                   validate_actionline (line, b);
                 };
               decltype (callback) null_callback;

	       cmd = read_command_lines (str.c_str (), from_tty, 1, 
                                         is_tracepoint (b) 
                                         ? validator : null_callback);

or use if/else:

    if (is_tracepoint (b))
      {
        cmd = read_command_lines (str.c_str (), from_tty, 1, 
                                  [=] (const char *line)
          {
            validate_actionline (line, b);
          };
      }
    else
     cmd = read_command_lines (str.c_str (), from_tty, 1, nullptr);

Thanks,
Pedro Alves

Re: [RFA 6/8] Use function_view in cli-script.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This is incorrect -- The 'reader' function_view is storing an address
Pedro> to a closure whose lifetime ends right after the function_view ctor
Pedro> is run.

Thanks for noticing this.  It completely slipped my mind.

Tom