[PATCHv2 0/5] refactoring towards Python MI command API

[PATCHv2 0/5] refactoring towards Python MI command API

[Andrew Burgess]

I originally posted these patches here:

  https://sourceware.org/pipermail/gdb-patches/2021-November/183231.html

but as that was buried in the middle of a general discussion thread I
was worried the series might not have been noticed.

The work in those patches is not my own, but is based on Jan's work
which has been previously posted here:

  https://sourceware.org/pipermail/gdb-patches/2019-May/158010.html

This series is not all of Jan's work, his series goes all of the way
to adding a Python API for creating MI commands.  This series is just
the refactoring ahead of the final Python changes, but I think these
changes stand on their own, so would like to have them considered for
acceptance.  We can then rebase the remained of Jan's work on top of
master once this series lands.

Pedro had some feedback on Jan's V3 series, but only one of the
feedback items applies to the work I'm submitting here:

  https://sourceware.org/pipermail/gdb-patches/2019-June/158431.html

I've gone thought that email and I believe that all of the issues
listed have been addressed.

Changes since _my_ V1 series:

  - rebased onto current upstream master, and

  - ensured that all of Pedro's feedback has been addressed.

Any additional feedback would be welcome.

Thanks,
Andrew


---

Andrew Burgess (1):
  gdb/mi: int to bool conversion in mi_execute_cli_command

Jan Vrany (4):
  gdb/mi: rename mi_lookup to mi_cmd_lookup
  gdb/mi: use std::map for MI commands in mi-cmds.c
  gdb/mi: use separate classes for different types of MI command
  gdb/mi: rename mi_cmd to mi_command

 gdb/mi/mi-cmd-info.c |   4 +-
 gdb/mi/mi-cmds.c     | 571 ++++++++++++++++++++++++-------------------
 gdb/mi/mi-cmds.h     |  70 ++++--
 gdb/mi/mi-main.c     |  51 +---
 gdb/mi/mi-main.h     |  12 +
 gdb/mi/mi-parse.c    |  20 +-
 gdb/mi/mi-parse.h    |   6 +-
 7 files changed, 401 insertions(+), 333 deletions(-)

-- 
2.25.4

[PATCHv2 1/5] gdb/mi: rename mi_lookup to mi_cmd_lookup

[Andrew Burgess]

From: Jan Vrany <jan.vrany@labware.com>

Lets give this function a more descriptive name.  I've also improved
the comments in the header and source files.

There should be no user visible changes after this commit.
---
 gdb/mi/mi-cmd-info.c | 2 +-
 gdb/mi/mi-cmds.c     | 5 +++--
 gdb/mi/mi-cmds.h     | 5 +++--
 gdb/mi/mi-parse.c    | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index 78377dc32e7..c2858519a4c 100644
--- a/gdb/mi/mi-cmd-info.c
+++ b/gdb/mi/mi-cmd-info.c
@@ -82,7 +82,7 @@ mi_cmd_info_gdb_mi_command (const char *command, char **argv, int argc)
   if (cmd_name[0] == '-')
     cmd_name++;
 
-  cmd = mi_lookup (cmd_name);
+  cmd = mi_cmd_lookup (cmd_name);
 
   ui_out_emit_tuple tuple_emitter (uiout, "command");
   uiout->field_string ("exists", cmd != NULL ? "true" : "false");
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 1ed8b6f9126..8899fdd3a1e 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -211,9 +211,10 @@ enum
     MI_TABLE_SIZE = 227
   };
 
-/* Exported function used to obtain info from the table.  */
+/* See mi-cmds.h.  */
+
 struct mi_cmd *
-mi_lookup (const char *command)
+mi_cmd_lookup (const char *command)
 {
   return *lookup_table (command);
 }
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 8da2e393919..e92737be8c8 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -164,9 +164,10 @@ struct mi_cmd
   int *suppress_notification;
 };
 
-/* Lookup a command in the MI command table.  */
+/* Lookup a command in the MI command table, returns nullptr if COMMAND is
+   not found.  */
 
-extern struct mi_cmd *mi_lookup (const char *command);
+extern struct mi_cmd *mi_cmd_lookup (const char *command);
 
 /* Debug flag */
 extern int mi_debug_p;
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index 4d6afa93e1f..b5b01cf4db0 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -272,7 +272,7 @@ mi_parse (const char *cmd, char **token)
   }
 
   /* Find the command in the MI table.  */
-  parse->cmd = mi_lookup (parse->command);
+  parse->cmd = mi_cmd_lookup (parse->command);
   if (parse->cmd == NULL)
     throw_error (UNDEFINED_COMMAND_ERROR,
 		 _("Undefined MI command: %s"), parse->command);
-- 
2.25.4

[PATCHv2 2/5] gdb/mi: use std::map for MI commands in mi-cmds.c

[Andrew Burgess]

From: Jan Vrany <jan.vrany@labware.com>

This changes the hashmap used in mi-cmds.c from a custom structure to
std::map.  Not only is replacing a custom container with a standard
one an improvement, but using std::map will make it easier to
dynamically add commands; which is something that is planned for a
later series, where we will allow MI commands to be implemented in
Python.

There should be no user visible changes after this commit.
---
 gdb/mi/mi-cmds.c | 478 +++++++++++++++++++++++------------------------
 1 file changed, 230 insertions(+), 248 deletions(-)

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 8899fdd3a1e..4999c9f81b3 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -22,283 +22,265 @@
 #include "top.h"
 #include "mi-cmds.h"
 #include "mi-main.h"
+#include <map>
+#include <string>
 
-struct mi_cmd;
-static struct mi_cmd **lookup_table (const char *command);
-static void build_table (struct mi_cmd *commands);
+/* A command held in the MI_CMD_TABLE.  */
 
-static struct mi_cmd mi_cmds[] =
-{
-/* Define a MI command of NAME, and its corresponding CLI command is
-   CLI_NAME.  */
-#define DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, CALLED)	\
-  { NAME, { CLI_NAME, ARGS_P}, NULL, CALLED }
-#define DEF_MI_CMD_CLI(NAME, CLI_NAME, ARGS_P) \
-  DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, NULL)
+typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
 
-/* Define a MI command of NAME, and implemented by function MI_FUNC.  */
-#define DEF_MI_CMD_MI_1(NAME, MI_FUNC, CALLED) \
-  { NAME, {NULL, 0}, MI_FUNC, CALLED }
-#define DEF_MI_CMD_MI(NAME, MI_FUNC) DEF_MI_CMD_MI_1(NAME, MI_FUNC, NULL)
+/* MI command table (built at run time). */
 
-  DEF_MI_CMD_MI ("ada-task-info", mi_cmd_ada_task_info),
-  DEF_MI_CMD_MI ("add-inferior", mi_cmd_add_inferior),
-  DEF_MI_CMD_CLI_1 ("break-after", "ignore", 1,
-		    &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("break-condition", mi_cmd_break_condition,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("break-commands", mi_cmd_break_commands,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_CLI_1 ("break-delete", "delete breakpoint", 1,
-		    &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_CLI_1 ("break-disable", "disable breakpoint", 1,
-		    &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_CLI_1 ("break-enable", "enable breakpoint", 1,
-		     &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_CLI ("break-info", "info break", 1),
-  DEF_MI_CMD_MI_1 ("break-insert", mi_cmd_break_insert,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("dprintf-insert", mi_cmd_dprintf_insert,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_CLI ("break-list", "info break", 0),
-  DEF_MI_CMD_MI_1 ("break-passcount", mi_cmd_break_passcount,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("break-watch", mi_cmd_break_watch,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-assert", mi_cmd_catch_assert,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-exception", mi_cmd_catch_exception,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-handlers", mi_cmd_catch_handlers,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-load", mi_cmd_catch_load,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-unload", mi_cmd_catch_unload,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-throw", mi_cmd_catch_throw,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-rethrow", mi_cmd_catch_rethrow,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI_1 ("catch-catch", mi_cmd_catch_catch,
-		   &mi_suppress_notification.breakpoint),
-  DEF_MI_CMD_MI ("complete", mi_cmd_complete),
-  DEF_MI_CMD_MI ("data-disassemble", mi_cmd_disassemble),
-  DEF_MI_CMD_MI ("data-evaluate-expression", mi_cmd_data_evaluate_expression),
-  DEF_MI_CMD_MI ("data-list-changed-registers",
-		 mi_cmd_data_list_changed_registers),
-  DEF_MI_CMD_MI ("data-list-register-names", mi_cmd_data_list_register_names),
-  DEF_MI_CMD_MI ("data-list-register-values", mi_cmd_data_list_register_values),
-  DEF_MI_CMD_MI ("data-read-memory", mi_cmd_data_read_memory),
-  DEF_MI_CMD_MI ("data-read-memory-bytes", mi_cmd_data_read_memory_bytes),
-  DEF_MI_CMD_MI_1 ("data-write-memory", mi_cmd_data_write_memory,
-		   &mi_suppress_notification.memory),
-  DEF_MI_CMD_MI_1 ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes,
-		   &mi_suppress_notification.memory),
-  DEF_MI_CMD_MI ("data-write-register-values",
-		 mi_cmd_data_write_register_values),
-  DEF_MI_CMD_MI ("enable-timings", mi_cmd_enable_timings),
-  DEF_MI_CMD_MI ("enable-pretty-printing", mi_cmd_enable_pretty_printing),
-  DEF_MI_CMD_MI ("enable-frame-filters", mi_cmd_enable_frame_filters),
-  DEF_MI_CMD_MI ("environment-cd", mi_cmd_env_cd),
-  DEF_MI_CMD_MI ("environment-directory", mi_cmd_env_dir),
-  DEF_MI_CMD_MI ("environment-path", mi_cmd_env_path),
-  DEF_MI_CMD_MI ("environment-pwd", mi_cmd_env_pwd),
-  DEF_MI_CMD_CLI_1 ("exec-arguments", "set args", 1,
-		    &mi_suppress_notification.cmd_param_changed),
-  DEF_MI_CMD_MI ("exec-continue", mi_cmd_exec_continue),
-  DEF_MI_CMD_MI ("exec-finish", mi_cmd_exec_finish),
-  DEF_MI_CMD_MI ("exec-jump", mi_cmd_exec_jump),
-  DEF_MI_CMD_MI ("exec-interrupt", mi_cmd_exec_interrupt),
-  DEF_MI_CMD_MI ("exec-next", mi_cmd_exec_next),
-  DEF_MI_CMD_MI ("exec-next-instruction", mi_cmd_exec_next_instruction),
-  DEF_MI_CMD_MI ("exec-return", mi_cmd_exec_return),
-  DEF_MI_CMD_MI ("exec-run", mi_cmd_exec_run),
-  DEF_MI_CMD_MI ("exec-step", mi_cmd_exec_step),
-  DEF_MI_CMD_MI ("exec-step-instruction", mi_cmd_exec_step_instruction),
-  DEF_MI_CMD_CLI ("exec-until", "until", 1),
-  DEF_MI_CMD_CLI ("file-exec-and-symbols", "file", 1),
-  DEF_MI_CMD_CLI ("file-exec-file", "exec-file", 1),
-  DEF_MI_CMD_MI ("file-list-exec-source-file",
-		 mi_cmd_file_list_exec_source_file),
-  DEF_MI_CMD_MI ("file-list-exec-source-files",
-		 mi_cmd_file_list_exec_source_files),
-  DEF_MI_CMD_MI ("file-list-shared-libraries",
-		 mi_cmd_file_list_shared_libraries),
-  DEF_MI_CMD_CLI ("file-symbol-file", "symbol-file", 1),
-  DEF_MI_CMD_MI ("fix-multi-location-breakpoint-output",
-		 mi_cmd_fix_multi_location_breakpoint_output),
-  DEF_MI_CMD_MI ("gdb-exit", mi_cmd_gdb_exit),
-  DEF_MI_CMD_CLI_1 ("gdb-set", "set", 1,
-		    &mi_suppress_notification.cmd_param_changed),
-  DEF_MI_CMD_CLI ("gdb-show", "show", 1),
-  DEF_MI_CMD_CLI ("gdb-version", "show version", 0),
-  DEF_MI_CMD_MI ("inferior-tty-set", mi_cmd_inferior_tty_set),
-  DEF_MI_CMD_MI ("inferior-tty-show", mi_cmd_inferior_tty_show),
-  DEF_MI_CMD_MI ("info-ada-exceptions", mi_cmd_info_ada_exceptions),
-  DEF_MI_CMD_MI ("info-gdb-mi-command", mi_cmd_info_gdb_mi_command),
-  DEF_MI_CMD_MI ("info-os", mi_cmd_info_os),
-  DEF_MI_CMD_MI ("interpreter-exec", mi_cmd_interpreter_exec),
-  DEF_MI_CMD_MI ("list-features", mi_cmd_list_features),
-  DEF_MI_CMD_MI ("list-target-features", mi_cmd_list_target_features),
-  DEF_MI_CMD_MI ("list-thread-groups", mi_cmd_list_thread_groups),
-  DEF_MI_CMD_MI ("remove-inferior", mi_cmd_remove_inferior),
-  DEF_MI_CMD_MI ("stack-info-depth", mi_cmd_stack_info_depth),
-  DEF_MI_CMD_MI ("stack-info-frame", mi_cmd_stack_info_frame),
-  DEF_MI_CMD_MI ("stack-list-arguments", mi_cmd_stack_list_args),
-  DEF_MI_CMD_MI ("stack-list-frames", mi_cmd_stack_list_frames),
-  DEF_MI_CMD_MI ("stack-list-locals", mi_cmd_stack_list_locals),
-  DEF_MI_CMD_MI ("stack-list-variables", mi_cmd_stack_list_variables),
-  DEF_MI_CMD_MI_1 ("stack-select-frame", mi_cmd_stack_select_frame,
-		   &mi_suppress_notification.user_selected_context),
-  DEF_MI_CMD_MI ("symbol-list-lines", mi_cmd_symbol_list_lines),
-  DEF_MI_CMD_MI ("symbol-info-functions", mi_cmd_symbol_info_functions),
-  DEF_MI_CMD_MI ("symbol-info-variables", mi_cmd_symbol_info_variables),
-  DEF_MI_CMD_MI ("symbol-info-types", mi_cmd_symbol_info_types),
-  DEF_MI_CMD_MI ("symbol-info-modules", mi_cmd_symbol_info_modules),
-  DEF_MI_CMD_MI ("symbol-info-module-functions",
-		 mi_cmd_symbol_info_module_functions),
-  DEF_MI_CMD_MI ("symbol-info-module-variables",
-		 mi_cmd_symbol_info_module_variables),
-  DEF_MI_CMD_CLI ("target-attach", "attach", 1),
-  DEF_MI_CMD_MI ("target-detach", mi_cmd_target_detach),
-  DEF_MI_CMD_CLI ("target-disconnect", "disconnect", 0),
-  DEF_MI_CMD_CLI ("target-download", "load", 1),
-  DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
-  DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
-  DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
-  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
-  DEF_MI_CMD_CLI ("target-select", "target", 1),
-  DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
-  DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
-  DEF_MI_CMD_MI_1 ("thread-select", mi_cmd_thread_select,
-		   &mi_suppress_notification.user_selected_context),
-  DEF_MI_CMD_MI ("trace-define-variable", mi_cmd_trace_define_variable),
-  DEF_MI_CMD_MI_1 ("trace-find", mi_cmd_trace_find,
-		   &mi_suppress_notification.traceframe),
-  DEF_MI_CMD_MI ("trace-frame-collected",
-		 mi_cmd_trace_frame_collected),
-  DEF_MI_CMD_MI ("trace-list-variables", mi_cmd_trace_list_variables),
-  DEF_MI_CMD_MI ("trace-save", mi_cmd_trace_save),
-  DEF_MI_CMD_MI ("trace-start", mi_cmd_trace_start),
-  DEF_MI_CMD_MI ("trace-status", mi_cmd_trace_status),
-  DEF_MI_CMD_MI ("trace-stop", mi_cmd_trace_stop),
-  DEF_MI_CMD_MI ("var-assign", mi_cmd_var_assign),
-  DEF_MI_CMD_MI ("var-create", mi_cmd_var_create),
-  DEF_MI_CMD_MI ("var-delete", mi_cmd_var_delete),
-  DEF_MI_CMD_MI ("var-evaluate-expression", mi_cmd_var_evaluate_expression),
-  DEF_MI_CMD_MI ("var-info-path-expression", mi_cmd_var_info_path_expression),
-  DEF_MI_CMD_MI ("var-info-expression", mi_cmd_var_info_expression),
-  DEF_MI_CMD_MI ("var-info-num-children", mi_cmd_var_info_num_children),
-  DEF_MI_CMD_MI ("var-info-type", mi_cmd_var_info_type),
-  DEF_MI_CMD_MI ("var-list-children", mi_cmd_var_list_children),
-  DEF_MI_CMD_MI ("var-set-format", mi_cmd_var_set_format),
-  DEF_MI_CMD_MI ("var-set-frozen", mi_cmd_var_set_frozen),
-  DEF_MI_CMD_MI ("var-set-update-range", mi_cmd_var_set_update_range),
-  DEF_MI_CMD_MI ("var-set-visualizer", mi_cmd_var_set_visualizer),
-  DEF_MI_CMD_MI ("var-show-attributes", mi_cmd_var_show_attributes),
-  DEF_MI_CMD_MI ("var-show-format", mi_cmd_var_show_format),
-  DEF_MI_CMD_MI ("var-update", mi_cmd_var_update),
-  { NULL, }
-};
+static std::map<std::string, mi_cmd_up> mi_cmd_table;
 
-/* Pointer to the mi command table (built at run time). */
+/* Insert COMMAND into the global mi_cmd_table.  Return false if
+   COMMAND->name already exists in mi_cmd_table, in which case COMMAND will
+   not have been added to mi_cmd_table.  Otherwise, return true, and
+   COMMAND was added to mi_cmd_table.  */
 
-static struct mi_cmd **mi_table;
+static bool
+insert_mi_cmd_entry (mi_cmd_up command)
+{
+  gdb_assert (command != nullptr);
+  gdb_assert (command->name != nullptr);
 
-/* A prime large enough to accomodate the entire command table.  */
-enum
-  {
-    MI_TABLE_SIZE = 227
-  };
+  std::string name (command->name);
 
-/* See mi-cmds.h.  */
+  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
+    return false;
 
-struct mi_cmd *
-mi_cmd_lookup (const char *command)
+  mi_cmd_table[name] = std::move (command);
+  return true;
+}
+
+/* Create an mi_cmd structure with name NAME.  */
+
+static mi_cmd_up
+create_mi_cmd (const char *name)
 {
-  return *lookup_table (command);
+  mi_cmd_up cmd (new mi_cmd ());
+  cmd->name = name;
+  return cmd;
 }
 
-/* Used for collecting hash hit/miss statistics.  */
+/* Create and register a new MI command with an MI specific implementation.
+   NAME must name an MI command that does not already exist, otherwise an
+   assertion will trigger.  */
 
-static struct
+static void
+add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
+	       int *suppress_notification = nullptr)
 {
-  int hit;
-  int miss;
-  int rehash;
-} stats;
+  mi_cmd_up cmd_up = create_mi_cmd (name);
+
+  cmd_up->cli.cmd = nullptr;
+  cmd_up->cli.args_p = 0;
+  cmd_up->argv_func = function;
+  cmd_up->suppress_notification = suppress_notification;
 
-/* Look up a command.  */
+  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  gdb_assert (success);
+}
+
+/* Create and register a new MI command implemented on top of a CLI
+   command.  NAME must name an MI command that does not already exist,
+   otherwise an assertion will trigger.  */
 
-static struct mi_cmd **
-lookup_table (const char *command)
+static void
+add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
+		int *suppress_notification = nullptr)
 {
-  const char *chp;
-  unsigned int index = 0;
+  mi_cmd_up cmd_up = create_mi_cmd (name);
 
-  /* Compute our hash.  */
-  for (chp = command; *chp; chp++)
-    {
-      /* We use a somewhat arbitrary formula.  */
-      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
-    }
+  cmd_up->cli.cmd = cli_name;
+  cmd_up->cli.args_p = args_p;
+  cmd_up->argv_func = nullptr;
+  cmd_up->suppress_notification = suppress_notification;
 
-  while (1)
-    {
-      struct mi_cmd **entry = &mi_table[index];
-      if ((*entry) == 0)
-	{
-	  /* not found, return pointer to next free. */
-	  stats.miss++;
-	  return entry;
-	}
-      if (strcmp (command, (*entry)->name) == 0)
-	{
-	  stats.hit++;
-	  return entry;		/* found */
-	}
-      index = (index + 1) % MI_TABLE_SIZE;
-      stats.rehash++;
-    }
+  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  gdb_assert (success);
 }
 
+/* Initialize MI_CMD_TABLE, the global map of MI commands.  */
+
 static void
-build_table (struct mi_cmd *commands)
+build_table ()
 {
-  int nr_rehash = 0;
-  int nr_entries = 0;
-  struct mi_cmd *command;
+  add_mi_cmd_mi ("ada-task-info", mi_cmd_ada_task_info);
+  add_mi_cmd_mi ("add-inferior", mi_cmd_add_inferior);
+  add_mi_cmd_cli ("break-after", "ignore", 1,
+		  &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("break-condition",mi_cmd_break_condition,
+		  &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("break-commands", mi_cmd_break_commands,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_cli ("break-delete", "delete breakpoint", 1,
+		  &mi_suppress_notification.breakpoint);
+  add_mi_cmd_cli ("break-disable", "disable breakpoint", 1,
+		  &mi_suppress_notification.breakpoint);
+  add_mi_cmd_cli ("break-enable", "enable breakpoint", 1,
+		  &mi_suppress_notification.breakpoint);
+  add_mi_cmd_cli ("break-info", "info break", 1);
+  add_mi_cmd_mi ("break-insert", mi_cmd_break_insert,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("dprintf-insert", mi_cmd_dprintf_insert,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_cli ("break-list", "info break", 0);
+  add_mi_cmd_mi ("break-passcount", mi_cmd_break_passcount,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("break-watch", mi_cmd_break_watch,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-assert", mi_cmd_catch_assert,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-exception", mi_cmd_catch_exception,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-handlers", mi_cmd_catch_handlers,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-load", mi_cmd_catch_load,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-unload", mi_cmd_catch_unload,
+		 &mi_suppress_notification.breakpoint);
+  add_mi_cmd_mi ("catch-throw", mi_cmd_catch_throw,
+                 &mi_suppress_notification.breakpoint),
+  add_mi_cmd_mi ("catch-rethrow", mi_cmd_catch_rethrow,
+                 &mi_suppress_notification.breakpoint),
+  add_mi_cmd_mi ("catch-catch", mi_cmd_catch_catch,
+                 &mi_suppress_notification.breakpoint),
+  add_mi_cmd_mi ("complete", mi_cmd_complete);
+  add_mi_cmd_mi ("data-disassemble", mi_cmd_disassemble);
+  add_mi_cmd_mi ("data-evaluate-expression", mi_cmd_data_evaluate_expression);
+  add_mi_cmd_mi ("data-list-changed-registers",
+		 mi_cmd_data_list_changed_registers);
+  add_mi_cmd_mi ("data-list-register-names", mi_cmd_data_list_register_names);
+  add_mi_cmd_mi ("data-list-register-values",
+		 mi_cmd_data_list_register_values);
+  add_mi_cmd_mi ("data-read-memory", mi_cmd_data_read_memory);
+  add_mi_cmd_mi ("data-read-memory-bytes", mi_cmd_data_read_memory_bytes);
+  add_mi_cmd_mi ("data-write-memory", mi_cmd_data_write_memory,
+		 &mi_suppress_notification.memory);
+  add_mi_cmd_mi ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes,
+		 &mi_suppress_notification.memory);
+  add_mi_cmd_mi ("data-write-register-values",
+		 mi_cmd_data_write_register_values);
+  add_mi_cmd_mi ("enable-timings", mi_cmd_enable_timings);
+  add_mi_cmd_mi ("enable-pretty-printing", mi_cmd_enable_pretty_printing);
+  add_mi_cmd_mi ("enable-frame-filters", mi_cmd_enable_frame_filters);
+  add_mi_cmd_mi ("environment-cd", mi_cmd_env_cd);
+  add_mi_cmd_mi ("environment-directory", mi_cmd_env_dir);
+  add_mi_cmd_mi ("environment-path", mi_cmd_env_path);
+  add_mi_cmd_mi ("environment-pwd", mi_cmd_env_pwd);
+  add_mi_cmd_cli ("exec-arguments", "set args", 1,
+		  &mi_suppress_notification.cmd_param_changed);
+  add_mi_cmd_mi ("exec-continue", mi_cmd_exec_continue);
+  add_mi_cmd_mi ("exec-finish", mi_cmd_exec_finish);
+  add_mi_cmd_mi ("exec-jump", mi_cmd_exec_jump);
+  add_mi_cmd_mi ("exec-interrupt", mi_cmd_exec_interrupt);
+  add_mi_cmd_mi ("exec-next", mi_cmd_exec_next);
+  add_mi_cmd_mi ("exec-next-instruction", mi_cmd_exec_next_instruction);
+  add_mi_cmd_mi ("exec-return", mi_cmd_exec_return);
+  add_mi_cmd_mi ("exec-run", mi_cmd_exec_run);
+  add_mi_cmd_mi ("exec-step", mi_cmd_exec_step);
+  add_mi_cmd_mi ("exec-step-instruction", mi_cmd_exec_step_instruction);
+  add_mi_cmd_cli ("exec-until", "until", 1);
+  add_mi_cmd_cli ("file-exec-and-symbols", "file", 1);
+  add_mi_cmd_cli ("file-exec-file", "exec-file", 1);
+  add_mi_cmd_mi ("file-list-exec-source-file",
+		 mi_cmd_file_list_exec_source_file);
+  add_mi_cmd_mi ("file-list-exec-source-files",
+		 mi_cmd_file_list_exec_source_files);
+  add_mi_cmd_mi ("file-list-shared-libraries",
+     mi_cmd_file_list_shared_libraries),
+  add_mi_cmd_cli ("file-symbol-file", "symbol-file", 1);
+  add_mi_cmd_mi ("fix-multi-location-breakpoint-output",
+		 mi_cmd_fix_multi_location_breakpoint_output),
+  add_mi_cmd_mi ("gdb-exit", mi_cmd_gdb_exit);
+  add_mi_cmd_cli ("gdb-set", "set", 1,
+		  &mi_suppress_notification.cmd_param_changed);
+  add_mi_cmd_cli ("gdb-show", "show", 1);
+  add_mi_cmd_cli ("gdb-version", "show version", 0);
+  add_mi_cmd_mi ("inferior-tty-set", mi_cmd_inferior_tty_set);
+  add_mi_cmd_mi ("inferior-tty-show", mi_cmd_inferior_tty_show);
+  add_mi_cmd_mi ("info-ada-exceptions", mi_cmd_info_ada_exceptions);
+  add_mi_cmd_mi ("info-gdb-mi-command", mi_cmd_info_gdb_mi_command);
+  add_mi_cmd_mi ("info-os", mi_cmd_info_os);
+  add_mi_cmd_mi ("interpreter-exec", mi_cmd_interpreter_exec);
+  add_mi_cmd_mi ("list-features", mi_cmd_list_features);
+  add_mi_cmd_mi ("list-target-features", mi_cmd_list_target_features);
+  add_mi_cmd_mi ("list-thread-groups", mi_cmd_list_thread_groups);
+  add_mi_cmd_mi ("remove-inferior", mi_cmd_remove_inferior);
+  add_mi_cmd_mi ("stack-info-depth", mi_cmd_stack_info_depth);
+  add_mi_cmd_mi ("stack-info-frame", mi_cmd_stack_info_frame);
+  add_mi_cmd_mi ("stack-list-arguments", mi_cmd_stack_list_args);
+  add_mi_cmd_mi ("stack-list-frames", mi_cmd_stack_list_frames);
+  add_mi_cmd_mi ("stack-list-locals", mi_cmd_stack_list_locals);
+  add_mi_cmd_mi ("stack-list-variables", mi_cmd_stack_list_variables);
+  add_mi_cmd_mi ("stack-select-frame", mi_cmd_stack_select_frame,
+		 &mi_suppress_notification.user_selected_context);
+  add_mi_cmd_mi ("symbol-list-lines", mi_cmd_symbol_list_lines);
+  add_mi_cmd_mi ("symbol-info-functions", mi_cmd_symbol_info_functions);
+  add_mi_cmd_mi ("symbol-info-variables", mi_cmd_symbol_info_variables);
+  add_mi_cmd_mi ("symbol-info-types", mi_cmd_symbol_info_types);
+  add_mi_cmd_mi ("symbol-info-modules", mi_cmd_symbol_info_modules);
+  add_mi_cmd_mi ("symbol-info-module-functions",
+                 mi_cmd_symbol_info_module_functions);
+  add_mi_cmd_mi ("symbol-info-module-variables",
+                 mi_cmd_symbol_info_module_variables);
+  add_mi_cmd_cli ("target-attach", "attach", 1);
+  add_mi_cmd_mi ("target-detach", mi_cmd_target_detach);
+  add_mi_cmd_cli ("target-disconnect", "disconnect", 0);
+  add_mi_cmd_cli ("target-download", "load", 1);
+  add_mi_cmd_mi ("target-file-delete", mi_cmd_target_file_delete);
+  add_mi_cmd_mi ("target-file-get", mi_cmd_target_file_get);
+  add_mi_cmd_mi ("target-file-put", mi_cmd_target_file_put);
+  add_mi_cmd_mi ("target-flash-erase", mi_cmd_target_flash_erase);
+  add_mi_cmd_cli ("target-select", "target", 1);
+  add_mi_cmd_mi ("thread-info", mi_cmd_thread_info);
+  add_mi_cmd_mi ("thread-list-ids", mi_cmd_thread_list_ids);
+  add_mi_cmd_mi ("thread-select", mi_cmd_thread_select,
+		 &mi_suppress_notification.user_selected_context);
+  add_mi_cmd_mi ("trace-define-variable", mi_cmd_trace_define_variable);
+  add_mi_cmd_mi ("trace-find", mi_cmd_trace_find,
+		 &mi_suppress_notification.traceframe);
+  add_mi_cmd_mi ("trace-frame-collected", mi_cmd_trace_frame_collected);
+  add_mi_cmd_mi ("trace-list-variables", mi_cmd_trace_list_variables);
+  add_mi_cmd_mi ("trace-save", mi_cmd_trace_save);
+  add_mi_cmd_mi ("trace-start", mi_cmd_trace_start);
+  add_mi_cmd_mi ("trace-status", mi_cmd_trace_status);
+  add_mi_cmd_mi ("trace-stop", mi_cmd_trace_stop);
+  add_mi_cmd_mi ("var-assign", mi_cmd_var_assign);
+  add_mi_cmd_mi ("var-create", mi_cmd_var_create);
+  add_mi_cmd_mi ("var-delete", mi_cmd_var_delete);
+  add_mi_cmd_mi ("var-evaluate-expression", mi_cmd_var_evaluate_expression);
+  add_mi_cmd_mi ("var-info-path-expression", mi_cmd_var_info_path_expression);
+  add_mi_cmd_mi ("var-info-expression", mi_cmd_var_info_expression);
+  add_mi_cmd_mi ("var-info-num-children", mi_cmd_var_info_num_children);
+  add_mi_cmd_mi ("var-info-type", mi_cmd_var_info_type);
+  add_mi_cmd_mi ("var-list-children", mi_cmd_var_list_children);
+  add_mi_cmd_mi ("var-set-format", mi_cmd_var_set_format);
+  add_mi_cmd_mi ("var-set-frozen", mi_cmd_var_set_frozen);
+  add_mi_cmd_mi ("var-set-update-range", mi_cmd_var_set_update_range);
+  add_mi_cmd_mi ("var-set-visualizer", mi_cmd_var_set_visualizer);
+  add_mi_cmd_mi ("var-show-attributes", mi_cmd_var_show_attributes);
+  add_mi_cmd_mi ("var-show-format", mi_cmd_var_show_format);
+  add_mi_cmd_mi ("var-update", mi_cmd_var_update);
+}
 
-  mi_table = XCNEWVEC (struct mi_cmd *, MI_TABLE_SIZE);
-  for (command = commands; command->name != 0; command++)
-    {
-      struct mi_cmd **entry = lookup_table (command->name);
+/* See mi-cmds.h.  */
+
+struct mi_cmd *
+mi_cmd_lookup (const char *command)
+{
+  gdb_assert (command != nullptr);
 
-      if (*entry)
-	internal_error (__FILE__, __LINE__,
-			_("command `%s' appears to be duplicated"),
-			command->name);
-      *entry = command;
-      /* FIXME lose these prints */
-      if (0)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "%-30s %2d\n",
-			      command->name, stats.rehash - nr_rehash);
-	}
-      nr_entries++;
-      nr_rehash = stats.rehash;
-    }
-  if (0)
-    {
-      fprintf_filtered (gdb_stdlog, "Average %3.1f\n",
-			(double) nr_rehash / (double) nr_entries);
-    }
+  auto it = mi_cmd_table.find (command);
+  if (it == mi_cmd_table.end ())
+    return nullptr;
+  return it->second.get ();
 }
 
 void _initialize_mi_cmds ();
 void
 _initialize_mi_cmds ()
 {
-  build_table (mi_cmds);
-  memset (&stats, 0, sizeof (stats));
+  build_table ();
 }
-- 
2.25.4

[PATCHv2 3/5] gdb/mi: int to bool conversion in mi_execute_cli_command

[Andrew Burgess]

Change an argument of mi_execute_cli_command from int to bool.  Update
the callers to take this into account.  Within mi_execute_cli_command,
update a comparison of a pointer to 0 with a comparison to nullptr,
and add an assert, if we are not using the argument string then the
string should be nullptr.  Also removed a cryptic 'gdb_????' comment,
which isn't really helpful.

There should be no user visible changes after this commit.
---
 gdb/mi/mi-main.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 311697b2d58..269c01d5176 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -90,7 +90,7 @@ int mi_proceeded;
 
 static void mi_cmd_execute (struct mi_parse *parse);
 
-static void mi_execute_cli_command (const char *cmd, int args_p,
+static void mi_execute_cli_command (const char *cmd, bool args_p,
 				    const char *args);
 static void mi_execute_async_cli_command (const char *cli_command,
 					  char **argv, int argc);
@@ -408,7 +408,7 @@ run_one_inferior (inferior *inf, bool start_p)
 {
   const char *run_cmd = start_p ? "start" : "run";
   struct target_ops *run_target = find_run_target ();
-  int async_p = mi_async && target_can_async_p (run_target);
+  bool async_p = mi_async && target_can_async_p (run_target);
 
   if (inf->pid != 0)
     {
@@ -473,7 +473,7 @@ mi_cmd_exec_run (const char *command, char **argv, int argc)
     {
       const char *run_cmd = start_p ? "start" : "run";
       struct target_ops *run_target = find_run_target ();
-      int async_p = mi_async && target_can_async_p (run_target);
+      bool async_p = mi_async && target_can_async_p (run_target);
 
       mi_execute_cli_command (run_cmd, async_p,
 			      async_p ? "&" : NULL);
@@ -2088,8 +2088,9 @@ mi_cmd_execute (struct mi_parse *parse)
       /* FIXME: DELETE THIS. */
       /* The operation is still implemented by a cli command.  */
       /* Must be a synchronous one.  */
-      mi_execute_cli_command (parse->cmd->cli.cmd, parse->cmd->cli.args_p,
-			      parse->args);
+      bool args_p = parse->cmd->cli.args_p != 0;
+      const char *args = args_p ? parse->args : nullptr;
+      mi_execute_cli_command (parse->cmd->cli.cmd, args_p, args);
     }
   else
     {
@@ -2109,18 +2110,21 @@ mi_cmd_execute (struct mi_parse *parse)
    Use only for synchronous commands.  */
 
 void
-mi_execute_cli_command (const char *cmd, int args_p, const char *args)
+mi_execute_cli_command (const char *cmd, bool args_p, const char *args)
 {
-  if (cmd != 0)
+  if (cmd != nullptr)
     {
-      std::string run = cmd;
+      std::string run (cmd);
 
       if (args_p)
 	run = run + " " + args;
+      else
+	gdb_assert (args == nullptr);
+
       if (mi_debug_p)
-	/* FIXME: gdb_???? */
 	fprintf_unfiltered (gdb_stdout, "cli=%s run=%s\n",
 			    cmd, run.c_str ());
+
       execute_command (run.c_str (), 0 /* from_tty */ );
     }
 }
-- 
2.25.4

[PATCHv2 4/5] gdb/mi: use separate classes for different types of MI command

[Andrew Burgess]

From: Jan Vrany <jan.vrany@labware.com>

This commit changes the infrastructure in mi-cmds.{c,h} to add new
sub-classes for the different types of MI command.  Instances of these
sub-classes are then created and added into mi_cmd_table.

The existing mi_cmd class becomes the abstract base class, this has an
invoke method and takes care of the suppress notifications handling,
before calling a do_invoke virtual method which is implemented by all
of the sub-classes.

There's currently two different sub-classes, one of pure MI commands,
and a second for MI commands that delegate to CLI commands.

There should be no user visible changes after this commit.
---
 gdb/mi/mi-cmds.c  | 140 +++++++++++++++++++++++++++++++++++++---------
 gdb/mi/mi-cmds.h  |  67 ++++++++++++++--------
 gdb/mi/mi-main.c  |  37 +-----------
 gdb/mi/mi-main.h  |  12 ++++
 gdb/mi/mi-parse.c |  18 +-----
 gdb/mi/mi-parse.h |   4 ++
 6 files changed, 179 insertions(+), 99 deletions(-)

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 4999c9f81b3..c3e5b4d3263 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -22,6 +22,7 @@
 #include "top.h"
 #include "mi-cmds.h"
 #include "mi-main.h"
+#include "mi-parse.h"
 #include <map>
 #include <string>
 
@@ -33,6 +34,81 @@ typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
 
 static std::map<std::string, mi_cmd_up> mi_cmd_table;
 
+/* MI command with a pure MI implementation.  */
+
+struct mi_command_mi : public mi_cmd
+{
+  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
+     constructor, FUNC is the function called from do_invoke, which
+     implements this MI command.  */
+  mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+                 int *suppress_notification)
+    : mi_cmd (name, suppress_notification),
+      m_argv_function (func)
+  {
+    gdb_assert (func != nullptr);
+  }
+
+protected:
+
+  /* Called when this MI command has been invoked, calls m_argv_function
+     with arguments contained within PARSE.  */
+  void do_invoke (struct mi_parse *parse) const override
+  {
+    mi_parse_argv (parse->args, parse);
+
+    if (parse->argv == nullptr)
+      error (_("Problem parsing arguments: %s %s"), parse->command,
+	     parse->args);
+
+    this->m_argv_function (parse->command, parse->argv, parse->argc);
+  }
+
+private:
+
+  /* The function that implements this MI command.  */
+  mi_cmd_argv_ftype *m_argv_function;
+};
+
+/* MI command implemented on top of a CLI command.  */
+
+struct mi_command_cli : public mi_cmd
+{
+  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
+     constructor, CLI_NAME is the name of a CLI command that should be
+     invoked to implement this MI command.  If ARGS_P is true then any
+     arguments from entered by the user as part of the MI command line are
+     forwarded to CLI_NAME as its argument string, otherwise, if ARGS_P is
+     false, nullptr is send to CLI_NAME as its argument string.  */
+  mi_command_cli (const char *name, const char *cli_name, bool args_p,
+                  int *suppress_notification)
+    : mi_cmd (name, suppress_notification),
+      m_cli_name (cli_name),
+      m_args_p (args_p)
+  { /* Nothing.  */ }
+
+protected:
+
+  /* Called when this MI command has been invoked, calls the m_cli_name
+     CLI function.  In m_args_p is true then the argument string from
+     within PARSE is passed through to the CLI function, otherwise nullptr
+     is passed through to the CLI function as its argument string.  */
+  void do_invoke (struct mi_parse *parse) const override
+  {
+    const char *args = m_args_p ? parse->args : nullptr;
+    mi_execute_cli_command (m_cli_name.c_str (), m_args_p ? 1 : 0,
+			    args);
+  }
+
+private:
+
+  /* The name of the CLI command to execute.  */
+  std::string m_cli_name;
+
+  /* Should we be passing an argument string to the m_cli_name function?  */
+  bool m_args_p;
+};
+
 /* Insert COMMAND into the global mi_cmd_table.  Return false if
    COMMAND->name already exists in mi_cmd_table, in which case COMMAND will
    not have been added to mi_cmd_table.  Otherwise, return true, and
@@ -42,9 +118,9 @@ static bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != nullptr);
-  gdb_assert (command->name != nullptr);
+  gdb_assert (!command->name ().empty ());
 
-  std::string name (command->name);
+  const std::string &name = command->name ();
 
   if (mi_cmd_table.find (name) != mi_cmd_table.end ())
     return false;
@@ -53,16 +129,6 @@ insert_mi_cmd_entry (mi_cmd_up command)
   return true;
 }
 
-/* Create an mi_cmd structure with name NAME.  */
-
-static mi_cmd_up
-create_mi_cmd (const char *name)
-{
-  mi_cmd_up cmd (new mi_cmd ());
-  cmd->name = name;
-  return cmd;
-}
-
 /* Create and register a new MI command with an MI specific implementation.
    NAME must name an MI command that does not already exist, otherwise an
    assertion will trigger.  */
@@ -71,14 +137,10 @@ static void
 add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
 	       int *suppress_notification = nullptr)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
-
-  cmd_up->cli.cmd = nullptr;
-  cmd_up->cli.args_p = 0;
-  cmd_up->argv_func = function;
-  cmd_up->suppress_notification = suppress_notification;
+  mi_cmd_up command (new mi_command_mi (name, function,
+                                        suppress_notification));
 
-  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
 }
 
@@ -90,18 +152,42 @@ static void
 add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 		int *suppress_notification = nullptr)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
+  mi_cmd_up command (new mi_command_cli (name, cli_name, args_p != 0,
+                                         suppress_notification));
 
-  cmd_up->cli.cmd = cli_name;
-  cmd_up->cli.args_p = args_p;
-  cmd_up->argv_func = nullptr;
-  cmd_up->suppress_notification = suppress_notification;
-
-  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
 }
 
-/* Initialize MI_CMD_TABLE, the global map of MI commands.  */
+/* See mi-cmds.h.  */
+
+mi_cmd::mi_cmd (const char *name, int *suppress_notification)
+  : m_name (name),
+    m_suppress_notification (suppress_notification)
+{ /* Nothing.  */ }
+
+/* See mi-cmds.h.  */
+
+void
+mi_cmd::invoke (struct mi_parse *parse) const
+{
+  gdb::optional<scoped_restore_tmpl<int>> restore
+    = do_suppress_notification ();
+  this->do_invoke (parse);
+}
+
+/* See mi-cmds.h.  */
+
+gdb::optional<scoped_restore_tmpl<int>>
+mi_cmd::do_suppress_notification () const
+{
+  if (m_suppress_notification != nullptr)
+    return scoped_restore_tmpl<int> (m_suppress_notification, 1);
+  else
+    return {};
+}
+
+/* Initialize the available MI commands.  */
 
 static void
 build_table ()
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index e92737be8c8..b859ff21da7 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -22,6 +22,7 @@
 #ifndef MI_MI_CMDS_H
 #define MI_MI_CMDS_H
 
+#include "gdbsupport/gdb_optional.h"
 enum print_values {
    PRINT_NO_VALUES,
    PRINT_ALL_VALUES,
@@ -137,37 +138,57 @@ extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
 extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
 extern mi_cmd_argv_ftype mi_cmd_complete;
 
-/* Description of a single command.  */
-
-struct mi_cli
-{
-  /* Corresponding CLI command.  If ARGS_P is non-zero, the MI
-     command's argument list is appended to the CLI command.  */
-  const char *cmd;
-  int args_p;
-};
+/* The abstract base class for all MI command types.  */
 
 struct mi_cmd
 {
-  /* Official name of the command.  */
-  const char *name;
-  /* The corresponding CLI command that can be used to implement this
-     MI command (if cli.lhs is non NULL).  */
-  struct mi_cli cli;
-  /* If non-null, the function implementing the MI command.  */
-  mi_cmd_argv_ftype *argv_func;
-  /* If non-null, the pointer to a field in
-     'struct mi_suppress_notification', which will be set to true by MI
-     command processor (mi-main.c:mi_cmd_execute) when this command is
-     being executed.  It will be set back to false when command has been
-     executed.  */
-  int *suppress_notification;
+  /* Constructor.  NAME is the name of this MI command, that is the
+     initial string the user will enter to run this command.  The
+     SUPPRESS_NOTIFICATION pointer is a flag which will be set to 1 when
+     this command is invoked, and reset to its previous value once the
+     command invocation has completed.  */
+  mi_cmd (const char *name, int *suppress_notification);
+
+  /* Destructor.  */
+  virtual ~mi_cmd () { /* Nothing.  */ }
+
+  /* Return the name of this command.  This is the command that the user
+     will actually type in, without any arguments.  */
+  const std::string &name () const
+  { return m_name; }
+
+  /* Execute the MI command.  Can throw an exception if something goes
+     wrong.  */
+  void invoke (struct mi_parse *parse) const;
+
+protected:
+
+  /* The core of command invocation, this needs to be overridden in each
+     base class.  PARSE is the parsed command line from the user.  */
+  virtual void do_invoke (struct mi_parse *parse) const = 0;
+
+private:
+
+  /* If this command was created with a suppress notifications pointer,
+     then this function will set the suppress flag and return a
+     gdb::optional with its value set to an object that will restore the
+     previous value of the suppress notifications flag.
+
+     If this command was created without a suppress notifications points,
+     then this function returns an empty gdb::optional.  */
+  gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
+
+  /* The name of the command.  */
+  std::string m_name;
+
+  /* Pointer to integer to set during command's invocation.  */
+  int *m_suppress_notification;
 };
 
 /* Lookup a command in the MI command table, returns nullptr if COMMAND is
    not found.  */
 
-extern struct mi_cmd *mi_cmd_lookup (const char *command);
+extern mi_cmd *mi_cmd_lookup (const char *command);
 
 /* Debug flag */
 extern int mi_debug_p;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 269c01d5176..710eef7e725 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -90,8 +90,6 @@ int mi_proceeded;
 
 static void mi_cmd_execute (struct mi_parse *parse);
 
-static void mi_execute_cli_command (const char *cmd, bool args_p,
-				    const char *args);
 static void mi_execute_async_cli_command (const char *cli_command,
 					  char **argv, int argc);
 static bool register_changed_p (int regnum, readonly_detached_regcache *,
@@ -1936,11 +1934,6 @@ mi_execute_command (const char *cmd, int from_tty)
     {
       ptid_t previous_ptid = inferior_ptid;
 
-      gdb::optional<scoped_restore_tmpl<int>> restore_suppress;
-
-      if (command->cmd != NULL && command->cmd->suppress_notification != NULL)
-	restore_suppress.emplace (command->cmd->suppress_notification, 1);
-
       command->token = token;
 
       if (do_timings)
@@ -2079,35 +2072,11 @@ mi_cmd_execute (struct mi_parse *parse)
 
   current_context = parse;
 
-  if (parse->cmd->argv_func != NULL)
-    {
-      parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
-    }
-  else if (parse->cmd->cli.cmd != 0)
-    {
-      /* FIXME: DELETE THIS. */
-      /* The operation is still implemented by a cli command.  */
-      /* Must be a synchronous one.  */
-      bool args_p = parse->cmd->cli.args_p != 0;
-      const char *args = args_p ? parse->args : nullptr;
-      mi_execute_cli_command (parse->cmd->cli.cmd, args_p, args);
-    }
-  else
-    {
-      /* FIXME: DELETE THIS.  */
-      string_file stb;
-
-      stb.puts ("Undefined mi command: ");
-      stb.putstr (parse->command, '"');
-      stb.puts (" (missing implementation)");
-
-      error_stream (stb);
-    }
+  gdb_assert (parse->cmd != nullptr);
+  parse->cmd->invoke (parse);
 }
 
-/* FIXME: This is just a hack so we can get some extra commands going.
-   We don't want to channel things through the CLI, but call libgdb directly.
-   Use only for synchronous commands.  */
+/* See mi-main.h.  */
 
 void
 mi_execute_cli_command (const char *cmd, bool args_p, const char *args)
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 7f986969450..064f1d587f2 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -54,6 +54,18 @@ struct mi_suppress_notification
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
+/* This is a hack so we can get some extra commands going, but has existed
+   within GDB for many years now.  Ideally we don't want to channel things
+   through the CLI, but implement all commands as pure MI commands with
+   their own implementation.
+
+   Execute the CLI command CMD, if ARGS_P is true then ARGS should be a
+   non-nullptr string containing arguments to add after CMD.  If ARGS_P is
+   false then ARGS must be nullptr.  */
+
+extern void mi_execute_cli_command (const char *cmd, bool args_p,
+				    const char *args);
+
 /* Implementation of -fix-multi-location-breakpoint-output.  */
 
 extern void mi_cmd_fix_multi_location_breakpoint_output (const char *command,
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index b5b01cf4db0..203024fb01c 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -106,7 +106,7 @@ mi_parse_escape (const char **string_ptr)
   return c;
 }
 
-static void
+void
 mi_parse_argv (const char *args, struct mi_parse *parse)
 {
   const char *chp = args;
@@ -363,20 +363,8 @@ mi_parse (const char *cmd, char **token)
       chp = skip_spaces (chp);
     }
 
-  /* For new argv commands, attempt to return the parsed argument
-     list.  */
-  if (parse->cmd->argv_func != NULL)
-    {
-      mi_parse_argv (chp, parse.get ());
-      if (parse->argv == NULL)
-	error (_("Problem parsing arguments: %s %s"), parse->command, chp);
-    }
-
-  /* FIXME: DELETE THIS */
-  /* For CLI commands, also return the remainder of the
-     command line as a single string. */
-  if (parse->cmd->cli.cmd != NULL)
-    parse->args = xstrdup (chp);
+  /* Save the rest of the arguments for the command.  */
+  parse->args = xstrdup (chp);
 
   /* Fully parsed, flag as an MI command.  */
   parse->op = MI_COMMAND;
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index a60ce5b3a20..ceaac72e867 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -79,4 +79,8 @@ extern std::unique_ptr<struct mi_parse> mi_parse (const char *cmd,
 
 enum print_values mi_parse_print_values (const char *name);
 
+/* Split ARGS into argc/argv and store the result in PARSE.  */
+
+extern void mi_parse_argv (const char *args, struct mi_parse *parse);
+
 #endif /* MI_MI_PARSE_H */
-- 
2.25.4

[PATCHv2 5/5] gdb/mi: rename mi_cmd to mi_command

[Andrew Burgess]

From: Jan Vrany <jan.vrany@labware.com>

Just give this class a new name, more inline with the name of the
sub-classes.  I've also updated mi_cmd_up to mi_command_up in
mi-cmds.c inline with this new naming scheme.

There should be no user visible changes after this commit.
---
 gdb/mi/mi-cmd-info.c |  2 +-
 gdb/mi/mi-cmds.c     | 34 +++++++++++++++++-----------------
 gdb/mi/mi-cmds.h     |  8 ++++----
 gdb/mi/mi-parse.h    |  2 +-
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index c2858519a4c..3bfd5918ce9 100644
--- a/gdb/mi/mi-cmd-info.c
+++ b/gdb/mi/mi-cmd-info.c
@@ -67,7 +67,7 @@ void
 mi_cmd_info_gdb_mi_command (const char *command, char **argv, int argc)
 {
   const char *cmd_name;
-  struct mi_cmd *cmd;
+  mi_command *cmd;
   struct ui_out *uiout = current_uiout;
 
   /* This command takes exactly one argument.  */
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c3e5b4d3263..304a244b923 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -28,22 +28,22 @@
 
 /* A command held in the MI_CMD_TABLE.  */
 
-typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
+typedef std::unique_ptr<struct mi_command> mi_command_up;
 
 /* MI command table (built at run time). */
 
-static std::map<std::string, mi_cmd_up> mi_cmd_table;
+static std::map<std::string, mi_command_up> mi_cmd_table;
 
 /* MI command with a pure MI implementation.  */
 
-struct mi_command_mi : public mi_cmd
+struct mi_command_mi : public mi_command
 {
-  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
+  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_command
      constructor, FUNC is the function called from do_invoke, which
      implements this MI command.  */
   mi_command_mi (const char *name, mi_cmd_argv_ftype func,
                  int *suppress_notification)
-    : mi_cmd (name, suppress_notification),
+    : mi_command (name, suppress_notification),
       m_argv_function (func)
   {
     gdb_assert (func != nullptr);
@@ -72,9 +72,9 @@ struct mi_command_mi : public mi_cmd
 
 /* MI command implemented on top of a CLI command.  */
 
-struct mi_command_cli : public mi_cmd
+struct mi_command_cli : public mi_command
 {
-  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
+  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_command
      constructor, CLI_NAME is the name of a CLI command that should be
      invoked to implement this MI command.  If ARGS_P is true then any
      arguments from entered by the user as part of the MI command line are
@@ -82,7 +82,7 @@ struct mi_command_cli : public mi_cmd
      false, nullptr is send to CLI_NAME as its argument string.  */
   mi_command_cli (const char *name, const char *cli_name, bool args_p,
                   int *suppress_notification)
-    : mi_cmd (name, suppress_notification),
+    : mi_command (name, suppress_notification),
       m_cli_name (cli_name),
       m_args_p (args_p)
   { /* Nothing.  */ }
@@ -115,7 +115,7 @@ struct mi_command_cli : public mi_cmd
    COMMAND was added to mi_cmd_table.  */
 
 static bool
-insert_mi_cmd_entry (mi_cmd_up command)
+insert_mi_cmd_entry (mi_command_up command)
 {
   gdb_assert (command != nullptr);
   gdb_assert (!command->name ().empty ());
@@ -137,8 +137,8 @@ static void
 add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
 	       int *suppress_notification = nullptr)
 {
-  mi_cmd_up command (new mi_command_mi (name, function,
-                                        suppress_notification));
+  mi_command_up command (new mi_command_mi (name, function,
+					    suppress_notification));
 
   bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
@@ -152,8 +152,8 @@ static void
 add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 		int *suppress_notification = nullptr)
 {
-  mi_cmd_up command (new mi_command_cli (name, cli_name, args_p != 0,
-                                         suppress_notification));
+  mi_command_up command (new mi_command_cli (name, cli_name, args_p != 0,
+					     suppress_notification));
 
   bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
@@ -161,7 +161,7 @@ add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 
 /* See mi-cmds.h.  */
 
-mi_cmd::mi_cmd (const char *name, int *suppress_notification)
+mi_command::mi_command (const char *name, int *suppress_notification)
   : m_name (name),
     m_suppress_notification (suppress_notification)
 { /* Nothing.  */ }
@@ -169,7 +169,7 @@ mi_cmd::mi_cmd (const char *name, int *suppress_notification)
 /* See mi-cmds.h.  */
 
 void
-mi_cmd::invoke (struct mi_parse *parse) const
+mi_command::invoke (struct mi_parse *parse) const
 {
   gdb::optional<scoped_restore_tmpl<int>> restore
     = do_suppress_notification ();
@@ -179,7 +179,7 @@ mi_cmd::invoke (struct mi_parse *parse) const
 /* See mi-cmds.h.  */
 
 gdb::optional<scoped_restore_tmpl<int>>
-mi_cmd::do_suppress_notification () const
+mi_command::do_suppress_notification () const
 {
   if (m_suppress_notification != nullptr)
     return scoped_restore_tmpl<int> (m_suppress_notification, 1);
@@ -353,7 +353,7 @@ build_table ()
 
 /* See mi-cmds.h.  */
 
-struct mi_cmd *
+mi_command *
 mi_cmd_lookup (const char *command)
 {
   gdb_assert (command != nullptr);
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index b859ff21da7..1340b5ec569 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -140,17 +140,17 @@ extern mi_cmd_argv_ftype mi_cmd_complete;
 
 /* The abstract base class for all MI command types.  */
 
-struct mi_cmd
+struct mi_command
 {
   /* Constructor.  NAME is the name of this MI command, that is the
      initial string the user will enter to run this command.  The
      SUPPRESS_NOTIFICATION pointer is a flag which will be set to 1 when
      this command is invoked, and reset to its previous value once the
      command invocation has completed.  */
-  mi_cmd (const char *name, int *suppress_notification);
+  mi_command (const char *name, int *suppress_notification);
 
   /* Destructor.  */
-  virtual ~mi_cmd () { /* Nothing.  */ }
+  virtual ~mi_command () { /* Nothing.  */ }
 
   /* Return the name of this command.  This is the command that the user
      will actually type in, without any arguments.  */
@@ -188,7 +188,7 @@ struct mi_cmd
 /* Lookup a command in the MI command table, returns nullptr if COMMAND is
    not found.  */
 
-extern mi_cmd *mi_cmd_lookup (const char *command);
+extern mi_command *mi_cmd_lookup (const char *command);
 
 /* Debug flag */
 extern int mi_debug_p;
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index ceaac72e867..44c3a49b1c0 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -49,7 +49,7 @@ struct mi_parse
     enum mi_command_type op;
     char *command;
     char *token;
-    const struct mi_cmd *cmd;
+    const struct mi_command *cmd;
     struct mi_timestamp *cmd_start;
     char *args;
     char **argv;
-- 
2.25.4

PING: [PATCHv2 0/5] refactoring towards Python MI command API

[Andrew Burgess]

Ping!

I'll give this series another week, then I'll go ahead and push it.  I
believe that Pedro has already looked through these patches in this
thread:

  https://sourceware.org/pipermail/gdb-patches/2019-May/158010.html

And I've addressed all the feedback that applies to the patches I'm
posting here.

Thanks,
Andrew

* Andrew Burgess <aburgess@redhat.com> [2021-12-03 16:29:55 +0000]:

> I originally posted these patches here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2021-November/183231.html
> 
> but as that was buried in the middle of a general discussion thread I
> was worried the series might not have been noticed.
> 
> The work in those patches is not my own, but is based on Jan's work
> which has been previously posted here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2019-May/158010.html
> 
> This series is not all of Jan's work, his series goes all of the way
> to adding a Python API for creating MI commands.  This series is just
> the refactoring ahead of the final Python changes, but I think these
> changes stand on their own, so would like to have them considered for
> acceptance.  We can then rebase the remained of Jan's work on top of
> master once this series lands.
> 
> Pedro had some feedback on Jan's V3 series, but only one of the
> feedback items applies to the work I'm submitting here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158431.html
> 
> I've gone thought that email and I believe that all of the issues
> listed have been addressed.
> 
> Changes since _my_ V1 series:
> 
>   - rebased onto current upstream master, and
> 
>   - ensured that all of Pedro's feedback has been addressed.
> 
> Any additional feedback would be welcome.
> 
> Thanks,
> Andrew
> 
> 
> ---
> 
> Andrew Burgess (1):
>   gdb/mi: int to bool conversion in mi_execute_cli_command
> 
> Jan Vrany (4):
>   gdb/mi: rename mi_lookup to mi_cmd_lookup
>   gdb/mi: use std::map for MI commands in mi-cmds.c
>   gdb/mi: use separate classes for different types of MI command
>   gdb/mi: rename mi_cmd to mi_command
> 
>  gdb/mi/mi-cmd-info.c |   4 +-
>  gdb/mi/mi-cmds.c     | 571 ++++++++++++++++++++++++-------------------
>  gdb/mi/mi-cmds.h     |  70 ++++--
>  gdb/mi/mi-main.c     |  51 +---
>  gdb/mi/mi-main.h     |  12 +
>  gdb/mi/mi-parse.c    |  20 +-
>  gdb/mi/mi-parse.h    |   6 +-
>  7 files changed, 401 insertions(+), 333 deletions(-)
> 
> -- 
> 2.25.4
> 

Re: [PATCHv2 1/5] gdb/mi: rename mi_lookup to mi_cmd_lookup

[Simon Marchi]

On 2021-12-03 11:29 a.m., Andrew Burgess via Gdb-patches wrote:
> From: Jan Vrany <jan.vrany@labware.com>
> 
> Lets give this function a more descriptive name.  I've also improved
> the comments in the header and source files.
> 
> There should be no user visible changes after this commit.

LGTM, could be pushed on its own.

Simon

Re: [PATCHv2 2/5] gdb/mi: use std::map for MI commands in mi-cmds.c

[Simon Marchi]

On 2021-12-03 11:29 a.m., Andrew Burgess via Gdb-patches wrote:
> From: Jan Vrany <jan.vrany@labware.com>
>
> This changes the hashmap used in mi-cmds.c from a custom structure to
> std::map.  Not only is replacing a custom container with a standard
> one an improvement, but using std::map will make it easier to
> dynamically add commands; which is something that is planned for a
> later series, where we will allow MI commands to be implemented in
> Python.
>
> There should be no user visible changes after this commit.

LGTM with some minor comments, no need to repost just for that.

> ---
>  gdb/mi/mi-cmds.c | 478 +++++++++++++++++++++++------------------------
>  1 file changed, 230 insertions(+), 248 deletions(-)
>
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 8899fdd3a1e..4999c9f81b3 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -22,283 +22,265 @@
>  #include "top.h"
>  #include "mi-cmds.h"
>  #include "mi-main.h"
> +#include <map>
> +#include <string>
>
> -struct mi_cmd;
> -static struct mi_cmd **lookup_table (const char *command);
> -static void build_table (struct mi_cmd *commands);
> +/* A command held in the MI_CMD_TABLE.  */
>
> -static struct mi_cmd mi_cmds[] =
> -{
> -/* Define a MI command of NAME, and its corresponding CLI command is
> -   CLI_NAME.  */
> -#define DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, CALLED)	\
> -  { NAME, { CLI_NAME, ARGS_P}, NULL, CALLED }
> -#define DEF_MI_CMD_CLI(NAME, CLI_NAME, ARGS_P) \
> -  DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, NULL)
> +typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;

Really a nit: since we're using C++11, I prefer the notation `using =
..`, I find it easier to read (since it looks like a standard
assignment, with the name on the left).

> +/* Create an mi_cmd structure with name NAME.  */
> +
> +static mi_cmd_up
> +create_mi_cmd (const char *name)
>  {
> -  return *lookup_table (command);
> +  mi_cmd_up cmd (new mi_cmd ());
> +  cmd->name = name;
> +  return cmd;
>  }

It looks like this could be a constructor of mi_cmd quite easily,
instead of being a free function.

>
> -/* Used for collecting hash hit/miss statistics.  */
> +/* Create and register a new MI command with an MI specific implementation.
> +   NAME must name an MI command that does not already exist, otherwise an
> +   assertion will trigger.  */
>
> -static struct
> +static void
> +add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
> +	       int *suppress_notification = nullptr)
>  {
> -  int hit;
> -  int miss;
> -  int rehash;
> -} stats;
> +  mi_cmd_up cmd_up = create_mi_cmd (name);
> +
> +  cmd_up->cli.cmd = nullptr;
> +  cmd_up->cli.args_p = 0;
> +  cmd_up->argv_func = function;
> +  cmd_up->suppress_notification = suppress_notification;

... and these could also be initialized in the mi_cmd constructor.  Up
to you to see if you want to make that change, it can also be changed
later.

>
> -/* Look up a command.  */
> +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> +  gdb_assert (success);
> +}
> +
> +/* Create and register a new MI command implemented on top of a CLI
> +   command.  NAME must name an MI command that does not already exist,
> +   otherwise an assertion will trigger.  */
>
> -static struct mi_cmd **
> -lookup_table (const char *command)
> +static void
> +add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
> +		int *suppress_notification = nullptr)
>  {
> -  const char *chp;
> -  unsigned int index = 0;
> +  mi_cmd_up cmd_up = create_mi_cmd (name);
>
> -  /* Compute our hash.  */
> -  for (chp = command; *chp; chp++)
> -    {
> -      /* We use a somewhat arbitrary formula.  */
> -      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
> -    }
> +  cmd_up->cli.cmd = cli_name;
> +  cmd_up->cli.args_p = args_p;
> +  cmd_up->argv_func = nullptr;
> +  cmd_up->suppress_notification = suppress_notification;
>
> -  while (1)
> -    {
> -      struct mi_cmd **entry = &mi_table[index];
> -      if ((*entry) == 0)
> -	{
> -	  /* not found, return pointer to next free. */
> -	  stats.miss++;
> -	  return entry;
> -	}
> -      if (strcmp (command, (*entry)->name) == 0)
> -	{
> -	  stats.hit++;
> -	  return entry;		/* found */
> -	}
> -      index = (index + 1) % MI_TABLE_SIZE;
> -      stats.rehash++;
> -    }
> +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> +  gdb_assert (success);
>  }
>
> +/* Initialize MI_CMD_TABLE, the global map of MI commands.  */
> +
>  static void
> -build_table (struct mi_cmd *commands)
> +build_table ()

Suggest renaming this to something like "add_builtin_mi_cmds", it would
be more descriptive.

Simon

Re: [PATCHv2 3/5] gdb/mi: int to bool conversion in mi_execute_cli_command

[Simon Marchi]

>  void
> -mi_execute_cli_command (const char *cmd, int args_p, const char *args)
> +mi_execute_cli_command (const char *cmd, bool args_p, const char *args)
>  {
> -  if (cmd != 0)
> +  if (cmd != nullptr)
>      {
> -      std::string run = cmd;
> +      std::string run (cmd);
>
>        if (args_p)
>  	run = run + " " + args;
> +      else
> +	gdb_assert (args == nullptr);

Alternatively, args could be changed to gdb::optional<const char *>.
LGTM in any case, and this can be pushed on its own I think.

Simon

Re: [PATCHv2 4/5] gdb/mi: use separate classes for different types of MI command

[Simon Marchi]

> +/* MI command implemented on top of a CLI command.  */
> +
> +struct mi_command_cli : public mi_cmd
> +{
> +  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
> +     constructor, CLI_NAME is the name of a CLI command that should be
> +     invoked to implement this MI command.  If ARGS_P is true then any
> +     arguments from entered by the user as part of the MI command line are
> +     forwarded to CLI_NAME as its argument string, otherwise, if ARGS_P is
> +     false, nullptr is send to CLI_NAME as its argument string.  */
> +  mi_command_cli (const char *name, const char *cli_name, bool args_p,
> +                  int *suppress_notification)
> +    : mi_cmd (name, suppress_notification),
> +      m_cli_name (cli_name),
> +      m_args_p (args_p)
> +  { /* Nothing.  */ }
> +
> +protected:
> +
> +  /* Called when this MI command has been invoked, calls the m_cli_name
> +     CLI function.  In m_args_p is true then the argument string from
> +     within PARSE is passed through to the CLI function, otherwise nullptr
> +     is passed through to the CLI function as its argument string.  */
> +  void do_invoke (struct mi_parse *parse) const override
> +  {
> +    const char *args = m_args_p ? parse->args : nullptr;
> +    mi_execute_cli_command (m_cli_name.c_str (), m_args_p ? 1 : 0,

Following the bool conversion, I think you could pass m_args_p directly.

> +			    args);
> +  }
> +
> +private:
> +
> +  /* The name of the CLI command to execute.  */
> +  std::string m_cli_name;

All CLI command names used here are statically allocated, so I think
this could be a `const char *`.

> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index e92737be8c8..b859ff21da7 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -22,6 +22,7 @@
>  #ifndef MI_MI_CMDS_H
>  #define MI_MI_CMDS_H
>
> +#include "gdbsupport/gdb_optional.h"
>  enum print_values {
>     PRINT_NO_VALUES,
>     PRINT_ALL_VALUES,

Empty line after the include.

> @@ -137,37 +138,57 @@ extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
>  extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
>  extern mi_cmd_argv_ftype mi_cmd_complete;
>
> -/* Description of a single command.  */
> -
> -struct mi_cli
> -{
> -  /* Corresponding CLI command.  If ARGS_P is non-zero, the MI
> -     command's argument list is appended to the CLI command.  */
> -  const char *cmd;
> -  int args_p;
> -};
> +/* The abstract base class for all MI command types.  */
>
>  struct mi_cmd
>  {
> -  /* Official name of the command.  */
> -  const char *name;
> -  /* The corresponding CLI command that can be used to implement this
> -     MI command (if cli.lhs is non NULL).  */
> -  struct mi_cli cli;
> -  /* If non-null, the function implementing the MI command.  */
> -  mi_cmd_argv_ftype *argv_func;
> -  /* If non-null, the pointer to a field in
> -     'struct mi_suppress_notification', which will be set to true by MI
> -     command processor (mi-main.c:mi_cmd_execute) when this command is
> -     being executed.  It will be set back to false when command has been
> -     executed.  */
> -  int *suppress_notification;
> +  /* Constructor.  NAME is the name of this MI command, that is the
> +     initial string the user will enter to run this command.  The
> +     SUPPRESS_NOTIFICATION pointer is a flag which will be set to 1 when
> +     this command is invoked, and reset to its previous value once the
> +     command invocation has completed.  */
> +  mi_cmd (const char *name, int *suppress_notification);

Just a precision, does "name" include include the leading dash?

> +
> +  /* Destructor.  */
> +  virtual ~mi_cmd () { /* Nothing.  */ }

 = default ?

> +
> +  /* Return the name of this command.  This is the command that the user
> +     will actually type in, without any arguments.  */
> +  const std::string &name () const
> +  { return m_name; }
> +
> +  /* Execute the MI command.  Can throw an exception if something goes
> +     wrong.  */
> +  void invoke (struct mi_parse *parse) const;
> +
> +protected:
> +
> +  /* The core of command invocation, this needs to be overridden in each
> +     base class.  PARSE is the parsed command line from the user.  */
> +  virtual void do_invoke (struct mi_parse *parse) const = 0;
> +
> +private:
> +
> +  /* If this command was created with a suppress notifications pointer,
> +     then this function will set the suppress flag and return a
> +     gdb::optional with its value set to an object that will restore the
> +     previous value of the suppress notifications flag.
> +
> +     If this command was created without a suppress notifications points,
> +     then this function returns an empty gdb::optional.  */
> +  gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
> +
> +  /* The name of the command.  */
> +  std::string m_name;

Built-in commands don't really need an allocated std::string, their name
is statically allocated.  So we could keep the name as a "const char *"
here.  Commands implemented in Python have a dynamically allocated name,
so they would keep an  std::string or gdb::unique_xmalloc_ptr<char> in
that derived class, for storage.  So they would end up using a bit more
space, but we would use less for built-in commands.  Just a suggestion,
not a blocker.

Simon

Re: [PATCHv2 5/5] gdb/mi: rename mi_cmd to mi_command

[Simon Marchi]

On 2021-12-03 11:30 a.m., Andrew Burgess via Gdb-patches wrote:
> From: Jan Vrany <jan.vrany@labware.com>
> 
> Just give this class a new name, more inline with the name of the
> sub-classes.  I've also updated mi_cmd_up to mi_command_up in
> mi-cmds.c inline with this new naming scheme.
> 
> There should be no user visible changes after this commit.

LGTM, thanks.

Simon

Re: [PATCHv2 2/5] gdb/mi: use std::map for MI commands in mi-cmds.c

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-12-13 10:51:02 -0500]:

> On 2021-12-03 11:29 a.m., Andrew Burgess via Gdb-patches wrote:
> > From: Jan Vrany <jan.vrany@labware.com>
> >
> > This changes the hashmap used in mi-cmds.c from a custom structure to
> > std::map.  Not only is replacing a custom container with a standard
> > one an improvement, but using std::map will make it easier to
> > dynamically add commands; which is something that is planned for a
> > later series, where we will allow MI commands to be implemented in
> > Python.
> >
> > There should be no user visible changes after this commit.
> 
> LGTM with some minor comments, no need to repost just for that.
> 
> > ---
> >  gdb/mi/mi-cmds.c | 478 +++++++++++++++++++++++------------------------
> >  1 file changed, 230 insertions(+), 248 deletions(-)
> >
> > diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> > index 8899fdd3a1e..4999c9f81b3 100644
> > --- a/gdb/mi/mi-cmds.c
> > +++ b/gdb/mi/mi-cmds.c
> > @@ -22,283 +22,265 @@
> >  #include "top.h"
> >  #include "mi-cmds.h"
> >  #include "mi-main.h"
> > +#include <map>
> > +#include <string>
> >
> > -struct mi_cmd;
> > -static struct mi_cmd **lookup_table (const char *command);
> > -static void build_table (struct mi_cmd *commands);
> > +/* A command held in the MI_CMD_TABLE.  */
> >
> > -static struct mi_cmd mi_cmds[] =
> > -{
> > -/* Define a MI command of NAME, and its corresponding CLI command is
> > -   CLI_NAME.  */
> > -#define DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, CALLED)	\
> > -  { NAME, { CLI_NAME, ARGS_P}, NULL, CALLED }
> > -#define DEF_MI_CMD_CLI(NAME, CLI_NAME, ARGS_P) \
> > -  DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, NULL)
> > +typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
> 
> Really a nit: since we're using C++11, I prefer the notation `using =
> ..`, I find it easier to read (since it looks like a standard
> assignment, with the name on the left).

I've made this change.

> 
> > +/* Create an mi_cmd structure with name NAME.  */
> > +
> > +static mi_cmd_up
> > +create_mi_cmd (const char *name)
> >  {
> > -  return *lookup_table (command);
> > +  mi_cmd_up cmd (new mi_cmd ());
> > +  cmd->name = name;
> > +  return cmd;
> >  }
> 
> It looks like this could be a constructor of mi_cmd quite easily,
> instead of being a free function.

I'm leaving this comment just for the record, I'm sure you saw this
when you reviewed these later patches...

I've not made any of the changes you suggested for moving code into
the constructor within this patch.  This code is all refactored as
part of a later patch in this series, with the result that all
initialisation is moved into the constructor(s) then.

> 
> >
> > -/* Used for collecting hash hit/miss statistics.  */
> > +/* Create and register a new MI command with an MI specific implementation.
> > +   NAME must name an MI command that does not already exist, otherwise an
> > +   assertion will trigger.  */
> >
> > -static struct
> > +static void
> > +add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
> > +	       int *suppress_notification = nullptr)
> >  {
> > -  int hit;
> > -  int miss;
> > -  int rehash;
> > -} stats;
> > +  mi_cmd_up cmd_up = create_mi_cmd (name);
> > +
> > +  cmd_up->cli.cmd = nullptr;
> > +  cmd_up->cli.args_p = 0;
> > +  cmd_up->argv_func = function;
> > +  cmd_up->suppress_notification = suppress_notification;
> 
> ... and these could also be initialized in the mi_cmd constructor.  Up
> to you to see if you want to make that change, it can also be changed
> later.

As above.


> 
> >
> > -/* Look up a command.  */
> > +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> > +  gdb_assert (success);
> > +}
> > +
> > +/* Create and register a new MI command implemented on top of a CLI
> > +   command.  NAME must name an MI command that does not already exist,
> > +   otherwise an assertion will trigger.  */
> >
> > -static struct mi_cmd **
> > -lookup_table (const char *command)
> > +static void
> > +add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
> > +		int *suppress_notification = nullptr)
> >  {
> > -  const char *chp;
> > -  unsigned int index = 0;
> > +  mi_cmd_up cmd_up = create_mi_cmd (name);
> >
> > -  /* Compute our hash.  */
> > -  for (chp = command; *chp; chp++)
> > -    {
> > -      /* We use a somewhat arbitrary formula.  */
> > -      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
> > -    }
> > +  cmd_up->cli.cmd = cli_name;
> > +  cmd_up->cli.args_p = args_p;
> > +  cmd_up->argv_func = nullptr;
> > +  cmd_up->suppress_notification = suppress_notification;
> >
> > -  while (1)
> > -    {
> > -      struct mi_cmd **entry = &mi_table[index];
> > -      if ((*entry) == 0)
> > -	{
> > -	  /* not found, return pointer to next free. */
> > -	  stats.miss++;
> > -	  return entry;
> > -	}
> > -      if (strcmp (command, (*entry)->name) == 0)
> > -	{
> > -	  stats.hit++;
> > -	  return entry;		/* found */
> > -	}
> > -      index = (index + 1) % MI_TABLE_SIZE;
> > -      stats.rehash++;
> > -    }
> > +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> > +  gdb_assert (success);
> >  }
> >
> > +/* Initialize MI_CMD_TABLE, the global map of MI commands.  */
> > +
> >  static void
> > -build_table (struct mi_cmd *commands)
> > +build_table ()
> 
> Suggest renaming this to something like "add_builtin_mi_cmds", it would
> be more descriptive.

I've done this as an extra patch, which applies at the end of this
series, I've gone ahead and pushed this new patch.  The patch is
included below, for the record.

Thanks,
Andrew

---

commit 78d4da9ae0d3447f28274a00b278f58ca7d8d1b2
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Dec 14 11:43:34 2021 +0000

    gdb/mi: rename build_table to add_builtin_mi_commands
    
    Just give the function build_table a more descriptive name.  There
    should be no user visible changes after this commit.

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 9c11db00b3a..d02e4b20562 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -190,7 +190,7 @@ mi_command::do_suppress_notification () const
 /* Initialize the available MI commands.  */
 
 static void
-build_table ()
+add_builtin_mi_commands ()
 {
   add_mi_cmd_mi ("ada-task-info", mi_cmd_ada_task_info);
   add_mi_cmd_mi ("add-inferior", mi_cmd_add_inferior);
@@ -368,5 +368,5 @@ void _initialize_mi_cmds ();
 void
 _initialize_mi_cmds ()
 {
-  build_table ();
+  add_builtin_mi_commands ();
 }

Re: [PATCHv2 3/5] gdb/mi: int to bool conversion in mi_execute_cli_command

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-12-13 10:54:44 -0500]:

> >  void
> > -mi_execute_cli_command (const char *cmd, int args_p, const char *args)
> > +mi_execute_cli_command (const char *cmd, bool args_p, const char *args)
> >  {
> > -  if (cmd != 0)
> > +  if (cmd != nullptr)
> >      {
> > -      std::string run = cmd;
> > +      std::string run (cmd);
> >
> >        if (args_p)
> >  	run = run + " " + args;
> > +      else
> > +	gdb_assert (args == nullptr);
> 
> Alternatively, args could be changed to gdb::optional<const char *>.
> LGTM in any case, and this can be pushed on its own I think.

I haven't made this change.  I did implement it, but it didn't seem to
save any code, nor make the code any more readable, so in the end I
dropped the change.

If you feel strongly, and wanted to make the change yourself, I
wouldn't try to stop it being merged though.

Thanks,
Andrew

Re: [PATCHv2 4/5] gdb/mi: use separate classes for different types of MI command

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-12-13 11:18:27 -0500]:

> > +/* MI command implemented on top of a CLI command.  */
> > +
> > +struct mi_command_cli : public mi_cmd
> > +{
> > +  /* Constructor.  For NAME and SUPPRESS_NOTIFICATION see mi_cmd
> > +     constructor, CLI_NAME is the name of a CLI command that should be
> > +     invoked to implement this MI command.  If ARGS_P is true then any
> > +     arguments from entered by the user as part of the MI command line are
> > +     forwarded to CLI_NAME as its argument string, otherwise, if ARGS_P is
> > +     false, nullptr is send to CLI_NAME as its argument string.  */
> > +  mi_command_cli (const char *name, const char *cli_name, bool args_p,
> > +                  int *suppress_notification)
> > +    : mi_cmd (name, suppress_notification),
> > +      m_cli_name (cli_name),
> > +      m_args_p (args_p)
> > +  { /* Nothing.  */ }
> > +
> > +protected:
> > +
> > +  /* Called when this MI command has been invoked, calls the m_cli_name
> > +     CLI function.  In m_args_p is true then the argument string from
> > +     within PARSE is passed through to the CLI function, otherwise nullptr
> > +     is passed through to the CLI function as its argument string.  */
> > +  void do_invoke (struct mi_parse *parse) const override
> > +  {
> > +    const char *args = m_args_p ? parse->args : nullptr;
> > +    mi_execute_cli_command (m_cli_name.c_str (), m_args_p ? 1 : 0,
> 
> Following the bool conversion, I think you could pass m_args_p irectly.

Good spot!  I made this change.

> 
> > +			    args);
> > +  }
> > +
> > +private:
> > +
> > +  /* The name of the CLI command to execute.  */
> > +  std::string m_cli_name;
> 
> All CLI command names used here are statically allocated, so I think
> this could be a `const char *`.

Thanks, I made this change.

> 
> > diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> > index e92737be8c8..b859ff21da7 100644
> > --- a/gdb/mi/mi-cmds.h
> > +++ b/gdb/mi/mi-cmds.h
> > @@ -22,6 +22,7 @@
> >  #ifndef MI_MI_CMDS_H
> >  #define MI_MI_CMDS_H
> >
> > +#include "gdbsupport/gdb_optional.h"
> >  enum print_values {
> >     PRINT_NO_VALUES,
> >     PRINT_ALL_VALUES,
> 
> Empty line after the include.

Fixed.


> 
> > @@ -137,37 +138,57 @@ extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
> >  extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
> >  extern mi_cmd_argv_ftype mi_cmd_complete;
> >
> > -/* Description of a single command.  */
> > -
> > -struct mi_cli
> > -{
> > -  /* Corresponding CLI command.  If ARGS_P is non-zero, the MI
> > -     command's argument list is appended to the CLI command.  */
> > -  const char *cmd;
> > -  int args_p;
> > -};
> > +/* The abstract base class for all MI command types.  */
> >
> >  struct mi_cmd
> >  {
> > -  /* Official name of the command.  */
> > -  const char *name;
> > -  /* The corresponding CLI command that can be used to implement this
> > -     MI command (if cli.lhs is non NULL).  */
> > -  struct mi_cli cli;
> > -  /* If non-null, the function implementing the MI command.  */
> > -  mi_cmd_argv_ftype *argv_func;
> > -  /* If non-null, the pointer to a field in
> > -     'struct mi_suppress_notification', which will be set to true by MI
> > -     command processor (mi-main.c:mi_cmd_execute) when this command is
> > -     being executed.  It will be set back to false when command has been
> > -     executed.  */
> > -  int *suppress_notification;
> > +  /* Constructor.  NAME is the name of this MI command, that is the
> > +     initial string the user will enter to run this command.  The
> > +     SUPPRESS_NOTIFICATION pointer is a flag which will be set to 1 when
> > +     this command is invoked, and reset to its previous value once the
> > +     command invocation has completed.  */
> > +  mi_cmd (const char *name, int *suppress_notification);
> 
> Just a precision, does "name" include include the leading dash?
> 
> > +
> > +  /* Destructor.  */
> > +  virtual ~mi_cmd () { /* Nothing.  */ }
> 
>  = default ?
> 
> > +
> > +  /* Return the name of this command.  This is the command that the user
> > +     will actually type in, without any arguments.  */
> > +  const std::string &name () const
> > +  { return m_name; }
> > +
> > +  /* Execute the MI command.  Can throw an exception if something goes
> > +     wrong.  */
> > +  void invoke (struct mi_parse *parse) const;
> > +
> > +protected:
> > +
> > +  /* The core of command invocation, this needs to be overridden in each
> > +     base class.  PARSE is the parsed command line from the user.  */
> > +  virtual void do_invoke (struct mi_parse *parse) const = 0;
> > +
> > +private:
> > +
> > +  /* If this command was created with a suppress notifications pointer,
> > +     then this function will set the suppress flag and return a
> > +     gdb::optional with its value set to an object that will restore the
> > +     previous value of the suppress notifications flag.
> > +
> > +     If this command was created without a suppress notifications points,
> > +     then this function returns an empty gdb::optional.  */
> > +  gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
> > +
> > +  /* The name of the command.  */
> > +  std::string m_name;
> 
> Built-in commands don't really need an allocated std::string, their name
> is statically allocated.  So we could keep the name as a "const char *"
> here.  Commands implemented in Python have a dynamically allocated name,
> so they would keep an  std::string or gdb::unique_xmalloc_ptr<char> in
> that derived class, for storage.  So they would end up using a bit more
> space, but we would use less for built-in commands.  Just a suggestion,
> not a blocker.

Thanks, I made this change.

Andrew

Re: [PATCHv2 0/5] refactoring towards Python MI command API

[Andrew Burgess]

I updated with the feedback from Simon, and pushed this series.

Thanks,
Andrew


* Andrew Burgess <aburgess@redhat.com> [2021-12-03 16:29:55 +0000]:

> I originally posted these patches here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2021-November/183231.html
> 
> but as that was buried in the middle of a general discussion thread I
> was worried the series might not have been noticed.
> 
> The work in those patches is not my own, but is based on Jan's work
> which has been previously posted here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2019-May/158010.html
> 
> This series is not all of Jan's work, his series goes all of the way
> to adding a Python API for creating MI commands.  This series is just
> the refactoring ahead of the final Python changes, but I think these
> changes stand on their own, so would like to have them considered for
> acceptance.  We can then rebase the remained of Jan's work on top of
> master once this series lands.
> 
> Pedro had some feedback on Jan's V3 series, but only one of the
> feedback items applies to the work I'm submitting here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2019-June/158431.html
> 
> I've gone thought that email and I believe that all of the issues
> listed have been addressed.
> 
> Changes since _my_ V1 series:
> 
>   - rebased onto current upstream master, and
> 
>   - ensured that all of Pedro's feedback has been addressed.
> 
> Any additional feedback would be welcome.
> 
> Thanks,
> Andrew
> 
> 
> ---
> 
> Andrew Burgess (1):
>   gdb/mi: int to bool conversion in mi_execute_cli_command
> 
> Jan Vrany (4):
>   gdb/mi: rename mi_lookup to mi_cmd_lookup
>   gdb/mi: use std::map for MI commands in mi-cmds.c
>   gdb/mi: use separate classes for different types of MI command
>   gdb/mi: rename mi_cmd to mi_command
> 
>  gdb/mi/mi-cmd-info.c |   4 +-
>  gdb/mi/mi-cmds.c     | 571 ++++++++++++++++++++++++-------------------
>  gdb/mi/mi-cmds.h     |  70 ++++--
>  gdb/mi/mi-main.c     |  51 +---
>  gdb/mi/mi-main.h     |  12 +
>  gdb/mi/mi-parse.c    |  20 +-
>  gdb/mi/mi-parse.h    |   6 +-
>  7 files changed, 401 insertions(+), 333 deletions(-)
> 
> -- 
> 2.25.4
>