[RFA] C++-ify skip.c

[RFA] C++-ify skip.c

[Tom Tromey]

I happened to notice that skiplist_entry, in skip.c, contains a
gdb::optional<compiled_regex> -- but that this object's destructor is
never run.  This can result in a memory leak.

This patch fixes the bug by applying a bit more C++: changing this
code to use new and delete, and std::unique_ptr; and removing cleanups
in the process.

Built and regression tested on x86-64 Fedora 25.

ChangeLog
2017-08-06  Tom Tromey  <tom@tromey.com>

	* skip.c (skiplist_entry): New constructor.
	(~skiplist_entry): New destructor.
	(skiplist_entry::enabled): Now bool.
	(make_skip_entry): Return a unique_ptr.  Use new.
	(free_skiplist_entry, free_skiplist_entry_cleanup)
	(make_free_skiplist_entry_cleanup): Remove.
	(skip_command, skip_disable_command, add_skiplist_entry): Update.
	(skip_delete_command): Update.  Use delete.
---
 gdb/ChangeLog | 11 ++++++++
 gdb/skip.c    | 89 +++++++++++++++++++++++------------------------------------
 2 files changed, 45 insertions(+), 55 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 722fade..12e0d02 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2017-08-06  Tom Tromey  <tom@tromey.com>
+
+	* skip.c (skiplist_entry): New constructor.
+	(~skiplist_entry): New destructor.
+	(skiplist_entry::enabled): Now bool.
+	(make_skip_entry): Return a unique_ptr.  Use new.
+	(free_skiplist_entry, free_skiplist_entry_cleanup)
+	(make_free_skiplist_entry_cleanup): Remove.
+	(skip_command, skip_disable_command, add_skiplist_entry): Update.
+	(skip_delete_command): Update.  Use delete.
+
 2017-08-05  Tom Tromey  <tom@tromey.com>
 
 	* compile/compile-object-load.c (compile_object_load): Use
diff --git a/gdb/skip.c b/gdb/skip.c
index bf44913..f3291f3 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -38,6 +38,24 @@
 
 struct skiplist_entry
 {
+  skiplist_entry (int file_is_glob_, const char *file_,
+		  int function_is_regexp_, const char *function_)
+    : number (-1),
+      file_is_glob (file_is_glob_),
+      file (file_ == NULL ? NULL : xstrdup (file_)),
+      function_is_regexp (function_is_regexp_),
+      function (function_ == NULL ? NULL : xstrdup (function_)),
+      enabled (true),
+      next (NULL)
+  {
+  }
+
+  ~skiplist_entry ()
+  {
+    xfree (file);
+    xfree (function);
+  }
+
   int number;
 
   /* Non-zero if FILE is a glob-style pattern.
@@ -60,12 +78,12 @@ struct skiplist_entry
   /* If this is a function regexp, the compiled form.  */
   gdb::optional<compiled_regex> compiled_function_regexp;
 
-  int enabled;
+  bool enabled;
 
   struct skiplist_entry *next;
 };
 
-static void add_skiplist_entry (struct skiplist_entry *e);
+static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
 
 static struct skiplist_entry *skiplist_entry_chain;
 static int skiplist_entry_count;
@@ -80,53 +98,18 @@ static int skiplist_entry_count;
 
 /* Create a skip object.  */
 
-static struct skiplist_entry *
+static std::unique_ptr<skiplist_entry>
 make_skip_entry (int file_is_glob, const char *file,
 		 int function_is_regexp, const char *function)
 {
-  struct skiplist_entry *e = XCNEW (struct skiplist_entry);
-
   gdb_assert (file != NULL || function != NULL);
   if (file_is_glob)
     gdb_assert (file != NULL);
   if (function_is_regexp)
     gdb_assert (function != NULL);
 
-  if (file != NULL)
-    e->file = xstrdup (file);
-  if (function != NULL)
-    e->function = xstrdup (function);
-  e->file_is_glob = file_is_glob;
-  e->function_is_regexp = function_is_regexp;
-  e->enabled = 1;
-
-  return e;
-}
-
-/* Free a skiplist entry.  */
-
-static void
-free_skiplist_entry (struct skiplist_entry *e)
-{
-  xfree (e->file);
-  xfree (e->function);
-  xfree (e);
-}
-
-/* Wrapper to free_skiplist_entry for use as a cleanup.  */
-
-static void
-free_skiplist_entry_cleanup (void *e)
-{
-  free_skiplist_entry ((struct skiplist_entry *) e);
-}
-
-/* Create a cleanup to free skiplist entry E.  */
-
-static struct cleanup *
-make_free_skiplist_entry_cleanup (struct skiplist_entry *e)
-{
-  return make_cleanup (free_skiplist_entry_cleanup, e);
+  return std::unique_ptr<skiplist_entry>
+    (new skiplist_entry (file_is_glob, file, function_is_regexp, function));
 }
 
 static void
@@ -217,7 +200,6 @@ skip_command (char *arg, int from_tty)
   const char *gfile = NULL;
   const char *function = NULL;
   const char *rfunction = NULL;
-  struct skiplist_entry *e;
   int i;
 
   if (arg == NULL)
@@ -291,16 +273,13 @@ skip_command (char *arg, int from_tty)
   gdb_assert (file != NULL || gfile != NULL
 	      || function != NULL || rfunction != NULL);
 
-  e = make_skip_entry (gfile != NULL, file ? file : gfile,
-		       rfunction != NULL, function ? function : rfunction);
+  std::unique_ptr<skiplist_entry> e
+    = make_skip_entry (gfile != NULL, file ? file : gfile, rfunction != NULL,
+		       function ? function : rfunction);
   if (rfunction != NULL)
-    {
-      struct cleanup *rf_cleanups = make_free_skiplist_entry_cleanup (e);
+    compile_skip_regexp (e.get (), _("regexp"));
 
-      compile_skip_regexp (e, _("regexp"));
-      discard_cleanups (rf_cleanups);
-    }
-  add_skiplist_entry (e);
+  add_skiplist_entry (std::move (e));
 
   /* I18N concerns drive some of the choices here (we can't piece together
      the output too much).  OTOH we want to keep this simple.  Therefore the
@@ -414,7 +393,7 @@ skip_enable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-        e->enabled = 1;
+        e->enabled = true;
         found = 1;
       }
 
@@ -431,7 +410,7 @@ skip_disable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-	e->enabled = 0;
+	e->enabled = false;
         found = 1;
       }
 
@@ -454,7 +433,7 @@ skip_delete_command (char *arg, int from_tty)
 	else
 	  skiplist_entry_chain = e->next;
 
-	free_skiplist_entry (e);
+	delete e;
         found = 1;
       }
     else
@@ -469,7 +448,7 @@ skip_delete_command (char *arg, int from_tty)
 /* Add the given skiplist entry to our list, and set the entry's number.  */
 
 static void
-add_skiplist_entry (struct skiplist_entry *e)
+add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
 {
   struct skiplist_entry *e1;
 
@@ -480,12 +459,12 @@ add_skiplist_entry (struct skiplist_entry *e)
 
   e1 = skiplist_entry_chain;
   if (e1 == NULL)
-    skiplist_entry_chain = e;
+    skiplist_entry_chain = e.release ();
   else
     {
       while (e1->next)
 	e1 = e1->next;
-      e1->next = e;
+      e1->next = e.release ();
     }
 }
 
-- 
2.9.4

Re: [RFA] C++-ify skip.c

[Simon Marchi]

On 2017-08-06 21:42, Tom Tromey wrote:
> I happened to notice that skiplist_entry, in skip.c, contains a
> gdb::optional<compiled_regex> -- but that this object's destructor is
> never run.  This can result in a memory leak.
> 
> This patch fixes the bug by applying a bit more C++: changing this
> code to use new and delete, and std::unique_ptr; and removing cleanups
> in the process.
> 
> Built and regression tested on x86-64 Fedora 25.

Ah thanks, I tripped on this with me poison-XNEW patch.  I didn't have 
the time to fix and post it yet.

> ChangeLog
> 2017-08-06  Tom Tromey  <tom@tromey.com>
> 
> 	* skip.c (skiplist_entry): New constructor.
> 	(~skiplist_entry): New destructor.
> 	(skiplist_entry::enabled): Now bool.
> 	(make_skip_entry): Return a unique_ptr.  Use new.
> 	(free_skiplist_entry, free_skiplist_entry_cleanup)
> 	(make_free_skiplist_entry_cleanup): Remove.
> 	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> 	(skip_delete_command): Update.  Use delete.
> ---
>  gdb/ChangeLog | 11 ++++++++
>  gdb/skip.c    | 89 
> +++++++++++++++++++++++------------------------------------
>  2 files changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 722fade..12e0d02 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,14 @@
> +2017-08-06  Tom Tromey  <tom@tromey.com>
> +
> +	* skip.c (skiplist_entry): New constructor.
> +	(~skiplist_entry): New destructor.
> +	(skiplist_entry::enabled): Now bool.
> +	(make_skip_entry): Return a unique_ptr.  Use new.
> +	(free_skiplist_entry, free_skiplist_entry_cleanup)
> +	(make_free_skiplist_entry_cleanup): Remove.
> +	(skip_command, skip_disable_command, add_skiplist_entry): Update.
> +	(skip_delete_command): Update.  Use delete.
> +
>  2017-08-05  Tom Tromey  <tom@tromey.com>
> 
>  	* compile/compile-object-load.c (compile_object_load): Use
> diff --git a/gdb/skip.c b/gdb/skip.c
> index bf44913..f3291f3 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -38,6 +38,24 @@
> 
>  struct skiplist_entry
>  {
> +  skiplist_entry (int file_is_glob_, const char *file_,
> +		  int function_is_regexp_, const char *function_)
> +    : number (-1),
> +      file_is_glob (file_is_glob_),

file_is_glob and function_is_regexp can probably be changed to bools 
too.

> +      file (file_ == NULL ? NULL : xstrdup (file_)),
> +      function_is_regexp (function_is_regexp_),
> +      function (function_ == NULL ? NULL : xstrdup (function_)),

If we are strdup'ing (making a copy) file and function, we might as well 
store them in std::strings, so we don't need to explicitly xfree them.  
And the implicitly defined copy constructor/assignment operator will 
also work (which is not the case now).  An empty string is probably 
enough to mean the field is not used (which is expressed by a NULL value 
currently).

Thanks,

Simon

Re: [RFA] C++-ify skip.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Ah thanks, I tripped on this with me poison-XNEW patch.  I didn't have
Simon> the time to fix and post it yet.

Ah, I didn't know about this.
I think I will wait for that before working in this area any more.

Tom

Re: [RFA] C++-ify skip.c

[Simon Marchi]

On 2017-08-07 15:57, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Ah thanks, I tripped on this with me poison-XNEW patch.  I 
> didn't have
> Simon> the time to fix and post it yet.
> 
> Ah, I didn't know about this.
> I think I will wait for that before working in this area any more.
> 
> Tom

There are many things to fix before the poison-xnew patch can go in, but 
I think the issue you found is worth fixing sooner than later.

Among the things that need to change is the opaque types with multiple 
definitions, like arch_lwp_info and private_thread_info.  These need to 
be made into actual class hierarchies.  If anybody is looking for things 
to do, feel free to pick up some patches and post them :)

https://github.com/simark/binutils-gdb/commits/poison-xnew

Simon

Re: [RFA] C++-ify skip.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> There are many things to fix before the poison-xnew patch can go in,
Simon> but I think the issue you found is worth fixing sooner than later.

Alright, here's a new version.

Tom

commit 2d47b197465b640fde68194b3ffe0c5dfa57c134
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Aug 5 16:40:56 2017 -0600

    C++-ify skip.c
    
    I happened to notice that skiplist_entry, in skip.c, contains a
    gdb::optional<compiled_regex> -- but that this object's destructor is
    never run.  This can result in a memory leak.
    
    This patch fixes the bug by applying a bit more C++: changing this
    code to use new and delete, and std::unique_ptr; and removing cleanups
    in the process.
    
    Built and regression tested on x86-64 Fedora 25.
    
    ChangeLog
    2017-08-08  Tom Tromey  <tom@tromey.com>
    
            * skip.c (skiplist_entry): New constructor.
            (skiplist_entry::enabled, skiplist_entry::function_is_regexp)
            (skiplist_entry::file_is_glob): Now bool.
            (skiplist_entry::file, skiplist_entry::function): Now
            std::string.
            (make_skip_entry): Return a unique_ptr.  Use new.
            (free_skiplist_entry, free_skiplist_entry_cleanup)
            (make_free_skiplist_entry_cleanup): Remove.
            (skip_command, skip_disable_command, add_skiplist_entry)
            (skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
            (skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
            (function_name_is_marked_for_skip): Update.
            (skip_delete_command): Update.  Use delete.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 722fade..643e407 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2017-08-08  Tom Tromey  <tom@tromey.com>
+
+	* skip.c (skiplist_entry): New constructor.
+	(skiplist_entry::enabled, skiplist_entry::function_is_regexp)
+	(skiplist_entry::file_is_glob): Now bool.
+	(skiplist_entry::file, skiplist_entry::function): Now
+	std::string.
+	(make_skip_entry): Return a unique_ptr.  Use new.
+	(free_skiplist_entry, free_skiplist_entry_cleanup)
+	(make_free_skiplist_entry_cleanup): Remove.
+	(skip_command, skip_disable_command, add_skiplist_entry)
+	(skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
+	(skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
+	(function_name_is_marked_for_skip): Update.
+	(skip_delete_command): Update.  Use delete.
+
 2017-08-05  Tom Tromey  <tom@tromey.com>
 
 	* compile/compile-object-load.c (compile_object_load): Use
diff --git a/gdb/skip.c b/gdb/skip.c
index bf44913..66e9282 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -38,34 +38,44 @@
 
 struct skiplist_entry
 {
+  skiplist_entry (bool file_is_glob_, std::string &&file_,
+		  bool function_is_regexp_, std::string &&function_)
+    : number (-1),
+      file_is_glob (file_is_glob_),
+      file (file_),
+      function_is_regexp (function_is_regexp_),
+      function (function_),
+      enabled (true),
+      next (NULL)
+  {
+  }
+
   int number;
 
-  /* Non-zero if FILE is a glob-style pattern.
-     Otherewise it is the plain file name (possibly with directories).  */
-  int file_is_glob;
+  /* True if FILE is a glob-style pattern.
+     Otherwise it is the plain file name (possibly with directories).  */
+  bool file_is_glob;
 
-  /* The name of the file or NULL.
-     The skiplist entry owns this pointer.  */
-  char *file;
+  /* The name of the file or empty if no name.  */
+  std::string file;
 
-  /* Non-zero if FUNCTION is a regexp.
+  /* True if FUNCTION is a regexp.
      Otherwise it is a plain function name (possibly with arguments,
      for C++).  */
-  int function_is_regexp;
+  bool function_is_regexp;
 
-  /* The name of the function or NULL.
-     The skiplist entry owns this pointer.  */
-  char *function;
+  /* The name of the function or empty if no name.  */
+  std::string function;
 
   /* If this is a function regexp, the compiled form.  */
   gdb::optional<compiled_regex> compiled_function_regexp;
 
-  int enabled;
+  bool enabled;
 
   struct skiplist_entry *next;
 };
 
-static void add_skiplist_entry (struct skiplist_entry *e);
+static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
 
 static struct skiplist_entry *skiplist_entry_chain;
 static int skiplist_entry_count;
@@ -80,53 +90,19 @@ static int skiplist_entry_count;
 
 /* Create a skip object.  */
 
-static struct skiplist_entry *
-make_skip_entry (int file_is_glob, const char *file,
-		 int function_is_regexp, const char *function)
+static std::unique_ptr<skiplist_entry>
+make_skip_entry (bool file_is_glob, std::string &&file,
+		 bool function_is_regexp, std::string &&function)
 {
-  struct skiplist_entry *e = XCNEW (struct skiplist_entry);
-
-  gdb_assert (file != NULL || function != NULL);
+  gdb_assert (!file.empty () || !function.empty ());
   if (file_is_glob)
-    gdb_assert (file != NULL);
+    gdb_assert (!file.empty ());
   if (function_is_regexp)
-    gdb_assert (function != NULL);
-
-  if (file != NULL)
-    e->file = xstrdup (file);
-  if (function != NULL)
-    e->function = xstrdup (function);
-  e->file_is_glob = file_is_glob;
-  e->function_is_regexp = function_is_regexp;
-  e->enabled = 1;
-
-  return e;
-}
-
-/* Free a skiplist entry.  */
-
-static void
-free_skiplist_entry (struct skiplist_entry *e)
-{
-  xfree (e->file);
-  xfree (e->function);
-  xfree (e);
-}
-
-/* Wrapper to free_skiplist_entry for use as a cleanup.  */
+    gdb_assert (!function.empty ());
 
-static void
-free_skiplist_entry_cleanup (void *e)
-{
-  free_skiplist_entry ((struct skiplist_entry *) e);
-}
-
-/* Create a cleanup to free skiplist entry E.  */
-
-static struct cleanup *
-make_free_skiplist_entry_cleanup (struct skiplist_entry *e)
-{
-  return make_cleanup (free_skiplist_entry_cleanup, e);
+  return std::unique_ptr<skiplist_entry>
+    (new skiplist_entry (file_is_glob, std::move (file),
+			 function_is_regexp, std::move (function)));
 }
 
 static void
@@ -150,7 +126,8 @@ skip_file_command (char *arg, int from_tty)
   else
     filename = arg;
 
-  add_skiplist_entry (make_skip_entry (0, filename, 0, NULL));
+  add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
+				       std::string ()));
 
   printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
 }
@@ -161,7 +138,8 @@ skip_file_command (char *arg, int from_tty)
 static void
 skip_function (const char *name)
 {
-  add_skiplist_entry (make_skip_entry (0, NULL, 0, name));
+  add_skiplist_entry (make_skip_entry (false, std::string (), false,
+				       std::string (name)));
 
   printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
 }
@@ -204,8 +182,8 @@ compile_skip_regexp (struct skiplist_entry *e, const char *message)
   flags |= REG_EXTENDED;
 #endif
 
-  gdb_assert (e->function_is_regexp && e->function != NULL);
-  e->compiled_function_regexp.emplace (e->function, flags, message);
+  gdb_assert (e->function_is_regexp && !e->function.empty ());
+  e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
 }
 
 /* Process "skip ..." that does not match "skip file" or "skip function".  */
@@ -217,7 +195,6 @@ skip_command (char *arg, int from_tty)
   const char *gfile = NULL;
   const char *function = NULL;
   const char *rfunction = NULL;
-  struct skiplist_entry *e;
   int i;
 
   if (arg == NULL)
@@ -291,16 +268,23 @@ skip_command (char *arg, int from_tty)
   gdb_assert (file != NULL || gfile != NULL
 	      || function != NULL || rfunction != NULL);
 
-  e = make_skip_entry (gfile != NULL, file ? file : gfile,
-		       rfunction != NULL, function ? function : rfunction);
+  std::string entry_file;
+  if (file != NULL)
+    entry_file = file;
+  else if (gfile != NULL)
+    entry_file = gfile;
+  std::string entry_function;
+  if (function != NULL)
+    entry_function = function;
+  else if (rfunction != NULL)
+    entry_function = rfunction;
+  std::unique_ptr<skiplist_entry> e
+    = make_skip_entry (gfile != NULL, std::move (entry_file),
+		       rfunction != NULL, std::move (entry_function));
   if (rfunction != NULL)
-    {
-      struct cleanup *rf_cleanups = make_free_skiplist_entry_cleanup (e);
+    compile_skip_regexp (e.get (), _("regexp"));
 
-      compile_skip_regexp (e, _("regexp"));
-      discard_cleanups (rf_cleanups);
-    }
-  add_skiplist_entry (e);
+  add_skiplist_entry (std::move (e));
 
   /* I18N concerns drive some of the choices here (we can't piece together
      the output too much).  OTOH we want to keep this simple.  Therefore the
@@ -392,14 +376,16 @@ skip_info (char *arg, int from_tty)
 	current_uiout->field_string ("regexp", "n"); /* 3 */
 
       current_uiout->field_string ("file",
-			   e->file ? e->file : "<none>"); /* 4 */
+				   e->file.empty () ? "<none>"
+				   : e->file.c_str ()); /* 4 */
       if (e->function_is_regexp)
 	current_uiout->field_string ("regexp", "y"); /* 5 */
       else
 	current_uiout->field_string ("regexp", "n"); /* 5 */
 
-      current_uiout->field_string (
-	"function", e->function ? e->function : "<none>"); /* 6 */
+      current_uiout->field_string ("function",
+				   e->function.empty () ? "<none>"
+				   : e->function.c_str ()); /* 6 */
 
       current_uiout->text ("\n");
     }
@@ -414,7 +400,7 @@ skip_enable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-        e->enabled = 1;
+        e->enabled = true;
         found = 1;
       }
 
@@ -431,7 +417,7 @@ skip_disable_command (char *arg, int from_tty)
   ALL_SKIPLIST_ENTRIES (e)
     if (arg == NULL || number_is_in_list (arg, e->number))
       {
-	e->enabled = 0;
+	e->enabled = false;
         found = 1;
       }
 
@@ -454,7 +440,7 @@ skip_delete_command (char *arg, int from_tty)
 	else
 	  skiplist_entry_chain = e->next;
 
-	free_skiplist_entry (e);
+	delete e;
         found = 1;
       }
     else
@@ -469,7 +455,7 @@ skip_delete_command (char *arg, int from_tty)
 /* Add the given skiplist entry to our list, and set the entry's number.  */
 
 static void
-add_skiplist_entry (struct skiplist_entry *e)
+add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
 {
   struct skiplist_entry *e1;
 
@@ -480,12 +466,12 @@ add_skiplist_entry (struct skiplist_entry *e)
 
   e1 = skiplist_entry_chain;
   if (e1 == NULL)
-    skiplist_entry_chain = e;
+    skiplist_entry_chain = e.release ();
   else
     {
       while (e1->next)
 	e1 = e1->next;
-      e1->next = e;
+      e1->next = e.release ();
     }
 }
 
@@ -495,28 +481,29 @@ static int
 skip_file_p (struct skiplist_entry *e,
 	     const struct symtab_and_line *function_sal)
 {
-  gdb_assert (e->file != NULL && !e->file_is_glob);
+  gdb_assert (!e->file.empty () && !e->file_is_glob);
 
   if (function_sal->symtab == NULL)
     return 0;
 
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (compare_filenames_for_search (function_sal->symtab->filename, e->file))
+  if (compare_filenames_for_search (function_sal->symtab->filename,
+				    e->file.c_str ()))
     return 1;
 
   /* Before we invoke realpath, which can get expensive when many
      files are involved, do a quick comparison of the basenames.  */
   if (!basenames_may_differ
       && filename_cmp (lbasename (function_sal->symtab->filename),
-		       lbasename (e->file)) != 0)
+		       lbasename (e->file.c_str ())) != 0)
     return 0;
 
   /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
   {
     const char *fullname = symtab_to_fullname (function_sal->symtab);
 
-    if (compare_filenames_for_search (fullname, e->file))
+    if (compare_filenames_for_search (fullname, e->file.c_str ()))
       return 1;
   }
 
@@ -529,14 +516,14 @@ static int
 skip_gfile_p (struct skiplist_entry *e,
 	      const struct symtab_and_line *function_sal)
 {
-  gdb_assert (e->file != NULL && e->file_is_glob);
+  gdb_assert (!e->file.empty () && e->file_is_glob);
 
   if (function_sal->symtab == NULL)
     return 0;
 
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (gdb_filename_fnmatch (e->file, function_sal->symtab->filename,
+  if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
 			    FNM_FILE_NAME | FNM_NOESCAPE) == 0)
     return 1;
 
@@ -546,7 +533,7 @@ skip_gfile_p (struct skiplist_entry *e,
      If the basename of the glob pattern is something like "*.c" then this
      isn't much of a win.  Oh well.  */
   if (!basenames_may_differ
-      && gdb_filename_fnmatch (lbasename (e->file),
+      && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
 			       lbasename (function_sal->symtab->filename),
 			       FNM_FILE_NAME | FNM_NOESCAPE) != 0)
     return 0;
@@ -555,7 +542,7 @@ skip_gfile_p (struct skiplist_entry *e,
   {
     const char *fullname = symtab_to_fullname (function_sal->symtab);
 
-    if (compare_glob_filenames_for_search (fullname, e->file))
+    if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
       return 1;
   }
 
@@ -567,8 +554,8 @@ skip_gfile_p (struct skiplist_entry *e,
 static int
 skip_function_p (struct skiplist_entry *e, const char *function_name)
 {
-  gdb_assert (e->function != NULL && !e->function_is_regexp);
-  return strcmp_iw (function_name, e->function) == 0;
+  gdb_assert (!e->function.empty () && !e->function_is_regexp);
+  return strcmp_iw (function_name, e->function.c_str ()) == 0;
 }
 
 /* Return non-zero if we're stopped at a function regexp to be skipped.  */
@@ -576,7 +563,7 @@ skip_function_p (struct skiplist_entry *e, const char *function_name)
 static int
 skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
 {
-  gdb_assert (e->function != NULL && e->function_is_regexp
+  gdb_assert (!e->function.empty () && e->function_is_regexp
 	      && e->compiled_function_regexp);
   return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
 	  == 0);
@@ -601,7 +588,7 @@ function_name_is_marked_for_skip (const char *function_name,
       if (!e->enabled)
 	continue;
 
-      if (e->file != NULL)
+      if (!e->file.empty ())
 	{
 	  if (e->file_is_glob)
 	    {
@@ -614,7 +601,7 @@ function_name_is_marked_for_skip (const char *function_name,
 		skip_by_file = 1;
 	    }
 	}
-      if (e->function != NULL)
+      if (!e->function.empty ())
 	{
 	  if (e->function_is_regexp)
 	    {
@@ -630,7 +617,7 @@ function_name_is_marked_for_skip (const char *function_name,
 
       /* If both file and function must match, make sure we don't errantly
 	 exit if only one of them match.  */
-      if (e->file != NULL && e->function != NULL)
+      if (!e->file.empty () && !e->function.empty ())
 	{
 	  if (skip_by_file && skip_by_function)
 	    return 1;

Re: [RFA] C++-ify skip.c

[Simon Marchi]

On 2017-08-08 21:47, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> There are many things to fix before the poison-xnew patch can go 
> in,
> Simon> but I think the issue you found is worth fixing sooner than 
> later.
> 
> Alright, here's a new version.
> 
> Tom
> 
> commit 2d47b197465b640fde68194b3ffe0c5dfa57c134
> Author: Tom Tromey <tom@tromey.com>
> Date:   Sat Aug 5 16:40:56 2017 -0600
> 
>     C++-ify skip.c
> 
>     I happened to notice that skiplist_entry, in skip.c, contains a
>     gdb::optional<compiled_regex> -- but that this object's destructor 
> is
>     never run.  This can result in a memory leak.
> 
>     This patch fixes the bug by applying a bit more C++: changing this
>     code to use new and delete, and std::unique_ptr; and removing 
> cleanups
>     in the process.
> 
>     Built and regression tested on x86-64 Fedora 25.
> 
>     ChangeLog
>     2017-08-08  Tom Tromey  <tom@tromey.com>
> 
>             * skip.c (skiplist_entry): New constructor.
>             (skiplist_entry::enabled, 
> skiplist_entry::function_is_regexp)
>             (skiplist_entry::file_is_glob): Now bool.
>             (skiplist_entry::file, skiplist_entry::function): Now
>             std::string.
>             (make_skip_entry): Return a unique_ptr.  Use new.
>             (free_skiplist_entry, free_skiplist_entry_cleanup)
>             (make_free_skiplist_entry_cleanup): Remove.
>             (skip_command, skip_disable_command, add_skiplist_entry)
>             (skip_form_bytes, compile_skip_regexp, skip_command, 
> skip_info)
>             (skip_file_p, skip_gfile_p, skip_function_p, 
> skip_rfunction_p)
>             (function_name_is_marked_for_skip): Update.
>             (skip_delete_command): Update.  Use delete.
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 722fade..643e407 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,19 @@
> +2017-08-08  Tom Tromey  <tom@tromey.com>
> +
> +	* skip.c (skiplist_entry): New constructor.
> +	(skiplist_entry::enabled, skiplist_entry::function_is_regexp)
> +	(skiplist_entry::file_is_glob): Now bool.
> +	(skiplist_entry::file, skiplist_entry::function): Now
> +	std::string.
> +	(make_skip_entry): Return a unique_ptr.  Use new.
> +	(free_skiplist_entry, free_skiplist_entry_cleanup)
> +	(make_free_skiplist_entry_cleanup): Remove.
> +	(skip_command, skip_disable_command, add_skiplist_entry)
> +	(skip_form_bytes, compile_skip_regexp, skip_command, skip_info)
> +	(skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p)
> +	(function_name_is_marked_for_skip): Update.
> +	(skip_delete_command): Update.  Use delete.
> +
>  2017-08-05  Tom Tromey  <tom@tromey.com>
> 
>  	* compile/compile-object-load.c (compile_object_load): Use
> diff --git a/gdb/skip.c b/gdb/skip.c
> index bf44913..66e9282 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -38,34 +38,44 @@
> 
>  struct skiplist_entry
>  {
> +  skiplist_entry (bool file_is_glob_, std::string &&file_,
> +		  bool function_is_regexp_, std::string &&function_)
> +    : number (-1),
> +      file_is_glob (file_is_glob_),
> +      file (file_),
> +      function_is_regexp (function_is_regexp_),
> +      function (function_),

I think you have to use std::move here to avoid a copy being made.  I 
wasn't sure so I made a small test if you want to try for yourself (or 
maybe find that I got it wrong): http://paste.ubuntu.com/25275614/

Otherwise, a simpler way would be to leave the constructor (and calling 
functions) accepting const char* and just change the fields themselves 
to std::string.  The string objects will be constructed using the const 
char* constructor when the skiplist_entry object itself will be 
constructed, so you know you for sure you won't have any unnecessary 
copies.  That would avoid complicating the calling functions as well.  
Either way (adding std::moves or this) are fine for me, it's as you 
wish.

Thanks,

Simon

Re: [RFA] C++-ify skip.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> That would avoid complicating the calling functions as well.
Simon> Either way (adding std::moves or this) are fine for me, it's as
Simon> you wish.

I've added the moves, and I'm checking it in.  Thanks.

Tom

[PATCH] More gdb/skip.c C++ification

[Pedro Alves]

On 08/09/2017 07:31 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> That would avoid complicating the calling functions as well.
> Simon> Either way (adding std::moves or this) are fine for me, it's as
> Simon> you wish.
> 
> I've added the moves, and I'm checking it in.  Thanks.
> 

Thanks much for the fix, and sorry for not realizing I was
introducing the leak.  

I thought I had a local branch somewhere that C++ified skip.c
some more, and I spent a bit today looking for it, but couldn't
find it.  As penance for introducing the bug in the first place,
and since I was staring at skip.c, I ended up (re?)coding what I
thought I had, on top of Tom's patch.

It's a bit of churn, but there's nothing really complicated here.
It's just the next logical steps after Tom's patch.  See commit
log below.

WDYT?

Tested on GNU/Linux.

From 94ca8b042a5696e020d614be64f4bb83c1d111e6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 10 Aug 2017 19:58:24 +0100
Subject: [PATCH] More gdb/skip.c C++ification

- Make skiplist_entry a class with private data members.
- Move all construction logic to the ctor.
- Make skip_file_p etc be methods of skiplist_entry.
- Use std::list for the skip entries chain.  Make the list own its
  elements.
- Add a skiplist_entry_up typedef and use it more.
- Get rid of the ALL_SKIPLIST_ENTRIES/ALL_SKIPLIST_ENTRIES_SAFE
  macros, use range-for / iterators instead.
- function_name_is_marked_for_skip 'function_sal' argument must be
  non-NULL, so make it a reference instead.

All skiplist_entry invariants are now controlled by skiplist_entry
methods/internals.  Some gdb_asserts disappear for being redundant.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* infrun.c (process_event_stop_test): Adjust
	function_name_is_marked_for_skip call.
	* skip.c: Include <list>.
	(skiplist_entry): Make it a class with private fields, and
	getters/setters.
	(skiplist_entry_up): New typedef.
	(skiplist_entry_chain): Delete.
	(skiplist_entries): New.
	(skiplist_entry_count): Delete.
	(highest_skiplist_entry_num): New.
	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
	(add_skiplist_entry): Delete.
	(skiplist_entry::skiplist_entry): New.
	(skiplist_entry::add_entry): New.
	(skip_file_command, skip_function): Adjust.
	(compile_skip_regexp): Delete.
	(skip_command): Don't compile regexp here.  Adjust to use
	skiplist_entry::add_entry.
	(skip_info): Adjust to use range-for, skiplist_entry_up and getters.
	(skip_enable_command, skip_disable_command): Adjust to use
	range-for, skiplist_entry_up and setters.
	(skip_delete_command): Adjust to use std::list of
	skiplist_entry_up.
	(add_skiplist_entry): Delete.
	(skip_file_p): Delete, refactored as ...
	(skiplist_entry::do_skip_file_p): ... this new method.
	(skip_gfile_p): Delete, refactored as ...
	(skiplist_entry::do_gskip_file_p): ... this new method.
	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
	(skiplist_entry::skip_function_p): ... this new method.
	(function_name_is_marked_for_skip): Now returns bool, and takes
	the function sal by const reference.  Adjust to use range-for,
	skiplist_entry_up and skiplist_entry methods.
	(_initialize_step_skip): Remove references to
	skiplist_entry_chain, skiplist_entry_count.
	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
	takes the function sal by const reference.
---
 gdb/infrun.c |   2 +-
 gdb/skip.c   | 426 +++++++++++++++++++++++++++--------------------------------
 gdb/skip.h   |   8 +-
 3 files changed, 198 insertions(+), 238 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8f966e2..d6723fd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6788,7 +6788,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  &tmp_sal))
+						  tmp_sal))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
diff --git a/gdb/skip.c b/gdb/skip.c
index 6ab6c91..b10f8fb 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -35,74 +35,122 @@
 #include "fnmatch.h"
 #include "gdb_regex.h"
 #include "common/gdb_optional.h"
+#include <list>
 
-struct skiplist_entry
+class skiplist_entry
 {
-  skiplist_entry (bool file_is_glob_, std::string &&file_,
-		  bool function_is_regexp_, std::string &&function_)
-    : number (-1),
-      file_is_glob (file_is_glob_),
-      file (std::move (file_)),
-      function_is_regexp (function_is_regexp_),
-      function (std::move (function_)),
-      enabled (true),
-      next (NULL)
-  {
-  }
-
-  int number;
+public:
+  /* Create a skiplist_entry object and add it to the chain.  */
+  static void add_entry (bool file_is_glob,
+			 std::string &&file,
+			 bool function_is_regexp,
+			 std::string &&function);
+
+  /* Return true if the skip entry has a file or glob-style file
+     pattern that matches FUNCTION_SAL.  */
+  bool skip_file_p (const symtab_and_line &function_sal);
+
+  /* Return true if the skip entry has a function or function regexp
+     that matches FUNCTION_NAME.  */
+  bool skip_function_p (const char *function_name);
+
+  /* Getters.  */
+  int number () const { return m_number; };
+  bool enabled () const { return m_enabled; };
+  bool file_is_glob () const { return m_file_is_glob; }
+  const std::string &file () const { return m_file; }
+  const std::string &function () const { return m_function; }
+  bool function_is_regexp () const { return m_function_is_regexp; }
+
+  /* Setters.  */
+  void enable () { m_enabled = true; };
+  void disable () { m_enabled = false; };
+
+private:
+  /* Use add_entry factory to construct instead.  */
+  skiplist_entry (bool file_is_glob, std::string &&file,
+		  bool function_is_regexp, std::string &&function);
+
+  /* Return true if we're stopped at a file to be skipped.  */
+  bool do_skip_file_p (const symtab_and_line &function_sal);
+
+  /* Return true if we're stopped at a globbed file to be skipped.  */
+  bool do_skip_gfile_p (const symtab_and_line &function_sal);
+
+private: /* data */
+  int m_number = -1;
 
   /* True if FILE is a glob-style pattern.
      Otherwise it is the plain file name (possibly with directories).  */
-  bool file_is_glob;
+  bool m_file_is_glob;
 
   /* The name of the file or empty if no name.  */
-  std::string file;
+  std::string m_file;
 
   /* True if FUNCTION is a regexp.
      Otherwise it is a plain function name (possibly with arguments,
      for C++).  */
-  bool function_is_regexp;
+  bool m_function_is_regexp;
 
   /* The name of the function or empty if no name.  */
-  std::string function;
+  std::string m_function;
 
   /* If this is a function regexp, the compiled form.  */
-  gdb::optional<compiled_regex> compiled_function_regexp;
+  gdb::optional<compiled_regex> m_compiled_function_regexp;
 
-  bool enabled;
-
-  struct skiplist_entry *next;
+  /* Enabled/disabled state.  */
+  bool m_enabled = true;
 };
 
-static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
+typedef std::unique_ptr<skiplist_entry> skiplist_entry_up;
 
-static struct skiplist_entry *skiplist_entry_chain;
-static int skiplist_entry_count;
+static std::list<skiplist_entry_up> skiplist_entries;
+static int highest_skiplist_entry_num = 0;
 
-#define ALL_SKIPLIST_ENTRIES(E) \
-  for (E = skiplist_entry_chain; E; E = E->next)
+skiplist_entry::skiplist_entry (bool file_is_glob,
+				std::string &&file,
+				bool function_is_regexp,
+				std::string &&function)
+  : m_file_is_glob (file_is_glob),
+    m_file (std::move (file)),
+    m_function_is_regexp (function_is_regexp),
+    m_function (std::move (function))
+{
+  gdb_assert (!m_file.empty () || !m_function.empty ());
 
-#define ALL_SKIPLIST_ENTRIES_SAFE(E,TMP) \
-  for (E = skiplist_entry_chain;         \
-       E ? (TMP = E->next, 1) : 0;       \
-       E = TMP)
+  if (m_file_is_glob)
+    gdb_assert (!m_file.empty ());
+
+  if (m_function_is_regexp)
+    {
+      gdb_assert (!m_function.empty ());
+
+      int flags = REG_NOSUB;
+#ifdef REG_EXTENDED
+      flags |= REG_EXTENDED;
+#endif
 
-/* Create a skip object.  */
+      gdb_assert (!m_function.empty ());
+      m_compiled_function_regexp.emplace (m_function.c_str (), flags,
+					  _("regexp"));
+    }
+}
 
-static std::unique_ptr<skiplist_entry>
-make_skip_entry (bool file_is_glob, std::string &&file,
-		 bool function_is_regexp, std::string &&function)
+void
+skiplist_entry::add_entry (bool file_is_glob, std::string &&file,
+			   bool function_is_regexp, std::string &&function)
 {
-  gdb_assert (!file.empty () || !function.empty ());
-  if (file_is_glob)
-    gdb_assert (!file.empty ());
-  if (function_is_regexp)
-    gdb_assert (!function.empty ());
-
-  return std::unique_ptr<skiplist_entry>
-    (new skiplist_entry (file_is_glob, std::move (file),
-			 function_is_regexp, std::move (function)));
+  skiplist_entry *e = new skiplist_entry (file_is_glob,
+					  std::move (file),
+					  function_is_regexp,
+					  std::move (function));
+
+  /* Add to the end of the chain so that the list of skiplist entries
+     is always in numerical order.  */
+  skiplist_entries.push_back (skiplist_entry_up (e));
+
+  /* Incremented after push_back, in case push_back throws.  */
+  e->m_number = ++highest_skiplist_entry_num;
 }
 
 static void
@@ -126,8 +174,8 @@ skip_file_command (char *arg, int from_tty)
   else
     filename = arg;
 
-  add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
-				       std::string ()));
+  skiplist_entry::add_entry (false, std::string (filename),
+			     false, std::string ());
 
   printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
 }
@@ -138,8 +186,7 @@ skip_file_command (char *arg, int from_tty)
 static void
 skip_function (const char *name)
 {
-  add_skiplist_entry (make_skip_entry (false, std::string (), false,
-				       std::string (name)));
+  skiplist_entry::add_entry (false, std::string (), false, std::string (name));
 
   printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
 }
@@ -169,23 +216,6 @@ skip_function_command (char *arg, int from_tty)
   skip_function (arg);
 }
 
-/* Compile the regexp in E.
-   An error is thrown if there's an error.
-   MESSAGE is used as a prefix of the error message.  */
-
-static void
-compile_skip_regexp (struct skiplist_entry *e, const char *message)
-{
-  int flags = REG_NOSUB;
-
-#ifdef REG_EXTENDED
-  flags |= REG_EXTENDED;
-#endif
-
-  gdb_assert (e->function_is_regexp && !e->function.empty ());
-  e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
-}
-
 /* Process "skip ..." that does not match "skip file" or "skip function".  */
 
 static void
@@ -273,18 +303,15 @@ skip_command (char *arg, int from_tty)
     entry_file = file;
   else if (gfile != NULL)
     entry_file = gfile;
+
   std::string entry_function;
   if (function != NULL)
     entry_function = function;
   else if (rfunction != NULL)
     entry_function = rfunction;
-  std::unique_ptr<skiplist_entry> e
-    = make_skip_entry (gfile != NULL, std::move (entry_file),
-		       rfunction != NULL, std::move (entry_function));
-  if (rfunction != NULL)
-    compile_skip_regexp (e.get (), _("regexp"));
 
-  add_skiplist_entry (std::move (e));
+  skiplist_entry::add_entry (gfile != NULL, std::move (entry_file),
+			     rfunction != NULL, std::move (entry_function));
 
   /* I18N concerns drive some of the choices here (we can't piece together
      the output too much).  OTOH we want to keep this simple.  Therefore the
@@ -321,7 +348,6 @@ skip_command (char *arg, int from_tty)
 static void
 skip_info (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
   int num_printable_entries = 0;
   struct value_print_options opts;
 
@@ -329,8 +355,8 @@ skip_info (char *arg, int from_tty)
 
   /* Count the number of rows in the table and see if we need space for a
      64-bit address anywhere.  */
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (const skiplist_entry_up &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e->number ()))
       num_printable_entries++;
 
   if (num_printable_entries == 0)
@@ -355,37 +381,36 @@ skip_info (char *arg, int from_tty)
   current_uiout->table_header (40, ui_noalign, "function", "Function"); /* 6 */
   current_uiout->table_body ();
 
-  ALL_SKIPLIST_ENTRIES (e)
+  for (const skiplist_entry_up &e : skiplist_entries)
     {
-
       QUIT;
-      if (arg != NULL && !number_is_in_list (arg, e->number))
+      if (arg != NULL && !number_is_in_list (arg, e->number ()))
 	continue;
 
       ui_out_emit_tuple tuple_emitter (current_uiout, "blklst-entry");
-      current_uiout->field_int ("number", e->number); /* 1 */
+      current_uiout->field_int ("number", e->number ()); /* 1 */
 
-      if (e->enabled)
+      if (e->enabled ())
 	current_uiout->field_string ("enabled", "y"); /* 2 */
       else
 	current_uiout->field_string ("enabled", "n"); /* 2 */
 
-      if (e->file_is_glob)
+      if (e->file_is_glob ())
 	current_uiout->field_string ("regexp", "y"); /* 3 */
       else
 	current_uiout->field_string ("regexp", "n"); /* 3 */
 
       current_uiout->field_string ("file",
-				   e->file.empty () ? "<none>"
-				   : e->file.c_str ()); /* 4 */
-      if (e->function_is_regexp)
+				   e->file ().empty () ? "<none>"
+				   : e->file ().c_str ()); /* 4 */
+      if (e->function_is_regexp ())
 	current_uiout->field_string ("regexp", "y"); /* 5 */
       else
 	current_uiout->field_string ("regexp", "n"); /* 5 */
 
       current_uiout->field_string ("function",
-				   e->function.empty () ? "<none>"
-				   : e->function.c_str ()); /* 6 */
+				   e->function ().empty () ? "<none>"
+				   : e->function ().c_str ()); /* 6 */
 
       current_uiout->text ("\n");
     }
@@ -394,14 +419,13 @@ skip_info (char *arg, int from_tty)
 static void
 skip_enable_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
-  int found = 0;
+  bool found = false;
 
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (const skiplist_entry_up &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e->number ()))
       {
-        e->enabled = true;
-        found = 1;
+	e->enable ();
+	found = true;
       }
 
   if (!found)
@@ -411,14 +435,13 @@ skip_enable_command (char *arg, int from_tty)
 static void
 skip_disable_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
-  int found = 0;
+  bool found = false;
 
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (const skiplist_entry_up &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e->number ()))
       {
-	e->enabled = false;
-        found = 1;
+	e->disable ();
+	found = true;
       }
 
   if (!found)
@@ -428,104 +451,62 @@ skip_disable_command (char *arg, int from_tty)
 static void
 skip_delete_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e, *temp, *b_prev;
-  int found = 0;
+  bool found = false;
 
-  b_prev = 0;
-  ALL_SKIPLIST_ENTRIES_SAFE (e, temp)
-    if (arg == NULL || number_is_in_list (arg, e->number))
-      {
-	if (b_prev != NULL)
-	  b_prev->next = e->next;
-	else
-	  skiplist_entry_chain = e->next;
+  for (auto it = skiplist_entries.begin (),
+	 end = skiplist_entries.end ();
+       it != end;)
+    {
+      const skiplist_entry_up &e = *it;
 
-	delete e;
-        found = 1;
-      }
-    else
-      {
-	b_prev = e;
-      }
+      if (arg == NULL || number_is_in_list (arg, e->number ()))
+	{
+	  it = skiplist_entries.erase (it);
+	  found = true;
+	}
+      else
+	++it;
+    }
 
   if (!found)
     error (_("No skiplist entries found with number %s."), arg);
 }
 
-/* Add the given skiplist entry to our list, and set the entry's number.  */
-
-static void
-add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
+bool
+skiplist_entry::do_skip_file_p (const symtab_and_line &function_sal)
 {
-  struct skiplist_entry *e1;
-
-  e->number = ++skiplist_entry_count;
-
-  /* Add to the end of the chain so that the list of
-     skiplist entries will be in numerical order.  */
-
-  e1 = skiplist_entry_chain;
-  if (e1 == NULL)
-    skiplist_entry_chain = e.release ();
-  else
-    {
-      while (e1->next)
-	e1 = e1->next;
-      e1->next = e.release ();
-    }
-}
-
-/* Return non-zero if we're stopped at a file to be skipped.  */
-
-static int
-skip_file_p (struct skiplist_entry *e,
-	     const struct symtab_and_line *function_sal)
-{
-  gdb_assert (!e->file.empty () && !e->file_is_glob);
-
-  if (function_sal->symtab == NULL)
-    return 0;
-
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (compare_filenames_for_search (function_sal->symtab->filename,
-				    e->file.c_str ()))
-    return 1;
+  if (compare_filenames_for_search (function_sal.symtab->filename,
+				    m_file.c_str ()))
+    return true;
 
   /* Before we invoke realpath, which can get expensive when many
      files are involved, do a quick comparison of the basenames.  */
   if (!basenames_may_differ
-      && filename_cmp (lbasename (function_sal->symtab->filename),
-		       lbasename (e->file.c_str ())) != 0)
-    return 0;
+      && filename_cmp (lbasename (function_sal.symtab->filename),
+		       lbasename (m_file.c_str ())) != 0)
+    return false;
 
   /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
   {
-    const char *fullname = symtab_to_fullname (function_sal->symtab);
+    const char *fullname = symtab_to_fullname (function_sal.symtab);
 
-    if (compare_filenames_for_search (fullname, e->file.c_str ()))
-      return 1;
+    if (compare_filenames_for_search (fullname, m_file.c_str ()))
+      return true;
   }
 
-  return 0;
+  return false;
 }
 
-/* Return non-zero if we're stopped at a globbed file to be skipped.  */
-
-static int
-skip_gfile_p (struct skiplist_entry *e,
-	      const struct symtab_and_line *function_sal)
+bool
+skiplist_entry::do_skip_gfile_p (const symtab_and_line &function_sal)
 {
-  gdb_assert (!e->file.empty () && e->file_is_glob);
-
-  if (function_sal->symtab == NULL)
-    return 0;
-
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
+  if (gdb_filename_fnmatch (m_file.c_str (), function_sal.symtab->filename,
 			    FNM_FILE_NAME | FNM_NOESCAPE) == 0)
-    return 1;
+    return true;
 
   /* Before we invoke symtab_to_fullname, which is expensive, do a quick
      comparison of the basenames.
@@ -533,101 +514,83 @@ skip_gfile_p (struct skiplist_entry *e,
      If the basename of the glob pattern is something like "*.c" then this
      isn't much of a win.  Oh well.  */
   if (!basenames_may_differ
-      && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
-			       lbasename (function_sal->symtab->filename),
+      && gdb_filename_fnmatch (lbasename (m_file.c_str ()),
+			       lbasename (function_sal.symtab->filename),
 			       FNM_FILE_NAME | FNM_NOESCAPE) != 0)
-    return 0;
+    return false;
 
   /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
   {
-    const char *fullname = symtab_to_fullname (function_sal->symtab);
+    const char *fullname = symtab_to_fullname (function_sal.symtab);
 
-    if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
-      return 1;
+    if (compare_glob_filenames_for_search (fullname, m_file.c_str ()))
+      return true;
   }
 
-  return 0;
+  return false;
 }
 
-/* Return non-zero if we're stopped at a function to be skipped.  */
-
-static int
-skip_function_p (struct skiplist_entry *e, const char *function_name)
+bool
+skiplist_entry::skip_file_p (const symtab_and_line &function_sal)
 {
-  gdb_assert (!e->function.empty () && !e->function_is_regexp);
-  return strcmp_iw (function_name, e->function.c_str ()) == 0;
-}
+  if (m_file.empty ())
+    return false;
 
-/* Return non-zero if we're stopped at a function regexp to be skipped.  */
+  if (function_sal.symtab == NULL)
+    return false;
 
-static int
-skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
+  if (m_file_is_glob)
+    return do_skip_gfile_p (function_sal);
+  else
+    return do_skip_file_p (function_sal);
+}
+
+bool
+skiplist_entry::skip_function_p (const char *function_name)
 {
-  gdb_assert (!e->function.empty () && e->function_is_regexp
-	      && e->compiled_function_regexp);
-  return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
-	  == 0);
+  if (m_function.empty ())
+    return false;
+
+  if (m_function_is_regexp)
+    {
+      gdb_assert (m_compiled_function_regexp);
+      return (m_compiled_function_regexp->exec (function_name, 0, NULL, 0)
+	      == 0);
+    }
+  else
+    return strcmp_iw (function_name, m_function.c_str ()) == 0;
 }
 
 /* See skip.h.  */
 
-int
+bool
 function_name_is_marked_for_skip (const char *function_name,
-				  const struct symtab_and_line *function_sal)
+				  const symtab_and_line &function_sal)
 {
-  struct skiplist_entry *e;
-
   if (function_name == NULL)
-    return 0;
+    return false;
 
-  ALL_SKIPLIST_ENTRIES (e)
+  for (const skiplist_entry_up &e : skiplist_entries)
     {
-      int skip_by_file = 0;
-      int skip_by_function = 0;
-
-      if (!e->enabled)
+      if (!e->enabled ())
 	continue;
 
-      if (!e->file.empty ())
-	{
-	  if (e->file_is_glob)
-	    {
-	      if (skip_gfile_p (e, function_sal))
-		skip_by_file = 1;
-	    }
-	  else
-	    {
-	      if (skip_file_p (e, function_sal))
-		skip_by_file = 1;
-	    }
-	}
-      if (!e->function.empty ())
-	{
-	  if (e->function_is_regexp)
-	    {
-	      if (skip_rfunction_p (e, function_name))
-		skip_by_function = 1;
-	    }
-	  else
-	    {
-	      if (skip_function_p (e, function_name))
-		skip_by_function = 1;
-	    }
-	}
+      bool skip_by_file = e->skip_file_p (function_sal);
+      bool skip_by_function = e->skip_function_p (function_name);
 
       /* If both file and function must match, make sure we don't errantly
 	 exit if only one of them match.  */
-      if (!e->file.empty () && !e->function.empty ())
+      if (!e->file ().empty () && !e->function ().empty ())
 	{
 	  if (skip_by_file && skip_by_function)
-	    return 1;
+	    return true;
 	}
       /* Only one of file/function is specified.  */
       else if (skip_by_file || skip_by_function)
-	return 1;
+	return true;
     }
 
-  return 0;
+  return false;
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -639,9 +602,6 @@ _initialize_step_skip (void)
   static struct cmd_list_element *skiplist = NULL;
   struct cmd_list_element *c;
 
-  skiplist_entry_chain = 0;
-  skiplist_entry_count = 0;
-
   add_prefix_cmd ("skip", class_breakpoint, skip_command, _("\
 Ignore a function while stepping.\n\
 \n\
diff --git a/gdb/skip.h b/gdb/skip.h
index dbc92d8..4e1b544 100644
--- a/gdb/skip.h
+++ b/gdb/skip.h
@@ -20,9 +20,9 @@
 
 struct symtab_and_line;
 
-/* Returns 1 if the given FUNCTION_NAME is marked for skip and shouldn't be
-   stepped into.  Otherwise, returns 0.  */
-int function_name_is_marked_for_skip (const char *function_name,
-				    const struct symtab_and_line *function_sal);
+/* Returns true if the given FUNCTION_NAME is marked for skip and
+   shouldn't be stepped into.  Otherwise, returns false.  */
+bool function_name_is_marked_for_skip (const char *function_name,
+				       const symtab_and_line &function_sal);
 
 #endif /* !defined (SKIP_H) */
-- 
2.5.5

Re: [PATCH] More gdb/skip.c C++ification

[Pedro Alves]

On 08/10/2017 08:34 PM, Pedro Alves wrote:
> On 08/09/2017 07:31 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>>
>> Simon> That would avoid complicating the calling functions as well.
>> Simon> Either way (adding std::moves or this) are fine for me, it's as
>> Simon> you wish.
>>
>> I've added the moves, and I'm checking it in.  Thanks.
>>
> 
> Thanks much for the fix, and sorry for not realizing I was
> introducing the leak.  
> 
> I thought I had a local branch somewhere that C++ified skip.c
> some more, and I spent a bit today looking for it, but couldn't
> find it.  As penance for introducing the bug in the first place,
> and since I was staring at skip.c, I ended up (re?)coding what I
> thought I had, on top of Tom's patch.
> 
> It's a bit of churn, but there's nothing really complicated here.
> It's just the next logical steps after Tom's patch.  See commit
> log below.
> 
> WDYT?
> 
> Tested on GNU/Linux.

Actually, now that I turn my automatic-brainless-C++ification-mode
switch off I can't really explain why I thought of making the entry
chain a std::list of pointers instead of a list of objects... (which
would spare the double allocation [node + element].)  I'll see about
changing that...

Thanks,
Pedro Alves

> 
> From 94ca8b042a5696e020d614be64f4bb83c1d111e6 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 10 Aug 2017 19:58:24 +0100
> Subject: [PATCH] More gdb/skip.c C++ification
> 
> - Make skiplist_entry a class with private data members.
> - Move all construction logic to the ctor.
> - Make skip_file_p etc be methods of skiplist_entry.
> - Use std::list for the skip entries chain.  Make the list own its
>   elements.
> - Add a skiplist_entry_up typedef and use it more.
> - Get rid of the ALL_SKIPLIST_ENTRIES/ALL_SKIPLIST_ENTRIES_SAFE
>   macros, use range-for / iterators instead.
> - function_name_is_marked_for_skip 'function_sal' argument must be
>   non-NULL, so make it a reference instead.
> 
> All skiplist_entry invariants are now controlled by skiplist_entry
> methods/internals.  Some gdb_asserts disappear for being redundant.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* infrun.c (process_event_stop_test): Adjust
> 	function_name_is_marked_for_skip call.
> 	* skip.c: Include <list>.
> 	(skiplist_entry): Make it a class with private fields, and
> 	getters/setters.
> 	(skiplist_entry_up): New typedef.
> 	(skiplist_entry_chain): Delete.
> 	(skiplist_entries): New.
> 	(skiplist_entry_count): Delete.
> 	(highest_skiplist_entry_num): New.
> 	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
> 	(add_skiplist_entry): Delete.
> 	(skiplist_entry::skiplist_entry): New.
> 	(skiplist_entry::add_entry): New.
> 	(skip_file_command, skip_function): Adjust.
> 	(compile_skip_regexp): Delete.
> 	(skip_command): Don't compile regexp here.  Adjust to use
> 	skiplist_entry::add_entry.
> 	(skip_info): Adjust to use range-for, skiplist_entry_up and getters.
> 	(skip_enable_command, skip_disable_command): Adjust to use
> 	range-for, skiplist_entry_up and setters.
> 	(skip_delete_command): Adjust to use std::list of
> 	skiplist_entry_up.
> 	(add_skiplist_entry): Delete.
> 	(skip_file_p): Delete, refactored as ...
> 	(skiplist_entry::do_skip_file_p): ... this new method.
> 	(skip_gfile_p): Delete, refactored as ...
> 	(skiplist_entry::do_gskip_file_p): ... this new method.
> 	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
> 	(skiplist_entry::skip_function_p): ... this new method.
> 	(function_name_is_marked_for_skip): Now returns bool, and takes
> 	the function sal by const reference.  Adjust to use range-for,
> 	skiplist_entry_up and skiplist_entry methods.
> 	(_initialize_step_skip): Remove references to
> 	skiplist_entry_chain, skiplist_entry_count.
> 	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
> 	takes the function sal by const reference.
> ---
>  gdb/infrun.c |   2 +-
>  gdb/skip.c   | 426 +++++++++++++++++++++++++++--------------------------------
>  gdb/skip.h   |   8 +-
>  3 files changed, 198 insertions(+), 238 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 8f966e2..d6723fd 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6788,7 +6788,7 @@ process_event_stop_test (struct execution_control_state *ecs)
>  	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
>  	if (tmp_sal.line != 0
>  	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
> -						  &tmp_sal))
> +						  tmp_sal))
>  	  {
>  	    if (execution_direction == EXEC_REVERSE)
>  	      handle_step_into_function_backward (gdbarch, ecs);
> diff --git a/gdb/skip.c b/gdb/skip.c
> index 6ab6c91..b10f8fb 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -35,74 +35,122 @@
>  #include "fnmatch.h"
>  #include "gdb_regex.h"
>  #include "common/gdb_optional.h"
> +#include <list>
>  
> -struct skiplist_entry
> +class skiplist_entry
>  {
> -  skiplist_entry (bool file_is_glob_, std::string &&file_,
> -		  bool function_is_regexp_, std::string &&function_)
> -    : number (-1),
> -      file_is_glob (file_is_glob_),
> -      file (std::move (file_)),
> -      function_is_regexp (function_is_regexp_),
> -      function (std::move (function_)),
> -      enabled (true),
> -      next (NULL)
> -  {
> -  }
> -
> -  int number;
> +public:
> +  /* Create a skiplist_entry object and add it to the chain.  */
> +  static void add_entry (bool file_is_glob,
> +			 std::string &&file,
> +			 bool function_is_regexp,
> +			 std::string &&function);
> +
> +  /* Return true if the skip entry has a file or glob-style file
> +     pattern that matches FUNCTION_SAL.  */
> +  bool skip_file_p (const symtab_and_line &function_sal);
> +
> +  /* Return true if the skip entry has a function or function regexp
> +     that matches FUNCTION_NAME.  */
> +  bool skip_function_p (const char *function_name);
> +
> +  /* Getters.  */
> +  int number () const { return m_number; };
> +  bool enabled () const { return m_enabled; };
> +  bool file_is_glob () const { return m_file_is_glob; }
> +  const std::string &file () const { return m_file; }
> +  const std::string &function () const { return m_function; }
> +  bool function_is_regexp () const { return m_function_is_regexp; }
> +
> +  /* Setters.  */
> +  void enable () { m_enabled = true; };
> +  void disable () { m_enabled = false; };
> +
> +private:
> +  /* Use add_entry factory to construct instead.  */
> +  skiplist_entry (bool file_is_glob, std::string &&file,
> +		  bool function_is_regexp, std::string &&function);
> +
> +  /* Return true if we're stopped at a file to be skipped.  */
> +  bool do_skip_file_p (const symtab_and_line &function_sal);
> +
> +  /* Return true if we're stopped at a globbed file to be skipped.  */
> +  bool do_skip_gfile_p (const symtab_and_line &function_sal);
> +
> +private: /* data */
> +  int m_number = -1;
>  
>    /* True if FILE is a glob-style pattern.
>       Otherwise it is the plain file name (possibly with directories).  */
> -  bool file_is_glob;
> +  bool m_file_is_glob;
>  
>    /* The name of the file or empty if no name.  */
> -  std::string file;
> +  std::string m_file;
>  
>    /* True if FUNCTION is a regexp.
>       Otherwise it is a plain function name (possibly with arguments,
>       for C++).  */
> -  bool function_is_regexp;
> +  bool m_function_is_regexp;
>  
>    /* The name of the function or empty if no name.  */
> -  std::string function;
> +  std::string m_function;
>  
>    /* If this is a function regexp, the compiled form.  */
> -  gdb::optional<compiled_regex> compiled_function_regexp;
> +  gdb::optional<compiled_regex> m_compiled_function_regexp;
>  
> -  bool enabled;
> -
> -  struct skiplist_entry *next;
> +  /* Enabled/disabled state.  */
> +  bool m_enabled = true;
>  };
>  
> -static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
> +typedef std::unique_ptr<skiplist_entry> skiplist_entry_up;
>  
> -static struct skiplist_entry *skiplist_entry_chain;
> -static int skiplist_entry_count;
> +static std::list<skiplist_entry_up> skiplist_entries;
> +static int highest_skiplist_entry_num = 0;
>  
> -#define ALL_SKIPLIST_ENTRIES(E) \
> -  for (E = skiplist_entry_chain; E; E = E->next)
> +skiplist_entry::skiplist_entry (bool file_is_glob,
> +				std::string &&file,
> +				bool function_is_regexp,
> +				std::string &&function)
> +  : m_file_is_glob (file_is_glob),
> +    m_file (std::move (file)),
> +    m_function_is_regexp (function_is_regexp),
> +    m_function (std::move (function))
> +{
> +  gdb_assert (!m_file.empty () || !m_function.empty ());
>  
> -#define ALL_SKIPLIST_ENTRIES_SAFE(E,TMP) \
> -  for (E = skiplist_entry_chain;         \
> -       E ? (TMP = E->next, 1) : 0;       \
> -       E = TMP)
> +  if (m_file_is_glob)
> +    gdb_assert (!m_file.empty ());
> +
> +  if (m_function_is_regexp)
> +    {
> +      gdb_assert (!m_function.empty ());
> +
> +      int flags = REG_NOSUB;
> +#ifdef REG_EXTENDED
> +      flags |= REG_EXTENDED;
> +#endif
>  
> -/* Create a skip object.  */
> +      gdb_assert (!m_function.empty ());
> +      m_compiled_function_regexp.emplace (m_function.c_str (), flags,
> +					  _("regexp"));
> +    }
> +}
>  
> -static std::unique_ptr<skiplist_entry>
> -make_skip_entry (bool file_is_glob, std::string &&file,
> -		 bool function_is_regexp, std::string &&function)
> +void
> +skiplist_entry::add_entry (bool file_is_glob, std::string &&file,
> +			   bool function_is_regexp, std::string &&function)
>  {
> -  gdb_assert (!file.empty () || !function.empty ());
> -  if (file_is_glob)
> -    gdb_assert (!file.empty ());
> -  if (function_is_regexp)
> -    gdb_assert (!function.empty ());
> -
> -  return std::unique_ptr<skiplist_entry>
> -    (new skiplist_entry (file_is_glob, std::move (file),
> -			 function_is_regexp, std::move (function)));
> +  skiplist_entry *e = new skiplist_entry (file_is_glob,
> +					  std::move (file),
> +					  function_is_regexp,
> +					  std::move (function));
> +
> +  /* Add to the end of the chain so that the list of skiplist entries
> +     is always in numerical order.  */
> +  skiplist_entries.push_back (skiplist_entry_up (e));
> +
> +  /* Incremented after push_back, in case push_back throws.  */
> +  e->m_number = ++highest_skiplist_entry_num;
>  }
>  
>  static void
> @@ -126,8 +174,8 @@ skip_file_command (char *arg, int from_tty)
>    else
>      filename = arg;
>  
> -  add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
> -				       std::string ()));
> +  skiplist_entry::add_entry (false, std::string (filename),
> +			     false, std::string ());
>  
>    printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
>  }
> @@ -138,8 +186,7 @@ skip_file_command (char *arg, int from_tty)
>  static void
>  skip_function (const char *name)
>  {
> -  add_skiplist_entry (make_skip_entry (false, std::string (), false,
> -				       std::string (name)));
> +  skiplist_entry::add_entry (false, std::string (), false, std::string (name));
>  
>    printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
>  }
> @@ -169,23 +216,6 @@ skip_function_command (char *arg, int from_tty)
>    skip_function (arg);
>  }
>  
> -/* Compile the regexp in E.
> -   An error is thrown if there's an error.
> -   MESSAGE is used as a prefix of the error message.  */
> -
> -static void
> -compile_skip_regexp (struct skiplist_entry *e, const char *message)
> -{
> -  int flags = REG_NOSUB;
> -
> -#ifdef REG_EXTENDED
> -  flags |= REG_EXTENDED;
> -#endif
> -
> -  gdb_assert (e->function_is_regexp && !e->function.empty ());
> -  e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
> -}
> -
>  /* Process "skip ..." that does not match "skip file" or "skip function".  */
>  
>  static void
> @@ -273,18 +303,15 @@ skip_command (char *arg, int from_tty)
>      entry_file = file;
>    else if (gfile != NULL)
>      entry_file = gfile;
> +
>    std::string entry_function;
>    if (function != NULL)
>      entry_function = function;
>    else if (rfunction != NULL)
>      entry_function = rfunction;
> -  std::unique_ptr<skiplist_entry> e
> -    = make_skip_entry (gfile != NULL, std::move (entry_file),
> -		       rfunction != NULL, std::move (entry_function));
> -  if (rfunction != NULL)
> -    compile_skip_regexp (e.get (), _("regexp"));
>  
> -  add_skiplist_entry (std::move (e));
> +  skiplist_entry::add_entry (gfile != NULL, std::move (entry_file),
> +			     rfunction != NULL, std::move (entry_function));
>  
>    /* I18N concerns drive some of the choices here (we can't piece together
>       the output too much).  OTOH we want to keep this simple.  Therefore the
> @@ -321,7 +348,6 @@ skip_command (char *arg, int from_tty)
>  static void
>  skip_info (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
>    int num_printable_entries = 0;
>    struct value_print_options opts;
>  
> @@ -329,8 +355,8 @@ skip_info (char *arg, int from_tty)
>  
>    /* Count the number of rows in the table and see if we need space for a
>       64-bit address anywhere.  */
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        num_printable_entries++;
>  
>    if (num_printable_entries == 0)
> @@ -355,37 +381,36 @@ skip_info (char *arg, int from_tty)
>    current_uiout->table_header (40, ui_noalign, "function", "Function"); /* 6 */
>    current_uiout->table_body ();
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> +  for (const skiplist_entry_up &e : skiplist_entries)
>      {
> -
>        QUIT;
> -      if (arg != NULL && !number_is_in_list (arg, e->number))
> +      if (arg != NULL && !number_is_in_list (arg, e->number ()))
>  	continue;
>  
>        ui_out_emit_tuple tuple_emitter (current_uiout, "blklst-entry");
> -      current_uiout->field_int ("number", e->number); /* 1 */
> +      current_uiout->field_int ("number", e->number ()); /* 1 */
>  
> -      if (e->enabled)
> +      if (e->enabled ())
>  	current_uiout->field_string ("enabled", "y"); /* 2 */
>        else
>  	current_uiout->field_string ("enabled", "n"); /* 2 */
>  
> -      if (e->file_is_glob)
> +      if (e->file_is_glob ())
>  	current_uiout->field_string ("regexp", "y"); /* 3 */
>        else
>  	current_uiout->field_string ("regexp", "n"); /* 3 */
>  
>        current_uiout->field_string ("file",
> -				   e->file.empty () ? "<none>"
> -				   : e->file.c_str ()); /* 4 */
> -      if (e->function_is_regexp)
> +				   e->file ().empty () ? "<none>"
> +				   : e->file ().c_str ()); /* 4 */
> +      if (e->function_is_regexp ())
>  	current_uiout->field_string ("regexp", "y"); /* 5 */
>        else
>  	current_uiout->field_string ("regexp", "n"); /* 5 */
>  
>        current_uiout->field_string ("function",
> -				   e->function.empty () ? "<none>"
> -				   : e->function.c_str ()); /* 6 */
> +				   e->function ().empty () ? "<none>"
> +				   : e->function ().c_str ()); /* 6 */
>  
>        current_uiout->text ("\n");
>      }
> @@ -394,14 +419,13 @@ skip_info (char *arg, int from_tty)
>  static void
>  skip_enable_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
> -  int found = 0;
> +  bool found = false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        {
> -        e->enabled = true;
> -        found = 1;
> +	e->enable ();
> +	found = true;
>        }
>  
>    if (!found)
> @@ -411,14 +435,13 @@ skip_enable_command (char *arg, int from_tty)
>  static void
>  skip_disable_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e;
> -  int found = 0;
> +  bool found = false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> +  for (const skiplist_entry_up &e : skiplist_entries)
> +    if (arg == NULL || number_is_in_list (arg, e->number ()))
>        {
> -	e->enabled = false;
> -        found = 1;
> +	e->disable ();
> +	found = true;
>        }
>  
>    if (!found)
> @@ -428,104 +451,62 @@ skip_disable_command (char *arg, int from_tty)
>  static void
>  skip_delete_command (char *arg, int from_tty)
>  {
> -  struct skiplist_entry *e, *temp, *b_prev;
> -  int found = 0;
> +  bool found = false;
>  
> -  b_prev = 0;
> -  ALL_SKIPLIST_ENTRIES_SAFE (e, temp)
> -    if (arg == NULL || number_is_in_list (arg, e->number))
> -      {
> -	if (b_prev != NULL)
> -	  b_prev->next = e->next;
> -	else
> -	  skiplist_entry_chain = e->next;
> +  for (auto it = skiplist_entries.begin (),
> +	 end = skiplist_entries.end ();
> +       it != end;)
> +    {
> +      const skiplist_entry_up &e = *it;
>  
> -	delete e;
> -        found = 1;
> -      }
> -    else
> -      {
> -	b_prev = e;
> -      }
> +      if (arg == NULL || number_is_in_list (arg, e->number ()))
> +	{
> +	  it = skiplist_entries.erase (it);
> +	  found = true;
> +	}
> +      else
> +	++it;
> +    }
>  
>    if (!found)
>      error (_("No skiplist entries found with number %s."), arg);
>  }
>  
> -/* Add the given skiplist entry to our list, and set the entry's number.  */
> -
> -static void
> -add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
> +bool
> +skiplist_entry::do_skip_file_p (const symtab_and_line &function_sal)
>  {
> -  struct skiplist_entry *e1;
> -
> -  e->number = ++skiplist_entry_count;
> -
> -  /* Add to the end of the chain so that the list of
> -     skiplist entries will be in numerical order.  */
> -
> -  e1 = skiplist_entry_chain;
> -  if (e1 == NULL)
> -    skiplist_entry_chain = e.release ();
> -  else
> -    {
> -      while (e1->next)
> -	e1 = e1->next;
> -      e1->next = e.release ();
> -    }
> -}
> -
> -/* Return non-zero if we're stopped at a file to be skipped.  */
> -
> -static int
> -skip_file_p (struct skiplist_entry *e,
> -	     const struct symtab_and_line *function_sal)
> -{
> -  gdb_assert (!e->file.empty () && !e->file_is_glob);
> -
> -  if (function_sal->symtab == NULL)
> -    return 0;
> -
>    /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
>       symtab_to_fullname as it may contain "./" etc.  */
> -  if (compare_filenames_for_search (function_sal->symtab->filename,
> -				    e->file.c_str ()))
> -    return 1;
> +  if (compare_filenames_for_search (function_sal.symtab->filename,
> +				    m_file.c_str ()))
> +    return true;
>  
>    /* Before we invoke realpath, which can get expensive when many
>       files are involved, do a quick comparison of the basenames.  */
>    if (!basenames_may_differ
> -      && filename_cmp (lbasename (function_sal->symtab->filename),
> -		       lbasename (e->file.c_str ())) != 0)
> -    return 0;
> +      && filename_cmp (lbasename (function_sal.symtab->filename),
> +		       lbasename (m_file.c_str ())) != 0)
> +    return false;
>  
>    /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
>    {
> -    const char *fullname = symtab_to_fullname (function_sal->symtab);
> +    const char *fullname = symtab_to_fullname (function_sal.symtab);
>  
> -    if (compare_filenames_for_search (fullname, e->file.c_str ()))
> -      return 1;
> +    if (compare_filenames_for_search (fullname, m_file.c_str ()))
> +      return true;
>    }
>  
> -  return 0;
> +  return false;
>  }
>  
> -/* Return non-zero if we're stopped at a globbed file to be skipped.  */
> -
> -static int
> -skip_gfile_p (struct skiplist_entry *e,
> -	      const struct symtab_and_line *function_sal)
> +bool
> +skiplist_entry::do_skip_gfile_p (const symtab_and_line &function_sal)
>  {
> -  gdb_assert (!e->file.empty () && e->file_is_glob);
> -
> -  if (function_sal->symtab == NULL)
> -    return 0;
> -
>    /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
>       symtab_to_fullname as it may contain "./" etc.  */
> -  if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
> +  if (gdb_filename_fnmatch (m_file.c_str (), function_sal.symtab->filename,
>  			    FNM_FILE_NAME | FNM_NOESCAPE) == 0)
> -    return 1;
> +    return true;
>  
>    /* Before we invoke symtab_to_fullname, which is expensive, do a quick
>       comparison of the basenames.
> @@ -533,101 +514,83 @@ skip_gfile_p (struct skiplist_entry *e,
>       If the basename of the glob pattern is something like "*.c" then this
>       isn't much of a win.  Oh well.  */
>    if (!basenames_may_differ
> -      && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
> -			       lbasename (function_sal->symtab->filename),
> +      && gdb_filename_fnmatch (lbasename (m_file.c_str ()),
> +			       lbasename (function_sal.symtab->filename),
>  			       FNM_FILE_NAME | FNM_NOESCAPE) != 0)
> -    return 0;
> +    return false;
>  
>    /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
>    {
> -    const char *fullname = symtab_to_fullname (function_sal->symtab);
> +    const char *fullname = symtab_to_fullname (function_sal.symtab);
>  
> -    if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
> -      return 1;
> +    if (compare_glob_filenames_for_search (fullname, m_file.c_str ()))
> +      return true;
>    }
>  
> -  return 0;
> +  return false;
>  }
>  
> -/* Return non-zero if we're stopped at a function to be skipped.  */
> -
> -static int
> -skip_function_p (struct skiplist_entry *e, const char *function_name)
> +bool
> +skiplist_entry::skip_file_p (const symtab_and_line &function_sal)
>  {
> -  gdb_assert (!e->function.empty () && !e->function_is_regexp);
> -  return strcmp_iw (function_name, e->function.c_str ()) == 0;
> -}
> +  if (m_file.empty ())
> +    return false;
>  
> -/* Return non-zero if we're stopped at a function regexp to be skipped.  */
> +  if (function_sal.symtab == NULL)
> +    return false;
>  
> -static int
> -skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
> +  if (m_file_is_glob)
> +    return do_skip_gfile_p (function_sal);
> +  else
> +    return do_skip_file_p (function_sal);
> +}
> +
> +bool
> +skiplist_entry::skip_function_p (const char *function_name)
>  {
> -  gdb_assert (!e->function.empty () && e->function_is_regexp
> -	      && e->compiled_function_regexp);
> -  return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
> -	  == 0);
> +  if (m_function.empty ())
> +    return false;
> +
> +  if (m_function_is_regexp)
> +    {
> +      gdb_assert (m_compiled_function_regexp);
> +      return (m_compiled_function_regexp->exec (function_name, 0, NULL, 0)
> +	      == 0);
> +    }
> +  else
> +    return strcmp_iw (function_name, m_function.c_str ()) == 0;
>  }
>  
>  /* See skip.h.  */
>  
> -int
> +bool
>  function_name_is_marked_for_skip (const char *function_name,
> -				  const struct symtab_and_line *function_sal)
> +				  const symtab_and_line &function_sal)
>  {
> -  struct skiplist_entry *e;
> -
>    if (function_name == NULL)
> -    return 0;
> +    return false;
>  
> -  ALL_SKIPLIST_ENTRIES (e)
> +  for (const skiplist_entry_up &e : skiplist_entries)
>      {
> -      int skip_by_file = 0;
> -      int skip_by_function = 0;
> -
> -      if (!e->enabled)
> +      if (!e->enabled ())
>  	continue;
>  
> -      if (!e->file.empty ())
> -	{
> -	  if (e->file_is_glob)
> -	    {
> -	      if (skip_gfile_p (e, function_sal))
> -		skip_by_file = 1;
> -	    }
> -	  else
> -	    {
> -	      if (skip_file_p (e, function_sal))
> -		skip_by_file = 1;
> -	    }
> -	}
> -      if (!e->function.empty ())
> -	{
> -	  if (e->function_is_regexp)
> -	    {
> -	      if (skip_rfunction_p (e, function_name))
> -		skip_by_function = 1;
> -	    }
> -	  else
> -	    {
> -	      if (skip_function_p (e, function_name))
> -		skip_by_function = 1;
> -	    }
> -	}
> +      bool skip_by_file = e->skip_file_p (function_sal);
> +      bool skip_by_function = e->skip_function_p (function_name);
>  
>        /* If both file and function must match, make sure we don't errantly
>  	 exit if only one of them match.  */
> -      if (!e->file.empty () && !e->function.empty ())
> +      if (!e->file ().empty () && !e->function ().empty ())
>  	{
>  	  if (skip_by_file && skip_by_function)
> -	    return 1;
> +	    return true;
>  	}
>        /* Only one of file/function is specified.  */
>        else if (skip_by_file || skip_by_function)
> -	return 1;
> +	return true;
>      }
>  
> -  return 0;
> +  return false;
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> @@ -639,9 +602,6 @@ _initialize_step_skip (void)
>    static struct cmd_list_element *skiplist = NULL;
>    struct cmd_list_element *c;
>  
> -  skiplist_entry_chain = 0;
> -  skiplist_entry_count = 0;
> -
>    add_prefix_cmd ("skip", class_breakpoint, skip_command, _("\
>  Ignore a function while stepping.\n\
>  \n\
> diff --git a/gdb/skip.h b/gdb/skip.h
> index dbc92d8..4e1b544 100644
> --- a/gdb/skip.h
> +++ b/gdb/skip.h
> @@ -20,9 +20,9 @@
>  
>  struct symtab_and_line;
>  
> -/* Returns 1 if the given FUNCTION_NAME is marked for skip and shouldn't be
> -   stepped into.  Otherwise, returns 0.  */
> -int function_name_is_marked_for_skip (const char *function_name,
> -				    const struct symtab_and_line *function_sal);
> +/* Returns true if the given FUNCTION_NAME is marked for skip and
> +   shouldn't be stepped into.  Otherwise, returns false.  */
> +bool function_name_is_marked_for_skip (const char *function_name,
> +				       const symtab_and_line &function_sal);
>  
>  #endif /* !defined (SKIP_H) */
> 

Re: [PATCH] More gdb/skip.c C++ification

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> It's a bit of churn, but there's nothing really complicated here.
Pedro> It's just the next logical steps after Tom's patch.  See commit
Pedro> log below.

Pedro> WDYT?

It all seems good to me (though I only skimmed it).

Tom

Re: [PATCH] More gdb/skip.c C++ification

[Pedro Alves]

On 08/10/2017 10:10 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> It's a bit of churn, but there's nothing really complicated here.
> Pedro> It's just the next logical steps after Tom's patch.  See commit
> Pedro> log below.
> 
> Pedro> WDYT?
> 
> It all seems good to me (though I only skimmed it).
> 

Thanks, I pushed it in with the std::list<T*> -> std::list<T>
change.  Only downside is that I had to make the ctor public again (with
the private class key idiom), so that I could use std::list::emplace_back.
If we ever need to make skiplist_entry polymorphic, or need to worry about
performance, we could switch back to unique_ptr, or likely better use a
std::vector of pointers, for better locality / memory access patterns.

From de7985c3cca1358b21b49a9872455e2032f48ee3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 11 Aug 2017 12:11:28 +0100
Subject: [PATCH] More gdb/skip.c C++ification

- Make skiplist_entry a class with private data members.
- Move all construction logic to the ctor.
- Make skip_file_p etc be methods of skiplist_entry.
- Use std::list for the skip entries chain.  Make the list own its
  elements.
- Get rid of the ALL_SKIPLIST_ENTRIES/ALL_SKIPLIST_ENTRIES_SAFE
  macros, use range-for / iterators instead.
- function_name_is_marked_for_skip 'function_sal' argument must be
  non-NULL, so make it a reference instead.

All skiplist_entry invariants are now controlled by skiplist_entry
methods/internals.  Some gdb_asserts disappear for being redundant.

gdb/ChangeLog:
2017-08-11  Pedro Alves  <palves@redhat.com>

	* infrun.c (process_event_stop_test): Adjust
	function_name_is_marked_for_skip call.
	* skip.c: Include <list>.
	(skiplist_entry): Make it a class with private fields, and
	getters/setters.
	(skiplist_entry_chain): Delete.
	(skiplist_entries): New.
	(skiplist_entry_count): Delete.
	(highest_skiplist_entry_num): New.
	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
	(add_skiplist_entry): Delete.
	(skiplist_entry::skiplist_entry): New.
	(skiplist_entry::add_entry): New.
	(skip_file_command, skip_function): Adjust.
	(compile_skip_regexp): Delete.
	(skip_command): Don't compile regexp here.  Adjust to use
	skiplist_entry::add_entry.
	(skip_info): Adjust to use range-for and getters.
	(skip_enable_command, skip_disable_command): Adjust to use
	range-for and setters.
	(skip_delete_command): Adjust to use std::list.
	(add_skiplist_entry): Delete.
	(skip_file_p): Delete, refactored as ...
	(skiplist_entry::do_skip_file_p): ... this new method.
	(skip_gfile_p): Delete, refactored as ...
	(skiplist_entry::do_gskip_file_p): ... this new method.
	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
	(skiplist_entry::skip_function_p): ... this new method.
	(function_name_is_marked_for_skip): Now returns bool, and takes
	the function sal by const reference.  Adjust to use range-for and
	skiplist_entry methods.
	(_initialize_step_skip): Remove references to
	skiplist_entry_chain, skiplist_entry_count.
	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
	takes the function sal by const reference.
---
 gdb/ChangeLog |  38 ++++++
 gdb/infrun.c  |   2 +-
 gdb/skip.c    | 433 +++++++++++++++++++++++++++-------------------------------
 gdb/skip.h    |   8 +-
 4 files changed, 243 insertions(+), 238 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 75a1ea8..c588291 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,41 @@
+2017-08-11  Pedro Alves  <palves@redhat.com>
+
+	* infrun.c (process_event_stop_test): Adjust
+	function_name_is_marked_for_skip call.
+	* skip.c: Include <list>.
+	(skiplist_entry): Make it a class with private fields, and
+	getters/setters.
+	(skiplist_entry_chain): Delete.
+	(skiplist_entries): New.
+	(skiplist_entry_count): Delete.
+	(highest_skiplist_entry_num): New.
+	(ALL_SKIPLIST_ENTRIES, ALL_SKIPLIST_ENTRIES_SAFE): Delete.
+	(add_skiplist_entry): Delete.
+	(skiplist_entry::skiplist_entry): New.
+	(skiplist_entry::add_entry): New.
+	(skip_file_command, skip_function): Adjust.
+	(compile_skip_regexp): Delete.
+	(skip_command): Don't compile regexp here.  Adjust to use
+	skiplist_entry::add_entry.
+	(skip_info): Adjust to use range-for and getters.
+	(skip_enable_command, skip_disable_command): Adjust to use
+	range-for and setters.
+	(skip_delete_command): Adjust to use std::list.
+	(add_skiplist_entry): Delete.
+	(skip_file_p): Delete, refactored as ...
+	(skiplist_entry::do_skip_file_p): ... this new method.
+	(skip_gfile_p): Delete, refactored as ...
+	(skiplist_entry::do_gskip_file_p): ... this new method.
+	(skip_function_p, skip_rfunction_p): Delete, refactored as ...
+	(skiplist_entry::skip_function_p): ... this new method.
+	(function_name_is_marked_for_skip): Now returns bool, and takes
+	the function sal by const reference.  Adjust to use range-for and
+	skiplist_entry methods.
+	(_initialize_step_skip): Remove references to
+	skiplist_entry_chain, skiplist_entry_count.
+	* skip.h (function_name_is_marked_for_skip): Now returns bool, and
+	takes the function sal by const reference.
+
 2017-08-11  Yao Qi  <yao.qi@linaro.org>
 
 	* dwarf2-frame.c (clear_pointer_cleanup): Remove.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8f966e2..d6723fd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6788,7 +6788,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  &tmp_sal))
+						  tmp_sal))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
diff --git a/gdb/skip.c b/gdb/skip.c
index 6ab6c91..9f77248 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -35,74 +35,129 @@
 #include "fnmatch.h"
 #include "gdb_regex.h"
 #include "common/gdb_optional.h"
+#include <list>
 
-struct skiplist_entry
+class skiplist_entry
 {
-  skiplist_entry (bool file_is_glob_, std::string &&file_,
-		  bool function_is_regexp_, std::string &&function_)
-    : number (-1),
-      file_is_glob (file_is_glob_),
-      file (std::move (file_)),
-      function_is_regexp (function_is_regexp_),
-      function (std::move (function_)),
-      enabled (true),
-      next (NULL)
-  {
-  }
-
-  int number;
+public:
+  /* Create a skiplist_entry object and add it to the chain.  */
+  static void add_entry (bool file_is_glob,
+			 std::string &&file,
+			 bool function_is_regexp,
+			 std::string &&function);
+
+  /* Return true if the skip entry has a file or glob-style file
+     pattern that matches FUNCTION_SAL.  */
+  bool skip_file_p (const symtab_and_line &function_sal) const;
+
+  /* Return true if the skip entry has a function or function regexp
+     that matches FUNCTION_NAME.  */
+  bool skip_function_p (const char *function_name) const;
+
+  /* Getters.  */
+  int number () const { return m_number; };
+  bool enabled () const { return m_enabled; };
+  bool file_is_glob () const { return m_file_is_glob; }
+  const std::string &file () const { return m_file; }
+  const std::string &function () const { return m_function; }
+  bool function_is_regexp () const { return m_function_is_regexp; }
+
+  /* Setters.  */
+  void enable () { m_enabled = true; };
+  void disable () { m_enabled = false; };
+
+  /* Disable copy.  */
+  skiplist_entry (const skiplist_entry &) = delete;
+  void operator= (const skiplist_entry &) = delete;
+
+private:
+  /* Key that grants access to the constructor.  */
+  struct private_key {};
+public:
+  /* Public so we can construct with container::emplace_back.  Since
+     it requires a private class key, it can't be called from outside.
+     Use the add_entry static factory method to construct instead.  */
+  skiplist_entry (bool file_is_glob, std::string &&file,
+		  bool function_is_regexp, std::string &&function,
+		  private_key);
+
+private:
+  /* Return true if we're stopped at a file to be skipped.  */
+  bool do_skip_file_p (const symtab_and_line &function_sal) const;
+
+  /* Return true if we're stopped at a globbed file to be skipped.  */
+  bool do_skip_gfile_p (const symtab_and_line &function_sal) const;
+
+private: /* data */
+  int m_number = -1;
 
   /* True if FILE is a glob-style pattern.
      Otherwise it is the plain file name (possibly with directories).  */
-  bool file_is_glob;
+  bool m_file_is_glob;
 
   /* The name of the file or empty if no name.  */
-  std::string file;
+  std::string m_file;
 
   /* True if FUNCTION is a regexp.
      Otherwise it is a plain function name (possibly with arguments,
      for C++).  */
-  bool function_is_regexp;
+  bool m_function_is_regexp;
 
   /* The name of the function or empty if no name.  */
-  std::string function;
+  std::string m_function;
 
   /* If this is a function regexp, the compiled form.  */
-  gdb::optional<compiled_regex> compiled_function_regexp;
+  gdb::optional<compiled_regex> m_compiled_function_regexp;
 
-  bool enabled;
-
-  struct skiplist_entry *next;
+  /* Enabled/disabled state.  */
+  bool m_enabled = true;
 };
 
-static void add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e);
+static std::list<skiplist_entry> skiplist_entries;
+static int highest_skiplist_entry_num = 0;
+
+skiplist_entry::skiplist_entry (bool file_is_glob,
+				std::string &&file,
+				bool function_is_regexp,
+				std::string &&function,
+				private_key)
+  : m_file_is_glob (file_is_glob),
+    m_file (std::move (file)),
+    m_function_is_regexp (function_is_regexp),
+    m_function (std::move (function))
+{
+  gdb_assert (!m_file.empty () || !m_function.empty ());
 
-static struct skiplist_entry *skiplist_entry_chain;
-static int skiplist_entry_count;
+  if (m_file_is_glob)
+    gdb_assert (!m_file.empty ());
 
-#define ALL_SKIPLIST_ENTRIES(E) \
-  for (E = skiplist_entry_chain; E; E = E->next)
+  if (m_function_is_regexp)
+    {
+      gdb_assert (!m_function.empty ());
 
-#define ALL_SKIPLIST_ENTRIES_SAFE(E,TMP) \
-  for (E = skiplist_entry_chain;         \
-       E ? (TMP = E->next, 1) : 0;       \
-       E = TMP)
+      int flags = REG_NOSUB;
+#ifdef REG_EXTENDED
+      flags |= REG_EXTENDED;
+#endif
 
-/* Create a skip object.  */
+      gdb_assert (!m_function.empty ());
+      m_compiled_function_regexp.emplace (m_function.c_str (), flags,
+					  _("regexp"));
+    }
+}
 
-static std::unique_ptr<skiplist_entry>
-make_skip_entry (bool file_is_glob, std::string &&file,
-		 bool function_is_regexp, std::string &&function)
+void
+skiplist_entry::add_entry (bool file_is_glob, std::string &&file,
+			   bool function_is_regexp, std::string &&function)
 {
-  gdb_assert (!file.empty () || !function.empty ());
-  if (file_is_glob)
-    gdb_assert (!file.empty ());
-  if (function_is_regexp)
-    gdb_assert (!function.empty ());
-
-  return std::unique_ptr<skiplist_entry>
-    (new skiplist_entry (file_is_glob, std::move (file),
-			 function_is_regexp, std::move (function)));
+  skiplist_entries.emplace_back (file_is_glob,
+				 std::move (file),
+				 function_is_regexp,
+				 std::move (function),
+				 private_key {});
+
+  /* Incremented after push_back, in case push_back throws.  */
+  skiplist_entries.back ().m_number = ++highest_skiplist_entry_num;
 }
 
 static void
@@ -126,8 +181,8 @@ skip_file_command (char *arg, int from_tty)
   else
     filename = arg;
 
-  add_skiplist_entry (make_skip_entry (false, std::string (filename), false,
-				       std::string ()));
+  skiplist_entry::add_entry (false, std::string (filename),
+			     false, std::string ());
 
   printf_filtered (_("File %s will be skipped when stepping.\n"), filename);
 }
@@ -138,8 +193,7 @@ skip_file_command (char *arg, int from_tty)
 static void
 skip_function (const char *name)
 {
-  add_skiplist_entry (make_skip_entry (false, std::string (), false,
-				       std::string (name)));
+  skiplist_entry::add_entry (false, std::string (), false, std::string (name));
 
   printf_filtered (_("Function %s will be skipped when stepping.\n"), name);
 }
@@ -169,23 +223,6 @@ skip_function_command (char *arg, int from_tty)
   skip_function (arg);
 }
 
-/* Compile the regexp in E.
-   An error is thrown if there's an error.
-   MESSAGE is used as a prefix of the error message.  */
-
-static void
-compile_skip_regexp (struct skiplist_entry *e, const char *message)
-{
-  int flags = REG_NOSUB;
-
-#ifdef REG_EXTENDED
-  flags |= REG_EXTENDED;
-#endif
-
-  gdb_assert (e->function_is_regexp && !e->function.empty ());
-  e->compiled_function_regexp.emplace (e->function.c_str (), flags, message);
-}
-
 /* Process "skip ..." that does not match "skip file" or "skip function".  */
 
 static void
@@ -273,18 +310,15 @@ skip_command (char *arg, int from_tty)
     entry_file = file;
   else if (gfile != NULL)
     entry_file = gfile;
+
   std::string entry_function;
   if (function != NULL)
     entry_function = function;
   else if (rfunction != NULL)
     entry_function = rfunction;
-  std::unique_ptr<skiplist_entry> e
-    = make_skip_entry (gfile != NULL, std::move (entry_file),
-		       rfunction != NULL, std::move (entry_function));
-  if (rfunction != NULL)
-    compile_skip_regexp (e.get (), _("regexp"));
 
-  add_skiplist_entry (std::move (e));
+  skiplist_entry::add_entry (gfile != NULL, std::move (entry_file),
+			     rfunction != NULL, std::move (entry_function));
 
   /* I18N concerns drive some of the choices here (we can't piece together
      the output too much).  OTOH we want to keep this simple.  Therefore the
@@ -321,7 +355,6 @@ skip_command (char *arg, int from_tty)
 static void
 skip_info (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
   int num_printable_entries = 0;
   struct value_print_options opts;
 
@@ -329,8 +362,8 @@ skip_info (char *arg, int from_tty)
 
   /* Count the number of rows in the table and see if we need space for a
      64-bit address anywhere.  */
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (const skiplist_entry &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e.number ()))
       num_printable_entries++;
 
   if (num_printable_entries == 0)
@@ -355,37 +388,36 @@ skip_info (char *arg, int from_tty)
   current_uiout->table_header (40, ui_noalign, "function", "Function"); /* 6 */
   current_uiout->table_body ();
 
-  ALL_SKIPLIST_ENTRIES (e)
+  for (const skiplist_entry &e : skiplist_entries)
     {
-
       QUIT;
-      if (arg != NULL && !number_is_in_list (arg, e->number))
+      if (arg != NULL && !number_is_in_list (arg, e.number ()))
 	continue;
 
       ui_out_emit_tuple tuple_emitter (current_uiout, "blklst-entry");
-      current_uiout->field_int ("number", e->number); /* 1 */
+      current_uiout->field_int ("number", e.number ()); /* 1 */
 
-      if (e->enabled)
+      if (e.enabled ())
 	current_uiout->field_string ("enabled", "y"); /* 2 */
       else
 	current_uiout->field_string ("enabled", "n"); /* 2 */
 
-      if (e->file_is_glob)
+      if (e.file_is_glob ())
 	current_uiout->field_string ("regexp", "y"); /* 3 */
       else
 	current_uiout->field_string ("regexp", "n"); /* 3 */
 
       current_uiout->field_string ("file",
-				   e->file.empty () ? "<none>"
-				   : e->file.c_str ()); /* 4 */
-      if (e->function_is_regexp)
+				   e.file ().empty () ? "<none>"
+				   : e.file ().c_str ()); /* 4 */
+      if (e.function_is_regexp ())
 	current_uiout->field_string ("regexp", "y"); /* 5 */
       else
 	current_uiout->field_string ("regexp", "n"); /* 5 */
 
       current_uiout->field_string ("function",
-				   e->function.empty () ? "<none>"
-				   : e->function.c_str ()); /* 6 */
+				   e.function ().empty () ? "<none>"
+				   : e.function ().c_str ()); /* 6 */
 
       current_uiout->text ("\n");
     }
@@ -394,14 +426,13 @@ skip_info (char *arg, int from_tty)
 static void
 skip_enable_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
-  int found = 0;
+  bool found = false;
 
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (skiplist_entry &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e.number ()))
       {
-        e->enabled = true;
-        found = 1;
+	e.enable ();
+	found = true;
       }
 
   if (!found)
@@ -411,14 +442,13 @@ skip_enable_command (char *arg, int from_tty)
 static void
 skip_disable_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e;
-  int found = 0;
+  bool found = false;
 
-  ALL_SKIPLIST_ENTRIES (e)
-    if (arg == NULL || number_is_in_list (arg, e->number))
+  for (skiplist_entry &e : skiplist_entries)
+    if (arg == NULL || number_is_in_list (arg, e.number ()))
       {
-	e->enabled = false;
-        found = 1;
+	e.disable ();
+	found = true;
       }
 
   if (!found)
@@ -428,104 +458,62 @@ skip_disable_command (char *arg, int from_tty)
 static void
 skip_delete_command (char *arg, int from_tty)
 {
-  struct skiplist_entry *e, *temp, *b_prev;
-  int found = 0;
+  bool found = false;
 
-  b_prev = 0;
-  ALL_SKIPLIST_ENTRIES_SAFE (e, temp)
-    if (arg == NULL || number_is_in_list (arg, e->number))
-      {
-	if (b_prev != NULL)
-	  b_prev->next = e->next;
-	else
-	  skiplist_entry_chain = e->next;
+  for (auto it = skiplist_entries.begin (),
+	 end = skiplist_entries.end ();
+       it != end;)
+    {
+      const skiplist_entry &e = *it;
 
-	delete e;
-        found = 1;
-      }
-    else
-      {
-	b_prev = e;
-      }
+      if (arg == NULL || number_is_in_list (arg, e.number ()))
+	{
+	  it = skiplist_entries.erase (it);
+	  found = true;
+	}
+      else
+	++it;
+    }
 
   if (!found)
     error (_("No skiplist entries found with number %s."), arg);
 }
 
-/* Add the given skiplist entry to our list, and set the entry's number.  */
-
-static void
-add_skiplist_entry (std::unique_ptr<skiplist_entry> &&e)
-{
-  struct skiplist_entry *e1;
-
-  e->number = ++skiplist_entry_count;
-
-  /* Add to the end of the chain so that the list of
-     skiplist entries will be in numerical order.  */
-
-  e1 = skiplist_entry_chain;
-  if (e1 == NULL)
-    skiplist_entry_chain = e.release ();
-  else
-    {
-      while (e1->next)
-	e1 = e1->next;
-      e1->next = e.release ();
-    }
-}
-
-/* Return non-zero if we're stopped at a file to be skipped.  */
-
-static int
-skip_file_p (struct skiplist_entry *e,
-	     const struct symtab_and_line *function_sal)
+bool
+skiplist_entry::do_skip_file_p (const symtab_and_line &function_sal) const
 {
-  gdb_assert (!e->file.empty () && !e->file_is_glob);
-
-  if (function_sal->symtab == NULL)
-    return 0;
-
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (compare_filenames_for_search (function_sal->symtab->filename,
-				    e->file.c_str ()))
-    return 1;
+  if (compare_filenames_for_search (function_sal.symtab->filename,
+				    m_file.c_str ()))
+    return true;
 
   /* Before we invoke realpath, which can get expensive when many
      files are involved, do a quick comparison of the basenames.  */
   if (!basenames_may_differ
-      && filename_cmp (lbasename (function_sal->symtab->filename),
-		       lbasename (e->file.c_str ())) != 0)
-    return 0;
+      && filename_cmp (lbasename (function_sal.symtab->filename),
+		       lbasename (m_file.c_str ())) != 0)
+    return false;
 
   /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
   {
-    const char *fullname = symtab_to_fullname (function_sal->symtab);
+    const char *fullname = symtab_to_fullname (function_sal.symtab);
 
-    if (compare_filenames_for_search (fullname, e->file.c_str ()))
-      return 1;
+    if (compare_filenames_for_search (fullname, m_file.c_str ()))
+      return true;
   }
 
-  return 0;
+  return false;
 }
 
-/* Return non-zero if we're stopped at a globbed file to be skipped.  */
-
-static int
-skip_gfile_p (struct skiplist_entry *e,
-	      const struct symtab_and_line *function_sal)
+bool
+skiplist_entry::do_skip_gfile_p (const symtab_and_line &function_sal) const
 {
-  gdb_assert (!e->file.empty () && e->file_is_glob);
-
-  if (function_sal->symtab == NULL)
-    return 0;
-
   /* Check first sole SYMTAB->FILENAME.  It may not be a substring of
      symtab_to_fullname as it may contain "./" etc.  */
-  if (gdb_filename_fnmatch (e->file.c_str (), function_sal->symtab->filename,
+  if (gdb_filename_fnmatch (m_file.c_str (), function_sal.symtab->filename,
 			    FNM_FILE_NAME | FNM_NOESCAPE) == 0)
-    return 1;
+    return true;
 
   /* Before we invoke symtab_to_fullname, which is expensive, do a quick
      comparison of the basenames.
@@ -533,101 +521,83 @@ skip_gfile_p (struct skiplist_entry *e,
      If the basename of the glob pattern is something like "*.c" then this
      isn't much of a win.  Oh well.  */
   if (!basenames_may_differ
-      && gdb_filename_fnmatch (lbasename (e->file.c_str ()),
-			       lbasename (function_sal->symtab->filename),
+      && gdb_filename_fnmatch (lbasename (m_file.c_str ()),
+			       lbasename (function_sal.symtab->filename),
 			       FNM_FILE_NAME | FNM_NOESCAPE) != 0)
-    return 0;
+    return false;
 
   /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
   {
-    const char *fullname = symtab_to_fullname (function_sal->symtab);
+    const char *fullname = symtab_to_fullname (function_sal.symtab);
 
-    if (compare_glob_filenames_for_search (fullname, e->file.c_str ()))
-      return 1;
+    if (compare_glob_filenames_for_search (fullname, m_file.c_str ()))
+      return true;
   }
 
-  return 0;
+  return false;
 }
 
-/* Return non-zero if we're stopped at a function to be skipped.  */
-
-static int
-skip_function_p (struct skiplist_entry *e, const char *function_name)
+bool
+skiplist_entry::skip_file_p (const symtab_and_line &function_sal) const
 {
-  gdb_assert (!e->function.empty () && !e->function_is_regexp);
-  return strcmp_iw (function_name, e->function.c_str ()) == 0;
-}
+  if (m_file.empty ())
+    return false;
 
-/* Return non-zero if we're stopped at a function regexp to be skipped.  */
+  if (function_sal.symtab == NULL)
+    return false;
+
+  if (m_file_is_glob)
+    return do_skip_gfile_p (function_sal);
+  else
+    return do_skip_file_p (function_sal);
+}
 
-static int
-skip_rfunction_p (struct skiplist_entry *e, const char *function_name)
+bool
+skiplist_entry::skip_function_p (const char *function_name) const
 {
-  gdb_assert (!e->function.empty () && e->function_is_regexp
-	      && e->compiled_function_regexp);
-  return (e->compiled_function_regexp->exec (function_name, 0, NULL, 0)
-	  == 0);
+  if (m_function.empty ())
+    return false;
+
+  if (m_function_is_regexp)
+    {
+      gdb_assert (m_compiled_function_regexp);
+      return (m_compiled_function_regexp->exec (function_name, 0, NULL, 0)
+	      == 0);
+    }
+  else
+    return strcmp_iw (function_name, m_function.c_str ()) == 0;
 }
 
 /* See skip.h.  */
 
-int
+bool
 function_name_is_marked_for_skip (const char *function_name,
-				  const struct symtab_and_line *function_sal)
+				  const symtab_and_line &function_sal)
 {
-  struct skiplist_entry *e;
-
   if (function_name == NULL)
-    return 0;
+    return false;
 
-  ALL_SKIPLIST_ENTRIES (e)
+  for (const skiplist_entry &e : skiplist_entries)
     {
-      int skip_by_file = 0;
-      int skip_by_function = 0;
-
-      if (!e->enabled)
+      if (!e.enabled ())
 	continue;
 
-      if (!e->file.empty ())
-	{
-	  if (e->file_is_glob)
-	    {
-	      if (skip_gfile_p (e, function_sal))
-		skip_by_file = 1;
-	    }
-	  else
-	    {
-	      if (skip_file_p (e, function_sal))
-		skip_by_file = 1;
-	    }
-	}
-      if (!e->function.empty ())
-	{
-	  if (e->function_is_regexp)
-	    {
-	      if (skip_rfunction_p (e, function_name))
-		skip_by_function = 1;
-	    }
-	  else
-	    {
-	      if (skip_function_p (e, function_name))
-		skip_by_function = 1;
-	    }
-	}
+      bool skip_by_file = e.skip_file_p (function_sal);
+      bool skip_by_function = e.skip_function_p (function_name);
 
       /* If both file and function must match, make sure we don't errantly
 	 exit if only one of them match.  */
-      if (!e->file.empty () && !e->function.empty ())
+      if (!e.file ().empty () && !e.function ().empty ())
 	{
 	  if (skip_by_file && skip_by_function)
-	    return 1;
+	    return true;
 	}
       /* Only one of file/function is specified.  */
       else if (skip_by_file || skip_by_function)
-	return 1;
+	return true;
     }
 
-  return 0;
+  return false;
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -639,9 +609,6 @@ _initialize_step_skip (void)
   static struct cmd_list_element *skiplist = NULL;
   struct cmd_list_element *c;
 
-  skiplist_entry_chain = 0;
-  skiplist_entry_count = 0;
-
   add_prefix_cmd ("skip", class_breakpoint, skip_command, _("\
 Ignore a function while stepping.\n\
 \n\
diff --git a/gdb/skip.h b/gdb/skip.h
index dbc92d8..4e1b544 100644
--- a/gdb/skip.h
+++ b/gdb/skip.h
@@ -20,9 +20,9 @@
 
 struct symtab_and_line;
 
-/* Returns 1 if the given FUNCTION_NAME is marked for skip and shouldn't be
-   stepped into.  Otherwise, returns 0.  */
-int function_name_is_marked_for_skip (const char *function_name,
-				    const struct symtab_and_line *function_sal);
+/* Returns true if the given FUNCTION_NAME is marked for skip and
+   shouldn't be stepped into.  Otherwise, returns false.  */
+bool function_name_is_marked_for_skip (const char *function_name,
+				       const symtab_and_line &function_sal);
 
 #endif /* !defined (SKIP_H) */
-- 
2.5.5