[1/3] Rename some trace functions

Message ID 0fde0f4d-8b1b-0c50-ee47-4be776427403@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Dec. 2, 2016, 4:47 p.m. UTC
  On 12/02/2016 03:46 PM, Pedro Alves wrote:
> On 12/02/2016 03:14 PM, Simon Marchi wrote:
> 
>>> So something like:
>>>
>>>  enum class trace_stop_reason
>>>  {
>>>    unknown,
>>>    never_run,
>>>    request,
>>>    overflow,
>>>    disconnection,
>>>    passcount,
>>>    error,
>>>  };
>>
>> It would make sense.
>>
> 
> Writing a patch.

Turns out I quoted the MI names, which are a bit different
from the RSP names.  Sigh...

Anyway, here's what it ends up looking like.  I changed
the text of the "request" stop reason, because I thought that
might be a tiny bit more user friendly for the case of the
user using an MI frontend who clicks some "trace stop" button
on the GUI instead of running a "command".

From b35218bacb988193d7b74a15ff288603c4c71134 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 2 Dec 2016 15:24:26 +0000
Subject: [PATCH] Make trace_stop_reason an enum class

gdb/testsuite/ChangeLog:

	* gdb.trace/tfile.exp: Adjust.
	* gdb.trace/tfind.exp: Adjust.
	* gdb.trace/tstatus.exp: Adjust.

gdb/ChangeLog:

	* tracefile-tfile.c (tfile_write_status, tfile_open): Adjust.
	* tracepoint.c (stop_reason_names): Rename to ...
	(rsp_trace_stop_reason_names): ... this.
	(get_rsp_name): New function.
	(trace_status_command): Adjust.
	(trace_status_mi): Adjust.
	(parse_trace_status): Adjust.  Use get_rsp_name.
	* tracepoint.h (trace_stop_reason): Now an enum class.
	(get_rsp_name): New declaration.
---
 gdb/testsuite/gdb.trace/tfile.exp   |  2 +-
 gdb/testsuite/gdb.trace/tfind.exp   |  2 +-
 gdb/testsuite/gdb.trace/tstatus.exp |  2 +-
 gdb/tracefile-tfile.c               |  8 ++--
 gdb/tracepoint.c                    | 74 +++++++++++++++++++++----------------
 gdb/tracepoint.h                    | 40 ++++++++++++++------
 6 files changed, 78 insertions(+), 50 deletions(-)
  

Comments

Simon Marchi Dec. 2, 2016, 6:47 p.m. UTC | #1
On 2016-12-02 11:47, Pedro Alves wrote:
>> Writing a patch.

I had started a patch as well, but was interrupted by a meeting, so I 
didn't get very far.  Thanks for doing it!

> Turns out I quoted the MI names, which are a bit different
> from the RSP names.  Sigh...
> 
> Anyway, here's what it ends up looking like.  I changed
> the text of the "request" stop reason, because I thought that
> might be a tiny bit more user friendly for the case of the
> user using an MI frontend who clicks some "trace stop" button
> on the GUI instead of running a "command".

The new message "Trace stopped on user request" looks good to me.

> +/* See tracepoint.h.  */
> +
> +const char *
> +get_rsp_name (trace_stop_reason reason)
> +{
> +  return rsp_trace_stop_reason_names[(int) reason];

Should we prefer static_cast<int>() over (int)?

It might be a good idea to check that the resulting integer is smaller 
than the array size.  If we ever add new stop reasons, we could forget 
to add array elements, so an error() here would catch it.

Otherwise, LGTM.
  

Patch

diff --git a/gdb/testsuite/gdb.trace/tfile.exp b/gdb/testsuite/gdb.trace/tfile.exp
index 9f89d3d..e4a8965 100644
--- a/gdb/testsuite/gdb.trace/tfile.exp
+++ b/gdb/testsuite/gdb.trace/tfile.exp
@@ -105,7 +105,7 @@  gdb_test "tfind" "Target failed to find requested trace frame." \
 
 gdb_test "tstatus" \
     "Using a trace file.*
-Trace stopped by a tstop command.*
+Trace stopped on user request.*
 Collected 1 trace frame.*
 Trace buffer has 256 bytes of 4096 bytes free \\(93% full\\).*
 Looking at trace frame 0, tracepoint .*" \
diff --git a/gdb/testsuite/gdb.trace/tfind.exp b/gdb/testsuite/gdb.trace/tfind.exp
index 39c54a1..6a8aba5 100644
--- a/gdb/testsuite/gdb.trace/tfind.exp
+++ b/gdb/testsuite/gdb.trace/tfind.exp
@@ -156,7 +156,7 @@  if { $return_me == 1 } then {
 }
 
 # test tstatus (when trace off)
-gdb_test "tstatus" "Trace stopped by a tstop command.*" \
+gdb_test "tstatus" "Trace stopped on user request.*" \
     "test tstatus off after tstop"
 
 ## record starting PC
diff --git a/gdb/testsuite/gdb.trace/tstatus.exp b/gdb/testsuite/gdb.trace/tstatus.exp
index c3957c2..6c8fc07 100644
--- a/gdb/testsuite/gdb.trace/tstatus.exp
+++ b/gdb/testsuite/gdb.trace/tstatus.exp
@@ -96,7 +96,7 @@  proc run_trace_experiment {} {
 
     set test "tstatus reports trace stop reason"
     gdb_test_multiple "tstatus" $test {
-	-re "(Trace stopped by a tstop command \\(because I can\\)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\.).*\r\n$gdb_prompt $" {
+	-re "(Trace stopped on user request \\(because I can\\)\..*Trace will stop if GDB disconnects\.\[\r\n\]+Trace user is me me me\.\[\r\n\]+Trace notes: different note\.\[\r\n\]+Not looking at any trace frame\.).*\r\n$gdb_prompt $" {
 	    set tstatus_output $expect_out(1,string)
 	    pass $test
 	}
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 57bb597..9daa2ef 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -136,9 +136,9 @@  tfile_write_status (struct trace_file_writer *self,
     = (struct tfile_trace_file_writer *) self;
 
   fprintf (writer->fp, "status %c;%s",
-	   (ts->running ? '1' : '0'), stop_reason_names[ts->stop_reason]);
-  if (ts->stop_reason == tracepoint_error
-      || ts->stop_reason == tstop_command)
+	   (ts->running ? '1' : '0'), get_rsp_name (ts->stop_reason));
+  if (ts->stop_reason == trace_stop_reason::error
+      || ts->stop_reason == trace_stop_reason::request)
     {
       char *buf = (char *) alloca (strlen (ts->stop_desc) * 2 + 1);
 
@@ -483,7 +483,7 @@  tfile_open (const char *arg, int from_tty)
   ts->filename = trace_filename;
   /* Set defaults in case there is no status line.  */
   ts->running_known = 0;
-  ts->stop_reason = trace_stop_reason_unknown;
+  ts->stop_reason = trace_stop_reason::unknown;
   ts->traceframe_count = -1;
   ts->buffer_free = 0;
   ts->disconnected_tracing = 0;
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 7435380..4ff1d76 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -190,7 +190,7 @@  extern void _initialize_tracepoint (void);
 
 static struct trace_status trace_status;
 
-const char *stop_reason_names[] = {
+static const char *rsp_trace_stop_reason_names[] = {
   "tunknown",
   "tnotrun",
   "tstop",
@@ -200,6 +200,14 @@  const char *stop_reason_names[] = {
   "terror"
 };
 
+/* See tracepoint.h.  */
+
+const char *
+get_rsp_name (trace_stop_reason reason)
+{
+  return rsp_trace_stop_reason_names[(int) reason];
+}
+
 struct trace_status *
 current_trace_status (void)
 {
@@ -1889,27 +1897,27 @@  trace_status_command (char *args, int from_tty)
     {
       switch (ts->stop_reason)
 	{
-	case trace_never_run:
+	case trace_stop_reason::never_run:
 	  printf_filtered (_("No trace has been run on the target.\n"));
 	  break;
-	case tstop_command:
+	case trace_stop_reason::request:
 	  if (ts->stop_desc)
-	    printf_filtered (_("Trace stopped by a tstop command (%s).\n"),
+	    printf_filtered (_("Trace stopped on user request (%s).\n"),
 			     ts->stop_desc);
 	  else
-	    printf_filtered (_("Trace stopped by a tstop command.\n"));
+	    printf_filtered (_("Trace stopped on user request.\n"));
 	  break;
-	case trace_buffer_full:
+	case trace_stop_reason::overflow:
 	  printf_filtered (_("Trace stopped because the buffer was full.\n"));
 	  break;
-	case trace_disconnected:
+	case trace_stop_reason::disconnection:
 	  printf_filtered (_("Trace stopped because of disconnection.\n"));
 	  break;
-	case tracepoint_passcount:
+	case trace_stop_reason::passcount:
 	  printf_filtered (_("Trace stopped by tracepoint %d.\n"),
 			   ts->stopping_tracepoint);
 	  break;
-	case tracepoint_error:
+	case trace_stop_reason::error:
 	  if (ts->stopping_tracepoint)
 	    printf_filtered (_("Trace stopped by an "
 			       "error (%s, tracepoint %d).\n"),
@@ -1918,12 +1926,12 @@  trace_status_command (char *args, int from_tty)
 	    printf_filtered (_("Trace stopped by an error (%s).\n"),
 			     ts->stop_desc);
 	  break;
-	case trace_stop_reason_unknown:
+	case trace_stop_reason::unknown:
 	  printf_filtered (_("Trace stopped for an unknown reason.\n"));
 	  break;
 	default:
 	  printf_filtered (_("Trace stopped for some other reason (%d).\n"),
-			   ts->stop_reason);
+			   (int) ts->stop_reason);
 	  break;
 	}
     }
@@ -2065,24 +2073,24 @@  trace_status_mi (int on_stop)
       if (!on_stop)
 	ui_out_field_string (uiout, "running", "0");
 
-      if (ts->stop_reason != trace_stop_reason_unknown)
+      if (ts->stop_reason != trace_stop_reason::unknown)
 	{
 	  switch (ts->stop_reason)
 	    {
-	    case tstop_command:
+	    case trace_stop_reason::request:
 	      stop_reason = "request";
 	      break;
-	    case trace_buffer_full:
+	    case trace_stop_reason::overflow:
 	      stop_reason = "overflow";
 	      break;
-	    case trace_disconnected:
+	    case trace_stop_reason::disconnection:
 	      stop_reason = "disconnection";
 	      break;
-	    case tracepoint_passcount:
+	    case trace_stop_reason::passcount:
 	      stop_reason = "passcount";
 	      stopping_tracepoint = ts->stopping_tracepoint;
 	      break;
-	    case tracepoint_error:
+	    case trace_stop_reason::error:
 	      stop_reason = "error";
 	      stopping_tracepoint = ts->stopping_tracepoint;
 	      break;
@@ -2094,7 +2102,7 @@  trace_status_mi (int on_stop)
 	      if (stopping_tracepoint != -1)
 		ui_out_field_int (uiout, "stopping-tracepoint",
 				  stopping_tracepoint);
-	      if (ts->stop_reason == tracepoint_error)
+	      if (ts->stop_reason == trace_stop_reason::error)
 		ui_out_field_string (uiout, "error-description",
 				     ts->stop_desc);
 	    }
@@ -3451,7 +3459,7 @@  parse_trace_status (char *line, struct trace_status *ts)
 
   ts->running_known = 1;
   ts->running = (*p++ == '1');
-  ts->stop_reason = trace_stop_reason_unknown;
+  ts->stop_reason = trace_stop_reason::unknown;
   xfree (ts->stop_desc);
   ts->stop_desc = NULL;
   ts->traceframe_count = -1;
@@ -3475,24 +3483,26 @@  Status line: '%s'\n"), p, line);
       p3 = strchr (p, ';');
       if (p3 == NULL)
 	p3 = p + strlen (p);
-      if (strncmp (p, stop_reason_names[trace_buffer_full], p1 - p) == 0)
+      if (strncmp (p, get_rsp_name (trace_stop_reason::overflow), p1 - p) == 0)
 	{
 	  p = unpack_varlen_hex (++p1, &val);
-	  ts->stop_reason = trace_buffer_full;
+	  ts->stop_reason = trace_stop_reason::overflow;
 	}
-      else if (strncmp (p, stop_reason_names[trace_never_run], p1 - p) == 0)
+      else if (strncmp (p, get_rsp_name (trace_stop_reason::never_run),
+			p1 - p) == 0)
 	{
 	  p = unpack_varlen_hex (++p1, &val);
-	  ts->stop_reason = trace_never_run;
+	  ts->stop_reason = trace_stop_reason::never_run;
 	}
-      else if (strncmp (p, stop_reason_names[tracepoint_passcount],
+      else if (strncmp (p, get_rsp_name (trace_stop_reason::passcount),
 			p1 - p) == 0)
 	{
 	  p = unpack_varlen_hex (++p1, &val);
-	  ts->stop_reason = tracepoint_passcount;
+	  ts->stop_reason = trace_stop_reason::passcount;
 	  ts->stopping_tracepoint = val;
 	}
-      else if (strncmp (p, stop_reason_names[tstop_command], p1 - p) == 0)
+      else if (strncmp (p, get_rsp_name (trace_stop_reason::request),
+			p1 - p) == 0)
 	{
 	  p2 = strchr (++p1, ':');
 	  if (!p2 || p2 > p3)
@@ -3510,14 +3520,16 @@  Status line: '%s'\n"), p, line);
 	    ts->stop_desc = xstrdup ("");
 
 	  p = unpack_varlen_hex (++p2, &val);
-	  ts->stop_reason = tstop_command;
+	  ts->stop_reason = trace_stop_reason::request;
 	}
-      else if (strncmp (p, stop_reason_names[trace_disconnected], p1 - p) == 0)
+      else if (strncmp (p, get_rsp_name (trace_stop_reason::disconnection),
+			p1 - p) == 0)
 	{
 	  p = unpack_varlen_hex (++p1, &val);
-	  ts->stop_reason = trace_disconnected;
+	  ts->stop_reason = trace_stop_reason::disconnection;
 	}
-      else if (strncmp (p, stop_reason_names[tracepoint_error], p1 - p) == 0)
+      else if (strncmp (p, get_rsp_name (trace_stop_reason::error),
+			p1 - p) == 0)
 	{
 	  p2 = strchr (++p1, ':');
 	  if (p2 != p1)
@@ -3531,7 +3543,7 @@  Status line: '%s'\n"), p, line);
 
 	  p = unpack_varlen_hex (++p2, &val);
 	  ts->stopping_tracepoint = val;
-	  ts->stop_reason = tracepoint_error;
+	  ts->stop_reason = trace_stop_reason::error;
 	}
       else if (strncmp (p, "tframes", p1 - p) == 0)
 	{
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 36eeee6..a5277f3 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -70,20 +70,38 @@  struct trace_state_variable
     int builtin;
    };
 
-/* The trace status encompasses various info about the general state
-   of the tracing run.  */
+/* The reason why the tracing was stopped last time.  */
 
-enum trace_stop_reason
+enum class trace_stop_reason
   {
-    trace_stop_reason_unknown,
-    trace_never_run,
-    tstop_command,
-    trace_buffer_full,
-    trace_disconnected,
-    tracepoint_passcount,
-    tracepoint_error
+    /* Unknown reason.  */
+    unknown,
+
+    /* No trace has been run yet.  */
+    never_run,
+
+    /* The tracing was stopped on user request, e.g., with the tstop
+       (CLI) or -trace-stop (MI) commands.  */
+    request,
+
+    /* The tracing buffer is full.  */
+    overflow,
+
+    /* Tracing was automatically stopped because GDB has
+       disconnected.  */
+    disconnection,
+
+    /* Tracing was stopped when a tracepoint was passed a maximal
+       number of times for that tracepoint.  */
+    passcount,
+
+    /* Tracing was stopped due to some error.  */
+    error,
   };
 
+/* Get the remote protocol string representation of REASON.  */
+extern const char *get_rsp_name (trace_stop_reason reason);
+
 struct trace_status
 {
   /* If the status is coming from a file rather than a live target,
@@ -158,8 +176,6 @@  extern char *default_collect;
 
 extern int trace_regblock_size;
 
-extern const char *stop_reason_names[];
-
 /* Struct to collect random info about tracepoints on the target.  */
 
 struct uploaded_tp