[PATCH 0/8] Minor C++-ification and cleanup in trace targets

[PATCH 0/8] Minor C++-ification and cleanup in trace targets

[Tom Tromey]

This series applies some minor C++-ification and cleanups to the trace
targets.

Regression tested on x86-64 Fedora 38.

---
Tom Tromey (8):
      Remove a use of xfree
      Replace use of xfree with byte_vector
      Use unique_ptr for trace_filename
      Use unique_ptr for trace_dirname
      Use function_view in traceframe_walk_blocks
      Constify tfile_interp_line
      Move definition of ctf_target type
      Change 'handle_id' to be a local variable

 gdb/tracectf.c        | 77 ++++++++++++++++++++++-------------------------
 gdb/tracefile-tfile.c | 82 ++++++++++++++++++++-------------------------------
 2 files changed, 68 insertions(+), 91 deletions(-)
---
base-commit: 4fb2abb59d16f6395162f2e8d9494cde4a37e4a1
change-id: 20230709-trace-cleanups-3aa3911e56cd

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/8] Remove a use of xfree

[Tom Tromey]

This removes a use of xfree from tracefile-tfile.c, replacing it with
a unique_xmalloc_ptr.
---
 gdb/tracefile-tfile.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 9203bcc2e4b..ff451c79e88 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -225,22 +225,19 @@ static void
 tfile_write_uploaded_tsv (struct trace_file_writer *self,
 			  struct uploaded_tsv *utsv)
 {
-  char *buf = NULL;
+  gdb::unique_xmalloc_ptr<char> buf;
   struct tfile_trace_file_writer *writer
     = (struct tfile_trace_file_writer *) self;
 
   if (utsv->name)
     {
-      buf = (char *) xmalloc (strlen (utsv->name) * 2 + 1);
-      bin2hex ((gdb_byte *) (utsv->name), buf, strlen (utsv->name));
+      buf.reset ((char *) xmalloc (strlen (utsv->name) * 2 + 1));
+      bin2hex ((gdb_byte *) (utsv->name), buf.get (), strlen (utsv->name));
     }
 
   fprintf (writer->fp, "tsv %x:%s:%x:%s\n",
 	   utsv->number, phex_nz (utsv->initial_value, 8),
-	   utsv->builtin, buf != NULL ? buf : "");
-
-  if (utsv->name)
-    xfree (buf);
+	   utsv->builtin, buf != NULL ? buf.get () : "");
 }
 
 #define MAX_TRACE_UPLOAD 2000

-- 
2.41.0

[PATCH 2/8] Replace use of xfree with byte_vector

[Tom Tromey]

This replaces a use of xfree with a byte_vector.
---
 gdb/tracectf.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index 16461711da1..8f6ea250f61 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -1358,10 +1358,9 @@ ctf_target::xfer_partial (enum target_object object,
 	    {
 	      const struct bt_definition *array
 		= bt_ctf_get_field (event, scope, "contents");
-	      gdb_byte *contents;
 	      int k;
 
-	      contents = (gdb_byte *) xmalloc (mlen);
+	      gdb::byte_vector contents (mlen);
 
 	      for (k = 0; k < mlen; k++)
 		{
@@ -1377,8 +1376,6 @@ ctf_target::xfer_partial (enum target_object object,
 
 	      memcpy (readbuf, &contents[offset - maddr], amt);
 
-	      xfree (contents);
-
 	      /* Restore the position.  */
 	      bt_iter_set_pos (bt_ctf_get_iter (ctf_iter), pos);
 

-- 
2.41.0

[PATCH 3/8] Use unique_ptr for trace_filename

[Tom Tromey]

This changes trace_filename to use unique_ptr, removing some manual
memory management.
---
 gdb/tracefile-tfile.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index ff451c79e88..364f8c1f08e 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -415,7 +415,7 @@ static tfile_target tfile_ops;
 
 #define TFILE_PID (1)
 
-static char *trace_filename;
+static gdb::unique_xmalloc_ptr<char> trace_filename;
 static int trace_fd = -1;
 static off_t trace_frames_offset;
 static off_t cur_offset;
@@ -441,7 +441,7 @@ tfile_read (gdb_byte *readbuf, int size)
 
   gotten = read (trace_fd, readbuf, size);
   if (gotten < 0)
-    perror_with_name (trace_filename);
+    perror_with_name (trace_filename.get ());
   else if (gotten < size)
     error (_("Premature end of file while reading trace file"));
 }
@@ -479,7 +479,7 @@ tfile_target_open (const char *arg, int from_tty)
 
   current_inferior ()->unpush_target (&tfile_ops);
 
-  trace_filename = filename.release ();
+  trace_filename = std::move (filename);
   trace_fd = scratch_chan;
 
   /* Make sure this is clear.  */
@@ -499,7 +499,7 @@ tfile_target_open (const char *arg, int from_tty)
   trace_regblock_size = 0;
   ts = current_trace_status ();
   /* We know we're working with a file.  Record its name.  */
-  ts->filename = trace_filename;
+  ts->filename = trace_filename.get ();
   /* Set defaults in case there is no status line.  */
   ts->running_known = 0;
   ts->stop_reason = trace_stop_reason_unknown;
@@ -620,8 +620,7 @@ tfile_target::close ()
 
   ::close (trace_fd);
   trace_fd = -1;
-  xfree (trace_filename);
-  trace_filename = NULL;
+  trace_filename.reset ();
   trace_tdesc.clear ();
 
   trace_reset_local_state ();
@@ -630,7 +629,7 @@ tfile_target::close ()
 void
 tfile_target::files_info ()
 {
-  gdb_printf ("\t`%s'\n", trace_filename);
+  gdb_printf ("\t`%s'\n", trace_filename.get ());
 }
 
 void

-- 
2.41.0

[PATCH 4/8] Use unique_ptr for trace_dirname

[Tom Tromey]

This changes trace_dirname to use unique_ptr, removing some manual
memory management.
---
 gdb/tracectf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index 8f6ea250f61..fca623b3f21 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -862,7 +862,7 @@ static struct bt_ctf_iter *ctf_iter = NULL;
 static struct bt_iter_pos *start_pos;
 
 /* The name of CTF directory.  */
-static char *trace_dirname;
+static gdb::unique_xmalloc_ptr<char> trace_dirname;
 
 static ctf_target ctf_ops;
 
@@ -1164,7 +1164,7 @@ ctf_target_open (const char *dirname, int from_tty)
   start_pos = bt_iter_get_pos (bt_ctf_get_iter (ctf_iter));
   gdb_assert (start_pos->type == BT_SEEK_RESTORE);
 
-  trace_dirname = xstrdup (dirname);
+  trace_dirname = make_unique_xstrdup (dirname);
   current_inferior ()->push_target (&ctf_ops);
 
   inferior_appeared (current_inferior (), CTF_PID);
@@ -1185,8 +1185,7 @@ void
 ctf_target::close ()
 {
   ctf_destroy ();
-  xfree (trace_dirname);
-  trace_dirname = NULL;
+  trace_dirname.reset ();
 
   switch_to_no_thread ();	/* Avoid confusion from thread stuff.  */
   exit_inferior_silent (current_inferior ());
@@ -1200,7 +1199,7 @@ ctf_target::close ()
 void
 ctf_target::files_info ()
 {
-  gdb_printf ("\t`%s'\n", trace_dirname);
+  gdb_printf ("\t`%s'\n", trace_dirname.get ());
 }
 
 /* This is the implementation of target_ops method to_fetch_registers.

-- 
2.41.0

[PATCH 5/8] Use function_view in traceframe_walk_blocks

[Tom Tromey]

This change traceframe_walk_blocks to take a function_view.  This
simplifies the code somewhat and lets us entirely remove one helper
function.
---
 gdb/tracefile-tfile.c | 50 ++++++++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 364f8c1f08e..d3304b0b355 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -773,33 +773,15 @@ tfile_target::trace_find (enum trace_find_type type, int num,
   return -1;
 }
 
-/* Prototype of the callback passed to tframe_walk_blocks.  */
-typedef int (*walk_blocks_callback_func) (char blocktype, void *data);
-
-/* Callback for traceframe_walk_blocks, used to find a given block
-   type in a traceframe.  */
-
-static int
-match_blocktype (char blocktype, void *data)
-{
-  char *wantedp = (char *) data;
-
-  if (*wantedp == blocktype)
-    return 1;
-
-  return 0;
-}
-
 /* Walk over all traceframe block starting at POS offset from
-   CUR_OFFSET, and call CALLBACK for each block found, passing in DATA
-   unmodified.  If CALLBACK returns true, this returns the position in
-   the traceframe where the block is found, relative to the start of
-   the traceframe (cur_offset).  Returns -1 if no callback call
-   returned true, indicating that all blocks have been walked.  */
+   CUR_OFFSET, and call CALLBACK for each block found.  If CALLBACK
+   returns true, this returns the position in the traceframe where the
+   block is found, relative to the start of the traceframe
+   (cur_offset).  Returns -1 if no callback call returned true,
+   indicating that all blocks have been walked.  */
 
 static int
-traceframe_walk_blocks (walk_blocks_callback_func callback,
-			int pos, void *data)
+traceframe_walk_blocks (gdb::function_view<bool (char)> callback, int pos)
 {
   /* Iterate through a traceframe's blocks, looking for a block of the
      requested type.  */
@@ -814,7 +796,7 @@ traceframe_walk_blocks (walk_blocks_callback_func callback,
 
       ++pos;
 
-      if ((*callback) (block_type, data))
+      if (callback (block_type))
 	return pos;
 
       switch (block_type)
@@ -854,7 +836,10 @@ traceframe_walk_blocks (walk_blocks_callback_func callback,
 static int
 traceframe_find_block_type (char type_wanted, int pos)
 {
-  return traceframe_walk_blocks (match_blocktype, pos, &type_wanted);
+  return traceframe_walk_blocks ([&] (char blocktype)
+    {
+      return blocktype == type_wanted;
+    }, pos);
 }
 
 /* Look for a block of saved registers in the traceframe, and get the
@@ -1060,11 +1045,9 @@ tfile_target::get_trace_state_variable_value (int tsvnum, LONGEST *val)
 /* Callback for traceframe_walk_blocks.  Builds a traceframe_info
    object for the tfile target's current traceframe.  */
 
-static int
-build_traceframe_info (char blocktype, void *data)
+static bool
+build_traceframe_info (char blocktype, struct traceframe_info *info)
 {
-  struct traceframe_info *info = (struct traceframe_info *) data;
-
   switch (blocktype)
     {
     case 'M':
@@ -1104,7 +1087,7 @@ build_traceframe_info (char blocktype, void *data)
       break;
     }
 
-  return 0;
+  return false;
 }
 
 traceframe_info_up
@@ -1112,7 +1095,10 @@ tfile_target::traceframe_info ()
 {
   traceframe_info_up info (new struct traceframe_info);
 
-  traceframe_walk_blocks (build_traceframe_info, 0, info.get ());
+  traceframe_walk_blocks ([&] (char blocktype)
+    {
+      return build_traceframe_info (blocktype, info.get ());
+    }, 0);
 
   return info;
 }

-- 
2.41.0

[PATCH 6/8] Constify tfile_interp_line

[Tom Tromey]

This adds 'const' to tfile_interp_line.
---
 gdb/tracefile-tfile.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index d3304b0b355..59055e7ea1a 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -424,7 +424,7 @@ int trace_regblock_size;
 static std::string trace_tdesc;
 
 static void tfile_append_tdesc_line (const char *line);
-static void tfile_interp_line (char *line,
+static void tfile_interp_line (const char *line,
 			       struct uploaded_tp **utpp,
 			       struct uploaded_tsv **utsvp);
 
@@ -574,15 +574,15 @@ tfile_target_open (const char *arg, int from_tty)
    file.  */
 
 static void
-tfile_interp_line (char *line, struct uploaded_tp **utpp,
+tfile_interp_line (const char *line, struct uploaded_tp **utpp,
 		   struct uploaded_tsv **utsvp)
 {
-  char *p = line;
+  const char *p = line;
 
   if (startswith (p, "R "))
     {
       p += strlen ("R ");
-      trace_regblock_size = strtol (p, &p, 16);
+      trace_regblock_size = strtol (p, nullptr, 16);
     }
   else if (startswith (p, "status "))
     {

-- 
2.41.0

[PATCH 7/8] Move definition of ctf_target type

[Tom Tromey]

This moves the definition of the ctf_target type into the
HAVE_LIBBABELTRACE block.  This type is only used in this block, so it
makes sense to only define it there.
---
 gdb/tracectf.c | 60 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index fca623b3f21..c2b55426b41 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -34,36 +34,6 @@
 #include "gdbsupport/filestuff.h"
 #include "gdbarch.h"
 
-/* The CTF target.  */
-
-static const target_info ctf_target_info = {
-  "ctf",
-  N_("CTF file"),
-  N_("(Use a CTF directory as a target.\n\
-Specify the filename of the CTF directory.")
-};
-
-class ctf_target final : public tracefile_target
-{
-public:
-  const target_info &info () const override
-  { return ctf_target_info; }
-
-  void close () override;
-  void fetch_registers (struct regcache *, int) override;
-  enum target_xfer_status xfer_partial (enum target_object object,
-						const char *annex,
-						gdb_byte *readbuf,
-						const gdb_byte *writebuf,
-						ULONGEST offset, ULONGEST len,
-						ULONGEST *xfered_len) override;
-  void files_info () override;
-  int trace_find (enum trace_find_type type, int num,
-			  CORE_ADDR addr1, CORE_ADDR addr2, int *tpp) override;
-  bool get_trace_state_variable_value (int tsv, LONGEST *val) override;
-  traceframe_info_up traceframe_info () override;
-};
-
 /* GDB saves trace buffers and other information (such as trace
    status) got from the remote target into Common Trace Format (CTF).
    The following types of information are expected to save in CTF:
@@ -854,6 +824,36 @@ ctf_trace_file_writer_new (void)
 #include <babeltrace/ctf/events.h>
 #include <babeltrace/ctf/iterator.h>
 
+/* The CTF target.  */
+
+static const target_info ctf_target_info = {
+  "ctf",
+  N_("CTF file"),
+  N_("(Use a CTF directory as a target.\n\
+Specify the filename of the CTF directory.")
+};
+
+class ctf_target final : public tracefile_target
+{
+public:
+  const target_info &info () const override
+  { return ctf_target_info; }
+
+  void close () override;
+  void fetch_registers (struct regcache *, int) override;
+  enum target_xfer_status xfer_partial (enum target_object object,
+						const char *annex,
+						gdb_byte *readbuf,
+						const gdb_byte *writebuf,
+						ULONGEST offset, ULONGEST len,
+						ULONGEST *xfered_len) override;
+  void files_info () override;
+  int trace_find (enum trace_find_type type, int num,
+			  CORE_ADDR addr1, CORE_ADDR addr2, int *tpp) override;
+  bool get_trace_state_variable_value (int tsv, LONGEST *val) override;
+  traceframe_info_up traceframe_info () override;
+};
+
 /* The struct pointer for current CTF directory.  */
 static int handle_id = -1;
 static struct bt_context *ctx = NULL;

-- 
2.41.0

[PATCH 8/8] Change 'handle_id' to be a local variable

[Tom Tromey]

The global variable 'handle_id' in tracectf.c is only used in a single
function, so change it to be a local variable.
---
 gdb/tracectf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index c2b55426b41..0ed2092c32b 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -855,7 +855,6 @@ class ctf_target final : public tracefile_target
 };
 
 /* The struct pointer for current CTF directory.  */
-static int handle_id = -1;
 static struct bt_context *ctx = NULL;
 static struct bt_ctf_iter *ctf_iter = NULL;
 /* The position of the first packet containing trace frame.  */
@@ -895,7 +894,7 @@ ctf_open_dir (const char *dirname)
   ctx = bt_context_create ();
   if (ctx == NULL)
     error (_("Unable to create bt_context"));
-  handle_id = bt_context_add_trace (ctx, dirname, "ctf", NULL, NULL, NULL);
+  int handle_id = bt_context_add_trace (ctx, dirname, "ctf", NULL, NULL, NULL);
   if (handle_id < 0)
     {
       ctf_destroy ();

-- 
2.41.0

Re: [PATCH 0/8] Minor C++-ification and cleanup in trace targets

[Keith Seitz]

On 7/9/23 10:01, Tom Tromey wrote:
> This series applies some minor C++-ification and cleanups to the trace
> targets.
> 
> Regression tested on x86-64 Fedora 38.

I've read through the series, and I don't see anything concerning here.

Only ~340 more uses of xfree left. :-)

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

Re: [PATCH 0/8] Minor C++-ification and cleanup in trace targets

[Tom Tromey]

> I've read through the series, and I don't see anything concerning here.
> Only ~340 more uses of xfree left. :-)

> Reviewed-by: Keith Seitz <keiths@redhat.com>

Thanks, I'm going to check it in.

Tom