Patchwork Make target_waitstatus_to_string return an std::string

login
register
mail settings
Submitter Simon Marchi
Date Aug. 12, 2017, 8:30 a.m.
Message ID <1502526609-31934-1-git-send-email-simon.marchi@ericsson.com>
Download mbox | patch
Permalink /patch/22109/
State New
Headers show

Comments

Simon Marchi - Aug. 12, 2017, 8:30 a.m.
A quite straightforward change.  It does "fix" leaks in record-btrace.c,
although since this is only used in debug printing code, it has no real
world impact.

gdb/ChangeLog:

	* target/waitstatus.h (target_waitstatus_to_string): Change
	return type to std::string.
	* target/waitstatus.c (target_waitstatus_to_string): Return
	std::string.
	* target.h (target_waitstatus_to_string): Remove declaration.
	* infrun.c (resume, clear_proceed_status_thread,
	print_target_wait_results, do_target_wait, save_waitstatus,
	stop_all_threads): Adjust.
	* record-btrace.c (record_btrace_wait): Adjust.
	* target-debug.h
	(target_debug_print_struct_target_waitstatus_p): Adjust.

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_wait_1): Adjust.
	* server.c (queue_stop_reply_callback): Adjust.
---
 gdb/gdbserver/linux-low.c |  7 +++----
 gdb/gdbserver/server.c    |  6 ++----
 gdb/infrun.c              | 43 +++++++++++++++++-------------------------
 gdb/record-btrace.c       |  8 ++++++--
 gdb/target-debug.h        |  5 ++---
 gdb/target.h              |  4 ----
 gdb/target/waitstatus.c   | 48 +++++++++++++++++++++++------------------------
 gdb/target/waitstatus.h   |  5 ++---
 8 files changed, 56 insertions(+), 70 deletions(-)
Simon Marchi - Sept. 3, 2017, 8:25 a.m.
On 2017-08-14 05:03 PM, Pedro Alves wrote:
> On 08/12/2017 09:30 AM, Simon Marchi wrote:
> 
> 
>> --- a/gdb/record-btrace.c
>> +++ b/gdb/record-btrace.c
>> @@ -2477,8 +2477,10 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
>>      {
>>        *status = btrace_step_no_resumed ();
>>  
>> +      std::string statstr = target_waitstatus_to_string (status);
>> +
>>        DEBUG ("wait ended by %s: %s", target_pid_to_str (null_ptid),
>> -	     target_waitstatus_to_string (status));
>> +	     statstr.c_str ());
> 
> I think:
> 
>         DEBUG ("wait ended by %s: %s", target_pid_to_str (null_ptid),
>  	     target_waitstatus_to_string (status).c_str ());
> 
> would be better, to avoid heap-allocating the string if
> debugging is not active.

Yes, and it's simpler, not sure why I didn't do that in the first place.

>> @@ -2567,10 +2569,12 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
>>    /* We moved the replay position but did not update registers.  */
>>    registers_changed_ptid (eventing->ptid);
>>  
>> +  std::string statstr = target_waitstatus_to_string (status);
>> +
>>    DEBUG ("wait ended by thread %s (%s): %s",
>>  	 print_thread_id (eventing),
>>  	 target_pid_to_str (eventing->ptid),
>> -	 target_waitstatus_to_string (status));
>> +	 statstr.c_str ());
> 
> Ditto.
> 
> Otherwise looks fine to me.

Pushed with those fixed, thanks!

Simon

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index fd46d85..6f4b26a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3733,12 +3733,11 @@  linux_wait_1 (ptid_t ptid,
     {
       if (event_child->waitstatus.kind != TARGET_WAITKIND_IGNORE)
 	{
-	  char *str;
+	  std::string str
+	    = target_waitstatus_to_string (&event_child->waitstatus);
 
-	  str = target_waitstatus_to_string (&event_child->waitstatus);
 	  debug_printf ("LWP %ld: extended event with waitstatus %s\n",
-			lwpid_of (get_lwp_thread (event_child)), str);
-	  xfree (str);
+			lwpid_of (get_lwp_thread (event_child)), str.c_str ());
 	}
       if (current_thread->last_resume_kind == resume_step)
 	{
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3838351..283fa8d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3116,14 +3116,12 @@  queue_stop_reply_callback (struct inferior_list_entry *entry, void *arg)
 	{
 	  if (debug_threads)
 	    {
-	      char *status_string
+	      std::string status_string
 		= target_waitstatus_to_string (&thread->last_status);
 
 	      debug_printf ("Reporting thread %s as already stopped with %s\n",
 			    target_pid_to_str (entry->id),
-			    status_string);
-
-	      xfree (status_string);
+			    status_string.c_str ());
 	    }
 
 	  gdb_assert (thread->last_status.kind != TARGET_WAITKIND_IGNORE);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d6723fd..964f7ff 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2413,15 +2413,14 @@  resume (enum gdb_signal sig)
     {
       if (debug_infrun)
 	{
-	  char *statstr;
+	  std::string statstr
+	    = target_waitstatus_to_string (&tp->suspend.waitstatus);
 
-	  statstr = target_waitstatus_to_string (&tp->suspend.waitstatus);
 	  fprintf_unfiltered (gdb_stdlog,
-			      "infrun: resume: thread %s has pending wait status %s "
-			      "(currently_stepping=%d).\n",
-			      target_pid_to_str (tp->ptid),  statstr,
+			      "infrun: resume: thread %s has pending wait "
+			      "status %s (currently_stepping=%d).\n",
+			      target_pid_to_str (tp->ptid), statstr.c_str (),
 			      currently_stepping (tp));
-	  xfree (statstr);
 	}
 
       tp->resumed = 1;
@@ -2824,16 +2823,15 @@  clear_proceed_status_thread (struct thread_info *tp)
 	}
       else if (debug_infrun)
 	{
-	  char *statstr;
+	  std::string statstr
+	    = target_waitstatus_to_string (&tp->suspend.waitstatus);
 
-	  statstr = target_waitstatus_to_string (&tp->suspend.waitstatus);
 	  fprintf_unfiltered (gdb_stdlog,
 			      "infrun: clear_proceed_status_thread: thread %s "
 			      "has pending wait status %s "
 			      "(currently_stepping=%d).\n",
-			      target_pid_to_str (tp->ptid), statstr,
+			      target_pid_to_str (tp->ptid), statstr.c_str (),
 			      currently_stepping (tp));
-	  xfree (statstr);
 	}
     }
 
@@ -3422,7 +3420,7 @@  void
 print_target_wait_results (ptid_t waiton_ptid, ptid_t result_ptid,
 			   const struct target_waitstatus *ws)
 {
-  char *status_string = target_waitstatus_to_string (ws);
+  std::string status_string = target_waitstatus_to_string (ws);
   string_file stb;
 
   /* The text is split over several lines because it was getting too long.
@@ -3442,13 +3440,11 @@  print_target_wait_results (ptid_t waiton_ptid, ptid_t result_ptid,
 	      ptid_get_lwp (result_ptid),
 	      ptid_get_tid (result_ptid),
 	      target_pid_to_str (result_ptid));
-  stb.printf ("infrun:   %s\n", status_string);
+  stb.printf ("infrun:   %s\n", status_string.c_str ());
 
   /* This uses %s in part to handle %'s in the text, but also to avoid
      a gcc error: the format attribute requires a string literal.  */
   fprintf_unfiltered (gdb_stdlog, "%s", stb.c_str ());
-
-  xfree (status_string);
 }
 
 /* Select a thread at random, out of those which are resumed and have
@@ -3570,14 +3566,13 @@  do_target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
     {
       if (debug_infrun)
 	{
-	  char *statstr;
+	  std::string statstr
+	    = target_waitstatus_to_string (&tp->suspend.waitstatus);
 
-	  statstr = target_waitstatus_to_string (&tp->suspend.waitstatus);
 	  fprintf_unfiltered (gdb_stdlog,
 			      "infrun: Using pending wait status %s for %s.\n",
-			      statstr,
+			      statstr.c_str (),
 			      target_pid_to_str (tp->ptid));
-	  xfree (statstr);
 	}
 
       /* Now that we've selected our final event LWP, un-adjust its PC
@@ -4406,16 +4401,14 @@  save_waitstatus (struct thread_info *tp, struct target_waitstatus *ws)
 
   if (debug_infrun)
     {
-      char *statstr;
+      std::string statstr = target_waitstatus_to_string (ws);
 
-      statstr = target_waitstatus_to_string (ws);
       fprintf_unfiltered (gdb_stdlog,
 			  "infrun: saving status %s for %d.%ld.%ld\n",
-			  statstr,
+			  statstr.c_str (),
 			  ptid_get_pid (tp->ptid),
 			  ptid_get_lwp (tp->ptid),
 			  ptid_get_tid (tp->ptid));
-      xfree (statstr);
     }
 
   /* Record for later.  */
@@ -4645,17 +4638,15 @@  stop_all_threads (void)
 
 		  if (debug_infrun)
 		    {
-		      char *statstr;
+		      std::string statstr = target_waitstatus_to_string (&ws);
 
-		      statstr = target_waitstatus_to_string (&ws);
 		      fprintf_unfiltered (gdb_stdlog,
 					  "infrun: target_wait %s, saving "
 					  "status for %d.%ld.%ld\n",
-					  statstr,
+					  statstr.c_str (),
 					  ptid_get_pid (t->ptid),
 					  ptid_get_lwp (t->ptid),
 					  ptid_get_tid (t->ptid));
-		      xfree (statstr);
 		    }
 
 		  /* Record for later.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index b216f1f..414db5f 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2477,8 +2477,10 @@  record_btrace_wait (struct target_ops *ops, ptid_t ptid,
     {
       *status = btrace_step_no_resumed ();
 
+      std::string statstr = target_waitstatus_to_string (status);
+
       DEBUG ("wait ended by %s: %s", target_pid_to_str (null_ptid),
-	     target_waitstatus_to_string (status));
+	     statstr.c_str ());
 
       do_cleanups (cleanups);
       return null_ptid;
@@ -2567,10 +2569,12 @@  record_btrace_wait (struct target_ops *ops, ptid_t ptid,
   /* We moved the replay position but did not update registers.  */
   registers_changed_ptid (eventing->ptid);
 
+  std::string statstr = target_waitstatus_to_string (status);
+
   DEBUG ("wait ended by thread %s (%s): %s",
 	 print_thread_id (eventing),
 	 target_pid_to_str (eventing->ptid),
-	 target_waitstatus_to_string (status));
+	 statstr.c_str ());
 
   do_cleanups (cleanups);
   return eventing->ptid;
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index 6923608..532e98d 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -166,10 +166,9 @@ 
 static void
 target_debug_print_struct_target_waitstatus_p (struct target_waitstatus *status)
 {
-  char *str = target_waitstatus_to_string (status);
+  std::string str = target_waitstatus_to_string (status);
 
-  fputs_unfiltered (str, gdb_stdlog);
-  xfree (str);
+  fputs_unfiltered (str.c_str (), gdb_stdlog);
 }
 
 
diff --git a/gdb/target.h b/gdb/target.h
index c0155c1..5971151 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -108,10 +108,6 @@  struct syscall
     const char *name;
   };
 
-/* Return a pretty printed form of target_waitstatus.
-   Space for the result is malloc'd, caller must free.  */
-extern char *target_waitstatus_to_string (const struct target_waitstatus *);
-
 /* Return a pretty printed form of TARGET_OPTIONS.
    Space for the result is malloc'd, caller must free.  */
 extern char *target_options_to_string (int target_options);
diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index c59d1b6..eede2d6 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -23,7 +23,7 @@ 
 /* Return a pretty printed form of target_waitstatus.
    Space for the result is malloc'd, caller must free.  */
 
-char *
+std::string
 target_waitstatus_to_string (const struct target_waitstatus *ws)
 {
   const char *kind_str = "status->kind = ";
@@ -31,44 +31,44 @@  target_waitstatus_to_string (const struct target_waitstatus *ws)
   switch (ws->kind)
     {
     case TARGET_WAITKIND_EXITED:
-      return xstrprintf ("%sexited, status = %d",
-			 kind_str, ws->value.integer);
+      return string_printf ("%sexited, status = %d",
+			    kind_str, ws->value.integer);
     case TARGET_WAITKIND_STOPPED:
-      return xstrprintf ("%sstopped, signal = %s",
-			 kind_str,
-			 gdb_signal_to_symbol_string (ws->value.sig));
+      return string_printf ("%sstopped, signal = %s",
+			    kind_str,
+			    gdb_signal_to_symbol_string (ws->value.sig));
     case TARGET_WAITKIND_SIGNALLED:
-      return xstrprintf ("%ssignalled, signal = %s",
-			 kind_str,
-			 gdb_signal_to_symbol_string (ws->value.sig));
+      return string_printf ("%ssignalled, signal = %s",
+			    kind_str,
+			    gdb_signal_to_symbol_string (ws->value.sig));
     case TARGET_WAITKIND_LOADED:
-      return xstrprintf ("%sloaded", kind_str);
+      return string_printf ("%sloaded", kind_str);
     case TARGET_WAITKIND_FORKED:
-      return xstrprintf ("%sforked", kind_str);
+      return string_printf ("%sforked", kind_str);
     case TARGET_WAITKIND_VFORKED:
-      return xstrprintf ("%svforked", kind_str);
+      return string_printf ("%svforked", kind_str);
     case TARGET_WAITKIND_EXECD:
-      return xstrprintf ("%sexecd", kind_str);
+      return string_printf ("%sexecd", kind_str);
     case TARGET_WAITKIND_VFORK_DONE:
-      return xstrprintf ("%svfork-done", kind_str);
+      return string_printf ("%svfork-done", kind_str);
     case TARGET_WAITKIND_SYSCALL_ENTRY:
-      return xstrprintf ("%sentered syscall", kind_str);
+      return string_printf ("%sentered syscall", kind_str);
     case TARGET_WAITKIND_SYSCALL_RETURN:
-      return xstrprintf ("%sexited syscall", kind_str);
+      return string_printf ("%sexited syscall", kind_str);
     case TARGET_WAITKIND_SPURIOUS:
-      return xstrprintf ("%sspurious", kind_str);
+      return string_printf ("%sspurious", kind_str);
     case TARGET_WAITKIND_IGNORE:
-      return xstrprintf ("%signore", kind_str);
+      return string_printf ("%signore", kind_str);
     case TARGET_WAITKIND_NO_HISTORY:
-      return xstrprintf ("%sno-history", kind_str);
+      return string_printf ("%sno-history", kind_str);
     case TARGET_WAITKIND_NO_RESUMED:
-      return xstrprintf ("%sno-resumed", kind_str);
+      return string_printf ("%sno-resumed", kind_str);
     case TARGET_WAITKIND_THREAD_CREATED:
-      return xstrprintf ("%sthread created", kind_str);
+      return string_printf ("%sthread created", kind_str);
     case TARGET_WAITKIND_THREAD_EXITED:
-      return xstrprintf ("%sthread exited, status = %d",
-			 kind_str, ws->value.integer);
+      return string_printf ("%sthread exited, status = %d",
+			    kind_str, ws->value.integer);
     default:
-      return xstrprintf ("%sunknown???", kind_str);
+      return string_printf ("%sunknown???", kind_str);
     }
 }
diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
index 52be390..8eee198 100644
--- a/gdb/target/waitstatus.h
+++ b/gdb/target/waitstatus.h
@@ -145,8 +145,7 @@  enum target_stop_reason
 
 /* Prototypes */
 
-/* Return a pretty printed form of target_waitstatus.
-   Space for the result is malloc'd, caller must free.  */
-extern char *target_waitstatus_to_string (const struct target_waitstatus *);
+/* Return a pretty printed form of target_waitstatus.  */
+std::string target_waitstatus_to_string (const struct target_waitstatus *);
 
 #endif /* WAITSTATUS_H */