Patchwork More gdb/skip.c C++ification

login
register
mail settings
Submitter Pedro Alves
Date Aug. 10, 2017, 7:34 p.m.
Message ID <57ff34fe-bafd-8b54-b7a2-eb675241da63@redhat.com>
Download mbox | patch
Permalink /patch/22064/
State New
Headers show

Comments

Pedro Alves - Aug. 10, 2017, 7:34 p.m.
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(-)
Pedro Alves - Aug. 10, 2017, 7:51 p.m.
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) */
>
Tom Tromey - Aug. 10, 2017, 9:10 p.m.
>>>>> "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

Patch

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) */