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

Message ID 20180419191539.661-7-tom@tromey.com
State Superseded
Headers show
Series
  • Various command-related improvements
Related show

Commit Message

Tom Tromey April 19, 2018, 7:15 p.m.
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(-)

-- 
2.13.6

Comments

Pedro Alves April 22, 2018, 7:02 p.m. | #1
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
Tom Tromey April 24, 2018, 11:38 p.m. | #2
>>>>> "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

Patch

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 */