[PATCH 0/4] Fix crash in "set python" and small cleanups

[PATCH 0/4] Fix crash in "set python" and small cleanups

[Marco Barisione]

I noticed by chance that the "set python" command with no arguments
causes a segfault.  This is due to a trivial mistake, the prefix command
meant to be "show python" is added to the list of set commands, and the
"set python" one is added to the list of show commands.

I didn't add a specific test because what's the chance of "set python"
regressing?  I tried writing a self test instead (like the ones in
gdb/unittests/command-def-selftests.c), but I couldn't figure out how to
detect this problem in a generic way.  Any idea?  Or it this patch
OK without a test?

This patch series also includes some cleanups which came up from the
work on the main fix.

Marco Barisione (4):
  gdb: Add an overloaded ui_out::text accepting a const std::string &
  gdb: Pass std::strings to ui_out::field_string () where convenient
  gdb: Fix crash in cmd_show_list () if the prefix doesn't contain "show
    "
  gdb: Add "set|show python" commands to the correct list

 gdb/ada-lang.c          |  4 ++--
 gdb/break-catch-sig.c   |  2 +-
 gdb/break-catch-throw.c |  2 +-
 gdb/cli/cli-setshow.c   | 32 ++++++++++++++++++++++++--------
 gdb/disasm.c            |  2 +-
 gdb/infcmd.c            |  2 +-
 gdb/inferior.c          |  2 +-
 gdb/linux-thread-db.c   |  2 +-
 gdb/mi/mi-cmd-var.c     | 22 +++++++++++-----------
 gdb/mi/mi-main.c        | 14 +++++++-------
 gdb/osdata.c            |  3 +--
 gdb/probe.c             |  5 ++---
 gdb/python/python.c     |  4 ++--
 gdb/source.c            |  2 +-
 gdb/target-connection.c |  2 +-
 gdb/thread.c            |  5 ++---
 gdb/tracepoint.c        |  4 ++--
 gdb/ui-out.c            | 11 +++++++++--
 gdb/ui-out.h            |  4 +++-
 19 files changed, 73 insertions(+), 51 deletions(-)

-- 
2.28.0

[PATCH 1/4] gdb: Add an overloaded ui_out::text accepting a const std::string &

[Marco Barisione]

gdb/ChangeLog:

	* ui-out.h (class ui_out): Add ui_out::text accepting a constant
	reference to a std::string.  Fix all callers using
	std::string::c_str.
	* ui-out.c (ui_out::text): Ditto.
---
 gdb/ada-lang.c        | 4 ++--
 gdb/cli/cli-setshow.c | 2 +-
 gdb/source.c          | 2 +-
 gdb/thread.c          | 2 +-
 gdb/ui-out.c          | 6 ++++++
 gdb/ui-out.h          | 1 +
 6 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2a257c2fd6d..1b506766259 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11865,7 +11865,7 @@ print_mention_exception (struct breakpoint *b)
 	  {
 	    std::string info = string_printf (_("`%s' Ada exception"),
 					      c->excep_string.c_str ());
-	    uiout->text (info.c_str ());
+	    uiout->text (info);
 	  }
 	else
 	  uiout->text (_("all Ada exceptions"));
@@ -11881,7 +11881,7 @@ print_mention_exception (struct breakpoint *b)
 	    std::string info
 	      = string_printf (_("`%s' Ada exception handlers"),
 			       c->excep_string.c_str ());
-	    uiout->text (info.c_str ());
+	    uiout->text (info);
 	  }
 	else
 	  uiout->text (_("all Ada exceptions handlers"));
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 82008ca8eed..f6a594d69a2 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -760,7 +760,7 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
 	      std::string prefixname = list->prefix->prefixname ();
 	      prefixname = (!list->prefix->is_prefix () ? ""
 			    : strstr (prefixname.c_str (), "show ") + 5);
-	      uiout->text (prefixname.c_str ());
+	      uiout->text (prefixname);
 	    }
 	  uiout->field_string ("name", list->name);
 	  uiout->text (":  ");
diff --git a/gdb/source.c b/gdb/source.c
index b6dab6eb236..54cb45f4e9d 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1394,7 +1394,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	  if (iter > start)
 	    {
 	      std::string text (start, iter);
-	      uiout->text (text.c_str ());
+	      uiout->text (text);
 	    }
 	  if (*iter == '\r')
 	    {
diff --git a/gdb/thread.c b/gdb/thread.c
index 3cd588e0b12..87b6cbf74fd 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1977,7 +1977,7 @@ print_selected_thread_frame (struct ui_out *uiout,
 	  uiout->text ("[Switching to thread ");
 	  uiout->field_string ("new-thread-id", print_thread_id (tp));
 	  uiout->text (" (");
-	  uiout->text (target_pid_to_str (inferior_ptid).c_str ());
+	  uiout->text (target_pid_to_str (inferior_ptid));
 	  uiout->text (")]");
 	}
     }
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index d67dae4f506..56251c9445a 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -581,6 +581,12 @@ ui_out::text (const char *string)
   do_text (string);
 }
 
+void
+ui_out::text (const std::string &string)
+{
+  text (string.c_str ());
+}
+
 void
 ui_out::call_do_message (const ui_file_style &style, const char *format,
 			 ...)
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index f14be479fa1..a06477df533 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -203,6 +203,7 @@ class ui_out
 
   void spaces (int numspaces);
   void text (const char *string);
+  void text (const std::string &string);
 
   /* Output a printf-style formatted string.  In addition to the usual
      printf format specs, this supports a few GDB-specific
-- 
2.28.0

Re: [PATCH 1/4] gdb: Add an overloaded ui_out::text accepting a const std::string &

[Simon Marchi]

On 2021-05-18 7:07 a.m., Marco Barisione via Gdb-patches wrote:> gdb/ChangeLog:
> 
> 	* ui-out.h (class ui_out): Add ui_out::text accepting a constant
> 	reference to a std::string.  Fix all callers using
> 	std::string::c_str.
> 	* ui-out.c (ui_out::text): Ditto.
> ---
>  gdb/ada-lang.c        | 4 ++--
>  gdb/cli/cli-setshow.c | 2 +-
>  gdb/source.c          | 2 +-
>  gdb/thread.c          | 2 +-
>  gdb/ui-out.c          | 6 ++++++
>  gdb/ui-out.h          | 1 +
>  6 files changed, 12 insertions(+), 5 deletions(-)

I was going to suggest: maybe we can have a single method that takes a
string_view instead.  But then it would mean computing the string length
for nothing when passing a char*, because we ultimately just pass down
the char pointer to the fprintf functions.

I would however suggest moving the implementation of that new method in
the header file, since it's just a small wrapper around the other "text"
method.  The compiler will likely in-line these calls, so it would
result in the same generated code as today, but with the benefit of not
having to use c_str in the caller.  In any case, the patch is ok with or
without that change.

Simon

Re: [PATCH 1/4] gdb: Add an overloaded ui_out::text accepting a const std::string &

[Marco Barisione]

On 18 May 2021, at 15:18, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> On 2021-05-18 7:07 a.m., Marco Barisione via Gdb-patches wrote:> gdb/ChangeLog:
>> 
>> 	* ui-out.h (class ui_out): Add ui_out::text accepting a constant
>> 	reference to a std::string.  Fix all callers using
>> 	std::string::c_str.
>> 	* ui-out.c (ui_out::text): Ditto.
>> ---
>> gdb/ada-lang.c        | 4 ++--
>> gdb/cli/cli-setshow.c | 2 +-
>> gdb/source.c          | 2 +-
>> gdb/thread.c          | 2 +-
>> gdb/ui-out.c          | 6 ++++++
>> gdb/ui-out.h          | 1 +
>> 6 files changed, 12 insertions(+), 5 deletions(-)
> 
> I was going to suggest: maybe we can have a single method that takes a
> string_view instead.  But then it would mean computing the string length
> for nothing when passing a char*, because we ultimately just pass down
> the char pointer to the fprintf functions.
> 
> I would however suggest moving the implementation of that new method in
> the header file, since it's just a small wrapper around the other "text"
> method.  The compiler will likely in-line these calls, so it would
> result in the same generated code as today, but with the benefit of not
> having to use c_str in the caller.  In any case, the patch is ok with or
> without that change.

I pushed both patches to master and just submitted another patch to move the
definitions to the header, but I forgot to CC you.

(I’m quite unsure about the correct style/indentation for inline functions
in classes as the existing code seems inconsistent.)

-- 
Marco Barisione

[PATCH 2/4] gdb: Pass std::strings to ui_out::field_string () where convenient

[Marco Barisione]

While adding a ui_out::text () overload accepting a std::string, I
noticed that several callers of ui_out::field_string () were converting
std::string instances to char pointers even if not necessary.

gdb/ChangeLog:

	* ui-out.c (ui_out::field_string): Add missing style_argument
	to the overload accepting a std::string, to make it equivalent
	to the char pointer version.
	* ui-out.h (class ui_out): Ditto.
	* break-catch-sig.c (signal_catchpoint_print_one): Do not
	convert std::strings to char pointers before passing them to
	ui_out::field_string ().
	* break-catch-throw.c (print_one_detail_exception_catchpoint):
	Ditto.
	* cli/cli-setshow.c (do_show_command): Ditto.
	* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn):
	Ditto.
	* infcmd.c (print_return_value_1): Ditto.
	* inferior.c (print_inferior): Ditto.
	* linux-thread-db.c (info_auto_load_libthread_db): Ditto.
	* mi/mi-cmd-var.c (print_varobj): Ditto.
	(mi_cmd_var_set_format): Ditto.
	(mi_cmd_var_info_type): Ditto.
	(mi_cmd_var_info_expression): Ditto.
	(mi_cmd_var_evaluate_expression): Ditto.
	(mi_cmd_var_assign): Ditto.
	(varobj_update_one): Ditto.
	* mi/mi-main.c (list_available_thread_groups): Ditto.
	(mi_cmd_data_read_memory_bytes): Ditto.
	(mi_cmd_trace_frame_collected): Ditto.
	* osdata.c (info_osdata): Ditto.
	* probe.c (info_probes_for_spops): Ditto.
	* target-connection.c (print_connection): Ditto.
	* thread.c (print_thread_info_1): Ditto.
	* tracepoint.c (print_one_static_tracepoint_marker): Ditto.
---
 gdb/break-catch-sig.c   |  2 +-
 gdb/break-catch-throw.c |  2 +-
 gdb/cli/cli-setshow.c   |  2 +-
 gdb/disasm.c            |  2 +-
 gdb/infcmd.c            |  2 +-
 gdb/inferior.c          |  2 +-
 gdb/linux-thread-db.c   |  2 +-
 gdb/mi/mi-cmd-var.c     | 22 +++++++++++-----------
 gdb/mi/mi-main.c        | 14 +++++++-------
 gdb/osdata.c            |  3 +--
 gdb/probe.c             |  5 ++---
 gdb/target-connection.c |  2 +-
 gdb/thread.c            |  3 +--
 gdb/tracepoint.c        |  4 ++--
 gdb/ui-out.c            |  5 +++--
 gdb/ui-out.h            |  3 ++-
 16 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
index 31a622c83e5..9530dea86ba 100644
--- a/gdb/break-catch-sig.c
+++ b/gdb/break-catch-sig.c
@@ -237,7 +237,7 @@ signal_catchpoint_print_one (struct breakpoint *b,
 
 	  text += name;
 	}
-      uiout->field_string ("what", text.c_str ());
+      uiout->field_string ("what", text);
     }
   else
     uiout->field_string ("what",
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index c8d5ccd5152..7fc6953b90c 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -300,7 +300,7 @@ print_one_detail_exception_catchpoint (const struct breakpoint *b,
   if (!cp->exception_rx.empty ())
     {
       uiout->text (_("\tmatching: "));
-      uiout->field_string ("regexp", cp->exception_rx.c_str ());
+      uiout->field_string ("regexp", cp->exception_rx);
       uiout->text ("\n");
     }
 }
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index f6a594d69a2..5fd5fd15c6a 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -714,7 +714,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
      versions of code to print the value out.  */
 
   if (uiout->is_mi_like_p ())
-    uiout->field_string ("value", val.c_str ());
+    uiout->field_string ("value", val);
   else
     {
       if (c->show_value_func != NULL)
diff --git a/gdb/disasm.c b/gdb/disasm.c
index eb69e89017b..70c54220a29 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -244,7 +244,7 @@ gdb_pretty_print_disassembler::pretty_print_insn (const struct disasm_insn *insn
 	   the future.  */
 	m_uiout->text (" <");
 	if (!omit_fname)
-	  m_uiout->field_string ("func-name", name.c_str (),
+	  m_uiout->field_string ("func-name", name,
 				 function_name_style.style ());
 	/* For negative offsets, avoid displaying them as +-N; the sign of
 	   the offset takes the place of the "+" here.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a22d815f230..4351409af50 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1539,7 +1539,7 @@ print_return_value_1 (struct ui_out *uiout, struct return_value_info *rv)
     {
       std::string type_name = type_to_string (rv->type);
       uiout->text ("Value returned has type: ");
-      uiout->field_string ("return-type", type_name.c_str ());
+      uiout->field_string ("return-type", type_name);
       uiout->text (".");
       uiout->text (" Cannot determine contents\n");
     }
diff --git a/gdb/inferior.c b/gdb/inferior.c
index a8779c354b5..059839ec962 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -529,7 +529,7 @@ print_inferior (struct ui_out *uiout, const char *requested_inferiors)
       uiout->field_string ("target-id", inferior_pid_to_str (inf->pid));
 
       std::string conn = uiout_field_connection (inf->process_target ());
-      uiout->field_string ("connection-id", conn.c_str ());
+      uiout->field_string ("connection-id", conn);
 
       if (inf->pspace->exec_filename != nullptr)
 	uiout->field_string ("exec", inf->pspace->exec_filename.get ());
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 2c75cd60794..9925b02e778 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1935,7 +1935,7 @@ info_auto_load_libthread_db (const char *args, int from_tty)
 	    i++;
 	  }
 
-	uiout->field_string ("pids", pids.c_str ());
+	uiout->field_string ("pids", pids);
 
 	uiout->text ("\n");
       }
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index e54a5e04970..61bc169304b 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -56,7 +56,7 @@ print_varobj (struct varobj *var, enum print_values print_values,
     {
       std::string exp = varobj_get_expression (var);
 
-      uiout->field_string ("exp", exp.c_str ());
+      uiout->field_string ("exp", exp);
     }
   uiout->field_signed ("numchild", varobj_get_num_children (var));
   
@@ -64,12 +64,12 @@ print_varobj (struct varobj *var, enum print_values print_values,
     {
       std::string val = varobj_get_value (var);
 
-      uiout->field_string ("value", val.c_str ());
+      uiout->field_string ("value", val);
     }
 
   std::string type = varobj_get_type (var);
   if (!type.empty ())
-    uiout->field_string ("type", type.c_str ());
+    uiout->field_string ("type", type);
 
   thread_id = varobj_get_thread_id (var);
   if (thread_id > 0)
@@ -236,7 +236,7 @@ mi_cmd_var_set_format (const char *command, char **argv, int argc)
  
   /* Report the value in the new format.  */
   std::string val = varobj_get_value (var);
-  uiout->field_string ("value", val.c_str ());
+  uiout->field_string ("value", val);
 }
 
 void
@@ -423,7 +423,7 @@ mi_cmd_var_info_type (const char *command, char **argv, int argc)
   var = varobj_get_handle (argv[0]);
 
   std::string type_name = varobj_get_type (var);
-  uiout->field_string ("type", type_name.c_str ());
+  uiout->field_string ("type", type_name);
 }
 
 void
@@ -461,7 +461,7 @@ mi_cmd_var_info_expression (const char *command, char **argv, int argc)
   uiout->field_string ("lang", lang->natural_name ());
 
   std::string exp = varobj_get_expression (var);
-  uiout->field_string ("exp", exp.c_str ());
+  uiout->field_string ("exp", exp);
 }
 
 void
@@ -545,13 +545,13 @@ mi_cmd_var_evaluate_expression (const char *command, char **argv, int argc)
     {
       std::string val = varobj_get_formatted_value (var, format);
 
-      uiout->field_string ("value", val.c_str ());
+      uiout->field_string ("value", val);
     }
   else
     {
       std::string val = varobj_get_value (var);
 
-      uiout->field_string ("value", val.c_str ());
+      uiout->field_string ("value", val);
     }
 }
 
@@ -582,7 +582,7 @@ mi_cmd_var_assign (const char *command, char **argv, int argc)
 	     "expression to variable object"));
 
   std::string val = varobj_get_value (var);
-  uiout->field_string ("value", val.c_str ());
+  uiout->field_string ("value", val);
 }
 
 /* Helper for mi_cmd_var_update - update each VAR.  */
@@ -692,7 +692,7 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 	    {
 	      std::string val = varobj_get_value (r.varobj);
 
-	      uiout->field_string ("value", val.c_str ());
+	      uiout->field_string ("value", val);
 	    }
 	  uiout->field_string ("in_scope", "true");
 	  break;
@@ -716,7 +716,7 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 	{
 	  std::string type_name = varobj_get_type (r.varobj);
 
-	  uiout->field_string ("new_type", type_name.c_str ());
+	  uiout->field_string ("new_type", type_name);
 	}
 
       if (r.type_changed || r.children_changed)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 582a55490d1..9d205f0208b 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -738,12 +738,12 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
 
       ui_out_emit_tuple tuple_emitter (uiout, NULL);
 
-      uiout->field_string ("id", pid->c_str ());
+      uiout->field_string ("id", *pid);
       uiout->field_string ("type", "process");
       if (cmd)
-	uiout->field_string ("description", cmd->c_str ());
+	uiout->field_string ("description", *cmd);
       if (user)
-	uiout->field_string ("user", user->c_str ());
+	uiout->field_string ("user", *user);
       if (cores)
 	output_cores (uiout, "cores", cores->c_str ());
 
@@ -762,9 +762,9 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
 		  const std::string *tid = get_osdata_column (child, "tid");
 		  const std::string *tcore = get_osdata_column (child, "core");
 
-		  uiout->field_string ("id", tid->c_str ());
+		  uiout->field_string ("id", *tid);
 		  if (tcore)
-		    uiout->field_string ("core", tcore->c_str ());
+		    uiout->field_string ("core", *tcore);
 		}
 	    }
 	}
@@ -1470,7 +1470,7 @@ mi_cmd_data_read_memory_bytes (const char *command, char **argv, int argc)
       std::string data = bin2hex (read_result.data.get (),
 				  (read_result.end - read_result.begin)
 				  * unit_size);
-      uiout->field_string ("contents", data.c_str ());
+      uiout->field_string ("contents", data);
     }
 }
 
@@ -2670,7 +2670,7 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc)
 	    if (target_read_memory (r.start, data.data (), r.length) == 0)
 	      {
 		std::string data_str = bin2hex (data.data (), r.length);
-		uiout->field_string ("contents", data_str.c_str ());
+		uiout->field_string ("contents", data_str);
 	      }
 	    else
 	      uiout->field_skip ("contents");
diff --git a/gdb/osdata.c b/gdb/osdata.c
index 2ff0f520b0b..bc621cb2332 100644
--- a/gdb/osdata.c
+++ b/gdb/osdata.c
@@ -273,8 +273,7 @@ info_osdata (const char *type)
 		 continue;
 
 	       snprintf (col_name, 32, "col%d", ix_cols);
-	       uiout->field_string (col_name,
-				    item.columns[ix_cols].value.c_str ());
+	       uiout->field_string (col_name, item.columns[ix_cols].value);
 	     }
 	 }
 
diff --git a/gdb/probe.c b/gdb/probe.c
index 9eccf82f25d..d9a06e53416 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -571,9 +571,8 @@ info_probes_for_spops (const char *arg, int from_tty,
 	ui_out_emit_tuple tuple_emitter (current_uiout, "probe");
 
 	current_uiout->field_string ("type", probe_type);
-	current_uiout->field_string ("provider",
-				     probe.prob->get_provider ().c_str ());
-	current_uiout->field_string ("name", probe.prob->get_name ().c_str ());
+	current_uiout->field_string ("provider", probe.prob->get_provider ());
+	current_uiout->field_string ("name", probe.prob->get_name ());
 	current_uiout->field_core_addr ("addr", probe.prob->get_gdbarch (),
 					probe.prob->get_relocated_address
 					(probe.objfile));
diff --git a/gdb/target-connection.c b/gdb/target-connection.c
index 2f8e01d77e5..a649423e07e 100644
--- a/gdb/target-connection.c
+++ b/gdb/target-connection.c
@@ -133,7 +133,7 @@ print_connection (struct ui_out *uiout, const char *requested_connections)
 
       uiout->field_signed ("number", t->connection_number);
 
-      uiout->field_string ("what", make_target_connection_string (t).c_str ());
+      uiout->field_string ("what", make_target_connection_string (t));
 
       uiout->field_string ("description", t->longname ());
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 87b6cbf74fd..40051013c0a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1184,8 +1184,7 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 	    }
 	  else
 	    {
-	      uiout->field_string ("target-id",
-				   thread_target_id_str (tp).c_str ());
+	      uiout->field_string ("target-id", thread_target_id_str (tp));
 	    }
 
 	  if (tp->state == THREAD_RUNNING)
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 7f6d3e4a16b..da0e976c869 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -3678,7 +3678,7 @@ print_one_static_tracepoint_marker (int count,
      identifier!  */
   uiout->field_signed ("count", count);
 
-  uiout->field_string ("marker-id", marker.str_id.c_str ());
+  uiout->field_string ("marker-id", marker.str_id);
 
   uiout->field_fmt ("enabled", "%c",
 		    !tracepoints.empty () ? 'y' : 'n');
@@ -3735,7 +3735,7 @@ print_one_static_tracepoint_marker (int count,
   uiout->text ("\n");
   uiout->text (extra_field_indent);
   uiout->text (_("Data: \""));
-  uiout->field_string ("extra-data", marker.extra.c_str ());
+  uiout->field_string ("extra-data", marker.extra);
   uiout->text ("\"\n");
 
   if (!tracepoints.empty ())
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 56251c9445a..c091af6cad3 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -528,9 +528,10 @@ ui_out::field_string (const char *fldname, const char *string,
 }
 
 void
-ui_out::field_string (const char *fldname, const std::string &string)
+ui_out::field_string (const char *fldname, const std::string &string,
+		      const ui_file_style &style)
 {
-  field_string (fldname, string.c_str ());
+  field_string (fldname, string.c_str (), style);
 }
 
 /* VARARGS */
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index a06477df533..46dfe28c1e1 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -191,7 +191,8 @@ class ui_out
 			CORE_ADDR address);
   void field_string (const char *fldname, const char *string,
 		     const ui_file_style &style = ui_file_style ());
-  void field_string (const char *fldname, const std::string &string);
+  void field_string (const char *fldname, const std::string &string,
+		     const ui_file_style &style = ui_file_style ());
   void field_stream (const char *fldname, string_file &stream,
 		     const ui_file_style &style = ui_file_style ());
   void field_skip (const char *fldname);
-- 
2.28.0

Re: [PATCH 2/4] gdb: Pass std::strings to ui_out::field_string () where convenient

[Simon Marchi]

On 2021-05-18 7:07 a.m., Marco Barisione via Gdb-patches wrote:
> While adding a ui_out::text () overload accepting a std::string, I
> noticed that several callers of ui_out::field_string () were converting
> std::string instances to char pointers even if not necessary.

LGTM.

Similarly to ui_out::text, I think it would make sense, maybe as a
follow patch, to move the implementation of the string& field_string
overload to the header file.  And I say that as the person who
introduced that overload :).

Simon

Re: [PATCH 2/4] gdb: Pass std::strings to ui_out::field_string () where convenient

[Tom Tromey]

>>>>> "Marco" == Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> writes:

Marco> While adding a ui_out::text () overload accepting a std::string, I
Marco> noticed that several callers of ui_out::field_string () were converting
Marco> std::string instances to char pointers even if not necessary.

Thank you.  This is ok.  I'd guess that these calls predate the
overload, but I didn't go check the history.

Tom

[PATCH 3/4] gdb: Fix crash in cmd_show_list () if the prefix doesn't contain "show "

[Marco Barisione]

In general, cmd_show_list () should be called on commands which contain
a component called "show", for instance for "show *" or "maintenance
show *" commands, but there's nothing preventing commands with different
names from using cmd_show_list ().

gdb/ChangeLog:

	* cli/cli-setshow.c (prefixname_without_show): Add.
	(cmd_show_list): Use prefixname_without_show.
---
 gdb/cli/cli-setshow.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 5fd5fd15c6a..a3b63bdaab0 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -726,6 +726,23 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
   c->func (c, NULL, from_tty);
 }
 
+/* Like C->prefixname (), but "show " and the text coming before it is
+ * skipped if present.  */
+
+static std::string
+prefixname_without_show (cmd_list_element *c)
+{
+  const char show[] = "show ";
+  const size_t show_len = sizeof (show) - 1 /* \0 terminator */;
+
+  std::string prefixname = c->prefixname ();
+  size_t pos = prefixname.find (show);
+  if (pos == std::string::npos)
+    return prefixname;
+  else
+    return prefixname.substr (pos + show_len);
+}
+
 /* Show all the settings in a list of show commands.  */
 
 void
@@ -743,11 +760,10 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
       if (list->is_prefix () && !list->is_alias ())
 	{
 	  ui_out_emit_tuple optionlist_emitter (uiout, "optionlist");
-	  std::string prefixname = list->prefixname ();
-	  const char *new_prefix = strstr (prefixname.c_str (), "show ") + 5;
 
 	  if (uiout->is_mi_like_p ())
-	    uiout->field_string ("prefix", new_prefix);
+	    uiout->field_string ("prefix",
+				 prefixname_without_show (list).c_str ());
 	  cmd_show_list (*list->subcommands, from_tty);
 	}
       else if (list->theclass != no_set_class && !list->is_alias ())
@@ -757,9 +773,9 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
 	  if (list->prefix != nullptr)
 	    {
 	      /* If we find a prefix, output it (with "show " skipped).  */
-	      std::string prefixname = list->prefix->prefixname ();
-	      prefixname = (!list->prefix->is_prefix () ? ""
-			    : strstr (prefixname.c_str (), "show ") + 5);
+	      std::string prefixname = (list->prefix->is_prefix ()
+					? prefixname_without_show (list->prefix)
+					: "");
 	      uiout->text (prefixname);
 	    }
 	  uiout->field_string ("name", list->name);
-- 
2.28.0

Re: [PATCH 3/4] gdb: Fix crash in cmd_show_list () if the prefix doesn't contain "show "

[Andrew Burgess]

* Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> [2021-05-18 12:07:39 +0100]:

> In general, cmd_show_list () should be called on commands which contain
> a component called "show", for instance for "show *" or "maintenance
> show *" commands, but there's nothing preventing commands with different
> names from using cmd_show_list ().
> 
> gdb/ChangeLog:
> 
> 	* cli/cli-setshow.c (prefixname_without_show): Add.
> 	(cmd_show_list): Use prefixname_without_show.

I sense this patch is related to the next one in this series, but it's
not clear to me if this is fixing a bug, or providing new
functionality.

Also, does this change the behaviour of GDB in someway that is visible
to the user?  If it does then you should consider writing a test.

Thanks,
Andrew


> ---
>  gdb/cli/cli-setshow.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index 5fd5fd15c6a..a3b63bdaab0 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -726,6 +726,23 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
>    c->func (c, NULL, from_tty);
>  }
>  
> +/* Like C->prefixname (), but "show " and the text coming before it is
> + * skipped if present.  */
> +
> +static std::string
> +prefixname_without_show (cmd_list_element *c)
> +{
> +  const char show[] = "show ";
> +  const size_t show_len = sizeof (show) - 1 /* \0 terminator */;
> +
> +  std::string prefixname = c->prefixname ();
> +  size_t pos = prefixname.find (show);
> +  if (pos == std::string::npos)
> +    return prefixname;
> +  else
> +    return prefixname.substr (pos + show_len);
> +}
> +
>  /* Show all the settings in a list of show commands.  */
>  
>  void
> @@ -743,11 +760,10 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
>        if (list->is_prefix () && !list->is_alias ())
>  	{
>  	  ui_out_emit_tuple optionlist_emitter (uiout, "optionlist");
> -	  std::string prefixname = list->prefixname ();
> -	  const char *new_prefix = strstr (prefixname.c_str (), "show ") + 5;
>  
>  	  if (uiout->is_mi_like_p ())
> -	    uiout->field_string ("prefix", new_prefix);
> +	    uiout->field_string ("prefix",
> +				 prefixname_without_show (list).c_str ());
>  	  cmd_show_list (*list->subcommands, from_tty);
>  	}
>        else if (list->theclass != no_set_class && !list->is_alias ())
> @@ -757,9 +773,9 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
>  	  if (list->prefix != nullptr)
>  	    {
>  	      /* If we find a prefix, output it (with "show " skipped).  */
> -	      std::string prefixname = list->prefix->prefixname ();
> -	      prefixname = (!list->prefix->is_prefix () ? ""
> -			    : strstr (prefixname.c_str (), "show ") + 5);
> +	      std::string prefixname = (list->prefix->is_prefix ()
> +					? prefixname_without_show (list->prefix)
> +					: "");
>  	      uiout->text (prefixname);
>  	    }
>  	  uiout->field_string ("name", list->name);
> -- 
> 2.28.0
> 

Re: [PATCH 3/4] gdb: Fix crash in cmd_show_list () if the prefix doesn't contain "show "

[Marco Barisione]

On 18 May 2021, at 14:41, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> * Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> [2021-05-18 12:07:39 +0100]:
> 
>> In general, cmd_show_list () should be called on commands which contain
>> a component called "show", for instance for "show *" or "maintenance
>> show *" commands, but there's nothing preventing commands with different
>> names from using cmd_show_list ().
>> 
>> gdb/ChangeLog:
>> 
>> 	* cli/cli-setshow.c (prefixname_without_show): Add.
>> 	(cmd_show_list): Use prefixname_without_show.
> 
> I sense this patch is related to the next one in this series, but it's
> not clear to me if this is fixing a bug, or providing new
> functionality.

It avoids a crash if a prefix command is added with add_show_prefix_cmd,
but the full command name doesn’t include the "show " string.  If you
don’t think that this should be allowed, then I could use an assertion
instead, which would at least provide a better explanation of what went
wrong.

> Also, does this change the behaviour of GDB in someway that is visible
> to the user?  If it does then you should consider writing a test.

This is visible to the user just because add_show_prefix_cmd is used
with setlist rather than showlist for the "set/show python" commands.
Otherwise, the user should not be able to see this.

There is no way to test this unless I add a command just for the sake
of a single test.

-- 
Marco Barisione

[PATCH 4/4] gdb: Add "set|show python" commands to the correct list

[Marco Barisione]

Previously, they were added to the wrong list which meant that "set
python" used called cmd_show_list, which used to crash as the name
of the command doesn't contain a part called "show".

gdb/ChangeLog:

	* python/python.c: Assign "set python" to the set list and
	"show python" to the show list.
---
 gdb/python/python.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4cea83c3837..1e687c3ac50 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1936,11 +1936,11 @@ This command is only a placeholder.")
   /* Add set/show python print-stack.  */
   add_basic_prefix_cmd ("python", no_class,
 			_("Prefix command for python preference settings."),
-			&user_show_python_list, 0, &showlist);
+			&user_set_python_list, 0, &setlist);
 
   add_show_prefix_cmd ("python", no_class,
 		       _("Prefix command for python preference settings."),
-		       &user_set_python_list, 0, &setlist);
+		       &user_show_python_list, 0, &showlist);
 
   add_setshow_enum_cmd ("print-stack", no_class, python_excp_enums,
 			&gdbpy_should_print_stack, _("\
-- 
2.28.0

Re: [PATCH 4/4] gdb: Add "set|show python" commands to the correct list

[Andrew Burgess]

* Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> [2021-05-18 12:07:40 +0100]:

> Previously, they were added to the wrong list which meant that "set
> python" used called cmd_show_list, which used to crash as the name
> of the command doesn't contain a part called "show".

It feels like this should be a pretty easy case to add a test for.

Thanks,
Andrew

> 
> gdb/ChangeLog:
> 
> 	* python/python.c: Assign "set python" to the set list and
> 	"show python" to the show list.
> ---
>  gdb/python/python.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 4cea83c3837..1e687c3ac50 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1936,11 +1936,11 @@ This command is only a placeholder.")
>    /* Add set/show python print-stack.  */
>    add_basic_prefix_cmd ("python", no_class,
>  			_("Prefix command for python preference settings."),
> -			&user_show_python_list, 0, &showlist);
> +			&user_set_python_list, 0, &setlist);
>  
>    add_show_prefix_cmd ("python", no_class,
>  		       _("Prefix command for python preference settings."),
> -		       &user_set_python_list, 0, &setlist);
> +		       &user_show_python_list, 0, &showlist);
>  
>    add_setshow_enum_cmd ("print-stack", no_class, python_excp_enums,
>  			&gdbpy_should_print_stack, _("\
> -- 
> 2.28.0
> 

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Andrew Burgess]

* Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> [2021-05-18 12:07:36 +0100]:

> I noticed by chance that the "set python" command with no arguments
> causes a segfault.  This is due to a trivial mistake, the prefix command
> meant to be "show python" is added to the list of set commands, and the
> "set python" one is added to the list of show commands.
> 
> I didn't add a specific test because what's the chance of "set python"
> regressing?

Don't we know the answer to that is 100% - it's currently regressed,
right?

>              I tried writing a self test instead (like the ones in
> gdb/unittests/command-def-selftests.c), but I couldn't figure out how to
> detect this problem in a generic way.  Any idea?  Or it this patch
> OK without a test?

I don't have any suggestions for a self test, but writing a "normal"
test should be pretty easy, personally, I'd just do that.

Thanks,
Andrew

> 
> This patch series also includes some cleanups which came up from the
> work on the main fix.
> 
> Marco Barisione (4):
>   gdb: Add an overloaded ui_out::text accepting a const std::string &
>   gdb: Pass std::strings to ui_out::field_string () where convenient
>   gdb: Fix crash in cmd_show_list () if the prefix doesn't contain "show
>     "
>   gdb: Add "set|show python" commands to the correct list
> 
>  gdb/ada-lang.c          |  4 ++--
>  gdb/break-catch-sig.c   |  2 +-
>  gdb/break-catch-throw.c |  2 +-
>  gdb/cli/cli-setshow.c   | 32 ++++++++++++++++++++++++--------
>  gdb/disasm.c            |  2 +-
>  gdb/infcmd.c            |  2 +-
>  gdb/inferior.c          |  2 +-
>  gdb/linux-thread-db.c   |  2 +-
>  gdb/mi/mi-cmd-var.c     | 22 +++++++++++-----------
>  gdb/mi/mi-main.c        | 14 +++++++-------
>  gdb/osdata.c            |  3 +--
>  gdb/probe.c             |  5 ++---
>  gdb/python/python.c     |  4 ++--
>  gdb/source.c            |  2 +-
>  gdb/target-connection.c |  2 +-
>  gdb/thread.c            |  5 ++---
>  gdb/tracepoint.c        |  4 ++--
>  gdb/ui-out.c            | 11 +++++++++--
>  gdb/ui-out.h            |  4 +++-
>  19 files changed, 73 insertions(+), 51 deletions(-)
> 
> -- 
> 2.28.0
> 

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Tom Tromey]

>> I didn't add a specific test because what's the chance of "set python"
>> regressing?

Andrew> Don't we know the answer to that is 100% - it's currently regressed,
Andrew> right?

I think he meant the chances of it happening again.

Anyway, Marco, in gdb it is normal to add tests for things like this.
It can just test "set python" and have a comment explaining that, once
upon a time, this caused a crash.

thanks,
Tom

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Philippe Waroquiers]

On Tue, 2021-05-18 at 08:50 -0600, Tom Tromey wrote:
> > > I didn't add a specific test because what's the chance of "set python"
> > > regressing?
> 
> Andrew> Don't we know the answer to that is 100% - it's currently regressed,
> Andrew> right?
> 
> I think he meant the chances of it happening again.
> 
> Anyway, Marco, in gdb it is normal to add tests for things like this.
> It can just test "set python" and have a comment explaining that, once
> upon a time, this caused a crash.
> 
> thanks,
> Tom
IIRC, it has happened already for a few other commands that launching a command
without argument caused a SEGV or similar.

So, maybe what could be done is a test that calls all set and show commands 
with something like
  (gdb) | help show | grep -e -- | sed -e 's/--.*$//' -e 's/,.*//' > launch_all_show
  # the second sed expression cleans up  'show check, show ch, show c'
  (gdb) source launch_all_show
and similar for the set.

However, the launch_all_set quickly stops on an error
as the set commands need some arguments
but that can be solved as soon as we have
  with ignore-errors -- source launch_all_set
or
  ignore-errors source launch_all_set
:)

With the above, we can verify automatically that all set and all show commands are not
crashing gdb.

Philippe

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Marco Barisione]

On 18 May 2021, at 20:34, Philippe Waroquiers via Gdb-patches <gdb-patches@sourceware.org> wrote:
> On Tue, 2021-05-18 at 08:50 -0600, Tom Tromey wrote:
>>>> I didn't add a specific test because what's the chance of "set python"
>>>> regressing?
>> 
>> Andrew> Don't we know the answer to that is 100% - it's currently regressed,
>> Andrew> right?
>> 
>> I think he meant the chances of it happening again.
>> 
>> Anyway, Marco, in gdb it is normal to add tests for things like this.
>> It can just test "set python" and have a comment explaining that, once
>> upon a time, this caused a crash.
>> 
>> thanks,
>> Tom
> IIRC, it has happened already for a few other commands that launching a command
> without argument caused a SEGV or similar.
> 
> So, maybe what could be done is a test that calls all set and show commands 
> with something like
>  (gdb) | help show | grep -e -- | sed -e 's/--.*$//' -e 's/,.*//' > launch_all_show
>  # the second sed expression cleans up  'show check, show ch, show c'
>  (gdb) source launch_all_show
> and similar for the set.
> 
> However, the launch_all_set quickly stops on an error
> as the set commands need some arguments
> but that can be solved as soon as we have
>  with ignore-errors -- source launch_all_set
> or
>  ignore-errors source launch_all_set
> :)
> 
> With the above, we can verify automatically that all set and all show commands are not
> crashing gdb.

I will explain what I attempted so it’s clearer what I meant.

I have a branch which adds a new "maintenance selftest" test which recursively
iterates over all commands reachable from showlist.
For each command, it discards the output (using null_stream) and executes the
command.  The test expects that no exception is raised and that no crash happens.
This works fine!  But the wrong "show python" didn’t actually cause any problem.

Then I tried the same for setlist.  Unlike for showlist, I allow gdb_exception
to be raised and just ignore it (most set commands will complain that they have
no got an argument).
This mostly works, but some set commands change settings when no argument is
given.  For instance, "set pagination" is equivalent to "set pagination on”, and
"set prompt" sets the prompt to the empty string (which made me think that the
test was stuck!).

A solution is to save cmd_list_element::var and restore it after executing the
command (like scoped_restore but at runtime), but that would only work for some
commands and not for "set python" which doesn’t have a variable to change.

A much easier solution is to just save a bunch of settings (pagination and
prompt for example) and accept that some other settings may have changed after
running self tests.  Whether this is acceptable probably depends on how many
people use "maintenance selftest" in an interactive session.

Is this all too complicated and not worth it?  Or would it be helpful to
prevent future bugs?

-- 
Marco Barisione

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Tom Tromey]

>>>>> "Marco" == Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> writes:

Marco> A much easier solution is to just save a bunch of settings (pagination and
Marco> prompt for example) and accept that some other settings may have changed after
Marco> running self tests.  Whether this is acceptable probably depends on how many
Marco> people use "maintenance selftest" in an interactive session.

This general principle is being discussed in another thread as well.
There, I think the consensus is that self-tests should not depend on, or
(meaningfully <- an important caveat due to things like cache flushing)
change, the global state.

Marco> Is this all too complicated and not worth it?  Or would it be helpful to
Marco> prevent future bugs?

IIUC in this particular case, the bug was caused by swapped set/show
arguments to a function.  Maybe a different solution would be to make
this impossible, by introducing a new type that pairs the two globals.

At the same time, testing the commands seems like a plus to me.
Perhaps we could have a list of "set" commands to skip, or something
along those lines.

Tom

Re: [PATCH 0/4] Fix crash in "set python" and small cleanups

[Marco Barisione]

On 24 May 2021, at 15:22, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Marco" == Marco Barisione via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Marco> A much easier solution is to just save a bunch of settings (pagination and
> Marco> prompt for example) and accept that some other settings may have changed after
> Marco> running self tests.  Whether this is acceptable probably depends on how many
> Marco> people use "maintenance selftest" in an interactive session.
> 
> This general principle is being discussed in another thread as well.
> There, I think the consensus is that self-tests should not depend on, or
> (meaningfully <- an important caveat due to things like cache flushing)
> change, the global state.

I’ve been following that thread and the one about ignoring errors.  Once the
patch for the latter is merged, I will submit a normal test (that is,
non-selftest) which tests all show and set commands.  Basically, I will
implement my idea but in TCL rather that in C.

-- 
Marco Barisione