[RFC 0/8] Create MI commands using python

[RFC 0/8] Create MI commands using python

[Jan Vrany]

This patch series adds a possibility to create new MI commands using python.

The code is based on a few year old attempt of Didier Nadeau who did the
heavy lifting. I merely updated his original code to work with today's GDB,
add tests and polished it a little.

At this point, there's no documentation. I expect a discussion and changes
in behavior and/or output - I'll write it once the rest is agreed on.

Didier Nadeau (3):
  Use std::map for MI commands in mi-cmds.c
  Use classes to represent MI Command instead of structures
  Create MI commands using python.

Jan Vrany (5):
  mi/python: C++ify python MI command handling code
  mi/python: Polish MI output of python commands
  mi/python: Handle python exception when executiong python-defined MI
    commands
  mi/python: Add tests for python-defined MI commands
  mi/python: Allow redefinition of python MI commands

 gdb/ChangeLog                           |  31 ++
 gdb/Makefile.in                         |   1 +
 gdb/mi/mi-cmd-info.c                    |   4 +-
 gdb/mi/mi-cmds.c                        | 537 +++++++++++++-----------
 gdb/mi/mi-cmds.h                        |  94 ++++-
 gdb/mi/mi-main.c                        |  18 +-
 gdb/mi/mi-main.h                        |   1 +
 gdb/mi/mi-parse.c                       |  20 +-
 gdb/mi/mi-parse.h                       |   6 +-
 gdb/python/py-micmd.c                   | 300 +++++++++++++
 gdb/python/py-micmd.h                   |   6 +
 gdb/python/python-internal.h            |   2 +
 gdb/python/python.c                     |   3 +-
 gdb/testsuite/ChangeLog                 |  11 +
 gdb/testsuite/gdb.python/py-mi-cmd-1.py |  27 ++
 gdb/testsuite/gdb.python/py-mi-cmd-2.py |  13 +
 gdb/testsuite/gdb.python/py-mi-cmd.exp  |  77 ++++
 17 files changed, 851 insertions(+), 300 deletions(-)
 create mode 100644 gdb/python/py-micmd.c
 create mode 100644 gdb/python/py-micmd.h
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd-1.py
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd-2.py
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.exp

-- 
2.20.1

[RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This changes the hashmap used in mi-cmds.c from a custom
structure to std::map. This is done to be able to add new
MI commands at runtime, the previous structure had a fixed
size array.

gdb/ChangeLog:
2017-02-02  Didier Nadeau  <didier.nadeau@gmail.com>

	* mi/mi-cmds.h (mi_cmd_up): New typedef.
	(mi_lookup): Rename to ...
	(mi_cmd_lookup): this.
	* mi/mi-cmds.c (_initialize_mi_cmds): Remove declaration.
	(lookup_table): Remove declaration.
	(build_table): Remove declaration.
	(mi_table): Rename to ...
	(mi_cmd_table): this and change to std::map.
	(insert_mi_cmd_entry): New function.
	(DEF_MI_CMD_CLI_1, DEF_MI_CMD_CLI, DEF_MI_CMD_MI,
	DEF_MI_CMD_MI_1): Remove macros.
	(mi_cmds): Remove array.
	(mi_lookup): Rename to ...
	(mi_cmd_lookup): this.
	(create_mi_cmd): New function.
	(struct mi_cmd_stats): Remove.
	(stats): Remove.
	(add_mi_cmd_cli, add_mi_cmd_mi): New functions.
	(lookup_table): Remove.
	(build_table): Remove parameter.
	* mi/mi-parse.c (mi_parse): Change call from mi_lookup to
	mi_cmd_lookup.
	* mi/mi-cmd-info.c (mi_cmd_info_gdb_mi_command): Change call
	from mi_lookup to mi_cmd_lookup.
---
 gdb/mi/mi-cmd-info.c |   2 +-
 gdb/mi/mi-cmds.c     | 462 ++++++++++++++++++++-----------------------
 gdb/mi/mi-cmds.h     |   4 +-
 gdb/mi/mi-parse.c    |   2 +-
 4 files changed, 224 insertions(+), 246 deletions(-)

diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index 39da6c489d..8645fac294 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 fe30ac2e82..2b3cf297cf 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -22,267 +22,243 @@
 #include "top.h"
 #include "mi-cmds.h"
 #include "mi-main.h"
+#include <map>
+#include <string>
+#include <memory>
 
-struct mi_cmd;
-static struct mi_cmd **lookup_table (const char *command);
-static void build_table (struct mi_cmd *commands);
+/* MI command table (built at run time). */
 
-static struct mi_cmd mi_cmds[] =
+static std::map<std::string, mi_cmd_up> mi_cmd_table;
+
+/* Insert a new mi-command into the command table.  Return true if
+   insertion was successful.  */
+
+static bool
+insert_mi_cmd_entry (mi_cmd_up command)
 {
-/* 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)
-
-/* 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)
-
-  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_CLI_1 ("break-condition","cond", 1,
-		  &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 ("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_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, }
-};
-
-/* Pointer to the mi command table (built at run time). */
-
-static struct mi_cmd **mi_table;
-
-/* A prime large enough to accomodate the entire command table.  */
-enum
-  {
-    MI_TABLE_SIZE = 227
-  };
-
-/* Exported function used to obtain info from the table.  */
-struct mi_cmd *
-mi_lookup (const char *command)
+  gdb_assert (command != NULL);
+  gdb_assert (command->name != NULL);
+
+  std::string name (command->name);
+
+  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
+    return false;
+
+  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 a pure MI implementation.  */
 
-struct mi_cmd_stats
+static void
+add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
+	       int *suppress_notification = NULL)
 {
-  int hit;
-  int miss;
-  int rehash;
-};
-struct mi_cmd_stats stats;
+  mi_cmd_up cmd_up = create_mi_cmd (name);
 
-/* Look up a command.  */
+  cmd_up->cli.cmd = NULL;
+  cmd_up->cli.args_p = 0;
+  cmd_up->argv_func = function;
+  cmd_up->suppress_notification = suppress_notification;
 
-static struct mi_cmd **
-lookup_table (const char *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.  */
+
+static void
+add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
+		int *suppress_notification = NULL)
 {
-  const char *chp;
-  unsigned int index = 0;
-
-  /* Compute our hash.  */
-  for (chp = command; *chp; chp++)
-    {
-      /* We use a somewhat arbitrary formula.  */
-      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
-    }
-
-  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++;
-    }
+  mi_cmd_up cmd_up = create_mi_cmd (name);
+
+  cmd_up->cli.cmd = cli_name;
+  cmd_up->cli.args_p = args_p;
+  cmd_up->argv_func = NULL;
+  cmd_up->suppress_notification = suppress_notification;
+
+  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  gdb_assert (success);
 }
 
 static void
-build_table (struct mi_cmd *commands)
+build_table ()
+{
+  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_cli ("break-condition","cond", 1,
+		  &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 ("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_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);
+}
+
+/* See mi-cmds.h.  */
+
+struct mi_cmd *
+mi_cmd_lookup (const char *command)
 {
-  int nr_rehash = 0;
-  int nr_entries = 0;
-  struct mi_cmd *command;
-
-  mi_table = XCNEWVEC (struct mi_cmd *, MI_TABLE_SIZE);
-  for (command = commands; command->name != 0; command++)
-    {
-      struct mi_cmd **entry = lookup_table (command->name);
-
-      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);
-    }
+  gdb_assert (command != NULL);
+
+  auto it = mi_cmd_table.find (command);
+
+  if (it == mi_cmd_table.end ())
+    return NULL;
+
+  return it->second.get ();
 }
 
 void
 _initialize_mi_cmds (void)
 {
-  build_table (mi_cmds);
-  memset (&stats, 0, sizeof (stats));
+  build_table ();
 }
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 7b22ce7813..97e1be819f 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -153,9 +153,11 @@ struct mi_cmd
   int *suppress_notification;
 };
 
+typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
+
 /* Lookup a command in the MI command table.  */
 
-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 cc6a4419d0..6c8d922d66 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.20.1

Re: [RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> From: Didier Nadeau <didier.nadeau@gmail.com>
Jan> This changes the hashmap used in mi-cmds.c from a custom
Jan> structure to std::map. This is done to be able to add new
Jan> MI commands at runtime, the previous structure had a fixed
Jan> size array.

Thanks for doing this.

Jan> +static std::map<std::string, mi_cmd_up> mi_cmd_table;

My main concern about this approach is the lifetime of the mi_cmd.

If running an MI command can change this table, then using a unique_ptr
is dangerous, because this code hands out raw pointers which might
become invalid.

If that can't happen, then I guess that's fine.  Otherwise it seems like
shared_ptr is a better choice, provided mi_cmd_lookup also returns
shared_ptr.

What do you think?

Jan> +  std::string name (command->name);
Jan> +
Jan> +  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
Jan> +    return false;
Jan> +
Jan> +  mi_cmd_table[name] = std::move (command);

Can gdb::string_view be used instead?
That would avoid an allocation, and the code is already assuming that
the strings live long enough.

Tom

Re: [RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Oh, I see now, sorry about that:

Jan> +  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
Jan> +    return false;

This disallows redefining a command, so it seems fine to me.

Tom

Re: [RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Jan Vrany]

On Thu, 2019-04-25 at 13:15 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> 
> Oh, I see now, sorry about that:
> 
> Jan> +  if (mi_cmd_table.fiund (name) != mi_cmd_table.end ())
> Jan> +    return false;
> 
> This disallows redefining a command, so it seems fine to me.

Actually, you were right! I quickly realized that it is useful
to refedine python MI coomands - mostly during development of 
commands itself. So last patch in a series allows that: 

@@ -43,7 +43,8 @@ insert_mi_cmd_entry (mi_cmd_up command)
   const std::string &name = command->name ();
 
   if (mi_cmd_table.find (name) != mi_cmd_table.end ())
-    return false;
+    if (! mi_cmd_table[name]->is_py_command ())
+      return false;
 
   mi_cmd_table[name] = std::move (command);

Thanks! 

> 
> Tom

Re: [RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Simon Marchi]

On 2019-04-18 11:23 a.m., Jan Vrany wrote:
> @@ -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 fe30ac2e82..2b3cf297cf 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -22,267 +22,243 @@
>  #include "top.h"
>  #include "mi-cmds.h"
>  #include "mi-main.h"
> +#include <map>
> +#include <string>
> +#include <memory>

Is there something that uses <memory>?

> +/* 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;
>  }

create_mi_cmd could be a constructor to mi_cmd instead.  In fact, there could be
two constructors of mi_cmd for each version (the mi-based MI commands and the
cli-based MI commands), each taking the appropriate parameters.

Simon

Re: [RFC 1/8] Use std::map for MI commands in mi-cmds.c

[Simon Marchi]

On 2019-05-03 6:40 p.m., Simon Marchi wrote:
> create_mi_cmd could be a constructor to mi_cmd instead.  In fact, there could be
> two constructors of mi_cmd for each version (the mi-based MI commands and the
> cli-based MI commands), each taking the appropriate parameters.

Woops, I just saw that this is done in patch 2, sorry about the noise :)

Simon

[RFC 4/8] mi/python: C++ify python MI command handling code

[Jan Vrany]

Use gdbpy_ref<> instead of manually calling Py_DECREF().
Use gdb::unique_xmalloc_ptr<> instead of manually calling xfree().

gdb/Changelog:

	* python/py-micmd.c (parse_mi_result): Use gdb::unique_xmalloc_ptr<>
	instead of manually calling xfree().
	(py_mi_invoke): Use gdbpy_ref<> instead of PyObject * with Py_DECREF().
	Do not call Py_DECREF() on individual strings in python argument list
	object as PyList_SetItem() steals reference.
	(micmdpy_parse_command_name): Fix formatting.
	(micmdpy_init): Use gdb::unique_xmalloc_ptr<> instead of manually
	calling xfree().
---
 gdb/ChangeLog         | 11 ++++++++++
 gdb/python/py-micmd.c | 50 ++++++++++++++++---------------------------
 2 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ef77fdbb5c..692b937fee 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2018-12-11  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* python/py-micmd.c (parse_mi_result): Use gdb::unique_xmalloc_ptr<>
+	instead of manually calling xfree().
+	(py_mi_invoke): Use gdbpy_ref<> instead of PyObject * with Py_DECREF().
+	Do not call Py_DECREF() on individual strings in python argument list
+	object as PyList_SetItem() steals reference.
+	(micmdpy_parse_command_name): Fix formatting.
+	(micmdpy_init): Use gdb::unique_xmalloc_ptr<> instead of manually
+	calling xfree().
+
 2019-04-17  Tom Tromey  <tromey@adacore.com>
 
 	* dwarf2read.c (dwarf2_init_complex_target_type): Check "tt"
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index ee612e2bc5..0683a02017 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -53,11 +53,9 @@ parse_mi_result (PyObject *result, const char *field_name)
       ui_out_emit_list list_emitter (uiout, field_name);
       for(i = 0; i < PyList_GET_SIZE (result); ++i)
 	{
-	  //struct cleanup *cleanup_item = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
           ui_out_emit_tuple tuple_emitter (uiout, NULL);
 	  item = PyList_GetItem (result, i);
 	  parse_mi_result (item, NULL);
-	  //do_cleanups (cleanup_item);
 	}      
     }
   else if ( PyDict_Check (result))
@@ -66,9 +64,8 @@ parse_mi_result (PyObject *result, const char *field_name)
       Py_ssize_t pos = 0;
       while ( PyDict_Next (result, &pos, &key, &value) )
 	{
-	  char *key_string = gdbpy_obj_to_string (key).release ();
-	  parse_mi_result (value, key_string);
-	  xfree ( (void *) key_string);
+	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));
+	  parse_mi_result (value, key_string.get ());
 	}
     }
 }
@@ -79,7 +76,6 @@ void
 py_mi_invoke (void *py_obj, char **argv, int argc)
 {
   micmdpy_object *obj = (micmdpy_object *) py_obj;
-  PyObject *argobj, *result,  **strings;
   int i;
 
   gdbpy_enter enter_py (get_current_arch (), current_language);
@@ -92,9 +88,8 @@ py_mi_invoke (void *py_obj, char **argv, int argc)
       error (_("Python command object missing 'invoke' method."));
     }
 
-  strings = (PyObject **) malloc (sizeof(PyObject *) * argc);
-  argobj = PyList_New (argc);
-  if ( !argobj)
+  gdbpy_ref<> argobj (PyList_New (argc));
+  if (argobj == nullptr)
     {
       gdbpy_print_stack ();
       error (_("Failed to create the python arguments list."));
@@ -102,28 +97,22 @@ py_mi_invoke (void *py_obj, char **argv, int argc)
 
   for (i = 0; i < argc; ++i)
     {
-      strings[i] = PyUnicode_Decode (argv[i], strlen(argv[i]), host_charset (), NULL);
-      if (PyList_SetItem (argobj, i, strings[i]) != 0)
+      /* Since PyList_SetItem steals the reference, we don't use
+       * gdbpy_ref<> to hold on arg string. */
+      PyObject* str  = PyUnicode_Decode (argv[i], strlen (argv[i]), host_charset (), NULL);
+      if (PyList_SetItem (argobj.get (), i, str) != 0)
 	{
 	  error (_("Failed to create the python arguments list."));
 	}
     }
 
-  result = PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst, argobj,
-				       NULL);
+  gdbpy_ref<> result (PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst, argobj.get (),
+				       NULL));
 
-  if (result)
+  if (result != nullptr)
     {
-      parse_mi_result (result, NULL);
-      Py_DECREF (result);
+      parse_mi_result (result.get (), NULL);
     }
-
-  Py_DECREF (argobj);
-  for (i = 0; i < argc; ++i)
-    {
-      Py_DECREF (strings[i]);
-    }
-  free (strings);
 }
 
 /* Parse the name of the MI command to register.
@@ -150,7 +139,7 @@ micmdpy_parse_command_name (const char *name)
 
   /* Skip preceding whitespaces. */
   /* Find first character of the final word.  */
-   for (; i > 0 && (isalnum (name[i - 1])
+  for (; i > 0 && (isalnum (name[i - 1])
  		   || name[i - 1] == '-'
  		   || name[i - 1] == '_');
         --i)
@@ -158,10 +147,10 @@ micmdpy_parse_command_name (const char *name)
    /* Skip the first dash to have to command name only.
     * i.e. -thread-info -> thread-info
     */
-   if(name[i] == '-' && i < len - 2)
+  if(name[i] == '-' && i < len - 2)
      i++;
 
-   if( i == lastchar)
+  if( i == lastchar)
     {
       PyErr_SetString (PyExc_RuntimeError, _("No command name found."));
       return NULL;
@@ -185,20 +174,19 @@ static int
 micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 {
   const char *name;
-  char *cmd_name;
 
   if(! PyArg_ParseTuple (args, "s", &name))
     return -1;
 
-  cmd_name = micmdpy_parse_command_name (name);
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name (micmdpy_parse_command_name (name));
+  if (cmd_name == nullptr)
     return -1;
 
   Py_INCREF (self);
 
   try
   {
-    mi_command *micommand = new mi_command_py (cmd_name, NULL, (void *) self);
+    mi_command *micommand = new mi_command_py (cmd_name.get (), NULL, (void *) self);
 
     bool result = insert_mi_cmd_entry (mi_cmd_up (micommand));
 
@@ -211,7 +199,6 @@ micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
   }
   catch (const gdb_exception &except)
   {
-    xfree (cmd_name);
     Py_DECREF (self);
     PyErr_Format (except.reason == RETURN_QUIT
 		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
@@ -219,7 +206,6 @@ micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
     return -1;
   }
 
-  xfree (cmd_name);
   return 0;
 }
 
-- 
2.20.1

Re: [RFC 4/8] mi/python: C++ify python MI command handling code

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> Use gdbpy_ref<> instead of manually calling Py_DECREF().
Jan> Use gdb::unique_xmalloc_ptr<> instead of manually calling xfree().

I'm reading these in order so of course my comments are all invalidated
by reading the next patch.

I'd just roll this one into the previous one.

Tom

[RFC 8/8] mi/python: Allow redefinition of python MI commands

[Jan Vrany]

Redefining python MI commands is especially useful when developing
then.

gdb/Changelog:

	* mi/mi-cmds.h (mi_command::is_py_command): New method.
	(mi_command_py::is_py_command) New method.
	* gdb/mi/mi-cmds.c: (mi_command::is_py_command): New method.
	(mi_command_py::is_py_command) New method.
	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.

gdb/testsuite/Changelog:

	* gdb.python/py-mi-cmd.exp: Add tests for python-defined MI command
	redefinition.
	* gdb.python/py-mi-cmd-2.py: New file.
---
 gdb/ChangeLog                           |  8 ++++++++
 gdb/mi/mi-cmds.c                        | 17 ++++++++++++++++-
 gdb/mi/mi-cmds.h                        |  5 +++++
 gdb/testsuite/ChangeLog                 |  6 ++++++
 gdb/testsuite/gdb.python/py-mi-cmd-2.py | 13 +++++++++++++
 5 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd-2.py

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1fd49e703f..1fb1cfc2e2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-12-12  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmds.h (mi_command::is_py_command): New method.
+	(mi_command_py::is_py_command) New method.
+	* gdb/mi/mi-cmds.c: (mi_command::is_py_command): New method.
+	(mi_command_py::is_py_command) New method.
+	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.
+
 2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* python/py-micmd.c (py_mi_invoke): Handle exceptions thrown in Python
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c4332e59cf..95abdac080 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -43,7 +43,8 @@ insert_mi_cmd_entry (mi_cmd_up command)
   const std::string &name = command->name ();
 
   if (mi_cmd_table.find (name) != mi_cmd_table.end ())
-    return false;
+    if (! mi_cmd_table[name]->is_py_command ())
+      return false;
 
   mi_cmd_table[name] = std::move (command);
 
@@ -82,6 +83,13 @@ mi_command::mi_command (const char *name, int *suppress_notification)
     m_suppress_notification (suppress_notification)
 {}
 
+bool
+mi_command::is_py_command()
+{
+  return false;
+}
+
+
 std::unique_ptr<scoped_restore_tmpl<int>>
 mi_command::do_suppress_notification ()
 {
@@ -152,6 +160,13 @@ mi_command_py::invoke (struct mi_parse *parse)
 
 }
 
+bool
+mi_command_py::is_py_command()
+{
+  return true;
+}
+
+
 /* Initialize the available MI commands.  */
 
 static void
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index a89e265de7..1f199a6434 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -140,6 +140,9 @@ class mi_command
     /* Execute the MI command.  */
     virtual void invoke (struct mi_parse *parse) = 0;
 
+    /* Return TRUE if command is python command, FALSE otherwise. */
+    virtual bool is_py_command();
+    
   protected:
     std::unique_ptr<scoped_restore_tmpl<int>> do_suppress_notification ();
 
@@ -188,6 +191,8 @@ class mi_command_py : public mi_command
 		   void *object);
     void invoke (struct mi_parse *parse) override;
 
+    bool is_py_command() override;
+
   private:
     void *pyobj;
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a71b3f25db..0172eada9e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-12-12  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.python/py-mi-cmd.exp: Add tests for python-defined MI command
+	redefinition. 
+	* gdb.python/py-mi-cmd-2.py: New file.
+
 2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* gdb.python/py-mi-cmd.exp: New file.
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd-2.py b/gdb/testsuite/gdb.python/py-mi-cmd-2.py
new file mode 100644
index 0000000000..5d5929d2fd
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd-2.py
@@ -0,0 +1,13 @@
+import gdb
+
+class pycmd(gdb.MICommand):
+    def invoke(self, argv):
+        if argv[0] == 'bogus':
+            return "not really"
+        else:
+            raise gdb.GdbError("Invalid parameter: %s" % argv[0])
+
+pycmd('-pycmd')
+pycmd('-info-gdb-mi-command')
+
+
-- 
2.20.1

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> Redefining python MI commands is especially useful when developing
Jan> then.

It seems like this could lead to difficulties with dangling pointers.
Some assurance on this would be good.

Tom

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Jan Vrany]

On Thu, 2019-04-25 at 13:50 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> 
> Jan> Redefining python MI commands is especially useful when developing
> Jan> then.
> 
> It seems like this could lead to difficulties with dangling pointers.
> Some assurance on this would be good.

I'm not sure what exactly do you mean. However, I changed mi_command_py to
use gdbpy_ref<> instead of PyObject* (or even void*). 

Therefore when one redefines the command:

+  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
+    if (! mi_cmd_table[name]->can_be_redefined ())
+      return false;
+
+  mi_cmd_table[name] = std::move (command);

then the old command (if any) is destructed so Py_DECREF() is called on
Python objech held on by mi_command_py() instance. 

I double checked by tracing it in GDB - I don't know how to write
the test for for this though. 

Does above address your concern? 

Thanks!

Jan

> 
> Tom

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> Redefining python MI commands is especially useful when developing
Jan> then.

>> It seems like this could lead to difficulties with dangling pointers.
>> Some assurance on this would be good.

Jan> I'm not sure what exactly do you mean. However, I changed mi_command_py to
Jan> use gdbpy_ref<> instead of PyObject* (or even void*). 

Jan> Therefore when one redefines the command:

Jan> +  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
Jan> +    if (! mi_cmd_table[name]->can_be_redefined ())
Jan> +      return false;
Jan> +
Jan> +  mi_cmd_table[name] = std::move (command);

Jan> then the old command (if any) is destructed so Py_DECREF() is called on
Jan> Python objech held on by mi_command_py() instance. 

Jan> I double checked by tracing it in GDB - I don't know how to write
Jan> the test for for this though. 

Jan> Does above address your concern? 

The issue isn't a leak but rather the possibility of using an object
after it's been destroyed.

That is, if an MI command can be running when it is replaced, the code
has to be sure that the command object isn't referenced after the
replacement -- because the old object will have been destroyed.

Tom

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Jan Vrany]

On Mon, 2019-05-06 at 15:40 -0600, Tom Tromey wrote:
> Jan> Does above address your concern? 
> 
> The issue isn't a leak but rather the possibility of using an object
> after it's been destroyed.
> 
> That is, if an MI command can be running when it is replaced, the code
> has to be sure that the command object isn't referenced after the
> replacement -- because the old object will have been destroyed.
> 

I see. I just added a test for this case into "almost finished" 
v2 of the patch series. There, this problem is kind of avoided by 
making sure that in mi_command_py::invoke anything from "this" 
mi_command_py object is not accessed AFTER calling the python code. 

However I agree that using shared_ptr is more robust solution.

Thanks! 

Jan



> Tom

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Simon Marchi]

On 2019-05-07 7:25 a.m., Jan Vrany wrote:
> I see. I just added a test for this case into "almost finished" 
> v2 of the patch series. There, this problem is kind of avoided by 
> making sure that in mi_command_py::invoke anything from "this" 
> mi_command_py object is not accessed AFTER calling the python code. 
> 
> However I agree that using shared_ptr is more robust solution.

If we know that we don't access that pointer after it is possibly stale, and
we document that fact properly, I think we can keep what you had initially.
Using shared_ptr has a cost, and it's not really essential here.

Simon

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Jan Vrany]

On Tue, 2019-05-07 at 09:09 -0400, Simon Marchi wrote:
> On 2019-05-07 7:25 a.m., Jan Vrany wrote:
> > I see. I just added a test for this case into "almost finished" 
> > v2 of the patch series. There, this problem is kind of avoided by 
> > making sure that in mi_command_py::invoke anything from "this" 
> > mi_command_py object is not accessed AFTER calling the python code. 
> > 
> > However I agree that using shared_ptr is more robust solution.
> 
> If we know that we don't access that pointer after it is possibly stale, and
> we document that fact properly, I think we can keep what you had initially.
> Using shared_ptr has a cost, and it's not really essential here.
> 

All right. Thanks!

Jan

> Simon

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Simon Marchi]

On 2019-05-07 9:19 a.m., Jan Vrany wrote:
> On Tue, 2019-05-07 at 09:09 -0400, Simon Marchi wrote:
>> On 2019-05-07 7:25 a.m., Jan Vrany wrote:
>>> I see. I just added a test for this case into "almost finished" 
>>> v2 of the patch series. There, this problem is kind of avoided by 
>>> making sure that in mi_command_py::invoke anything from "this" 
>>> mi_command_py object is not accessed AFTER calling the python code. 
>>>
>>> However I agree that using shared_ptr is more robust solution.
>>
>> If we know that we don't access that pointer after it is possibly stale, and
>> we document that fact properly, I think we can keep what you had initially.
>> Using shared_ptr has a cost, and it's not really essential here.
>>
> 
> All right. Thanks!

Well that's my opinion, let's see what Tom thinks about it.

Simon

Re: [RFC 8/8] mi/python: Allow redefinition of python MI commands

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>>> If we know that we don't access that pointer after it is possibly stale, and
>>> we document that fact properly, I think we can keep what you had initially.
>>> Using shared_ptr has a cost, and it's not really essential here.
>>> 
>> 
>> All right. Thanks!

Simon> Well that's my opinion, let's see what Tom thinks about it.

I'm of two minds.

On the one hand, this sort of approach is fragile.  It's susceptible to
introducing bugs in the future.

On the other hand, it seems somewhat unlikely to actually be a cause of
bugs, since it's doubtful that more code will be added here.

I tend to suspect that using shared_ptr here is not too expensive, but I
don't mind either way I guess.

Tom

[RFC 7/8] mi/python: Add tests for python-defined MI commands

[Jan Vrany]

gdb/testsuite/Changelog:

	* gdb.python/py-mi-cmd.exp: New file.
	* gdb.python/py-mi-cmd-1.py: New file.
---
 gdb/testsuite/ChangeLog                 |  5 ++
 gdb/testsuite/gdb.python/py-mi-cmd-1.py | 27 +++++++++
 gdb/testsuite/gdb.python/py-mi-cmd.exp  | 77 +++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd-1.py
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.exp

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d46422635b..a71b3f25db 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.python/py-mi-cmd.exp: New file.
+	* gdb.python/py-mi-cmd-1.py: New file.
+
 2019-04-15  Leszek Swirski  <leszeks@google.com>
 
 	* gdb.arch/amd64-eval.cc: New file.
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd-1.py b/gdb/testsuite/gdb.python/py-mi-cmd-1.py
new file mode 100644
index 0000000000..2e73c022a3
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd-1.py
@@ -0,0 +1,27 @@
+import gdb
+
+class pycmd(gdb.MICommand):
+    def invoke(self, argv):
+        if argv[0] == 'int':
+            return 42
+        elif argv[0] == 'str':
+            return "Hello world!"
+        elif argv[0] == 'ary':
+            return [ 'Hello', 42 ]
+        elif argv[0] == "dct":
+            return { 'hello' : 'world', 'times' : 42}
+        elif argv[0] == 'tpl':
+            return ( 42 , 'Hello' )
+        elif argv[0] == 'itr':
+            return iter([1,2,3])
+        elif argv[0] == 'nn1':
+            return None
+        elif argv[0] == 'nn2':
+            return [ None ]
+        elif argv[0] == 'exp':
+            raise gdb.GdbError()
+        else:
+            raise gdb.GdbError("Invalid parameter: %s" % argv[0])
+
+pycmd('-pycmd')
+
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.exp b/gdb/testsuite/gdb.python/py-mi-cmd.exp
new file mode 100644
index 0000000000..17afb6c83e
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd.exp
@@ -0,0 +1,77 @@
+# Copyright (C) 2018 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Tests whether -var-info-path-expression fails as documented when
+# invoked on a dynamic varobj.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi2"
+
+gdb_exit
+if {[mi_gdb_start]} {
+    continue
+}
+
+mi_gdb_test "set python print-stack full" \
+  ".*\\^done" \
+  "set python print-stack full"
+
+standard_testfile
+#
+# Start here
+#
+mi_gdb_test "source ${srcdir}/${subdir}/${testfile}-1.py" \
+  ".*\\^done" \
+  "load python file"
+
+
+mi_gdb_test "-pycmd int" \
+  "\\^done,result=\"42\"" \
+  "-pycmd int"
+
+mi_gdb_test "-pycmd str" \
+  "\\^done,result=\"Hello world!\"" \
+  "-pycmd str"
+
+mi_gdb_test "-pycmd ary" \
+  "\\^done,result=\\\[\"Hello\",\"42\"\\\]" \
+  "-pycmd ary"
+
+mi_gdb_test "-pycmd dct" \
+  "\\^done,result={hello=\"world\",times=\"42\"}" \
+  "-pycmd dct"
+
+mi_gdb_test "-pycmd tpl" \
+  "\\^done,result=\\\[\"42\",\"Hello\"\\\]" \
+  "-pycmd tpl"
+
+mi_gdb_test "-pycmd itr" \
+  "\\^done,result=\\\[\"1\",\"2\",\"3\"\\\]" \
+  "-pycmd itr"
+
+mi_gdb_test "-pycmd nn1" \
+  "\\^done" \
+  "-pycmd nn1"
+
+mi_gdb_test "-pycmd nn2" \
+  "\\^done,result=\\\[\"None\"\\\]" \
+  "-pycmd nn2"
+
+mi_gdb_test "-pycmd bogus" \
+  "\\^error,msg=\"Invalid parameter: bogus\"" \
+  "-pycmd bogus"
+
+mi_gdb_test "-pycmd exp" \
+  "\\^error,msg=\"Failed to execute command\"" \
+  "-pycmd exp"
-- 
2.20.1

Re: [RFC 7/8] mi/python: Add tests for python-defined MI commands

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> gdb/testsuite/Changelog:
Jan> 	* gdb.python/py-mi-cmd.exp: New file.
Jan> 	* gdb.python/py-mi-cmd-1.py: New file.

Thanks for doing this.  This looks good.

Jan> diff --git a/gdb/testsuite/gdb.python/py-mi-cmd-1.py b/gdb/testsuite/gdb.python/py-mi-cmd-1.py
Jan> new file mode 100644
Jan> index 0000000000..2e73c022a3
Jan> --- /dev/null
Jan> +++ b/gdb/testsuite/gdb.python/py-mi-cmd-1.py
Jan> @@ -0,0 +1,27 @@
Jan> +import gdb
Jan> +

New files need copyright headers.

Jan> +gdb_exit
Jan> +if {[mi_gdb_start]} {
Jan> +    continue
Jan> +}

This should also check whether Python tests can be run.

Tom

[RFC 3/8] Create MI commands using python.

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This commit allows an user to create custom MI commands using Python
similarly to what is possible for Python CLI commands.

A new subclass of mi_command is defined for Python MI commands,
mi_command_py. A new file, py-micmd.c contains the logic for Python
MI commands.

gdb/ChangeLog
2017-02-10  Didier Nadeau  <didier.nadeau@gmail.com>

    * Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
    * mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
    (mi_command_py::mi_command_py): New function.
    (mi_command_py::invoke): New function.
    * mi/mi-cmds.h (mi_command_py): New class.
    (insert_mi_cmd_entry): New declaration.
    * python/py-micmd.c: New file
    (micmdpy_object): New struct.
    (micmdpy_object): New typedef.
    (parse_mi_result): New function.
    (py_mi_invoke): New function.
    (micmdpy_parse_command_name): New function.
    (micmdpy_init): New function.
    (gdbpy_initialize_micommands): New function.
    * python/py-micmd.h: New file
    (py_mi_invoke): New function.
    * python/python-internal.h
    (gdbpy_initialize_micommands): New declaration.
    * python/python.c
    (_initialize_python): New call to gdbpy_initialize_micommands.
---
 gdb/Makefile.in              |   1 +
 gdb/mi/mi-cmds.c             |  27 +++-
 gdb/mi/mi-cmds.h             |  19 +++
 gdb/python/py-micmd.c        | 290 +++++++++++++++++++++++++++++++++++
 gdb/python/py-micmd.h        |   6 +
 gdb/python/python-internal.h |   2 +
 gdb/python/python.c          |   3 +-
 7 files changed, 344 insertions(+), 4 deletions(-)
 create mode 100644 gdb/python/py-micmd.c
 create mode 100644 gdb/python/py-micmd.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0f49578360..28866962bc 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -382,6 +382,7 @@ SUBDIR_PYTHON_SRCS = \
 	python/py-instruction.c \
 	python/py-lazy-string.c \
 	python/py-linetable.c \
+	python/py-micmd.c \
 	python/py-newobjfileevent.c \
 	python/py-objfile.c \
 	python/py-param.c \
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 566d6c1885..c4332e59cf 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -23,6 +23,7 @@
 #include "mi-cmds.h"
 #include "mi-main.h"
 #include "mi-parse.h"
+#include "python/py-micmd.h"
 #include <map>
 #include <string>
 #include <memory>
@@ -31,10 +32,9 @@
 
 static std::map<std::string, mi_cmd_up> mi_cmd_table;
 
-/* Insert a new mi-command into the command table.  Return true if
-   insertion was successful.  */
+/* See mi-cmds.h.  */
 
-static bool
+bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != NULL);
@@ -131,6 +131,27 @@ mi_command_cli::invoke (struct mi_parse *parse)
 			  parse->args);
 }
 
+mi_command_py::mi_command_py (const char *name, int *suppress_notification,
+			      void *object)
+  : mi_command (name, suppress_notification),
+    pyobj (object)
+{}
+
+void
+mi_command_py::invoke (struct mi_parse *parse)
+{
+  std::unique_ptr<scoped_restore_tmpl <int>> restore
+      = do_suppress_notification ();
+
+  mi_parse_argv (parse->args, parse);
+
+  if (parse->argv == NULL)
+      error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
+
+  py_mi_invoke (this->pyobj, parse->argv, parse->argc);
+
+}
+
 /* Initialize the available MI commands.  */
 
 static void
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index eae023c5da..a89e265de7 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -179,6 +179,20 @@ class mi_command_cli : public mi_command
     int m_args_p;
 };
 
+/* MI command implemented on top of a Python command.  */
+
+class mi_command_py : public mi_command
+{
+  public:
+    mi_command_py (const char *name, int *suppress_notification,
+		   void *object);
+    void invoke (struct mi_parse *parse) override;
+
+  private:
+    void *pyobj;
+
+};
+
 typedef std::unique_ptr<mi_command> mi_cmd_up;
 
 /* Lookup a command in the MI command table.  */
@@ -190,4 +204,9 @@ extern int mi_debug_p;
 
 extern void mi_execute_command (const char *cmd, int from_tty);
 
+/* Insert a new mi-command into the command table.  Return true if
+   insertion was successful.  */
+
+extern bool insert_mi_cmd_entry (mi_cmd_up command);
+
 #endif /* MI_MI_CMDS_H */
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
new file mode 100644
index 0000000000..ee612e2bc5
--- /dev/null
+++ b/gdb/python/py-micmd.c
@@ -0,0 +1,290 @@
+/* gdb MI commands implemented in Python  */
+
+#include <string.h>
+
+#include "defs.h"
+#include "arch-utils.h"
+#include "value.h"
+#include "python-internal.h"
+#include "charset.h"
+#include "gdbcmd.h"
+#include "cli/cli-decode.h"
+#include "completer.h"
+#include "language.h"
+#include "mi/mi-cmds.h"
+
+struct micmdpy_object
+{
+  PyObject_HEAD
+};
+
+typedef struct micmdpy_object micmdpy_object;
+
+static PyObject *invoke_cst;
+
+extern PyTypeObject micmdpy_object_type
+  CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");
+
+/* If the command invoked returns a list, this function parses it and create an
+   appropriate MI out output.
+
+   The returned values must be Python string, and can be contained within Python
+   lists and dictionaries. It is possible to have a multiple levels of lists
+   and/or dictionaries.  */
+
+static void
+parse_mi_result (PyObject *result, const char *field_name)
+{
+  struct ui_out *uiout = current_uiout;
+
+  if(!field_name)
+    field_name = "default";
+
+  if (PyString_Check(result))
+    {
+      const char *string = gdbpy_obj_to_string (result).release ();
+      uiout->field_string (field_name, string);
+      xfree ( (void *)string);
+    }
+  else if (PyList_Check (result))
+    {
+      PyObject *item;
+      Py_ssize_t i = 0;      
+      ui_out_emit_list list_emitter (uiout, field_name);
+      for(i = 0; i < PyList_GET_SIZE (result); ++i)
+	{
+	  //struct cleanup *cleanup_item = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+          ui_out_emit_tuple tuple_emitter (uiout, NULL);
+	  item = PyList_GetItem (result, i);
+	  parse_mi_result (item, NULL);
+	  //do_cleanups (cleanup_item);
+	}      
+    }
+  else if ( PyDict_Check (result))
+    {
+      PyObject *key, *value;
+      Py_ssize_t pos = 0;
+      while ( PyDict_Next (result, &pos, &key, &value) )
+	{
+	  char *key_string = gdbpy_obj_to_string (key).release ();
+	  parse_mi_result (value, key_string);
+	  xfree ( (void *) key_string);
+	}
+    }
+}
+
+/* Called from mi_command_py's invoke to invoke the command.  */
+
+void
+py_mi_invoke (void *py_obj, char **argv, int argc)
+{
+  micmdpy_object *obj = (micmdpy_object *) py_obj;
+  PyObject *argobj, *result,  **strings;
+  int i;
+
+  gdbpy_enter enter_py (get_current_arch (), current_language);
+
+  if (! obj)
+    error(_("Invalid invocation of Python micommand object."));
+
+  if (! PyObject_HasAttr ((PyObject *) obj, invoke_cst))
+    {
+      error (_("Python command object missing 'invoke' method."));
+    }
+
+  strings = (PyObject **) malloc (sizeof(PyObject *) * argc);
+  argobj = PyList_New (argc);
+  if ( !argobj)
+    {
+      gdbpy_print_stack ();
+      error (_("Failed to create the python arguments list."));
+    }
+
+  for (i = 0; i < argc; ++i)
+    {
+      strings[i] = PyUnicode_Decode (argv[i], strlen(argv[i]), host_charset (), NULL);
+      if (PyList_SetItem (argobj, i, strings[i]) != 0)
+	{
+	  error (_("Failed to create the python arguments list."));
+	}
+    }
+
+  result = PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst, argobj,
+				       NULL);
+
+  if (result)
+    {
+      parse_mi_result (result, NULL);
+      Py_DECREF (result);
+    }
+
+  Py_DECREF (argobj);
+  for (i = 0; i < argc; ++i)
+    {
+      Py_DECREF (strings[i]);
+    }
+  free (strings);
+}
+
+/* Parse the name of the MI command to register.
+
+   This function returns the xmalloc()d name of the new command. On error
+   sets the Python error and returns NULL.  */
+
+static char *
+micmdpy_parse_command_name (const char *name)
+{
+  int len = strlen (name);
+  int i, lastchar;
+  char *result;
+
+  /* Skip trailing whitespaces. */
+  for (i = len - 1; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
+    ;
+  if (i < 0)
+    {
+      PyErr_SetString (PyExc_RuntimeError, _("No command name found."));
+      return NULL;
+    }
+  lastchar = i;
+
+  /* Skip preceding whitespaces. */
+  /* Find first character of the final word.  */
+   for (; i > 0 && (isalnum (name[i - 1])
+ 		   || name[i - 1] == '-'
+ 		   || name[i - 1] == '_');
+        --i)
+     ;
+   /* Skip the first dash to have to command name only.
+    * i.e. -thread-info -> thread-info
+    */
+   if(name[i] == '-' && i < len - 2)
+     i++;
+
+   if( i == lastchar)
+    {
+      PyErr_SetString (PyExc_RuntimeError, _("No command name found."));
+      return NULL;
+    }
+
+  result = (char *) xmalloc (lastchar - i + 2);
+  memcpy(result, &name[i], lastchar - i + 1);
+  result[lastchar - i + 1] = '\0';
+
+  return result;
+}
+
+/* Object initializer; sets up gdb-side structures for MI command.
+
+   Use: __init__(NAME).
+
+   NAME is the name of the MI command to register. It should starts with a dash
+   as a traditional MI command does.  */
+
+static int
+micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
+{
+  const char *name;
+  char *cmd_name;
+
+  if(! PyArg_ParseTuple (args, "s", &name))
+    return -1;
+
+  cmd_name = micmdpy_parse_command_name (name);
+  if (! cmd_name)
+    return -1;
+
+  Py_INCREF (self);
+
+  try
+  {
+    mi_command *micommand = new mi_command_py (cmd_name, NULL, (void *) self);
+
+    bool result = insert_mi_cmd_entry (mi_cmd_up (micommand));
+
+    if (! result)
+      {
+	PyErr_Format (PyExc_RuntimeError,
+		      _("Unable to insert command. The name might already be in use."));
+	return -1;
+      }
+  }
+  catch (const gdb_exception &except)
+  {
+    xfree (cmd_name);
+    Py_DECREF (self);
+    PyErr_Format (except.reason == RETURN_QUIT
+		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
+		    "%s", except.message);
+    return -1;
+  }
+
+  xfree (cmd_name);
+  return 0;
+}
+
+/* Initialize the MI command object.  */
+
+int
+gdbpy_initialize_micommands(void)
+{
+  micmdpy_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&micmdpy_object_type) < 0)
+    return -1;
+
+  if (gdb_pymodule_addobject (gdb_module, "MICommand",
+  			      (PyObject *) &micmdpy_object_type) < 0)
+      return -1;
+
+  invoke_cst = PyString_FromString("invoke");
+  if (invoke_cst == NULL)
+    return -1;
+  
+  return 0;
+}
+
+static PyMethodDef micmdpy_object_methods[] =
+    {
+	{ 0 }
+    };
+
+PyTypeObject micmdpy_object_type =
+{
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.MICommand",		  /*tp_name*/
+  sizeof (micmdpy_object),	  /*tp_basicsize*/
+  0,				  /*tp_itemsize*/
+  0,				  /*tp_dealloc*/
+  0,				  /*tp_print*/
+  0,				  /*tp_getattr*/
+  0,				  /*tp_setattr*/
+  0,				  /*tp_compare*/
+  0,				  /*tp_repr*/
+  0,				  /*tp_as_number*/
+  0,				  /*tp_as_sequence*/
+  0,				  /*tp_as_mapping*/
+  0,				  /*tp_hash */
+  0,				  /*tp_call*/
+  0,				  /*tp_str*/
+  0,				  /*tp_getattro*/
+  0,				  /*tp_setattro*/
+  0,				  /*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /*tp_flags*/
+  "GDB mi-command object",		  /* tp_doc */
+  0,				  /* tp_traverse */
+  0,				  /* tp_clear */
+  0,				  /* tp_richcompare */
+  0,				  /* tp_weaklistoffset */
+  0,				  /* tp_iter */
+  0,				  /* tp_iternext */
+  micmdpy_object_methods,	  /* tp_methods */
+  0,				  /* tp_members */
+  0,				  /* tp_getset */
+  0,				  /* tp_base */
+  0,				  /* tp_dict */
+  0,				  /* tp_descr_get */
+  0,				  /* tp_descr_set */
+  0,				  /* tp_dictoffset */
+  micmdpy_init,			  /* tp_init */
+  0,				  /* tp_alloc */
+};
diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h
new file mode 100644
index 0000000000..98f5305003
--- /dev/null
+++ b/gdb/python/py-micmd.h
@@ -0,0 +1,6 @@
+#ifndef PY_MICMDS_H
+#define PY_MICMDS_H
+
+void py_mi_invoke (void *py_obj, char **argv, int argc);
+
+#endif
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 449926ca87..26606b4b36 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -539,6 +539,8 @@ int gdbpy_initialize_xmethods (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_unwind (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+int gdbpy_initialize_micommands (void)
+  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 
 /* A wrapper for PyErr_Fetch that handles reference counting for the
    caller.  */
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4dad8ec10d..5184b99e49 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1698,7 +1698,8 @@ do_start_initialization ()
       || gdbpy_initialize_event () < 0
       || gdbpy_initialize_arch () < 0
       || gdbpy_initialize_xmethods () < 0
-      || gdbpy_initialize_unwind () < 0)
+      || gdbpy_initialize_unwind () < 0
+      || gdbpy_initialize_micommands () < 0)
     return false;
 
 #define GDB_PY_DEFINE_EVENT_TYPE(name, py_name, doc, base)	\
-- 
2.20.1

Re: [RFC 3/8] Create MI commands using python.

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Lots of nits on this one, mostly I think because it was converted from
C?  Anything, nothing very major, just lots of little things.

Jan> +mi_command_py::mi_command_py (const char *name, int *suppress_notification,
Jan> +			      void *object)
Jan> +  : mi_command (name, suppress_notification),
Jan> +    pyobj (object)
Jan> +{}

I think it would be better to put this sort of thing somewhere in the
Python code, so that it can use gdbpy_ref<> rather than "void *".

Jan> +  py_mi_invoke (this->pyobj, parse->argv, parse->argc);

This sort of indirection wouldn't be needed then either.

Jan> +class mi_command_py : public mi_command
Jan> +{
Jan> +  public:
Jan> +    mi_command_py (const char *name, int *suppress_notification,
Jan> +		   void *object);
Jan> +    void invoke (struct mi_parse *parse) override;
Jan> +
Jan> +  private:
Jan> +    void *pyobj;
Jan> +
Jan> +};

Extra blank line before the "}".

Jan> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
Jan> new file mode 100644
Jan> index 0000000000..ee612e2bc5
Jan> --- /dev/null
Jan> +++ b/gdb/python/py-micmd.c
Jan> @@ -0,0 +1,290 @@
Jan> +/* gdb MI commands implemented in Python  */
Jan> +
Jan> +#include <string.h>

Files need a copyright header.

Jan> +#include "defs.h"

defs.h must come first.  I wonder if string.h is needed at all.

Jan> +/* If the command invoked returns a list, this function parses it and create an
Jan> +   appropriate MI out output.
Jan> +
Jan> +   The returned values must be Python string, and can be contained within Python
Jan> +   lists and dictionaries. It is possible to have a multiple levels of lists
Jan> +   and/or dictionaries.  */
Jan> +
Jan> +static void
Jan> +parse_mi_result (PyObject *result, const char *field_name)
Jan> +{
Jan> +  struct ui_out *uiout = current_uiout;
Jan> +
Jan> +  if(!field_name)

gdb+gnu style would be:  "if (field_name == nullptr)"

Jan> +  if (PyString_Check(result))

Need a space before open parens.
There are a few instances of this.

Jan> +    {
Jan> +      const char *string = gdbpy_obj_to_string (result).release ();
Jan> +      uiout->field_string (field_name, string);
Jan> +      xfree ( (void *)string);

Better to take advantage of the unique pointer, like:

   gdb::unique_xmalloc_ptr<char> string = gdbpy_obj_to_string (result);
   uiout->field_string (field_name, string.get ());

However, this code is ignoring Python errors.  Probably this function
should use one of the standard conventions and return -1 on error.

Jan> +	  //struct cleanup *cleanup_item = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);

There are a couple of these; remove them.

Jan> +	  item = PyList_GetItem (result, i);

This can fail, so it needs a check.

Jan> +  else if ( PyDict_Check (result))

There's an extra space after paren.

Jan> +	  char *key_string = gdbpy_obj_to_string (key).release ();

Error checking.

Jan> +	  xfree ( (void *) key_string);

Again, no need to do this explicitly.

Jan> +  PyObject *argobj, *result,  **strings;

Extra space.

Jan> +  if (! obj)
Jan> +    error(_("Invalid invocation of Python micommand object."));

This could probably be improved somehow, for instance mentioning the MI
command name.  I think that's sort of standard in MI commands, so this
should follow it in all the errors, I think.

Jan> +  strings = (PyObject **) malloc (sizeof(PyObject *) * argc);

Not sure this is needed, see below.  But if it is it should use XNEWVEC
or xmalloc.

Jan> +  for (i = 0; i < argc; ++i)
Jan> +    {
Jan> +      strings[i] = PyUnicode_Decode (argv[i], strlen(argv[i]), host_charset (), NULL);
Jan> +      if (PyList_SetItem (argobj, i, strings[i]) != 0)

PyList_SetItem steals a reference, so I think the ref-counting in this
function is incorrect.  But, since it steals a reference, I think
"strings" isn't needed.

Jan> +	{
Jan> +	  error (_("Failed to create the python arguments list."));

This will leak references.  However if you use gdbpy_ref<>, you should
be ok.

Jan> +  /* Skip preceding whitespaces. */
Jan> +  /* Find first character of the final word.  */

Not sure if the first comment documents something that should be there
and is not, or if it should just be removed.

Probably the latter, I think it's fine to require command names to be
valid.  I'd probably skip stripping the trailing whitespace as well.

Jan> +  cmd_name = micmdpy_parse_command_name (name);
Jan> +  if (! cmd_name)
Jan> +    return -1;

Like this could just give an error if the NAME isn't a valid MI command
name, and not bother with trying to fix it up, or copy it.

Jan> +    PyErr_Format (except.reason == RETURN_QUIT
Jan> +		    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
Jan> +		    "%s", except.message);

I think GDB_PY_SET_HANDLE_EXCEPTION is better for this.

Jan> +/* Initialize the MI command object.  */
Jan> +
Jan> +int
Jan> +gdbpy_initialize_micommands(void)

Don't need (void) any more.

Jan> +++ b/gdb/python/py-micmd.h
Jan> @@ -0,0 +1,6 @@
Jan> +#ifndef PY_MICMDS_H
Jan> +#define PY_MICMDS_H
Jan> +
Jan> +void py_mi_invoke (void *py_obj, char **argv, int argc);
Jan> +

I'd just stick this in python-internal.h and not have a new header.

thanks,
Tom

[RFC 2/8] Use classes to represent MI Command instead of structures

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This commit changes the infrastructure of mi-cmds.h and associated
files to use classes instead of structure to populate the hashmap
containing the commands.

The base class is virtual and there is one subclass MI commands
implemented with pure MI implementation and another for MI commands
implemented over a CLI command.

Logic for suppress_notification and parsing of ARGV/ARGC has been moved
to the classes implementation.

gdb/ChangeLog:
2017-02-03  Didier Nadeau  <didier.nadeau@gmail.com>

    * mi/mi-cmds.c (create_mi_cmd): Remove.
    (mi_command::mi_command): New function.
    (mi_command::do_suppress_notification): New function.
    (mi_command_mi::mi_command_mi): New function.
    (mi_command_mi::invoke): New function.
    (mi_command_cli::mi_command_cli): New function.
    (mi_command_cli::invoke): New function.
    (mi_cmd_lookup): Change return type.
    * mi/mi-cmds.h (struct mi_cli): Remove.
    (struct mi_cmd): Remove.
    (class mi_command): New class.
    (class mi_command_mi): New class.
    (class mi_command_cli): New class.
    (mi_cmd_loopkup): Change return type.
    * mi/mi-main.c (mi_execute_cli_command): Remove declaration.
    (mi_execute_command): Remove suppress_notification handling.
    (mi_cmd_execute): Remove call to argv_func.
    (mi_cmd_execute): Remove call to mi_execute_cli_command.
    (mi_cmd_execute): New call to mi_command::invoke.
    * mi/mi-main.h (mi_execute_cli_command): New declaration.
    * mi/mi-parse.c (mi_parse): Remove call to mi_parse_argv.
    * mi/mi-parse.h (struct mi_parse): Remove field struct mi_cmd.
    (struct mi_parse): New field class mi_command.
    (mi_parse_argv): New declaration.
---
 gdb/mi/mi-cmd-info.c |  2 +-
 gdb/mi/mi-cmds.c     | 97 +++++++++++++++++++++++++++++++-------------
 gdb/mi/mi-cmds.h     | 70 ++++++++++++++++++++++----------
 gdb/mi/mi-main.c     | 18 +-------
 gdb/mi/mi-main.h     |  1 +
 gdb/mi/mi-parse.c    | 18 ++------
 gdb/mi/mi-parse.h    |  6 ++-
 7 files changed, 128 insertions(+), 84 deletions(-)

diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index 8645fac294..3d8bd68264 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 2b3cf297cf..566d6c1885 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>
 #include <memory>
@@ -37,9 +38,9 @@ static bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != NULL);
-  gdb_assert (command->name != NULL);
+  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;
@@ -49,32 +50,16 @@ 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 a pure MI implementation.  */
 
 static void
 add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
 	       int *suppress_notification = NULL)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
+  mi_command *micommand = new mi_command_mi (name, function,
+					     suppress_notification);
 
-  cmd_up->cli.cmd = NULL;
-  cmd_up->cli.args_p = 0;
-  cmd_up->argv_func = function;
-  cmd_up->suppress_notification = suppress_notification;
-
-  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
   gdb_assert (success);
 }
 
@@ -84,17 +69,70 @@ static void
 add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 		int *suppress_notification = NULL)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
-
-  cmd_up->cli.cmd = cli_name;
-  cmd_up->cli.args_p = args_p;
-  cmd_up->argv_func = NULL;
-  cmd_up->suppress_notification = suppress_notification;
+  mi_command *micommand = new mi_command_cli (name, cli_name, args_p,
+					      suppress_notification);
 
-  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
   gdb_assert (success);
 }
 
+/* See mi-cmds.h  */
+mi_command::mi_command (const char *name, int *suppress_notification)
+  : m_name (name),
+    m_suppress_notification (suppress_notification)
+{}
+
+std::unique_ptr<scoped_restore_tmpl<int>>
+mi_command::do_suppress_notification ()
+{
+  if (m_suppress_notification != NULL)
+    return std::unique_ptr<scoped_restore_tmpl <int>> (
+	new scoped_restore_tmpl <int> (m_suppress_notification, 1));
+
+  return std::unique_ptr<scoped_restore_tmpl <int>> ();
+}
+
+mi_command_mi::mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+			      int *suppress_notification)
+  : mi_command (name, suppress_notification),
+    m_argv_function (func)
+{
+  gdb_assert (func != NULL);
+}
+
+void
+mi_command_mi::invoke (struct mi_parse *parse)
+{
+  std::unique_ptr<scoped_restore_tmpl <int>> restore
+    = do_suppress_notification ();
+
+  mi_parse_argv (parse->args, parse);
+
+  if (parse->argv == NULL)
+    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
+
+  this->m_argv_function (parse->command, parse->argv, parse->argc);
+}
+
+mi_command_cli::mi_command_cli (const char *name, const char *cli_name,
+				int args_p, int *suppress_notification)
+  : mi_command (name, suppress_notification),
+    m_cli_name (cli_name),
+    m_args_p (args_p)
+{}
+
+void
+mi_command_cli::invoke (struct mi_parse *parse)
+{
+  std::unique_ptr<scoped_restore_tmpl <int>> restore
+    = do_suppress_notification ();
+
+  mi_execute_cli_command (this->m_cli_name.c_str (), this->m_args_p,
+			  parse->args);
+}
+
+/* Initialize the available MI commands.  */
+
 static void
 build_table ()
 {
@@ -244,7 +282,7 @@ build_table ()
 
 /* See mi-cmds.h.  */
 
-struct mi_cmd *
+mi_command *
 mi_cmd_lookup (const char *command)
 {
   gdb_assert (command != NULL);
@@ -262,3 +300,4 @@ _initialize_mi_cmds (void)
 {
   build_table ();
 }
+
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 97e1be819f..eae023c5da 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -126,38 +126,64 @@ extern mi_cmd_argv_ftype mi_cmd_enable_pretty_printing;
 extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
 extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
 
-/* Description of a single command.  */
+/* mi_command base virtual class.  */
 
-struct mi_cli
+class mi_command
 {
-  /* 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;
+  public:
+    mi_command (const char *name, int *suppress_notification);
+    virtual ~mi_command () {};
+
+    const std::string &name ()
+    { return m_name; }
+
+    /* Execute the MI command.  */
+    virtual void invoke (struct mi_parse *parse) = 0;
+
+  protected:
+    std::unique_ptr<scoped_restore_tmpl<int>> do_suppress_notification ();
+
+  private:
+
+    /* The name of the command.  */
+    std::string m_name;
+
+    /* Pointer to integer to set during command's invocation.  */
+    int *m_suppress_notification;
 };
 
-struct mi_cmd
+/* MI command with a pure MI implementation.  */
+
+class mi_command_mi : public mi_command
+{
+  public:
+    mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+		   int *suppress_notification);
+    void invoke (struct mi_parse *parse) override;
+
+  private:
+    mi_cmd_argv_ftype *m_argv_function;
+};
+
+/* MI command implemented on top of a CLI command.  */
+
+class mi_command_cli : public mi_command
 {
-  /* 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;
+  public:
+    mi_command_cli (const char *name, const char *cli_name, int args_p,
+		  int *suppress_notification);
+    void invoke (struct mi_parse *parse) override;
+
+  private:
+    std::string m_cli_name;
+    int m_args_p;
 };
 
-typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
+typedef std::unique_ptr<mi_command> mi_cmd_up;
 
 /* Lookup a command in the MI command table.  */
 
-extern struct 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-main.c b/gdb/mi/mi-main.c
index 17ca807471..481f09f799 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -90,9 +90,6 @@ int running_result_record_printed = 1;
 int mi_proceeded;
 
 static void mi_cmd_execute (struct mi_parse *parse);
-
-static void mi_execute_cli_command (const char *cmd, int 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 *,
@@ -1954,9 +1951,6 @@ mi_execute_command (const char *cmd, int from_tty)
 
       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)
@@ -2095,17 +2089,9 @@ 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)
+  if (parse->cmd != NULL)
     {
-      /* 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);
+      parse->cmd->invoke (parse);
     }
   else
     {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 72c4e59428..3733bc37ac 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -54,6 +54,7 @@ struct mi_suppress_notification
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
+void mi_execute_cli_command (const char *cmd, int 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 6c8d922d66..7ed2b2bbc6 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 2262ff56f9..94bd48219c 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;
+    mi_command *cmd;
     struct mi_timestamp *cmd_start;
     char *args;
     char **argv;
@@ -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.  */
+
+void mi_parse_argv (const char *args, struct mi_parse *parse);
+
 #endif /* MI_MI_PARSE_H */
-- 
2.20.1

Re: [RFC 2/8] Use classes to represent MI Command instead of structures

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> +std::unique_ptr<scoped_restore_tmpl<int>>
Jan> +mi_command::do_suppress_notification ()
Jan> +{
Jan> +  if (m_suppress_notification != NULL)
Jan> +    return std::unique_ptr<scoped_restore_tmpl <int>> (
Jan> +	new scoped_restore_tmpl <int> (m_suppress_notification, 1));
Jan> +
Jan> +  return std::unique_ptr<scoped_restore_tmpl <int>> ();

Rather than returning a unique_ptr, I think this could directly return a
scoped_restore_tmpl<int>.  Then, if m_suppress_notification is NULL,
just use the address of a dummy variable instead.

Jan> +void
Jan> +mi_command_cli::invoke (struct mi_parse *parse)
Jan> +{
Jan> +  std::unique_ptr<scoped_restore_tmpl <int>> restore
Jan> +    = do_suppress_notification ();

Alternatively, if all invoke methods have to start this way, make invoke
non-virtual and then have it do the setup and then call a virtual
do_invoke function.  Then invoke can use a gdb::optional<...>.

Tom

Re: [RFC 2/8] Use classes to represent MI Command instead of structures

[Simon Marchi]

On 2019-04-25 3:25 p.m., Tom Tromey wrote:
> Rather than returning a unique_ptr, I think this could directly return a
> scoped_restore_tmpl<int>.  Then, if m_suppress_notification is NULL,
> just use the address of a dummy variable instead.

Or a gdb::optional?

Simon

Re: [RFC 2/8] Use classes to represent MI Command instead of structures

[Simon Marchi]

On 2019-05-03 6:49 p.m., Simon Marchi wrote:
> On 2019-04-25 3:25 p.m., Tom Tromey wrote:
>> Rather than returning a unique_ptr, I think this could directly return a
>> scoped_restore_tmpl<int>.  Then, if m_suppress_notification is NULL,
>> just use the address of a dummy variable instead.
> 
> Or a gdb::optional?

Oh, just saw that Tom suggested that too:

> Alternatively, if all invoke methods have to start this way, make invoke
> non-virtual and then have it do the setup and then call a virtual
> do_invoke function.  Then invoke can use a gdb::optional<...>.

I think that's a good idea, but do_suppress_notification could return an
optional in any case.

Simon

[RFC 6/8] mi/python: Handle python exception when executiong python-defined MI commands

[Jan Vrany]

Respond with ^error,msg="..." when an unhandled python exception is thrown
while invoking python-defined MI command. The error message is taken from
python exception object.

gdb/Changelog:

	* python/py-micmd.c (py_mi_invoke): Handle exceptions thrown in Python
	code.
---
 gdb/ChangeLog         |  5 +++++
 gdb/python/py-micmd.c | 18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8bbaaba5ad..1fd49e703f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* python/py-micmd.c (py_mi_invoke): Handle exceptions thrown in Python
+	code. 
+
 2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* python/py-micmd.c (parse_mi_result): Polish to make the output
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index bbd746fa32..a099bb48cd 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -115,14 +115,28 @@ py_mi_invoke (void *py_obj, char **argv, int argc)
 	  error (_("Failed to create the python arguments list."));
 	}
     }
-
+  
+  gdb_assert (PyErr_Occurred () == NULL);
   gdbpy_ref<> result (PyObject_CallMethodObjArgs ((PyObject *) obj, invoke_cst, argobj.get (),
 				       NULL));
+  if (PyErr_Occurred () != NULL)
+    {
+      gdbpy_err_fetch ex;
+      gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string());
 
-  if (result != nullptr)
+      if (ex_msg == NULL || *ex_msg == '\0')
+        error (_("Failed to execute command"));
+      else
+        error (_(ex_msg.get ()));
+    }
+  else if (result != nullptr)
     {
       if (Py_None != result) parse_mi_result (result.get (), "result");
     }
+  else
+    {
+      error (_("Command invoke() method returned NULL but no python exception is set"));
+    }
 }
 
 /* Parse the name of the MI command to register.
-- 
2.20.1

Re: [RFC 6/8] mi/python: Handle python exception when executiong python-defined MI commands

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> Respond with ^error,msg="..." when an unhandled python exception is thrown
Jan> while invoking python-defined MI command. The error message is taken from
Jan> python exception object.

Jan> gdb/Changelog:

Jan> 	* python/py-micmd.c (py_mi_invoke): Handle exceptions thrown in Python
Jan> 	code.

This seems reasonable but it also seems like it should just be part of
the patch adding this new function.  If you're worried about joint
authorship, it's pretty common to just list two names in the ChangeLog.

thanks,
Tom

Re: [RFC 6/8] mi/python: Handle python exception when executiong python-defined MI commands

[Jan Vrany]

> This seems reasonable but it also seems like it should just be part of
> the patch adding this new function.  If you're worried about joint
> authorship, it's pretty common to just list two names in the ChangeLog.
> 

Yes, that's the reason why commits are structured this way. I did not want
to change Didier's commuts without him. 

I'll do as you suggested and squash commits. 

Thanks for reviewing! 

Jan

> thanks,
> Tom

Re: [RFC 0/8] Create MI commands using python

[Simon Marchi]

On 2019-04-18 11:23 a.m., Jan Vrany wrote:
> This patch series adds a possibility to create new MI commands using python.
> 
> The code is based on a few year old attempt of Didier Nadeau who did the
> heavy lifting. I merely updated his original code to work with today's GDB,
> add tests and polished it a little.
> 
> At this point, there's no documentation. I expect a discussion and changes
> in behavior and/or output - I'll write it once the rest is agreed on.

Hi Jan,

Thanks for doing this!

Here's the original thread:
https://www.sourceware.org/ml/gdb-patches/2017-02/msg00088.html

One thing Pedro asked for (rightfully), is some kind of rationale: what's the
point of this feature.  I'll try to provide some of it, since I helped Didier
with this prototype at the time (done in an academic context).  Jan, please
complete/correct as needed to share your own point of view.

As a GDB frontend developer, you want to display some application-specific data
in your nice GUI frontend.  That data is extracted from the application's data
structure in memory, and possibly from doing some kind of bookkeeping (putting
breakpoints at strategic points to record things) or whatnot.

Today, it would be possible to do all of this in the front-end: you can read from
memory and evaluate expressions to extract data from data structures in memory
and you can put breakpoints to catch important events (which you try to hide from
the user by resuming execution).

The downside to this is that all the logic to reconstitute legible data from the
data structures may be non-trivial, and all that logic will be in the front-end.
If you want to support more than one front-end, or want to provide some similar
feature from the command line as well, it will have to be duplicated.

So the idea is to have a single implementation in Python, and have it accessible
to the frontend.  Today, you could implement a custom CLI command in Python and
call it from MI, from your frontend.  This is not ideal because the output would
be CLI output, difficult to parse from MI (it's hard to know which output comes
from which command).

So the idea would be for front-ends to be able to load some scripts like:

class MyMICommand(gdb.MICommand):
  def __init__(self):
    super('-my-mi-command')

  def invoke(self, args):
    return {'foo': 'bar'}

MyMICommand()

This would define the '-my-mi-command' command, which when called, would return:

  123-my-my-command
  123^done,foo="bar"

So to answer one question Pedro had:

> I suppose they'll build the MI output "manually" ?

Ideally not.  They return a Python object (dict, list, string, int) which map pretty
easily to MI data types.

Simon

Re: [RFC 0/8] Create MI commands using python

[Jan Vrany]

Hi,

On Thu, 2019-04-18 at 12:03 -0400, Simon Marchi wrote:
> On 2019-04-18 11:23 a.m., Jan Vrany wrote:
> > This patch series adds a possibility to create new MI commands using python.
> > 
> > The code is based on a few year old attempt of Didier Nadeau who did the
> > heavy lifting. I merely updated his original code to work with today's GDB,
> > add tests and polished it a little.
> > 
> > At this point, there's no documentation. I expect a discussion and changes
> > in behavior and/or output - I'll write it once the rest is agreed on.
> 
> Hi Jan,
> 
> Thanks for doing this!
> 
> Here's the original thread:
> https://www.sourceware.org/ml/gdb-patches/2017-02/msg00088.html
> 
> One thing Pedro asked for (rightfully), is some kind of rationale: what's the
> point of this feature.  I'll try to provide some of it, since I helped Didier
> with this prototype at the time (done in an academic context).  Jan, please
> complete/correct as needed to share your own point of view.
> 
> As a GDB frontend developer, you want to display some application-specific data
> in your nice GUI frontend.  That data is extracted from the application's data
> structure in memory, and possibly from doing some kind of bookkeeping (putting
> breakpoints at strategic points to record things) or whatnot.
> 
> Today, it would be possible to do all of this in the front-end: you can read from
> memory and evaluate expressions to extract data from data structures in memory
> and you can put breakpoints to catch important events (which you try to hide from
> the user by resuming execution).
> 
> The downside to this is that all the logic to reconstitute legible data from the
> data structures may be non-trivial, and all that logic will be in the front-end.
> If you want to support more than one front-end, or want to provide some similar
> feature from the command line as well, it will have to be duplicated.
> 
> So the idea is to have a single implementation in Python, and have it accessible
> to the frontend.  Today, you could implement a custom CLI command in Python and
> call it from MI, from your frontend.  This is not ideal because the output would
> be CLI output, difficult to parse from MI (it's hard to know which output comes
> from which command).
> erSo the idea would be for front-ends to be able to load some scripts like:
> 

Exactly! I could not have said it better. Another aspect may be performance - doing 
extensive data structure analysis by walking variable object and/or inferior memory 
in frontend can easily result in hundreds of MI commands being sent. 

> class MyMICommand(gdb.MICommand):
>   def __init__(self):
>     super('-my-mi-command')
> 
>   def invoke(self, args):
>     return {'foo': 'bar'}
> 
> MyMICommand()
> 
> This would define the '-my-mi-command' command, which when called, would return:
> 
>   123-my-my-command
>   123^done,foo="bar"
> 
> So to answer one question Pedro had:
> 
> > I suppose they'll build the MI output "manually" ?
> 
> Ideally not.  They return a Python object (dict, list, string, int) which map pretty
> easily to MI data types.

Exactly. See parse_mi_result() in python/py-micmd.c
( https://bitbucket.org/janvrany/binutils-gdb/src/67fea07b27f59b5eb1dd99fe340d8222521e7df2/gdb/python/py-micmd.c?at=users%2Fjv%2Fvdb&fileviewer=file-view-default#py-micmd.c-36 )

To support even more OO style of programming, we can say that for 
any other object ("generic" case in the code above), a define API
method - say .to_mi_record() - which must return one of the recognized
objects (string, int, sequence, dict, iterator). 

I decided not to implement this until I get more experience with using 
Python-defined MI commands for real. 

Jan

[RFC 5/8] mi/python: Polish MI output of python commands

[Jan Vrany]

This commits makes MI representation of python-implemented MI command
more "natural". Add support for tuples, iterators and generic sequences.

gdb/Changelog:

	* python/py-micmd.c (parse_mi_result): Polish to make the output
	more "natural". Add support for tuples, iterators and general sequences.
	(py_mi_invoke): Do not call Py_DECREF on argument strings since
	PyList_SetItem borrows the reference. If python MI command returns
	None, do not print the result on MI output channel and just respond with
	^done.
---
 gdb/ChangeLog         |  7 ++++++
 gdb/python/py-micmd.c | 54 +++++++++++++++++++++++++------------------
 2 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 692b937fee..8bbaaba5ad 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* python/py-micmd.c (parse_mi_result): Polish to make the output
+	more "natural". Add support for tuples, iterators and general sequences.
+	(py_mi_invoke): If python MI command returns None, do not print the
+	result on MI output channel and just respond with "^done".
+
 2018-12-11  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* python/py-micmd.c (parse_mi_result): Use gdb::unique_xmalloc_ptr<>
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 0683a02017..bbd746fa32 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -37,37 +37,47 @@ parse_mi_result (PyObject *result, const char *field_name)
 {
   struct ui_out *uiout = current_uiout;
 
-  if(!field_name)
-    field_name = "default";
-
-  if (PyString_Check(result))
-    {
-      const char *string = gdbpy_obj_to_string (result).release ();
-      uiout->field_string (field_name, string);
-      xfree ( (void *)string);
-    }
-  else if (PyList_Check (result))
+  if (PyString_Check (result))
     {
-      PyObject *item;
-      Py_ssize_t i = 0;      
-      ui_out_emit_list list_emitter (uiout, field_name);
-      for(i = 0; i < PyList_GET_SIZE (result); ++i)
-	{
-          ui_out_emit_tuple tuple_emitter (uiout, NULL);
-	  item = PyList_GetItem (result, i);
-	  parse_mi_result (item, NULL);
-	}      
+      goto generic;
     }
-  else if ( PyDict_Check (result))
+  else if (PyDict_Check (result))
     {
       PyObject *key, *value;
       Py_ssize_t pos = 0;
+      ui_out_emit_tuple tuple_emitter (uiout, field_name);
       while ( PyDict_Next (result, &pos, &key, &value) )
 	{
 	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));
-	  parse_mi_result (value, key_string.get ());
+          parse_mi_result (value, key_string.get ());
 	}
     }
+  else if (PySequence_Check (result))
+    {
+      PyObject *item;
+      Py_ssize_t i = 0;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      for(i = 0; i < PySequence_Size (result); ++i)
+        {
+          item = PySequence_GetItem (result, i);
+          parse_mi_result (item, NULL);
+        }
+    }
+  else if (PyIter_Check (result))
+    {
+      gdbpy_ref<> item;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      while (item.reset(PyIter_Next (result)) , item != nullptr)
+        {
+          parse_mi_result (item.get (), NULL);
+        }
+    }
+  else
+    {
+      generic:
+      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
+      uiout->field_string (field_name, string.get ());
+    }
 }
 
 /* Called from mi_command_py's invoke to invoke the command.  */
@@ -111,7 +121,7 @@ py_mi_invoke (void *py_obj, char **argv, int argc)
 
   if (result != nullptr)
     {
-      parse_mi_result (result.get (), NULL);
+      if (Py_None != result) parse_mi_result (result.get (), "result");
     }
 }
 
-- 
2.20.1

Re: [RFC 5/8] mi/python: Polish MI output of python commands

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> This commits makes MI representation of python-implemented MI command
Jan> more "natural". Add support for tuples, iterators and generic sequences.

I think this one should also be merged into an earlier one.

Tom

Re: [RFC 0/8] Create MI commands using python

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> This patch series adds a possibility to create new MI commands using python.
Jan> The code is based on a few year old attempt of Didier Nadeau who did the
Jan> heavy lifting. I merely updated his original code to work with today's GDB,
Jan> add tests and polished it a little.

Does Didier have a copyright assignment in place?
That's a requirement for a larger patch.

thanks,
Tom

Re: [RFC 0/8] Create MI commands using python

[Simon Marchi]

On 2019-04-25 2:03 p.m., Tom Tromey wrote:
>>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> 
> Jan> This patch series adds a possibility to create new MI commands using python.
> Jan> The code is based on a few year old attempt of Didier Nadeau who did the
> Jan> heavy lifting. I merely updated his original code to work with today's GDB,
> Jan> add tests and polished it a little.
> 
> Does Didier have a copyright assignment in place?
> That's a requirement for a larger patch.

I just checked - yes it did, good news!

Simon

Re: [RFC 0/8] Create MI commands using python

[Simon Marchi]

On 2019-04-25 3:00 p.m., Simon Marchi wrote:
> On 2019-04-25 2:03 p.m., Tom Tromey wrote:
>>>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
>>
>> Jan> This patch series adds a possibility to create new MI commands using python.
>> Jan> The code is based on a few year old attempt of Didier Nadeau who did the
>> Jan> heavy lifting. I merely updated his original code to work with today's GDB,
>> Jan> add tests and polished it a little.
>>
>> Does Didier have a copyright assignment in place?
>> That's a requirement for a larger patch.
> 
> I just checked - yes it did, good news!

s/ it / he / !

[PATCH v2 5/5] mi/python: Add tests for python-defined MI commands

[Jan Vrany]

gdb/testsuite/Changelog:

	* gdb.python/py-mi-cmd.exp: New file.
	* gdb.python/py-mi-cmd.py: New file.
---
 gdb/testsuite/ChangeLog                |   5 ++
 gdb/testsuite/gdb.python/py-mi-cmd.exp | 109 +++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-mi-cmd.py  |  39 +++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.exp
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.py

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d46422635b..935db3bc3b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-05-03  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.python/py-mi-cmd.exp: New file.
+	* gdb.python/py-mi-cmd.py: New file.
+
 2019-04-15  Leszek Swirski  <leszeks@google.com>
 
 	* gdb.arch/amd64-eval.cc: New file.
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.exp b/gdb/testsuite/gdb.python/py-mi-cmd.exp
new file mode 100644
index 0000000000..e6226c4c63
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd.exp
@@ -0,0 +1,109 @@
+# Copyright (C) 2018 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test custom MI commands implemented in Python.
+
+load_lib gdb-python.exp
+load_lib mi-support.exp
+set MIFLAGS "-i=mi2"
+
+gdb_exit
+if {[mi_gdb_start]} {
+    continue
+}
+
+if {[lsearch -exact [mi_get_features] python] < 0} {
+    unsupported "python support is disabled"
+    return -1
+}
+
+standard_testfile
+#
+# Start here
+#
+
+
+mi_gdb_test "set python print-stack full" \
+  ".*\\^done" \
+  "set python print-stack full"
+
+mi_gdb_test "source ${srcdir}/${subdir}/${testfile}.py" \
+  ".*\\^done" \
+  "load python file"
+
+mi_gdb_test "python pycmd1('-pycmd')" \
+  ".*\\^done" \
+  "Define -pycmd MI command"
+
+
+mi_gdb_test "-pycmd int" \
+  "\\^done,result=\"42\"" \
+  "-pycmd int"
+
+mi_gdb_test "-pycmd str" \
+  "\\^done,result=\"Hello world!\"" \
+  "-pycmd str"
+
+mi_gdb_test "-pycmd ary" \
+  "\\^done,result=\\\[\"Hello\",\"42\"\\\]" \
+  "-pycmd ary"
+
+mi_gdb_test "-pycmd dct" \
+  "\\^done,result={hello=\"world\",times=\"42\"}" \
+  "-pycmd dct"
+
+mi_gdb_test "-pycmd tpl" \
+  "\\^done,result=\\\[\"42\",\"Hello\"\\\]" \
+  "-pycmd tpl"
+
+mi_gdb_test "-pycmd itr" \
+  "\\^done,result=\\\[\"1\",\"2\",\"3\"\\\]" \
+  "-pycmd itr"
+
+mi_gdb_test "-pycmd nn1" \
+  "\\^done" \
+  "-pycmd nn1"
+
+mi_gdb_test "-pycmd nn2" \
+  "\\^done,result=\\\[\"None\"\\\]" \
+  "-pycmd nn2"
+
+mi_gdb_test "-pycmd bogus" \
+  "\\^error,msg=\"-pycmd: Invalid parameter: bogus\"" \
+  "-pycmd bogus"
+
+mi_gdb_test "-pycmd exp" \
+  "\\^error,msg=\"-pycmd: failed to execute command\"" \
+  "-pycmd exp"
+
+mi_gdb_test "python pycmd2('-pycmd')" \
+  ".*\\^done" \
+  "Redefine -pycmd MI command from CLI command"
+
+mi_gdb_test "-pycmd str" \
+  "\\^done,result=\"Ciao!\"" \
+  "-pycmd str"
+
+mi_gdb_test "-pycmd int" \
+  "\\^error,msg=\"-pycmd: Invalid parameter: int\"" \
+  "-pycmd int"
+
+mi_gdb_test "-pycmd red" \
+    "\\^error,msg=\"-pycmd: Command redefined but we failing anyway\"" \
+  "Redefine -pycmd MI command from Python MI command"
+
+mi_gdb_test "-pycmd int" \
+  "\\^done,result=\"42\"" \
+  "-pycmd int"
+
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.py b/gdb/testsuite/gdb.python/py-mi-cmd.py
new file mode 100644
index 0000000000..76ab46c97d
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd.py
@@ -0,0 +1,39 @@
+import gdb
+
+class pycmd1(gdb.MICommand):
+    def invoke(self, argv):
+        if argv[0] == 'int':
+            return 42
+        elif argv[0] == 'str':
+            return "Hello world!"
+        elif argv[0] == 'ary':
+            return [ 'Hello', 42 ]
+        elif argv[0] == "dct":
+            return { 'hello' : 'world', 'times' : 42}
+        elif argv[0] == 'tpl':
+            return ( 42 , 'Hello' )
+        elif argv[0] == 'itr':
+            return iter([1,2,3])
+        elif argv[0] == 'nn1':
+            return None
+        elif argv[0] == 'nn2':
+            return [ None ]
+        elif argv[0] == 'red':
+            pycmd2('-pycmd')
+            return None
+        elif argv[0] == 'exp':
+            raise gdb.GdbError()
+        else:
+            raise gdb.GdbError("Invalid parameter: %s" % argv[0])
+
+
+class pycmd2(gdb.MICommand):
+    def invoke(self, argv):
+        if argv[0] == 'str':
+            return "Ciao!"
+        elif argv[0] == 'red':
+            pycmd1('-pycmd')
+            raise gdb.GdbError("Command redefined but we failing anyway")
+        else:
+            raise gdb.GdbError("Invalid parameter: %s" % argv[0])
+
-- 
2.20.1

[PATCH v2 3/5] Create MI commands using python.

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This commit allows an user to create custom MI commands using Python
similarly to what is possible for Python CLI commands.

A new subclass of mi_command is defined for Python MI commands,
mi_command_py. A new file, py-micmd.c contains the logic for Python
MI commands.

gdb/ChangeLog

    * Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
    * mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
    (mi_cmd_table): Remove static.
    * mi/mi-cmds.h (insert_mi_cmd_entry): New declaration.
    (mi_cmd_table): New declaration.
    * python/py-micmd.c: New file
    (parse_mi_result): New function.
    (micmdpy_init): New function.
    (gdbpy_initialize_micommands): New function.
    (mi_command_py): New class.
    * python/py-micmd.h: New file
    (micmdpy_object): New struct.
    (micmdpy_object): New typedef.
    (mi_command_py): New class.
    * python/python-internal.h
    (gdbpy_initialize_micommands): New declaration.
    * python/python.c
    (_initialize_python): New call to gdbpy_initialize_micommands.
    (finalize_python): Finalize Python MI commands.
---
 gdb/ChangeLog                |  23 +++
 gdb/Makefile.in              |   1 +
 gdb/mi/mi-cmds.c             |   7 +-
 gdb/mi/mi-cmds.h             |   9 ++
 gdb/python/py-micmd.c        | 300 +++++++++++++++++++++++++++++++++++
 gdb/python/py-micmd.h        |  51 ++++++
 gdb/python/python-internal.h |   2 +
 gdb/python/python.c          |  13 +-
 8 files changed, 401 insertions(+), 5 deletions(-)
 create mode 100644 gdb/python/py-micmd.c
 create mode 100644 gdb/python/py-micmd.h

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 03eb42555e..4cbe97002b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,26 @@
+2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
+            Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
+	* mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
+	(mi_cmd_table): Remove static.
+	* mi/mi-cmds.h (insert_mi_cmd_entry): New declaration.
+	(mi_cmd_table): New declaration.
+	* python/py-micmd.c: New file
+	(parse_mi_result): New function.
+	(micmdpy_init): New function.
+	(gdbpy_initialize_micommands): New function.
+	(mi_command_py): New class.
+	* python/py-micmd.h: New file
+	(micmdpy_object): New struct.
+	(micmdpy_object): New typedef.
+	(mi_command_py): New class.
+	* python/python-internal.h
+	(gdbpy_initialize_micommands): New declaration.
+	* python/python.c
+	(_initialize_python): New call to gdbpy_initialize_micommands.
+	(finalize_python): Finalize Python MI commands.
+
 2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
 	    Jan Vrany <jan.vrany@fit.cvut.cz>
 
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0f49578360..28866962bc 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -382,6 +382,7 @@ SUBDIR_PYTHON_SRCS = \
 	python/py-instruction.c \
 	python/py-lazy-string.c \
 	python/py-linetable.c \
+	python/py-micmd.c \
 	python/py-newobjfileevent.c \
 	python/py-objfile.c \
 	python/py-param.c \
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 44f3813fdc..7e1d2c3f2d 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -28,12 +28,11 @@
 
 /* MI command table (built at run time). */
 
-static std::map<std::string, mi_cmd_up> mi_cmd_table;
+std::map<std::string, mi_cmd_up> mi_cmd_table;
 
-/* Insert a new mi-command into the command table.  Return true if
-   insertion was successful.  */
+/* See mi-cmds.h.  */
 
-static bool
+bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != NULL);
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index fc432a16b9..a2757bae20 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -22,6 +22,8 @@
 #ifndef MI_MI_CMDS_H
 #define MI_MI_CMDS_H
 
+#include <map>
+
 enum print_values {
    PRINT_NO_VALUES,
    PRINT_ALL_VALUES,
@@ -190,9 +192,16 @@ typedef std::unique_ptr<mi_command> mi_cmd_up;
 
 extern mi_command *mi_cmd_lookup (const char *command);
 
+extern std::map<std::string, mi_cmd_up> mi_cmd_table;
+
 /* Debug flag */
 extern int mi_debug_p;
 
 extern void mi_execute_command (const char *cmd, int from_tty);
 
+/* Insert a new mi-command into the command table.  Return true if
+   insertion was successful.  */
+
+extern bool insert_mi_cmd_entry (mi_cmd_up command);
+
 #endif /* MI_MI_CMDS_H */
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
new file mode 100644
index 0000000000..c8d429bb4b
--- /dev/null
+++ b/gdb/python/py-micmd.c
@@ -0,0 +1,300 @@
+/* MI Command Set for GDB, the GNU debugger.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* gdb MI commands implemented in Python  */
+
+#include "defs.h"
+#include "python-internal.h"
+#include "python/py-micmd.h"
+#include "arch-utils.h"
+#include "charset.h"
+#include "language.h"
+
+#include <string>
+
+static PyObject *invoke_cst;
+
+extern PyTypeObject
+  micmdpy_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");
+
+/* If the command invoked returns a list, this function parses it and create an
+   appropriate MI out output.
+
+   The returned values must be Python string, and can be contained within Python
+   lists and dictionaries. It is possible to have a multiple levels of lists
+   and/or dictionaries.  */
+
+static void
+parse_mi_result (PyObject *result, const char *field_name)
+{
+  struct ui_out *uiout = current_uiout;
+
+  if (PyString_Check (result))
+    {
+      goto generic;
+    }
+  else if (PyDict_Check (result))
+    {
+      PyObject *key, *value;
+      Py_ssize_t pos = 0;
+      ui_out_emit_tuple tuple_emitter (uiout, field_name);
+      while (PyDict_Next (result, &pos, &key, &value))
+	{
+	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));
+	  parse_mi_result (value, key_string.get ());
+	}
+    }
+  else if (PySequence_Check (result))
+    {
+      PyObject *item;
+      Py_ssize_t i = 0;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      for (i = 0; i < PySequence_Size (result); ++i)
+	{
+	  item = PySequence_GetItem (result, i);
+	  parse_mi_result (item, NULL);
+	}
+    }
+  else if (PyIter_Check (result))
+    {
+      gdbpy_ref<> item;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      while (item.reset (PyIter_Next (result)), item != nullptr)
+	{
+	  parse_mi_result (item.get (), NULL);
+	}
+    }
+  else
+    {
+    generic:
+      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
+      uiout->field_string (field_name, string.get ());
+    }
+}
+
+/* Object initializer; sets up gdb-side structures for MI command.
+
+   Use: __init__(NAME).
+
+   NAME is the name of the MI command to register.  It must start with a dash
+   as a traditional MI commands do.  */
+
+static int
+micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
+{
+  const char *name;
+
+  if (!PyArg_ParseTuple (args, "s", &name))
+    return -1;
+
+  /* Validate command name */
+  const int name_len = strlen (name);
+  if (name_len < 2)
+    {
+      PyErr_Format (PyExc_RuntimeError, _("MI command name is empty"));
+      return -1;
+    }
+  else if ((name[0] != '-') || !isalnum (name[1]))
+    {
+      PyErr_Format (PyExc_RuntimeError,
+		    _("MI command name does not start with '-'"
+		      " followed by a letter or digit"));
+      return -1;
+    }
+  else
+    for (int i = 2; i < name_len; i++)
+      {
+	if (!isalnum (name[i]) && name[i] != '-')
+	  {
+	    PyErr_Format (PyExc_RuntimeError,
+			  _("MI command name contains invalid character: %c"),
+			  name[i]);
+	    return -1;
+	  }
+      }
+
+  gdbpy_ref<> self_ref = gdbpy_ref<>::new_reference (self);
+  try
+    {
+      mi_command *micommand = new mi_command_py (name + 1, NULL, self_ref);
+
+      bool result = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
+
+      if (!result)
+	{
+	  PyErr_Format (
+	    PyExc_RuntimeError,
+	    _("Unable to insert command. The name might already be in use."));
+	  return -1;
+	}
+    }
+  catch (const gdb_exception &except)
+    {
+      GDB_PY_SET_HANDLE_EXCEPTION (except);
+    }
+
+  return 0;
+}
+
+mi_command_py::mi_command_py (const char *name, int *suppress_notification,
+			      gdbpy_ref<> object)
+  : mi_command (name, suppress_notification), pyobj (object)
+{
+}
+
+void
+mi_command_py::invoke (struct mi_parse *parse)
+{
+  std::unique_ptr<scoped_restore_tmpl<int>> restore
+    = do_suppress_notification ();
+
+  mi_parse_argv (parse->args, parse);
+
+  if (parse->argv == NULL)
+    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
+
+  PyObject *obj = this->pyobj.get ();
+  int i;
+
+  gdbpy_enter enter_py (get_current_arch (), current_language);
+
+  if (!obj)
+    error (_("-%s: invalid invocation of Python micommand object."),
+	   name ().c_str ());
+
+  if (!PyObject_HasAttr (obj, invoke_cst))
+    {
+      error (_("-%s: python command object missing 'invoke' method."),
+	     name ().c_str ());
+    }
+
+  gdbpy_ref<> argobj (PyList_New (parse->argc));
+  if (argobj == nullptr)
+    {
+      gdbpy_print_stack ();
+      error (_("-%s: failed to create the python arguments list."),
+	     name ().c_str ());
+    }
+
+  for (i = 0; i < parse->argc; ++i)
+    {
+      /* Since PyList_SetItem steals the reference, we don't use
+       * gdbpy_ref<> to hold on arg string. */
+      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
+					host_charset (), NULL);
+      if (PyList_SetItem (argobj.get (), i, str) != 0)
+	{
+	  error (_("-%s: failed to create the python arguments list."),
+		 name ().c_str ());
+	}
+    }
+
+  gdb_assert (PyErr_Occurred () == NULL);
+  gdbpy_ref<> result (
+    PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
+  if (PyErr_Occurred () != NULL)
+    {
+      gdbpy_err_fetch ex;
+      gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
+
+      if (ex_msg == NULL || *ex_msg == '\0')
+	error (_("-%s: failed to execute command"), name ().c_str ());
+      else
+	error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
+    }
+  else if (result != nullptr)
+    {
+      if (Py_None != result)
+	parse_mi_result (result.get (), "result");
+    }
+  else
+    {
+      error (
+	_("-%s: command invoke() method returned NULL but no python exception "
+	  "is set"),
+	name ().c_str ());
+    }
+}
+
+void mi_command_py::finalize ()
+{
+  this->pyobj.reset (nullptr);
+}
+
+/* Initialize the MI command object.  */
+
+int
+gdbpy_initialize_micommands ()
+{
+  micmdpy_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&micmdpy_object_type) < 0)
+    return -1;
+
+  if (gdb_pymodule_addobject (gdb_module, "MICommand",
+			      (PyObject *) &micmdpy_object_type)
+      < 0)
+    return -1;
+
+  invoke_cst = PyString_FromString ("invoke");
+  if (invoke_cst == NULL)
+    return -1;
+
+  return 0;
+}
+
+static PyMethodDef micmdpy_object_methods[] = {{0}};
+
+PyTypeObject micmdpy_object_type = {
+  PyVarObject_HEAD_INIT (NULL, 0) "gdb.MICommand", /*tp_name */
+  sizeof (micmdpy_object),			   /*tp_basicsize */
+  0,						   /*tp_itemsize */
+  0,						   /*tp_dealloc */
+  0,						   /*tp_print */
+  0,						   /*tp_getattr */
+  0,						   /*tp_setattr */
+  0,						   /*tp_compare */
+  0,						   /*tp_repr */
+  0,						   /*tp_as_number */
+  0,						   /*tp_as_sequence */
+  0,						   /*tp_as_mapping */
+  0,						   /*tp_hash */
+  0,						   /*tp_call */
+  0,						   /*tp_str */
+  0,						   /*tp_getattro */
+  0,						   /*tp_setattro */
+  0,						   /*tp_as_buffer */
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,	/*tp_flags */
+  "GDB mi-command object",			   /* tp_doc */
+  0,						   /* tp_traverse */
+  0,						   /* tp_clear */
+  0,						   /* tp_richcompare */
+  0,						   /* tp_weaklistoffset */
+  0,						   /* tp_iter */
+  0,						   /* tp_iternext */
+  micmdpy_object_methods,			   /* tp_methods */
+  0,						   /* tp_members */
+  0,						   /* tp_getset */
+  0,						   /* tp_base */
+  0,						   /* tp_dict */
+  0,						   /* tp_descr_get */
+  0,						   /* tp_descr_set */
+  0,						   /* tp_dictoffset */
+  micmdpy_init,					   /* tp_init */
+  0,						   /* tp_alloc */
+};
diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h
new file mode 100644
index 0000000000..418de41a10
--- /dev/null
+++ b/gdb/python/py-micmd.h
@@ -0,0 +1,51 @@
+/* MI Command Set for GDB, the GNU debugger.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef PY_MICMDS_H
+#define PY_MICMDS_H
+
+#include "mi/mi-cmds.h"
+#include "mi/mi-parse.h"
+#include "python-internal.h"
+#include "python/py-ref.h"
+
+struct micmdpy_object
+{
+  PyObject_HEAD
+};
+
+typedef struct micmdpy_object micmdpy_object;
+
+/* MI command implemented on top of a Python command.  */
+class mi_command_py : public mi_command
+{
+public:
+  mi_command_py (const char *name, int *suppress_notification,
+		 gdbpy_ref<> object);
+  void invoke (struct mi_parse *parse) override;
+
+  /* This is called just before shutting down a Python interpreter
+     to release python object implementing the command */
+  void finalize ();
+
+private:
+  gdbpy_ref<> pyobj;
+};
+
+#endif
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 449926ca87..26606b4b36 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -539,6 +539,8 @@ int gdbpy_initialize_xmethods (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_unwind (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+int gdbpy_initialize_micommands (void)
+  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 
 /* A wrapper for PyErr_Fetch that handles reference counting for the
    caller.  */
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4dad8ec10d..ee89c603a5 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -36,6 +36,8 @@
 #include <ctype.h>
 #include "location.h"
 #include "ser-event.h"
+#include "mi/mi-cmds.h"
+#include "py-micmd.h"
 
 /* Declared constants and enum for python stack printing.  */
 static const char python_excp_none[] = "none";
@@ -1564,6 +1566,14 @@ finalize_python (void *ignore)
   python_gdbarch = target_gdbarch ();
   python_language = current_language;
 
+  for (auto const& name_and_cmd : mi_cmd_table)
+    {
+      mi_command    *cmd = name_and_cmd.second.get ();
+      mi_command_py *cmd_py = dynamic_cast<mi_command_py*> (cmd);
+      if (cmd_py != nullptr)
+        cmd_py->finalize ();
+    }
+
   Py_Finalize ();
 
   restore_active_ext_lang (previous_active);
@@ -1698,7 +1708,8 @@ do_start_initialization ()
       || gdbpy_initialize_event () < 0
       || gdbpy_initialize_arch () < 0
       || gdbpy_initialize_xmethods () < 0
-      || gdbpy_initialize_unwind () < 0)
+      || gdbpy_initialize_unwind () < 0
+      || gdbpy_initialize_micommands () < 0)
     return false;
 
 #define GDB_PY_DEFINE_EVENT_TYPE(name, py_name, doc, base)	\
-- 
2.20.1

Re: [PATCH v2 3/5] Create MI commands using python.

[Simon Marchi]

On 2019-05-14 7:24 a.m., Jan Vrany wrote:
> From: Didier Nadeau <didier.nadeau@gmail.com>
> 
> This commit allows an user to create custom MI commands using Python
> similarly to what is possible for Python CLI commands.
> 
> A new subclass of mi_command is defined for Python MI commands,
> mi_command_py. A new file, py-micmd.c contains the logic for Python
> MI commands.
> 
> gdb/ChangeLog
> 
>     * Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
>     * mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
>     (mi_cmd_table): Remove static.
>     * mi/mi-cmds.h (insert_mi_cmd_entry): New declaration.
>     (mi_cmd_table): New declaration.
>     * python/py-micmd.c: New file
>     (parse_mi_result): New function.
>     (micmdpy_init): New function.
>     (gdbpy_initialize_micommands): New function.
>     (mi_command_py): New class.
>     * python/py-micmd.h: New file
>     (micmdpy_object): New struct.
>     (micmdpy_object): New typedef.
>     (mi_command_py): New class.
>     * python/python-internal.h
>     (gdbpy_initialize_micommands): New declaration.
>     * python/python.c
>     (_initialize_python): New call to gdbpy_initialize_micommands.
>     (finalize_python): Finalize Python MI commands.
> ---
>  gdb/ChangeLog                |  23 +++
>  gdb/Makefile.in              |   1 +
>  gdb/mi/mi-cmds.c             |   7 +-
>  gdb/mi/mi-cmds.h             |   9 ++
>  gdb/python/py-micmd.c        | 300 +++++++++++++++++++++++++++++++++++
>  gdb/python/py-micmd.h        |  51 ++++++
>  gdb/python/python-internal.h |   2 +
>  gdb/python/python.c          |  13 +-
>  8 files changed, 401 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/python/py-micmd.c
>  create mode 100644 gdb/python/py-micmd.h
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 03eb42555e..4cbe97002b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,26 @@
> +2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
> +            Jan Vrany  <jan.vrany@fit.cvut.cz>
> +
> +	* Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
> +	* mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
> +	(mi_cmd_table): Remove static.
> +	* mi/mi-cmds.h (insert_mi_cmd_entry): New declaration.
> +	(mi_cmd_table): New declaration.
> +	* python/py-micmd.c: New file
> +	(parse_mi_result): New function.
> +	(micmdpy_init): New function.
> +	(gdbpy_initialize_micommands): New function.
> +	(mi_command_py): New class.
> +	* python/py-micmd.h: New file
> +	(micmdpy_object): New struct.
> +	(micmdpy_object): New typedef.
> +	(mi_command_py): New class.
> +	* python/python-internal.h
> +	(gdbpy_initialize_micommands): New declaration.
> +	* python/python.c
> +	(_initialize_python): New call to gdbpy_initialize_micommands.
> +	(finalize_python): Finalize Python MI commands.
> +
>  2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
>  	    Jan Vrany <jan.vrany@fit.cvut.cz>
>  
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 0f49578360..28866962bc 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -382,6 +382,7 @@ SUBDIR_PYTHON_SRCS = \
>  	python/py-instruction.c \
>  	python/py-lazy-string.c \
>  	python/py-linetable.c \
> +	python/py-micmd.c \
>  	python/py-newobjfileevent.c \
>  	python/py-objfile.c \
>  	python/py-param.c \
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 44f3813fdc..7e1d2c3f2d 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -28,12 +28,11 @@
>  
>  /* MI command table (built at run time). */
>  
> -static std::map<std::string, mi_cmd_up> mi_cmd_table;
> +std::map<std::string, mi_cmd_up> mi_cmd_table;
>  
> -/* Insert a new mi-command into the command table.  Return true if
> -   insertion was successful.  */
> +/* See mi-cmds.h.  */
>  
> -static bool
> +bool
>  insert_mi_cmd_entry (mi_cmd_up command)
>  {
>    gdb_assert (command != NULL);
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index fc432a16b9..a2757bae20 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -22,6 +22,8 @@
>  #ifndef MI_MI_CMDS_H
>  #define MI_MI_CMDS_H
>  
> +#include <map>
> +
>  enum print_values {
>     PRINT_NO_VALUES,
>     PRINT_ALL_VALUES,
> @@ -190,9 +192,16 @@ typedef std::unique_ptr<mi_command> mi_cmd_up;
>  
>  extern mi_command *mi_cmd_lookup (const char *command);
>  
> +extern std::map<std::string, mi_cmd_up> mi_cmd_table;
> +
>  /* Debug flag */
>  extern int mi_debug_p;
>  
>  extern void mi_execute_command (const char *cmd, int from_tty);
>  
> +/* Insert a new mi-command into the command table.  Return true if
> +   insertion was successful.  */
> +
> +extern bool insert_mi_cmd_entry (mi_cmd_up command);
> +
>  #endif /* MI_MI_CMDS_H */
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> new file mode 100644
> index 0000000000..c8d429bb4b
> --- /dev/null
> +++ b/gdb/python/py-micmd.c
> @@ -0,0 +1,300 @@
> +/* MI Command Set for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* gdb MI commands implemented in Python  */
> +
> +#include "defs.h"
> +#include "python-internal.h"
> +#include "python/py-micmd.h"
> +#include "arch-utils.h"
> +#include "charset.h"
> +#include "language.h"
> +
> +#include <string>
> +
> +static PyObject *invoke_cst;
> +
> +extern PyTypeObject
> +  micmdpy_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");
> +
> +/* If the command invoked returns a list, this function parses it and create an
> +   appropriate MI out output.
> +
> +   The returned values must be Python string, and can be contained within Python
> +   lists and dictionaries. It is possible to have a multiple levels of lists
> +   and/or dictionaries.  */
> +
> +static void
> +parse_mi_result (PyObject *result, const char *field_name)
> +{
> +  struct ui_out *uiout = current_uiout;
> +
> +  if (PyString_Check (result))
> +    {
> +      goto generic;

Hmm is this goto really necessary?  Can't you just remove this if, and execution will
go directly in the else?  Or is a string considered a sequence by PySequence_Check?

In any case, there's gotta be a better way to organize this that doesn't require the goto.

> +    }
> +  else if (PyDict_Check (result))
> +    {
> +      PyObject *key, *value;
> +      Py_ssize_t pos = 0;
> +      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> +      while (PyDict_Next (result, &pos, &key, &value))
> +	{
> +	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));

When parsing a dict, I think it would be good to enforce that keys are strings, I
don't really see the use case to allow arbitrary objects there.  In the MI, keys
will be strings, and I don't really see the use case of letting the user do

return {
  MyObject(): "foo"
}

where the str(MyObject()) will be used as the key.  By validating that keys are
strings, I think we can help the user catch errors earlier in development.

> +	  parse_mi_result (value, key_string.get ());
> +	}
> +    }
> +  else if (PySequence_Check (result))
> +    {
> +      PyObject *item;
> +      Py_ssize_t i = 0;

Declare these variables in the most specific scope possible (the for loop header for i,
in the for loop for item).

> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      for (i = 0; i < PySequence_Size (result); ++i)
> +	{
> +	  item = PySequence_GetItem (result, i);

PySequence_GetItem returns a new reference, which must be dropped.

Also, you could probably use PySequence_ITEM which will be a bit faster.  It should be
safe, since we know result is a PySequence and we don't use negative indices.

> +	  parse_mi_result (item, NULL);
> +	}
> +    }
> +  else if (PyIter_Check (result))
> +    {
> +      gdbpy_ref<> item;
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      while (item.reset (PyIter_Next (result)), item != nullptr)
> +	{
> +	  parse_mi_result (item.get (), NULL);
> +	}

Remove extra curly braces.

> +    }
> +  else
> +    {
> +    generic:
> +      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
> +      uiout->field_string (field_name, string.get ());
> +    }
> +}
> +
> +/* Object initializer; sets up gdb-side structures for MI command.
> +
> +   Use: __init__(NAME).
> +
> +   NAME is the name of the MI command to register.  It must start with a dash
> +   as a traditional MI commands do.  */

"as traditional MI commands", drop the "a"

> +
> +static int
> +micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  const char *name;
> +
> +  if (!PyArg_ParseTuple (args, "s", &name))
> +    return -1;
> +
> +  /* Validate command name */
> +  const int name_len = strlen (name);
> +  if (name_len < 2)

This should probably name_len == 0, to match the error message.

> +    {
> +      PyErr_Format (PyExc_RuntimeError, _("MI command name is empty"));
> +      return -1;
> +    }
> +  else if ((name[0] != '-') || !isalnum (name[1]))
> +    {
> +      PyErr_Format (PyExc_RuntimeError,
> +		    _("MI command name does not start with '-'"
> +		      " followed by a letter or digit"));
> +      return -1;
> +    }
Could all these error messages be just calls to error(), handled by the catch below?

We would then raise some gdb.error exceptions instead of RuntimeError, maybe it's desirable?

> +  else
> +    for (int i = 2; i < name_len; i++)
> +      {
> +	if (!isalnum (name[i]) && name[i] != '-')
> +	  {
> +	    PyErr_Format (PyExc_RuntimeError,
> +			  _("MI command name contains invalid character: %c"),
> +			  name[i]);
> +	    return -1;
> +	  }
> +      }
> +
> +  gdbpy_ref<> self_ref = gdbpy_ref<>::new_reference (self);
> +  try
> +    {
> +      mi_command *micommand = new mi_command_py (name + 1, NULL, self_ref);
> +
> +      bool result = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));

Make micommand a mi_cmd_up directly:

  mi_cmd_up micommand (new mi_command_py (...));

> +
> +      if (!result)
> +	{
> +	  PyErr_Format (
> +	    PyExc_RuntimeError,
> +	    _("Unable to insert command. The name might already be in use."));
> +	  return -1;
> +	}
> +    }
> +  catch (const gdb_exception &except)
> +    {
> +      GDB_PY_SET_HANDLE_EXCEPTION (except);
> +    }
> +
> +  return 0;
> +}
> +
> +mi_command_py::mi_command_py (const char *name, int *suppress_notification,
> +			      gdbpy_ref<> object)
> +  : mi_command (name, suppress_notification), pyobj (object)
> +{
> +}
> +
> +void
> +mi_command_py::invoke (struct mi_parse *parse)
> +{
> +  std::unique_ptr<scoped_restore_tmpl<int>> restore
> +    = do_suppress_notification ();
> +
> +  mi_parse_argv (parse->args, parse);
> +
> +  if (parse->argv == NULL)
> +    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
> +
> +  PyObject *obj = this->pyobj.get ();
> +  int i;
> +
> +  gdbpy_enter enter_py (get_current_arch (), current_language);
> +
> +  if (!obj)
> +    error (_("-%s: invalid invocation of Python micommand object."),
> +	   name ().c_str ());

Can this condition happen because of a mistake by the user, or it would only be because
of a logic error in GDB?  If it's the latter, it should be a gdb_assert (obj != nullptr);

> +
> +  if (!PyObject_HasAttr (obj, invoke_cst))
> +    {
> +      error (_("-%s: python command object missing 'invoke' method."),
> +	     name ().c_str ());
> +    }

I would make that check (presence of invoke method) when the command is registered.  It would
help users catch mistakes earlier.   Here, it could just be a gdb_assert to confirm it exists.

> +
> +  gdbpy_ref<> argobj (PyList_New (parse->argc));
> +  if (argobj == nullptr)
> +    {
> +      gdbpy_print_stack ();
> +      error (_("-%s: failed to create the python arguments list."),
> +	     name ().c_str ());
> +    }

It would be nice to be consistent in how we write "Python" in our messages to the user.
I suggest "Python", with the capital letter, as it's how the language is named.

> +
> +  for (i = 0; i < parse->argc; ++i)

Declare i in the for loop header.

> +    {
> +      /* Since PyList_SetItem steals the reference, we don't use
> +       * gdbpy_ref<> to hold on arg string. */
> +      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
> +					host_charset (), NULL);
> +      if (PyList_SetItem (argobj.get (), i, str) != 0)

Actually, I would suggest using gdbpy_ref<>, but then releasing it:

  PyList_SetItem (..., str.release ());

It illustrates better the transfer of ownership, and there's no window where
the reference is unmanaged.

Just wondering, because I know that unicode handling is very different between
Python 2 and 3: did you test with both Python versions?

> +	{
> +	  error (_("-%s: failed to create the python arguments list."),
> +		 name ().c_str ());
> +	}
> +    }
> +
> +  gdb_assert (PyErr_Occurred () == NULL);
> +  gdbpy_ref<> result (
> +    PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
> +  if (PyErr_Occurred () != NULL)
> +    {
> +      gdbpy_err_fetch ex;
> +      gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
> +
> +      if (ex_msg == NULL || *ex_msg == '\0')
> +	error (_("-%s: failed to execute command"), name ().c_str ());
> +      else
> +	error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
> +    }
> +  else if (result != nullptr)
> +    {
> +      if (Py_None != result)
> +	parse_mi_result (result.get (), "result");
> +    }
> +  else
> +    {
> +      error (
> +	_("-%s: command invoke() method returned NULL but no python exception "
> +	  "is set"),
> +	name ().c_str ());

I am curious, did you actually hit this case, where no Python exception was
raised and invoke returned NULL?  I thought the two were pretty coupled
(if return value is NULL, it means there's an exception raise and vice versa).

> +    }
> +}
> +
> +void mi_command_py::finalize ()
> +{
> +  this->pyobj.reset (nullptr);
> +}
> +
> +/* Initialize the MI command object.  */
> +
> +int
> +gdbpy_initialize_micommands ()
> +{
> +  micmdpy_object_type.tp_new = PyType_GenericNew;
> +  if (PyType_Ready (&micmdpy_object_type) < 0)
> +    return -1;
> +
> +  if (gdb_pymodule_addobject (gdb_module, "MICommand",
> +			      (PyObject *) &micmdpy_object_type)
> +      < 0)
> +    return -1;
> +
> +  invoke_cst = PyString_FromString ("invoke");
> +  if (invoke_cst == NULL)
> +    return -1;
> +
> +  return 0;
> +}
> +
> +static PyMethodDef micmdpy_object_methods[] = {{0}};
> +
> +PyTypeObject micmdpy_object_type = {
> +  PyVarObject_HEAD_INIT (NULL, 0) "gdb.MICommand", /*tp_name */
> +  sizeof (micmdpy_object),			   /*tp_basicsize */
> +  0,						   /*tp_itemsize */
> +  0,						   /*tp_dealloc */
> +  0,						   /*tp_print */
> +  0,						   /*tp_getattr */
> +  0,						   /*tp_setattr */
> +  0,						   /*tp_compare */
> +  0,						   /*tp_repr */
> +  0,						   /*tp_as_number */
> +  0,						   /*tp_as_sequence */
> +  0,						   /*tp_as_mapping */
> +  0,						   /*tp_hash */
> +  0,						   /*tp_call */
> +  0,						   /*tp_str */
> +  0,						   /*tp_getattro */
> +  0,						   /*tp_setattro */
> +  0,						   /*tp_as_buffer */
> +  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,	/*tp_flags */
> +  "GDB mi-command object",			   /* tp_doc */
> +  0,						   /* tp_traverse */
> +  0,						   /* tp_clear */
> +  0,						   /* tp_richcompare */
> +  0,						   /* tp_weaklistoffset */
> +  0,						   /* tp_iter */
> +  0,						   /* tp_iternext */
> +  micmdpy_object_methods,			   /* tp_methods */
> +  0,						   /* tp_members */
> +  0,						   /* tp_getset */
> +  0,						   /* tp_base */
> +  0,						   /* tp_dict */
> +  0,						   /* tp_descr_get */
> +  0,						   /* tp_descr_set */
> +  0,						   /* tp_dictoffset */
> +  micmdpy_init,					   /* tp_init */
> +  0,						   /* tp_alloc */
> +};
> diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h
> new file mode 100644
> index 0000000000..418de41a10
> --- /dev/null
> +++ b/gdb/python/py-micmd.h
> @@ -0,0 +1,51 @@
> +/* MI Command Set for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef PY_MICMDS_H
> +#define PY_MICMDS_H
> +
> +#include "mi/mi-cmds.h"
> +#include "mi/mi-parse.h"
> +#include "python-internal.h"
> +#include "python/py-ref.h"
> +
> +struct micmdpy_object
> +{
> +  PyObject_HEAD
> +};
> +
> +typedef struct micmdpy_object micmdpy_object;
> +
> +/* MI command implemented on top of a Python command.  */

I find this description a bit misleading, what's a Python command?
The only thing "Python command" can refer to is the gdb.Command class, use
to implement CLI commands in Python, but these Python MI commands are not
related.  I would suggest instead:

/* MI command implemented in Python.  */

> +class mi_command_py : public mi_command
> +{
> +public:
> +  mi_command_py (const char *name, int *suppress_notification,
> +		 gdbpy_ref<> object);

If the suppress_notification parameter is not used for Python commands, you can remove it.

Could you document the constructor?  The OBJECT parameter in particular is a bit opaque.

> +  void invoke (struct mi_parse *parse) override;

This should be private, like in the previous patch.

> +
> +  /* This is called just before shutting down a Python interpreter
> +     to release python object implementing the command */
> +  void finalize ();
> +
> +private:
> +  gdbpy_ref<> pyobj;
> +};
> +
> +#endif
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 449926ca87..26606b4b36 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -539,6 +539,8 @@ int gdbpy_initialize_xmethods (void)
>    CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
>  int gdbpy_initialize_unwind (void)
>    CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
> +int gdbpy_initialize_micommands (void)
> +  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
>  
>  /* A wrapper for PyErr_Fetch that handles reference counting for the
>     caller.  */
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 4dad8ec10d..ee89c603a5 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -36,6 +36,8 @@
>  #include <ctype.h>
>  #include "location.h"
>  #include "ser-event.h"
> +#include "mi/mi-cmds.h"
> +#include "py-micmd.h"
>  
>  /* Declared constants and enum for python stack printing.  */
>  static const char python_excp_none[] = "none";
> @@ -1564,6 +1566,14 @@ finalize_python (void *ignore)
>    python_gdbarch = target_gdbarch ();
>    python_language = current_language;
>  
> +  for (auto const& name_and_cmd : mi_cmd_table)

Use:

  for (const auto &name_and_cmd : mi_cmd_table)

> +    {
> +      mi_command    *cmd = name_and_cmd.second.get ();
> +      mi_command_py *cmd_py = dynamic_cast<mi_command_py*> (cmd);

Don't align variables like this, use regular formatting.

> +      if (cmd_py != nullptr)
> +        cmd_py->finalize ();
> +    }
> +
>    Py_Finalize ();
>  
>    restore_active_ext_lang (previous_active);
> @@ -1698,7 +1708,8 @@ do_start_initialization ()
>        || gdbpy_initialize_event () < 0
>        || gdbpy_initialize_arch () < 0
>        || gdbpy_initialize_xmethods () < 0
> -      || gdbpy_initialize_unwind () < 0)
> +      || gdbpy_initialize_unwind () < 0
> +      || gdbpy_initialize_micommands () < 0)
>      return false;
>  
>  #define GDB_PY_DEFINE_EVENT_TYPE(name, py_name, doc, base)	\
> 

Simon

Re: [PATCH v2 3/5] Create MI commands using python.

[Jan Vrany]

Hi,

thanks a lot! I agree with most of comments and fixed 
them already in upcoming v3 patch. 

For the rest, sae below. 

> > > +    {
> > > +      /* Since PyList_SetItem steals the reference, we don't use
> > > +       * gdbpy_ref<> to hold on arg string. */
> > > +      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
> > > +                                     host_charset (), NULL);
> > > +      if (PyList_SetItem (argobj.get (), i, str) != 0)
> > 
> > Just wondering, because I know that unicode handling is very different between
> > Python 2 and 3: did you test with both Python versions?
> > 

No. I will do that before sending v3. 

> > > +  gdbpy_ref<> result (
> > > +    PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
> > > +  if (PyErr_Occurred () != NULL)
> > > +    {
> > > +      gdbpy_err_fetch ex;
> > > +      gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
> > > +
> > > +      if (ex_msg == NULL || *ex_msg == '\0')
> > > +     error (_("-%s: failed to execute command"), name ().c_str ());
> > > +      else
> > > +     error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
> > > +    }
> > > +  else if (result != nullptr)
> > > +    {
> > > +      if (Py_None != result)
> > > +     parse_mi_result (result.get (), "result");
> > > +    }
> > > +  else
> > > +    {
> > > +      error (
> > > +     _("-%s: command invoke() method returned NULL but no python exception "
> > > +       "is set"),
> > > +     name ().c_str ());
> > 
> > I am curious, did you actually hit this case, where no Python exception was
> > raised and invoke returned NULL?  I thought the two were pretty coupled
> > (if return value is NULL, it means there's an exception raise and vice versa).
> > 

Yes, it looks like. I guess I took this code from Didier. I removed the else
branch in v3. 

> > > +  void invoke (struct mi_parse *parse) override;
> > 
> > This should be private, like in the previous patch.
> > 

Ah, this is "bigger" problem, we should override 
do_invoke() which is declared protected in superclass. Again,fixed in v3. 

Thanks a lot!

Jan

[PATCH v2 0/5] Create MI commands using python

[Jan Vrany]

This is a rework of previous patch series based on Tom's and Simon's
comments. I may happen that I overlooked something since some of
the issues were addressed in subsequent commits and a lot of code
was moved around.

This version *does not* use std::shared_ptr to hold on MI commands, instead
it makes sure that `this` pointer is not used after it me become stale.

Changes v1 -> v2

* squashed following commits into one:
  - 8e65585bdab0 Create MI commands using python.
  - 9e2afadd2d4d03 mi/python: C++ify python MI command handling code
  - 0491a0634f7de4 mi/python: Polish MI output of python commands
* moved class mi_command_py to python code
* use gdbpy_ref<> instead of void* to hold on Python object
  in mi_command_py
* release python objects held on by mi_command_py objects
  when finalizing Python
* merged py_mi_invoke() into mi_command_py::invoke()
* added missing copyright headers
* dropped micmdpy_parse_command_name()
* prefixed error messages by MI command name except for
  "Problem parsing arguments:" since old C-implemented MI
  commands do not prefix command name.
* make do_suppress_notification() to return gdb::optional
* split invoke to invoke and do_invoke, making invoke to perform
  common initialization
* add test redefining Python MI command while the very same command
  is running
* documented the fact that in mi_command_py::do_invoke() `this` pointer cannot
  be used after invoking python code since it may got stale.

Didier Nadeau (3):
  Use std::map for MI commands in mi-cmds.c
  Use classes to represent MI Command instead of structures
  Create MI commands using python.

Jan Vrany (2):
  mi/python: Allow redefinition of python MI commands
  mi/python: Add tests for python-defined MI commands

 gdb/ChangeLog                          |  88 +++++
 gdb/Makefile.in                        |   1 +
 gdb/mi/mi-cmd-info.c                   |   4 +-
 gdb/mi/mi-cmds.c                       | 501 +++++++++++++------------
 gdb/mi/mi-cmds.h                       |  87 +++--
 gdb/mi/mi-main.c                       |  18 +-
 gdb/mi/mi-main.h                       |   1 +
 gdb/mi/mi-parse.c                      |  20 +-
 gdb/mi/mi-parse.h                      |   6 +-
 gdb/python/py-micmd.c                  | 308 +++++++++++++++
 gdb/python/py-micmd.h                  |  52 +++
 gdb/python/python-internal.h           |   2 +
 gdb/python/python.c                    |  13 +-
 gdb/testsuite/ChangeLog                |   5 +
 gdb/testsuite/gdb.python/py-mi-cmd.exp | 109 ++++++
 gdb/testsuite/gdb.python/py-mi-cmd.py  |  39 ++
 16 files changed, 959 insertions(+), 295 deletions(-)
 create mode 100644 gdb/python/py-micmd.c
 create mode 100644 gdb/python/py-micmd.h
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.exp
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd.py

-- 
2.20.1

[PATCH v2 2/5] Use classes to represent MI Command instead of structures

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This commit changes the infrastructure of mi-cmds.h and associated
files to use classes instead of structure to populate the hashmap
containing the commands.

The base class is virtual and there is one subclass MI commands
implemented with pure MI implementation and another for MI commands
implemented over a CLI command.

Logic for suppress_notification and parsing of ARGV/ARGC has been moved
to the classes implementation.

gdb/ChangeLog:

	* mi/mi-cmds.c (create_mi_cmd): Remove.
	(mi_command::mi_command): New function.
	(mi_command::do_suppress_notification): New function.
	(mi_command::invoke): New function.
	(mi_command_mi::mi_command_mi): New function.
	(mi_command_mi::do_invoke): New function.
	(mi_command_cli::mi_command_cli): New function.
	(mi_command_cli::do_invoke): New function.
	(mi_cmd_lookup): Change return type.
	* mi/mi-cmds.h (struct mi_cli): Remove.
	(struct mi_cmd): Remove.
	(class mi_command): New class.
	(class mi_command_mi): New class.
	(class mi_command_cli): New class.
	(mi_cmd_loopkup): Change return type.
	* mi/mi-main.c (mi_execute_cli_command): Remove declaration.
	(mi_execute_command): Remove suppress_notification handling.
	(mi_cmd_execute): Remove call to argv_func.
	(mi_cmd_execute): Remove call to mi_execute_cli_command.
	(mi_cmd_execute): New call to mi_command::invoke.
	* mi/mi-main.h (mi_execute_cli_command): New declaration.
	* mi/mi-parse.c (mi_parse): Remove call to mi_parse_argv.
	* mi/mi-parse.h (struct mi_parse): Remove field struct mi_cmd.
	(struct mi_parse): New field class mi_command.
	(mi_parse_argv): New declaration.
---
 gdb/ChangeLog        |  29 ++++++++++++
 gdb/mi/mi-cmd-info.c |   2 +-
 gdb/mi/mi-cmds.c     | 103 ++++++++++++++++++++++++++++++-------------
 gdb/mi/mi-cmds.h     |  75 ++++++++++++++++++++++---------
 gdb/mi/mi-main.c     |  18 +-------
 gdb/mi/mi-main.h     |   1 +
 gdb/mi/mi-parse.c    |  18 ++------
 gdb/mi/mi-parse.h    |   6 ++-
 8 files changed, 167 insertions(+), 85 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d48d93e827..03eb42555e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,32 @@
+2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
+	    Jan Vrany <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmds.c (create_mi_cmd): Remove.
+	(mi_command::mi_command): New function.
+	(mi_command::do_suppress_notification): New function.
+	(mi_command::invoke): New function.
+	(mi_command_mi::mi_command_mi): New function.
+	(mi_command_mi::do_invoke): New function.
+	(mi_command_cli::mi_command_cli): New function.
+	(mi_command_cli::do_invoke): New function.
+	(mi_cmd_lookup): Change return type.
+	* mi/mi-cmds.h (struct mi_cli): Remove.
+	(struct mi_cmd): Remove.
+	(class mi_command): New class.
+	(class mi_command_mi): New class.
+	(class mi_command_cli): New class.
+	(mi_cmd_loopkup): Change return type.
+	* mi/mi-main.c (mi_execute_cli_command): Remove declaration.
+	(mi_execute_command): Remove suppress_notification handling.
+	(mi_cmd_execute): Remove call to argv_func.
+	(mi_cmd_execute): Remove call to mi_execute_cli_command.
+	(mi_cmd_execute): New call to mi_command::invoke.
+	* mi/mi-main.h (mi_execute_cli_command): New declaration.
+	* mi/mi-parse.c (mi_parse): Remove call to mi_parse_argv.
+	* mi/mi-parse.h (struct mi_parse): Remove field struct mi_cmd.
+	(struct mi_parse): New field class mi_command.
+	(mi_parse_argv): New declaration.
+
 2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
             Jan Vrany <jan.vrany@fit.cvut.cz>
 
diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index 8645fac294..3d8bd68264 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 ce7ca35a6a..44f3813fdc 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>
 
@@ -36,9 +37,9 @@ static bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != NULL);
-  gdb_assert (command->name != NULL);
+  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;
@@ -48,32 +49,16 @@ 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 a pure MI implementation.  */
 
 static void
 add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
 	       int *suppress_notification = NULL)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
-
-  cmd_up->cli.cmd = NULL;
-  cmd_up->cli.args_p = 0;
-  cmd_up->argv_func = function;
-  cmd_up->suppress_notification = suppress_notification;
+  mi_command *micommand = 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 (mi_cmd_up (micommand)));
   gdb_assert (success);
 }
 
@@ -83,17 +68,74 @@ static void
 add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 		int *suppress_notification = NULL)
 {
-  mi_cmd_up cmd_up = create_mi_cmd (name);
+  mi_command *micommand = new mi_command_cli (name, cli_name, args_p,
+					      suppress_notification);
 
-  cmd_up->cli.cmd = cli_name;
-  cmd_up->cli.args_p = args_p;
-  cmd_up->argv_func = NULL;
-  cmd_up->suppress_notification = suppress_notification;
-
-  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
   gdb_assert (success);
 }
 
+/* See mi-cmds.h  */
+mi_command::mi_command (const char *name, int *suppress_notification)
+  : m_name (name),
+    m_suppress_notification (suppress_notification)
+{}
+
+
+void
+mi_command::invoke (struct mi_parse *parse)
+{
+  gdb::optional<scoped_restore_tmpl<int>> restore
+    = do_suppress_notification ();
+  this->do_invoke(parse);
+}
+
+gdb::optional<scoped_restore_tmpl<int>>
+mi_command::do_suppress_notification ()
+{
+  if (m_suppress_notification != NULL)
+    return scoped_restore_tmpl<int> (m_suppress_notification, 1);
+  else
+    {
+      return gdb::optional<scoped_restore_tmpl<int>> ();
+    }
+}
+
+mi_command_mi::mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+			      int *suppress_notification)
+  : mi_command (name, suppress_notification),
+    m_argv_function (func)
+{
+  gdb_assert (func != NULL);
+}
+
+void
+mi_command_mi::do_invoke (struct mi_parse *parse)
+{
+  mi_parse_argv (parse->args, parse);
+
+  if (parse->argv == NULL)
+    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
+
+  this->m_argv_function (parse->command, parse->argv, parse->argc);
+}
+
+mi_command_cli::mi_command_cli (const char *name, const char *cli_name,
+				int args_p, int *suppress_notification)
+  : mi_command (name, suppress_notification),
+    m_cli_name (cli_name),
+    m_args_p (args_p)
+{}
+
+void
+mi_command_cli::do_invoke (struct mi_parse *parse)
+{
+  mi_execute_cli_command (this->m_cli_name.c_str (), this->m_args_p,
+			  parse->args);
+}
+
+/* Initialize the available MI commands.  */
+
 static void
 build_table ()
 {
@@ -126,7 +168,7 @@ build_table ()
   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);		 
+		 &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,
@@ -243,7 +285,7 @@ build_table ()
 
 /* See mi-cmds.h.  */
 
-struct mi_cmd *
+mi_command *
 mi_cmd_lookup (const char *command)
 {
   gdb_assert (command != NULL);
@@ -261,3 +303,4 @@ _initialize_mi_cmds (void)
 {
   build_table ();
 }
+
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 97e1be819f..fc432a16b9 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -126,38 +126,69 @@ extern mi_cmd_argv_ftype mi_cmd_enable_pretty_printing;
 extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
 extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
 
-/* Description of a single command.  */
+/* mi_command base virtual class.  */
 
-struct mi_cli
+class mi_command
 {
-  /* 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;
+  public:
+    mi_command (const char *name, int *suppress_notification);
+    virtual ~mi_command () {};
+
+    const std::string &name ()
+    { return m_name; }
+
+    /* Execute the MI command.  */
+    void invoke (struct mi_parse *parse);
+
+  protected:
+    gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification ();
+    virtual void do_invoke(struct mi_parse *parse) = 0;
+
+  private:
+
+    /* The name of the command.  */
+    std::string m_name;
+
+    /* Pointer to integer to set during command's invocation.  */
+    int *m_suppress_notification;
 };
 
-struct mi_cmd
+/* MI command with a pure MI implementation.  */
+
+class mi_command_mi : public mi_command
 {
-  /* 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;
+  public:
+    mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+		   int *suppress_notification);
+
+  protected:
+    void do_invoke(struct mi_parse *parse) override;
+
+  private:
+    mi_cmd_argv_ftype *m_argv_function;
+};
+
+/* MI command implemented on top of a CLI command.  */
+
+class mi_command_cli : public mi_command
+{
+  public:
+    mi_command_cli (const char *name, const char *cli_name, int args_p,
+		  int *suppress_notification);
+
+  protected:
+    void do_invoke(struct mi_parse *parse) override;
+
+  private:
+    std::string m_cli_name;
+    int m_args_p;
 };
 
-typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
+typedef std::unique_ptr<mi_command> mi_cmd_up;
 
 /* Lookup a command in the MI command table.  */
 
-extern struct 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-main.c b/gdb/mi/mi-main.c
index 17ca807471..481f09f799 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -90,9 +90,6 @@ int running_result_record_printed = 1;
 int mi_proceeded;
 
 static void mi_cmd_execute (struct mi_parse *parse);
-
-static void mi_execute_cli_command (const char *cmd, int 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 *,
@@ -1954,9 +1951,6 @@ mi_execute_command (const char *cmd, int from_tty)
 
       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)
@@ -2095,17 +2089,9 @@ 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)
+  if (parse->cmd != NULL)
     {
-      /* 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);
+      parse->cmd->invoke (parse);
     }
   else
     {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 72c4e59428..3733bc37ac 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -54,6 +54,7 @@ struct mi_suppress_notification
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
+void mi_execute_cli_command (const char *cmd, int 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 6c8d922d66..7ed2b2bbc6 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 2262ff56f9..94bd48219c 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;
+    mi_command *cmd;
     struct mi_timestamp *cmd_start;
     char *args;
     char **argv;
@@ -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.  */
+
+void mi_parse_argv (const char *args, struct mi_parse *parse);
+
 #endif /* MI_MI_PARSE_H */
-- 
2.20.1

Re: [PATCH v2 2/5] Use classes to represent MI Command instead of structures

[Simon Marchi]

Hi Jan,

This looks ok to me, I have noted a few styling issues, I fixed them as I went
through the code, so see the patch at the end which you can apply to your patch.

> @@ -36,9 +37,9 @@ static bool
>  insert_mi_cmd_entry (mi_cmd_up command)
>  {
>    gdb_assert (command != NULL);
> -  gdb_assert (command->name != NULL);
> +  gdb_assert (! command->name ().empty ());

Remove space after !.

>  /* Create and register a new MI command with a pure MI implementation.  */
>  
>  static void
>  add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
>  	       int *suppress_notification = NULL)
>  {
> -  mi_cmd_up cmd_up = create_mi_cmd (name);
> -
> -  cmd_up->cli.cmd = NULL;
> -  cmd_up->cli.args_p = 0;
> -  cmd_up->argv_func = function;
> -  cmd_up->suppress_notification = suppress_notification;
> +  mi_command *micommand = 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 (mi_cmd_up (micommand)));

It would make more sense to make the local variable an mi_cmd_up.

>    gdb_assert (success);
>  }
>  
> @@ -83,17 +68,74 @@ static void
>  add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
>  		int *suppress_notification = NULL)
>  {
> -  mi_cmd_up cmd_up = create_mi_cmd (name);
> +  mi_command *micommand = new mi_command_cli (name, cli_name, args_p,
> +					      suppress_notification);

Likewise.

>  
> -  cmd_up->cli.cmd = cli_name;
> -  cmd_up->cli.args_p = args_p;
> -  cmd_up->argv_func = NULL;
> -  cmd_up->suppress_notification = suppress_notification;
> -
> -  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> +  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
>    gdb_assert (success);
>  }
>  
> +/* See mi-cmds.h  */
> +mi_command::mi_command (const char *name, int *suppress_notification)
> +  : m_name (name),
> +    m_suppress_notification (suppress_notification)
> +{}
> +
> +
> +void
> +mi_command::invoke (struct mi_parse *parse)
> +{
> +  gdb::optional<scoped_restore_tmpl<int>> restore
> +    = do_suppress_notification ();
> +  this->do_invoke(parse);

Missing space.

> +}
> +
> +gdb::optional<scoped_restore_tmpl<int>>
> +mi_command::do_suppress_notification ()
> +{
> +  if (m_suppress_notification != NULL)
> +    return scoped_restore_tmpl<int> (m_suppress_notification, 1);
> +  else
> +    {
> +      return gdb::optional<scoped_restore_tmpl<int>> ();
> +    }

Remove the curly braces.  Also, this can be shortened to "return {};".


> @@ -126,38 +126,69 @@ extern mi_cmd_argv_ftype mi_cmd_enable_pretty_printing;
>  extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters;
>  extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
>  
> -/* Description of a single command.  */
> +/* mi_command base virtual class.  */
>  
> -struct mi_cli
> +class mi_command
>  {
> -  /* 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;
> +  public:
> +    mi_command (const char *name, int *suppress_notification);
> +    virtual ~mi_command () {};

Use the default destructor:

  virtual ~mi_command () = default;

> +
> +    const std::string &name ()
> +    { return m_name; }
> +
> +    /* Execute the MI command.  */
> +    void invoke (struct mi_parse *parse);
> +
> +  protected:
> +    gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification ();
> +    virtual void do_invoke(struct mi_parse *parse) = 0;

It would be useful to have some short doc for these.

Also, I think do_invoke could be private.

> +
> +  private:
> +
> +    /* The name of the command.  */
> +    std::string m_name;
> +
> +    /* Pointer to integer to set during command's invocation.  */
> +    int *m_suppress_notification;
>  };

Unindent the content of the class: the public/private should be at the
same column as the "class" keyword.

> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 17ca807471..481f09f799 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -90,9 +90,6 @@ int running_result_record_printed = 1;
>  int mi_proceeded;
>  
>  static void mi_cmd_execute (struct mi_parse *parse);
> -
> -static void mi_execute_cli_command (const char *cmd, int 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 *,
> @@ -1954,9 +1951,6 @@ mi_execute_command (const char *cmd, int from_tty)
>  
>        gdb::optional<scoped_restore_tmpl<int>> restore_suppress;

This ends up ununsed and can be removed.

>  
> -      if (command->cmd != NULL && command->cmd->suppress_notification != NULL)
> -	restore_suppress.emplace (command->cmd->suppress_notification, 1);
> -
>        command->token = token;
>  
>        if (do_timings)
> @@ -2095,17 +2089,9 @@ 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)
> +  if (parse->cmd != NULL)
>      {
> -      /* 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);
> +      parse->cmd->invoke (parse);

Remove resulting extra curly braces.

Here's the diff that should fix pretty much all these comments.

From 6e7b648b887e59eaadb01919ca0d665dae2b3e19 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 16 May 2019 22:00:00 -0400
Subject: [PATCH] fixup patch 2

---
 gdb/mi/mi-cmds.c | 21 ++++++++---------
 gdb/mi/mi-cmds.h | 60 +++++++++++++++++++++++++-----------------------
 gdb/mi/mi-main.c |  6 +----
 3 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 44f3813fdc9a..7702011e39df 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -37,7 +37,7 @@ static bool
 insert_mi_cmd_entry (mi_cmd_up command)
 {
   gdb_assert (command != NULL);
-  gdb_assert (! command->name ().empty ());
+  gdb_assert (!command->name ().empty ());

   const std::string &name = command->name ();

@@ -55,10 +55,9 @@ static void
 add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
 	       int *suppress_notification = NULL)
 {
-  mi_command *micommand = new mi_command_mi (name, function,
-					     suppress_notification);
+  mi_cmd_up command (new mi_command_mi (name, function, suppress_notification));

-  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
+  bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
 }

@@ -68,14 +67,15 @@ static void
 add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
 		int *suppress_notification = NULL)
 {
-  mi_command *micommand = new mi_command_cli (name, cli_name, args_p,
-					      suppress_notification);
+  mi_cmd_up command (new mi_command_cli (name, cli_name, args_p,
+					 suppress_notification));

-  bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
+  bool success = insert_mi_cmd_entry (std::move (command));
   gdb_assert (success);
 }

 /* See mi-cmds.h  */
+
 mi_command::mi_command (const char *name, int *suppress_notification)
   : m_name (name),
     m_suppress_notification (suppress_notification)
@@ -87,7 +87,7 @@ mi_command::invoke (struct mi_parse *parse)
 {
   gdb::optional<scoped_restore_tmpl<int>> restore
     = do_suppress_notification ();
-  this->do_invoke(parse);
+  this->do_invoke (parse);
 }

 gdb::optional<scoped_restore_tmpl<int>>
@@ -96,9 +96,7 @@ mi_command::do_suppress_notification ()
   if (m_suppress_notification != NULL)
     return scoped_restore_tmpl<int> (m_suppress_notification, 1);
   else
-    {
-      return gdb::optional<scoped_restore_tmpl<int>> ();
-    }
+    return {};
 }

 mi_command_mi::mi_command_mi (const char *name, mi_cmd_argv_ftype func,
@@ -303,4 +301,3 @@ _initialize_mi_cmds (void)
 {
   build_table ();
 }
-
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index fc432a16b9cc..12b9696c5f41 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -130,58 +130,60 @@ extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;

 class mi_command
 {
-  public:
-    mi_command (const char *name, int *suppress_notification);
-    virtual ~mi_command () {};
+public:
+  mi_command (const char *name, int *suppress_notification);
+  virtual ~mi_command () = default;

-    const std::string &name ()
-    { return m_name; }
+  const std::string &name ()
+  { return m_name; }

-    /* Execute the MI command.  */
-    void invoke (struct mi_parse *parse);
+  /* Execute the MI command.  */
+  void invoke (struct mi_parse *parse);

-  protected:
-    gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification ();
-    virtual void do_invoke(struct mi_parse *parse) = 0;
+protected:
+  /* Set the notification suppression flag for the command.  Return a
+     scoped_restore that undoes it.  */
+  gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification ();

-  private:
+private:
+  /* Implementation of the behavior of the MI command, to be implemented by
+     child classes.  */
+  virtual void do_invoke (struct mi_parse *parse) = 0;

-    /* The name of the command.  */
-    std::string m_name;
+  /* The name of the command.  */
+  std::string m_name;

-    /* Pointer to integer to set during command's invocation.  */
-    int *m_suppress_notification;
+  /* Pointer to integer to set during command's invocation.  */
+  int *m_suppress_notification;
 };

 /* MI command with a pure MI implementation.  */

 class mi_command_mi : public mi_command
 {
-  public:
-    mi_command_mi (const char *name, mi_cmd_argv_ftype func,
-		   int *suppress_notification);
+public:
+  mi_command_mi (const char *name, mi_cmd_argv_ftype func,
+		 int *suppress_notification);

-  protected:
-    void do_invoke(struct mi_parse *parse) override;
+private:
+  virtual void do_invoke (struct mi_parse *parse) override;

-  private:
-    mi_cmd_argv_ftype *m_argv_function;
+  mi_cmd_argv_ftype *m_argv_function;
 };

 /* MI command implemented on top of a CLI command.  */

 class mi_command_cli : public mi_command
 {
-  public:
-    mi_command_cli (const char *name, const char *cli_name, int args_p,
+public:
+  mi_command_cli (const char *name, const char *cli_name, int args_p,
 		  int *suppress_notification);

-  protected:
-    void do_invoke(struct mi_parse *parse) override;
+private:
+  virtual void do_invoke (struct mi_parse *parse) override;

-  private:
-    std::string m_cli_name;
-    int m_args_p;
+  std::string m_cli_name;
+  int m_args_p;
 };

 typedef std::unique_ptr<mi_command> mi_cmd_up;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 754091656248..ccb7bf45bdf5 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1949,8 +1949,6 @@ mi_execute_command (const char *cmd, int from_tty)
     {
       ptid_t previous_ptid = inferior_ptid;

-      gdb::optional<scoped_restore_tmpl<int>> restore_suppress;
-
       command->token = token;

       if (do_timings)
@@ -2090,9 +2088,7 @@ mi_cmd_execute (struct mi_parse *parse)
   current_context = parse;

   if (parse->cmd != NULL)
-    {
-      parse->cmd->invoke (parse);
-    }
+    parse->cmd->invoke (parse);
   else
     {
       /* FIXME: DELETE THIS.  */
-- 
2.21.0

[PATCH v2 1/5] Use std::map for MI commands in mi-cmds.c

[Jan Vrany]

From: Didier Nadeau <didier.nadeau@gmail.com>

This changes the hashmap used in mi-cmds.c from a custom
structure to std::map. This is done to be able to add new
MI commands at runtime, the previous structure had a fixed
size array.

gdb/ChangeLog:

	* mi/mi-cmds.h (mi_cmd_up): New typedef.
	(mi_lookup): Rename to ...
	(mi_cmd_lookup): this.
	* mi/mi-cmds.c (_initialize_mi_cmds): Remove declaration.
	(lookup_table): Remove declaration.
	(build_table): Remove declaration.
	(mi_table): Rename to ...
	(mi_cmd_table): this and change to std::map.
	(insert_mi_cmd_entry): New function.
	(DEF_MI_CMD_CLI_1, DEF_MI_CMD_CLI, DEF_MI_CMD_MI,
	DEF_MI_CMD_MI_1): Remove macros.
	(mi_cmds): Remove array.
	(mi_lookup): Rename to ...
	(mi_cmd_lookup): this.
	(create_mi_cmd): New function.
	(struct mi_cmd_stats): Remove.
	(stats): Remove.
	(add_mi_cmd_cli, add_mi_cmd_mi): New functions.
	(lookup_table): Remove.
	(build_table): Remove parameter.
	* mi/mi-parse.c (mi_parse): Change call from mi_lookup to
	mi_cmd_lookup.
	* mi/mi-cmd-info.c (mi_cmd_info_gdb_mi_command): Change call
	from mi_lookup to mi_cmd_lookup.
---
 gdb/ChangeLog        |  28 +++
 gdb/mi/mi-cmd-info.c |   2 +-
 gdb/mi/mi-cmds.c     | 461 ++++++++++++++++++++-----------------------
 gdb/mi/mi-cmds.h     |   4 +-
 gdb/mi/mi-parse.c    |   2 +-
 5 files changed, 251 insertions(+), 246 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ef77fdbb5c..d48d93e827 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,31 @@
+2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
+            Jan Vrany <jan.vrany@fit.cvut.cz>
+
+        * mi/mi-cmds.h (mi_cmd_up): New typedef.
+        (mi_lookup): Rename to ...
+        (mi_cmd_lookup): this.
+        * mi/mi-cmds.c (_initialize_mi_cmds): Remove declaration.
+        (lookup_table): Remove declaration.
+        (build_table): Remove declaration.
+        (mi_table): Rename to ...
+        (mi_cmd_table): this and change to std::map.
+        (insert_mi_cmd_entry): New function.
+        (DEF_MI_CMD_CLI_1, DEF_MI_CMD_CLI, DEF_MI_CMD_MI,
+        DEF_MI_CMD_MI_1): Remove macros.
+        (mi_cmds): Remove array.
+        (mi_lookup): Rename to ...
+        (mi_cmd_lookup): this.
+        (create_mi_cmd): New function.
+        (struct mi_cmd_stats): Remove.
+        (stats): Remove.
+        (add_mi_cmd_cli, add_mi_cmd_mi): New functions.
+        (lookup_table): Remove.
+        (build_table): Remove parameter.
+        * mi/mi-parse.c (mi_parse): Change call from mi_lookup to
+        mi_cmd_lookup.
+        * mi/mi-cmd-info.c (mi_cmd_info_gdb_mi_command): Change call
+        from mi_lookup to mi_cmd_lookup.
+
 2019-04-17  Tom Tromey  <tromey@adacore.com>
 
 	* dwarf2read.c (dwarf2_init_complex_target_type): Check "tt"
diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
index 39da6c489d..8645fac294 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 fe30ac2e82..ce7ca35a6a 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -22,267 +22,242 @@
 #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);
+/* MI command table (built at run time). */
 
-static struct mi_cmd mi_cmds[] =
+static std::map<std::string, mi_cmd_up> mi_cmd_table;
+
+/* Insert a new mi-command into the command table.  Return true if
+   insertion was successful.  */
+
+static bool
+insert_mi_cmd_entry (mi_cmd_up command)
 {
-/* 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)
-
-/* 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)
-
-  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_CLI_1 ("break-condition","cond", 1,
-		  &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 ("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_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, }
-};
-
-/* Pointer to the mi command table (built at run time). */
-
-static struct mi_cmd **mi_table;
-
-/* A prime large enough to accomodate the entire command table.  */
-enum
-  {
-    MI_TABLE_SIZE = 227
-  };
-
-/* Exported function used to obtain info from the table.  */
-struct mi_cmd *
-mi_lookup (const char *command)
+  gdb_assert (command != NULL);
+  gdb_assert (command->name != NULL);
+
+  std::string name (command->name);
+
+  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
+    return false;
+
+  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 a pure MI implementation.  */
 
-struct mi_cmd_stats
+static void
+add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
+	       int *suppress_notification = NULL)
 {
-  int hit;
-  int miss;
-  int rehash;
-};
-struct mi_cmd_stats stats;
+  mi_cmd_up cmd_up = create_mi_cmd (name);
 
-/* Look up a command.  */
+  cmd_up->cli.cmd = NULL;
+  cmd_up->cli.args_p = 0;
+  cmd_up->argv_func = function;
+  cmd_up->suppress_notification = suppress_notification;
 
-static struct mi_cmd **
-lookup_table (const char *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.  */
+
+static void
+add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
+		int *suppress_notification = NULL)
 {
-  const char *chp;
-  unsigned int index = 0;
-
-  /* Compute our hash.  */
-  for (chp = command; *chp; chp++)
-    {
-      /* We use a somewhat arbitrary formula.  */
-      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
-    }
-
-  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++;
-    }
+  mi_cmd_up cmd_up = create_mi_cmd (name);
+
+  cmd_up->cli.cmd = cli_name;
+  cmd_up->cli.args_p = args_p;
+  cmd_up->argv_func = NULL;
+  cmd_up->suppress_notification = suppress_notification;
+
+  bool success = insert_mi_cmd_entry (std::move (cmd_up));
+  gdb_assert (success);
 }
 
 static void
-build_table (struct mi_cmd *commands)
+build_table ()
+{
+  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_cli ("break-condition","cond", 1,
+		  &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 ("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_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);
+}
+
+/* See mi-cmds.h.  */
+
+struct mi_cmd *
+mi_cmd_lookup (const char *command)
 {
-  int nr_rehash = 0;
-  int nr_entries = 0;
-  struct mi_cmd *command;
-
-  mi_table = XCNEWVEC (struct mi_cmd *, MI_TABLE_SIZE);
-  for (command = commands; command->name != 0; command++)
-    {
-      struct mi_cmd **entry = lookup_table (command->name);
-
-      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);
-    }
+  gdb_assert (command != NULL);
+
+  auto it = mi_cmd_table.find (command);
+
+  if (it == mi_cmd_table.end ())
+    return NULL;
+
+  return it->second.get ();
 }
 
 void
 _initialize_mi_cmds (void)
 {
-  build_table (mi_cmds);
-  memset (&stats, 0, sizeof (stats));
+  build_table ();
 }
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 7b22ce7813..97e1be819f 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -153,9 +153,11 @@ struct mi_cmd
   int *suppress_notification;
 };
 
+typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;
+
 /* Lookup a command in the MI command table.  */
 
-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 cc6a4419d0..6c8d922d66 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.20.1

[PATCH v2 4/5] mi/python: Allow redefinition of python MI commands

[Jan Vrany]

Redefining python MI commands is especially useful when developing
then.

gdb/Changelog:

	* mi/mi-cmds.h (mi_command::can_be_redefined): New method.
	* mi/mi-cmds.c: (mi_command::can_be_redefined): New method.
	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.
	* python/py-micmd.h (mi_command_py::can_be_redefined): New method.
	* python/py-micmd.c: (mi_command_py::can_be_redefined): New method.
---
 gdb/ChangeLog         |  8 ++++++++
 gdb/mi/mi-cmds.c      | 10 +++++++++-
 gdb/mi/mi-cmds.h      |  3 +++
 gdb/python/py-micmd.c | 34 +++++++++++++++++++++-------------
 gdb/python/py-micmd.h |  3 ++-
 5 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4cbe97002b..0a5d95b4e3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2019-05-02  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmds.h (mi_command::can_be_redefined): New method.
+	* mi/mi-cmds.c: (mi_command::can_be_redefined): New method.
+	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.
+	* python/py-micmd.h (mi_command_py::can_be_redefined): New method.
+	* python/py-micmd.c: (mi_command_py::can_be_redefined): New method.
+
 2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
             Jan Vrany  <jan.vrany@fit.cvut.cz>
 
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 7e1d2c3f2d..d7ac76d450 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -41,7 +41,8 @@ insert_mi_cmd_entry (mi_cmd_up command)
   const std::string &name = command->name ();
 
   if (mi_cmd_table.find (name) != mi_cmd_table.end ())
-    return false;
+    if (! mi_cmd_table[name]->can_be_redefined ())
+      return false;
 
   mi_cmd_table[name] = std::move (command);
 
@@ -80,6 +81,13 @@ mi_command::mi_command (const char *name, int *suppress_notification)
     m_suppress_notification (suppress_notification)
 {}
 
+bool
+mi_command::can_be_redefined()
+{
+  return false;
+}
+
+
 
 void
 mi_command::invoke (struct mi_parse *parse)
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index a2757bae20..dd2738f831 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -142,6 +142,9 @@ class mi_command
     /* Execute the MI command.  */
     void invoke (struct mi_parse *parse);
 
+    /* Return TRUE if command can be redefined, FALSE otherwise. */
+    virtual bool can_be_redefined();
+
   protected:
     gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification ();
     virtual void do_invoke(struct mi_parse *parse) = 0;
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index c8d429bb4b..994d715d02 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -158,17 +158,22 @@ mi_command_py::mi_command_py (const char *name, int *suppress_notification,
 {
 }
 
-void
-mi_command_py::invoke (struct mi_parse *parse)
+bool
+mi_command_py::can_be_redefined()
 {
-  std::unique_ptr<scoped_restore_tmpl<int>> restore
-    = do_suppress_notification ();
+  return true;
+}
+
 
+void
+mi_command_py::do_invoke (struct mi_parse *parse)
+{
   mi_parse_argv (parse->args, parse);
 
   if (parse->argv == NULL)
     error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
 
+  std::string name (this->name ());
   PyObject *obj = this->pyobj.get ();
   int i;
 
@@ -176,20 +181,19 @@ mi_command_py::invoke (struct mi_parse *parse)
 
   if (!obj)
     error (_("-%s: invalid invocation of Python micommand object."),
-	   name ().c_str ());
+	   name.c_str ());
 
   if (!PyObject_HasAttr (obj, invoke_cst))
     {
       error (_("-%s: python command object missing 'invoke' method."),
-	     name ().c_str ());
+	     name.c_str ());
     }
-
   gdbpy_ref<> argobj (PyList_New (parse->argc));
   if (argobj == nullptr)
     {
       gdbpy_print_stack ();
       error (_("-%s: failed to create the python arguments list."),
-	     name ().c_str ());
+	     name.c_str ());
     }
 
   for (i = 0; i < parse->argc; ++i)
@@ -201,11 +205,16 @@ mi_command_py::invoke (struct mi_parse *parse)
       if (PyList_SetItem (argobj.get (), i, str) != 0)
 	{
 	  error (_("-%s: failed to create the python arguments list."),
-		 name ().c_str ());
+		 name.c_str ());
 	}
     }
 
   gdb_assert (PyErr_Occurred () == NULL);
+
+  /* From this point on, THIS must not be used since Python code may replace
+     the the very same command that is currently executing.  This in turn leads
+     to desctruction of THIS making it invalid.  See insert_mi_cmd_entry. */
+
   gdbpy_ref<> result (
     PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
   if (PyErr_Occurred () != NULL)
@@ -214,9 +223,9 @@ mi_command_py::invoke (struct mi_parse *parse)
       gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
 
       if (ex_msg == NULL || *ex_msg == '\0')
-	error (_("-%s: failed to execute command"), name ().c_str ());
+	error (_("-%s: failed to execute command"), name.c_str ());
       else
-	error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
+	error (_("-%s: %s"), name.c_str (), ex_msg.get ());
     }
   else if (result != nullptr)
     {
@@ -228,7 +237,7 @@ mi_command_py::invoke (struct mi_parse *parse)
       error (
 	_("-%s: command invoke() method returned NULL but no python exception "
 	  "is set"),
-	name ().c_str ());
+	name.c_str ());
     }
 }
 
@@ -236,7 +245,6 @@ void mi_command_py::finalize ()
 {
   this->pyobj.reset (nullptr);
 }
-
 /* Initialize the MI command object.  */
 
 int
diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h
index 418de41a10..eaa5a6c47c 100644
--- a/gdb/python/py-micmd.h
+++ b/gdb/python/py-micmd.h
@@ -38,7 +38,7 @@ class mi_command_py : public mi_command
 public:
   mi_command_py (const char *name, int *suppress_notification,
 		 gdbpy_ref<> object);
-  void invoke (struct mi_parse *parse) override;
+  bool can_be_redefined() override;
 
   /* This is called just before shutting down a Python interpreter
      to release python object implementing the command */
@@ -46,6 +46,7 @@ public:
 
 private:
   gdbpy_ref<> pyobj;
+  virtual void do_invoke(struct mi_parse *parse) override;
 };
 
 #endif
-- 
2.20.1