[PATCH][gdb] Make execute_command_to_string return string on throw

[PATCH][gdb] Make execute_command_to_string return string on throw

[Tom de Vries]

Hi,

The pattern for using execute_command_to_string is:
...
  std::string output;
  output = execute_fn_to_string (fn, term_out);
...

This results in a problem when using it in a try/catch:
...
  try
    {
      output = execute_fn_to_string (fn, term_out)
    }
  catch (const gdb_exception &e)
    {
      /* Use output.  */
    }
...

If an expection was thrown during execute_fn_to_string, then the output
remains unassigned, while it could be worthwhile to known what output was
generated by gdb before the expection was thrown.

Fix this by returning the string using a parameter instead:
...
  execute_fn_to_string (output, fn, term_out)
...

Also add a variant without string parameter, to support places where the
function is used while ignoring the result:
...
  execute_fn_to_string (fn, term_out)
...

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb] Make execute_command_to_string return string on throw

---
 gdb/complaints.c    |  4 ++--
 gdb/gdbcmd.h        | 12 +++++++++---
 gdb/guile/guile.c   |  2 +-
 gdb/python/python.c |  6 +++---
 gdb/stack.c         |  4 ++--
 gdb/thread.c        |  5 +++--
 gdb/top.c           | 34 +++++++++++++++++++++++++++-------
 7 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 525a3a7eacf..a823fcb0020 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -91,7 +91,7 @@ test_complaints ()
   do									\
     {									\
       std::string output;						\
-      output = execute_fn_to_string ([]() { complaint (STR); }, false);	\
+      execute_fn_to_string (output, []() { complaint (STR); }, false);	\
       std::string expected						\
 	= _("During symbol reading: ") + std::string (STR "\n");	\
       SELF_CHECK (output == expected);					\
@@ -102,7 +102,7 @@ test_complaints ()
   do									\
     {									\
       std::string output;						\
-      output = execute_fn_to_string ([]() { complaint (STR); }, false);	\
+      execute_fn_to_string (output, []() { complaint (STR); }, false);	\
       SELF_CHECK (output.empty ());					\
       SELF_CHECK (counters[STR] == CNT);				\
     } while (0)
diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h
index 27550c1faee..05ffd7fd0ee 100644
--- a/gdb/gdbcmd.h
+++ b/gdb/gdbcmd.h
@@ -145,7 +145,8 @@ extern void execute_fn_to_ui_file (struct ui_file *file, std::function<void(void
    (e.g. with styling).  When TERM_OUT is false raw output will be collected
    (e.g. no styling).  */
 
-extern std::string execute_fn_to_string (std::function<void(void)> fn, bool term_out);
+extern void execute_fn_to_string (std::string &res,
+				  std::function<void(void)> fn, bool term_out);
 
 /* As execute_fn_to_ui_file, but run execute_command for P and FROM_TTY.  */
 
@@ -154,8 +155,13 @@ extern void execute_command_to_ui_file (struct ui_file *file,
 
 /* As execute_fn_to_string, but run execute_command for P and FROM_TTY.  */
 
-extern std::string execute_command_to_string (const char *p, int from_tty,
-					      bool term_out);
+extern void execute_command_to_string (std::string &res, const char *p,
+				       int from_tty, bool term_out);
+
+/* As execute_command_to_string, but ignore resulting string.  */
+
+extern void execute_command_to_string (const char *p,
+				       int from_tty, bool term_out);
 
 extern void print_command_line (struct command_line *, unsigned int,
 				struct ui_file *);
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index a28dee37ed8..8ba840cba6a 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -302,7 +302,7 @@ gdbscm_execute_gdb_command (SCM command_scm, SCM rest)
 
       scoped_restore preventer = prevent_dont_repeat ();
       if (to_string)
-	to_string_res = execute_command_to_string (command, from_tty, false);
+	execute_command_to_string (to_string_res, command, from_tty, false);
       else
 	execute_command (command, from_tty);
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 5918bb414a3..34c1be76a16 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1888,11 +1888,11 @@ namespace selftests {
 static void
 test_python ()
 {
-#define CMD execute_command_to_string ("python print(5)", 0, true);
+#define CMD(S) execute_command_to_string (S, "python print(5)", 0, true)
 
   std::string output;
 
-  output = CMD;
+  CMD (output);
   SELF_CHECK (output == "5\n");
   output.clear ();
 
@@ -1901,7 +1901,7 @@ test_python ()
     = make_scoped_restore (&gdb_python_initialized, 0);
   try
     {
-      output = CMD;
+      CMD (output);
     }
   catch (const gdb_exception &e)
     {
diff --git a/gdb/stack.c b/gdb/stack.c
index 516e4d45696..edfda3a4e55 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -3026,8 +3026,8 @@ frame_apply_command_count (const char *which_command,
 	       set to the selected frame.  */
 	    scoped_restore_current_thread restore_fi_current_frame;
 
-	    cmd_result = execute_command_to_string
-	      (cmd, from_tty, gdb_stdout->term_out ());
+	    execute_command_to_string
+	      (cmd_result, cmd, from_tty, gdb_stdout->term_out ());
 	  }
 	  fi = get_selected_frame (_("frame apply "
 				     "unable to get selected frame."));
diff --git a/gdb/thread.c b/gdb/thread.c
index 10c3dcd6991..019c658bc47 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1457,8 +1457,9 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty,
 
   try
     {
-      std::string cmd_result = execute_command_to_string
-	(cmd, from_tty, gdb_stdout->term_out ());
+      std::string cmd_result;
+      execute_command_to_string
+	(cmd_result, cmd, from_tty, gdb_stdout->term_out ());
       if (!flags.silent || cmd_result.length () > 0)
 	{
 	  if (!flags.quiet)
diff --git a/gdb/top.c b/gdb/top.c
index 7e95ed3969c..ff240eed657 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -728,13 +728,22 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
 
 /* See gdbcmd.h.  */
 
-std::string
-execute_fn_to_string (std::function<void(void)> fn, bool term_out)
+void
+execute_fn_to_string (std::string &res, std::function<void(void)> fn,
+		      bool term_out)
 {
   string_file str_file (term_out);
 
-  execute_fn_to_ui_file (&str_file, fn);
-  return std::move (str_file.string ());
+  try {
+    execute_fn_to_ui_file (&str_file, fn);
+  } catch (...) {
+    /* Finally.  */
+    res = std::move (str_file.string ());
+    throw;
+  }
+
+  /* And finally.  */
+  res = std::move (str_file.string ());
 }
 
 /* See gdbcmd.h.  */
@@ -748,12 +757,23 @@ execute_command_to_ui_file (struct ui_file *file,
 
 /* See gdbcmd.h.  */
 
-std::string
+void
+execute_command_to_string (std::string &res, const char *p, int from_tty,
+			   bool term_out)
+{
+  execute_fn_to_string (res, [=]() { execute_command (p, from_tty); },
+			term_out);
+}
+
+/* See gdbcmd.h.  */
+
+void
 execute_command_to_string (const char *p, int from_tty,
 			   bool term_out)
 {
-  return
-    execute_fn_to_string ([=]() { execute_command (p, from_tty); }, term_out);
+  std::string dummy;
+  execute_fn_to_string (dummy, [=]() { execute_command (p, from_tty); },
+			term_out);
 }
 \f
 /* When nonzero, cause dont_repeat to do nothing.  This should only be

[PING][PATCH][gdb] Make execute_command_to_string return string on throw

[Tom de Vries]

On 9/11/21 2:02 PM, Tom de Vries wrote:
> Hi,
> 
> The pattern for using execute_command_to_string is:
> ...
>   std::string output;
>   output = execute_fn_to_string (fn, term_out);
> ...
> 
> This results in a problem when using it in a try/catch:
> ...
>   try
>     {
>       output = execute_fn_to_string (fn, term_out)
>     }
>   catch (const gdb_exception &e)
>     {
>       /* Use output.  */
>     }
> ...
> 
> If an expection was thrown during execute_fn_to_string, then the output
> remains unassigned, while it could be worthwhile to known what output was
> generated by gdb before the expection was thrown.
> 
> Fix this by returning the string using a parameter instead:
> ...
>   execute_fn_to_string (output, fn, term_out)
> ...
> 
> Also add a variant without string parameter, to support places where the
> function is used while ignoring the result:
> ...
>   execute_fn_to_string (fn, term_out)
> ...
> 
> Tested on x86_64-linux.
> 
> Any comments?
> 

Ping.

Thanks,
- Tom

> [gdb] Make execute_command_to_string return string on throw
> 
> ---
>  gdb/complaints.c    |  4 ++--
>  gdb/gdbcmd.h        | 12 +++++++++---
>  gdb/guile/guile.c   |  2 +-
>  gdb/python/python.c |  6 +++---
>  gdb/stack.c         |  4 ++--
>  gdb/thread.c        |  5 +++--
>  gdb/top.c           | 34 +++++++++++++++++++++++++++-------
>  7 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/complaints.c b/gdb/complaints.c
> index 525a3a7eacf..a823fcb0020 100644
> --- a/gdb/complaints.c
> +++ b/gdb/complaints.c
> @@ -91,7 +91,7 @@ test_complaints ()
>    do									\
>      {									\
>        std::string output;						\
> -      output = execute_fn_to_string ([]() { complaint (STR); }, false);	\
> +      execute_fn_to_string (output, []() { complaint (STR); }, false);	\
>        std::string expected						\
>  	= _("During symbol reading: ") + std::string (STR "\n");	\
>        SELF_CHECK (output == expected);					\
> @@ -102,7 +102,7 @@ test_complaints ()
>    do									\
>      {									\
>        std::string output;						\
> -      output = execute_fn_to_string ([]() { complaint (STR); }, false);	\
> +      execute_fn_to_string (output, []() { complaint (STR); }, false);	\
>        SELF_CHECK (output.empty ());					\
>        SELF_CHECK (counters[STR] == CNT);				\
>      } while (0)
> diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h
> index 27550c1faee..05ffd7fd0ee 100644
> --- a/gdb/gdbcmd.h
> +++ b/gdb/gdbcmd.h
> @@ -145,7 +145,8 @@ extern void execute_fn_to_ui_file (struct ui_file *file, std::function<void(void
>     (e.g. with styling).  When TERM_OUT is false raw output will be collected
>     (e.g. no styling).  */
>  
> -extern std::string execute_fn_to_string (std::function<void(void)> fn, bool term_out);
> +extern void execute_fn_to_string (std::string &res,
> +				  std::function<void(void)> fn, bool term_out);
>  
>  /* As execute_fn_to_ui_file, but run execute_command for P and FROM_TTY.  */
>  
> @@ -154,8 +155,13 @@ extern void execute_command_to_ui_file (struct ui_file *file,
>  
>  /* As execute_fn_to_string, but run execute_command for P and FROM_TTY.  */
>  
> -extern std::string execute_command_to_string (const char *p, int from_tty,
> -					      bool term_out);
> +extern void execute_command_to_string (std::string &res, const char *p,
> +				       int from_tty, bool term_out);
> +
> +/* As execute_command_to_string, but ignore resulting string.  */
> +
> +extern void execute_command_to_string (const char *p,
> +				       int from_tty, bool term_out);
>  
>  extern void print_command_line (struct command_line *, unsigned int,
>  				struct ui_file *);
> diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> index a28dee37ed8..8ba840cba6a 100644
> --- a/gdb/guile/guile.c
> +++ b/gdb/guile/guile.c
> @@ -302,7 +302,7 @@ gdbscm_execute_gdb_command (SCM command_scm, SCM rest)
>  
>        scoped_restore preventer = prevent_dont_repeat ();
>        if (to_string)
> -	to_string_res = execute_command_to_string (command, from_tty, false);
> +	execute_command_to_string (to_string_res, command, from_tty, false);
>        else
>  	execute_command (command, from_tty);
>  
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 5918bb414a3..34c1be76a16 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1888,11 +1888,11 @@ namespace selftests {
>  static void
>  test_python ()
>  {
> -#define CMD execute_command_to_string ("python print(5)", 0, true);
> +#define CMD(S) execute_command_to_string (S, "python print(5)", 0, true)
>  
>    std::string output;
>  
> -  output = CMD;
> +  CMD (output);
>    SELF_CHECK (output == "5\n");
>    output.clear ();
>  
> @@ -1901,7 +1901,7 @@ test_python ()
>      = make_scoped_restore (&gdb_python_initialized, 0);
>    try
>      {
> -      output = CMD;
> +      CMD (output);
>      }
>    catch (const gdb_exception &e)
>      {
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 516e4d45696..edfda3a4e55 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -3026,8 +3026,8 @@ frame_apply_command_count (const char *which_command,
>  	       set to the selected frame.  */
>  	    scoped_restore_current_thread restore_fi_current_frame;
>  
> -	    cmd_result = execute_command_to_string
> -	      (cmd, from_tty, gdb_stdout->term_out ());
> +	    execute_command_to_string
> +	      (cmd_result, cmd, from_tty, gdb_stdout->term_out ());
>  	  }
>  	  fi = get_selected_frame (_("frame apply "
>  				     "unable to get selected frame."));
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 10c3dcd6991..019c658bc47 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1457,8 +1457,9 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty,
>  
>    try
>      {
> -      std::string cmd_result = execute_command_to_string
> -	(cmd, from_tty, gdb_stdout->term_out ());
> +      std::string cmd_result;
> +      execute_command_to_string
> +	(cmd_result, cmd, from_tty, gdb_stdout->term_out ());
>        if (!flags.silent || cmd_result.length () > 0)
>  	{
>  	  if (!flags.quiet)
> diff --git a/gdb/top.c b/gdb/top.c
> index 7e95ed3969c..ff240eed657 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -728,13 +728,22 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
>  
>  /* See gdbcmd.h.  */
>  
> -std::string
> -execute_fn_to_string (std::function<void(void)> fn, bool term_out)
> +void
> +execute_fn_to_string (std::string &res, std::function<void(void)> fn,
> +		      bool term_out)
>  {
>    string_file str_file (term_out);
>  
> -  execute_fn_to_ui_file (&str_file, fn);
> -  return std::move (str_file.string ());
> +  try {
> +    execute_fn_to_ui_file (&str_file, fn);
> +  } catch (...) {
> +    /* Finally.  */
> +    res = std::move (str_file.string ());
> +    throw;
> +  }
> +
> +  /* And finally.  */
> +  res = std::move (str_file.string ());
>  }
>  
>  /* See gdbcmd.h.  */
> @@ -748,12 +757,23 @@ execute_command_to_ui_file (struct ui_file *file,
>  
>  /* See gdbcmd.h.  */
>  
> -std::string
> +void
> +execute_command_to_string (std::string &res, const char *p, int from_tty,
> +			   bool term_out)
> +{
> +  execute_fn_to_string (res, [=]() { execute_command (p, from_tty); },
> +			term_out);
> +}
> +
> +/* See gdbcmd.h.  */
> +
> +void
>  execute_command_to_string (const char *p, int from_tty,
>  			   bool term_out)
>  {
> -  return
> -    execute_fn_to_string ([=]() { execute_command (p, from_tty); }, term_out);
> +  std::string dummy;
> +  execute_fn_to_string (dummy, [=]() { execute_command (p, from_tty); },
> +			term_out);
>  }
>  \f
>  /* When nonzero, cause dont_repeat to do nothing.  This should only be
> 

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Simon Marchi]

> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -728,13 +728,22 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
>
>  /* See gdbcmd.h.  */
>
> -std::string
> -execute_fn_to_string (std::function<void(void)> fn, bool term_out)
> +void
> +execute_fn_to_string (std::string &res, std::function<void(void)> fn,
> +		      bool term_out)
>  {
>    string_file str_file (term_out);
>
> -  execute_fn_to_ui_file (&str_file, fn);
> -  return std::move (str_file.string ());
> +  try {
> +    execute_fn_to_ui_file (&str_file, fn);
> +  } catch (...) {
> +    /* Finally.  */
> +    res = std::move (str_file.string ());
> +    throw;
> +  }

Formatting.

The code LGTM, but could you write / update a test for this?

Simon

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Tom de Vries]

On 10/7/21 5:33 PM, Simon Marchi wrote:
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -728,13 +728,22 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
>>
>>  /* See gdbcmd.h.  */
>>
>> -std::string
>> -execute_fn_to_string (std::function<void(void)> fn, bool term_out)
>> +void
>> +execute_fn_to_string (std::string &res, std::function<void(void)> fn,
>> +		      bool term_out)
>>  {
>>    string_file str_file (term_out);
>>
>> -  execute_fn_to_ui_file (&str_file, fn);
>> -  return std::move (str_file.string ());
>> +  try {
>> +    execute_fn_to_ui_file (&str_file, fn);
>> +  } catch (...) {
>> +    /* Finally.  */
>> +    res = std::move (str_file.string ());
>> +    throw;
>> +  }
> 
> Formatting.
> 

Ack, fixed.

> The code LGTM, but could you write / update a test for this?

The patch "[gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as
unittest" uses this.  It checks the output of a command that is also
throwing a "Error while executing Python code".

Thanks,
- Tom

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Simon Marchi]

On 2021-10-07 12:15 p.m., Tom de Vries via Gdb-patches wrote:
> On 10/7/21 5:33 PM, Simon Marchi wrote:
>>> --- a/gdb/top.c
>>> +++ b/gdb/top.c
>>> @@ -728,13 +728,22 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
>>>
>>>  /* See gdbcmd.h.  */
>>>
>>> -std::string
>>> -execute_fn_to_string (std::function<void(void)> fn, bool term_out)
>>> +void
>>> +execute_fn_to_string (std::string &res, std::function<void(void)> fn,
>>> +		      bool term_out)
>>>  {
>>>    string_file str_file (term_out);
>>>
>>> -  execute_fn_to_ui_file (&str_file, fn);
>>> -  return std::move (str_file.string ());
>>> +  try {
>>> +    execute_fn_to_ui_file (&str_file, fn);
>>> +  } catch (...) {
>>> +    /* Finally.  */
>>> +    res = std::move (str_file.string ());
>>> +    throw;
>>> +  }
>>
>> Formatting.
>>
> 
> Ack, fixed.
> 
>> The code LGTM, but could you write / update a test for this?
> 
> The patch "[gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as
> unittest" uses this.  It checks the output of a command that is also
> throwing a "Error while executing Python code".
> 
> Thanks,
> - Tom
> 


Ok, that's fine with me.

Simon

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Tom Tromey]

>> The code LGTM, but could you write / update a test for this?

Tom> The patch "[gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as
Tom> unittest" uses this.  It checks the output of a command that is also
Tom> throwing a "Error while executing Python code".

FWIW the new self-test fails for me:

(gdb) maint selftest python
Running selftest python.
5
Self test failed: self-test failed at ../../binutils-gdb/gdb/python/python.c:1932
Ran 1 unit tests, 1 failed

thanks,
Tom

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Tom de Vries]

On 10/20/21 8:53 PM, Tom Tromey wrote:
>>> The code LGTM, but could you write / update a test for this?
> 
> Tom> The patch "[gdb/testsuite] Reimplement gdb.gdb/python-interrupts.exp as
> Tom> unittest" uses this.  It checks the output of a command that is also
> Tom> throwing a "Error while executing Python code".
> 
> FWIW the new self-test fails for me:
> 
> (gdb) maint selftest python
> Running selftest python.
> 5
> Self test failed: self-test failed at ../../binutils-gdb/gdb/python/python.c:1932
> Ran 1 unit tests, 1 failed

Hmm, I get:
...
$ gdb -q
(gdb) maint selftest python
Running selftest python.
Ran 1 unit tests, 0 failed
(gdb)
...

Do you have any idea how I could reproduce the fail you're seeing?

FWIW, I don't understand the '5' showing up.  The test is using
execute_fn_to_ui_file which should take care of redirecting things.

Thanks,
- Tom

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Do you have any idea how I could reproduce the fail you're seeing?

Tom> FWIW, I don't understand the '5' showing up.  The test is using
Tom> execute_fn_to_ui_file which should take care of redirecting things.

It's a mystery to me as well.  I didn't do anything special AFAIK.
I'll try debugging it soon to see if I can come up with something.

Tom

Re: [PATCH][gdb] Make execute_command_to_string return string on throw

[Tom Tromey]

Tom> FWIW, I don't understand the '5' showing up.  The test is using
Tom> execute_fn_to_ui_file which should take care of redirecting things.

> It's a mystery to me as well.  I didn't do anything special AFAIK.
> I'll try debugging it soon to see if I can come up with something.

Ok, it's because I ran "./gdb -nx" and didn't pass --data-directory.
So, user error.  Sorry about that.

Tom