[PATCH 0/2] C++-ify print_variable_and_value_data

[PATCH 0/2] C++-ify print_variable_and_value_data

[Tom Tromey]

This series C++-ifies print_variable_and_value_data.  The first patch
is just a refactoring to make this more possible.

Regression tested on x86-64 Fedora 43.

Signed-off-by: Tom Tromey <tromey@adacore.com>
---
Tom Tromey (2):
      Allow move of compiled_regex
      C++-ify print_variable_and_value_data

 gdb/stack.c             | 70 +++++++++++++++++++++++--------------------------
 gdbsupport/gdb_regex.cc | 13 +++++++--
 gdbsupport/gdb_regex.h  | 30 ++++++++++++++++++++-
 3 files changed, 73 insertions(+), 40 deletions(-)
---
base-commit: 989ac09520f7d421a6be515ea6b346c597111eab
change-id: 20260424-print-var-and-cleanup-d12345dc86e8

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

[PATCH 1/2] Allow move of compiled_regex

[Tom Tromey]

This patch makes it possible to move a compiled_regex.
---
 gdbsupport/gdb_regex.cc | 13 +++++++++++--
 gdbsupport/gdb_regex.h  | 30 +++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/gdbsupport/gdb_regex.cc b/gdbsupport/gdb_regex.cc
index 111e5ff24a8..6774aa4c805 100644
--- a/gdbsupport/gdb_regex.cc
+++ b/gdbsupport/gdb_regex.cc
@@ -33,17 +33,25 @@ compiled_regex::compiled_regex (const char *regex, int cflags,
       regerror (code, &m_pattern, err.data (), length);
       error (("%s: %s"), message, err.data ());
     }
+
+  m_valid = true;
 }
 
-compiled_regex::~compiled_regex ()
+void
+compiled_regex::clear ()
 {
-  regfree (&m_pattern);
+  if (m_valid)
+    {
+      regfree (&m_pattern);
+      m_valid = false;
+    }
 }
 
 int
 compiled_regex::exec (const char *string, size_t nmatch,
 		      regmatch_t pmatch[], int eflags) const
 {
+  gdb_assert (m_valid);
   return regexec (&m_pattern, string, nmatch, pmatch, eflags);
 }
 
@@ -52,5 +60,6 @@ compiled_regex::search (const char *string,
 			int length, int start, int range,
 			struct re_registers *regs)
 {
+  gdb_assert (m_valid);
   return re_search (&m_pattern, string, length, start, range, regs);
 }
diff --git a/gdbsupport/gdb_regex.h b/gdbsupport/gdb_regex.h
index 63fd9c32119..f7d3190839f 100644
--- a/gdbsupport/gdb_regex.h
+++ b/gdbsupport/gdb_regex.h
@@ -35,10 +35,32 @@ class compiled_regex
 		  const char *message)
     ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (4);
 
-  ~compiled_regex ();
+  ~compiled_regex ()
+  {
+    clear ();
+  }
 
   DISABLE_COPY_AND_ASSIGN (compiled_regex);
 
+  compiled_regex (compiled_regex &&other)
+    : m_valid (other.m_valid),
+      m_pattern (other.m_pattern)
+  {
+    other.m_valid = false;
+  }
+
+  compiled_regex &operator= (compiled_regex &&other)
+  {
+    if (&other != this)
+      {
+	clear ();
+	m_valid = other.m_valid;
+	m_pattern = other.m_pattern;
+	other.m_valid = false;
+      }
+    return *this;
+  }
+
   /* Wrapper around ::regexec.  */
   int exec (const char *string,
 	    size_t nmatch, regmatch_t pmatch[],
@@ -50,6 +72,12 @@ class compiled_regex
 	      int range, struct re_registers *regs);
 
 private:
+  /* Free the compiled pattern, if necessary.  */
+  void clear ();
+
+  /* True if the compiled pattern is valid.  */
+  bool m_valid = false;
+
   /* The compiled pattern.  */
   regex_t m_pattern;
 };

-- 
2.53.0

Re: [PATCH 1/2] Allow move of compiled_regex

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

> This patch makes it possible to move a compiled_regex.
> ---
>  gdbsupport/gdb_regex.cc | 13 +++++++++++--
>  gdbsupport/gdb_regex.h  | 30 +++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/gdbsupport/gdb_regex.cc b/gdbsupport/gdb_regex.cc
> index 111e5ff24a8..6774aa4c805 100644
> --- a/gdbsupport/gdb_regex.cc
> +++ b/gdbsupport/gdb_regex.cc
> @@ -33,17 +33,25 @@ compiled_regex::compiled_regex (const char *regex, int cflags,
>        regerror (code, &m_pattern, err.data (), length);
>        error (("%s: %s"), message, err.data ());
>      }
> +
> +  m_valid = true;
>  }
>  
> -compiled_regex::~compiled_regex ()
> +void
> +compiled_regex::clear ()
>  {
> -  regfree (&m_pattern);
> +  if (m_valid)
> +    {
> +      regfree (&m_pattern);
> +      m_valid = false;
> +    }
>  }
>  
>  int
>  compiled_regex::exec (const char *string, size_t nmatch,
>  		      regmatch_t pmatch[], int eflags) const
>  {
> +  gdb_assert (m_valid);
>    return regexec (&m_pattern, string, nmatch, pmatch, eflags);
>  }
>  
> @@ -52,5 +60,6 @@ compiled_regex::search (const char *string,
>  			int length, int start, int range,
>  			struct re_registers *regs)
>  {
> +  gdb_assert (m_valid);
>    return re_search (&m_pattern, string, length, start, range, regs);
>  }
> diff --git a/gdbsupport/gdb_regex.h b/gdbsupport/gdb_regex.h
> index 63fd9c32119..f7d3190839f 100644
> --- a/gdbsupport/gdb_regex.h
> +++ b/gdbsupport/gdb_regex.h
> @@ -35,10 +35,32 @@ class compiled_regex

I think the comment above the compiled_regex class needs to be updated.
It currently says:

  /* A compiled regex.  This is mainly a wrapper around regex_t.  The
     the constructor throws on regcomp error and the destructor is
     responsible for calling regfree.  The former means that it's not
     possible to create an instance of compiled_regex that isn't
     compiled, hence the name.  */

[ NOTE: There's a pre-existing typo: "The the constructor throws ...". ]

The sentence: "The former means that it's not possible to create an
instance of compiled_regex that isn't compiled, hence the name." is now
out of date I think.  Previously, if I did:

  compiled_regex re ( ... args ...);

then RE was either (a) valid, or (b) an exception was thrown.  However,
this is no longer the case:

  compiled_regex re ( ... args ...);
  compiled_regex re2 (std::move (re));
  /* Now RE is in a non-compiled state.  */

So the comment needs to be updated to indicate that a compiled_regexp is
no longer always compiled.

With a suitable comment in place:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>  		  const char *message)
>      ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (4);
>  
> -  ~compiled_regex ();
> +  ~compiled_regex ()
> +  {
> +    clear ();
> +  }
>  
>    DISABLE_COPY_AND_ASSIGN (compiled_regex);
>  
> +  compiled_regex (compiled_regex &&other)
> +    : m_valid (other.m_valid),
> +      m_pattern (other.m_pattern)
> +  {
> +    other.m_valid = false;
> +  }
> +
> +  compiled_regex &operator= (compiled_regex &&other)
> +  {
> +    if (&other != this)
> +      {
> +	clear ();
> +	m_valid = other.m_valid;
> +	m_pattern = other.m_pattern;
> +	other.m_valid = false;
> +      }
> +    return *this;
> +  }
> +
>    /* Wrapper around ::regexec.  */
>    int exec (const char *string,
>  	    size_t nmatch, regmatch_t pmatch[],
> @@ -50,6 +72,12 @@ class compiled_regex
>  	      int range, struct re_registers *regs);
>  
>  private:
> +  /* Free the compiled pattern, if necessary.  */
> +  void clear ();
> +
> +  /* True if the compiled pattern is valid.  */
> +  bool m_valid = false;
> +
>    /* The compiled pattern.  */
>    regex_t m_pattern;
>  };
>
> -- 
> 2.53.0

[PATCH 2/2] C++-ify print_variable_and_value_data

[Tom Tromey]

While working on a different patch, I decided to C++-ify
print_variable_and_value_data somewhat.  This patch gives it a
constructor and changes it to use bool.

Also, I noticed that this code does not really need to use a frame_id.
Using a frame_info_ptr is just as good due to the "reinflation"
behavior of that class.
---
 gdb/stack.c | 70 +++++++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 6fcc26417e2..8fabf6bea83 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2249,14 +2249,31 @@ iterate_over_block_local_vars (const struct block *block,
 
 struct print_variable_and_value_data
 {
+  print_variable_and_value_data (const char *regexp,
+				 const char *t_regexp,
+				 const frame_info_ptr &frame,
+				 int num_tabs,
+				 ui_file *stream)
+    : preg (prepare_reg (regexp)),
+      treg (prepare_reg (t_regexp)),
+      frame (frame),
+      num_tabs (num_tabs),
+      stream (stream)
+  {
+  }
+
   std::optional<compiled_regex> preg;
   std::optional<compiled_regex> treg;
-  struct frame_id frame_id;
+  frame_info_ptr frame;
   int num_tabs;
-  struct ui_file *stream;
-  int values_printed;
+  ui_file *stream;
+  bool values_printed = false;
 
   void operator() (const char *print_name, struct symbol *sym);
+
+private:
+
+  std::optional<compiled_regex> prepare_reg (const char *regexp);
 };
 
 /* The callback for the locals and args iterators.  */
@@ -2265,8 +2282,6 @@ void
 print_variable_and_value_data::operator() (const char *print_name,
 					   struct symbol *sym)
 {
-  frame_info_ptr frame;
-
   if (preg.has_value ()
       && preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
     return;
@@ -2276,32 +2291,25 @@ print_variable_and_value_data::operator() (const char *print_name,
   if (language_def (sym->language ())->symbol_printing_suppressed (sym))
     return;
 
-  frame = frame_find_by_id (frame_id);
-  if (frame == NULL)
-    {
-      warning (_("Unable to restore previously selected frame."));
-      return;
-    }
-
   print_variable_and_value (print_name, sym, frame, stream, num_tabs);
 
-  values_printed = 1;
+  values_printed = true;
 }
 
-/* Prepares the regular expression REG from REGEXP.
-   If REGEXP is NULL, it results in an empty regular expression.  */
+/* Prepares a regular expression from REGEXP.  If REGEXP is NULL, it
+   results in an empty object.  */
 
-static void
-prepare_reg (const char *regexp, std::optional<compiled_regex> *reg)
+std::optional<compiled_regex>
+print_variable_and_value_data::prepare_reg (const char *regexp)
 {
-  if (regexp != NULL)
+  std::optional<compiled_regex> result;
+  if (regexp != nullptr)
     {
       int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
 				? REG_ICASE : 0);
-      reg->emplace (regexp, cflags, _("Invalid regexp"));
+      result.emplace (regexp, cflags, _("Invalid regexp"));
     }
-  else
-    reg->reset ();
+  return result;
 }
 
 /* Print all variables from the innermost up to the function block of FRAME.
@@ -2319,7 +2327,6 @@ print_frame_local_vars (const frame_info_ptr &frame,
 			const char *regexp, const char *t_regexp,
 			int num_tabs, struct ui_file *stream)
 {
-  struct print_variable_and_value_data cb_data;
   const struct block *block;
   std::optional<CORE_ADDR> pc;
 
@@ -2339,12 +2346,8 @@ print_frame_local_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.preg);
-  prepare_reg (t_regexp, &cb_data.treg);
-  cb_data.frame_id = get_frame_id (frame);
-  cb_data.num_tabs = 4 * num_tabs;
-  cb_data.stream = stream;
-  cb_data.values_printed = 0;
+  print_variable_and_value_data cb_data (regexp, t_regexp, frame,
+					 4 * num_tabs, stream);
 
   /* Temporarily change the selected frame to the given FRAME.
      This allows routines that rely on the selected frame instead
@@ -2481,8 +2484,6 @@ print_frame_arg_vars (const frame_info_ptr &frame,
 		      const char *regexp, const char *t_regexp,
 		      struct ui_file *stream)
 {
-  struct print_variable_and_value_data cb_data;
-  struct symbol *func;
   std::optional<CORE_ADDR> pc;
 
   if (!(pc = get_frame_pc_if_available (frame)))
@@ -2493,7 +2494,7 @@ print_frame_arg_vars (const frame_info_ptr &frame,
       return;
     }
 
-  func = get_frame_function (frame);
+  symbol *func = get_frame_function (frame);
   if (func == NULL)
     {
       if (!quiet)
@@ -2501,12 +2502,7 @@ print_frame_arg_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.preg);
-  prepare_reg (t_regexp, &cb_data.treg);
-  cb_data.frame_id = get_frame_id (frame);
-  cb_data.num_tabs = 0;
-  cb_data.stream = stream;
-  cb_data.values_printed = 0;
+  print_variable_and_value_data cb_data (regexp, t_regexp, frame, 0, stream);
 
   iterate_over_block_arg_vars (func->value_block (), cb_data);
 

-- 
2.53.0

Re: [PATCH 2/2] C++-ify print_variable_and_value_data

[Andrew Burgess]

Hi Tom,

This mostly looks fine, there's just one thing that I think I wanted to
discus...

Tom Tromey <tromey@adacore.com> writes:

> While working on a different patch, I decided to C++-ify
> print_variable_and_value_data somewhat.  This patch gives it a
> constructor and changes it to use bool.
>
> Also, I noticed that this code does not really need to use a frame_id.
> Using a frame_info_ptr is just as good due to the "reinflation"
> behavior of that class.

This is a super slim edge case, but I think it might be worth mentioning
at this point in the commit message.  I'm kind of aware of this right
now because I'm looking at some frame related problems, so I'm thinking
a log about frame_info_ptr, and reinflation...

> @@ -2276,32 +2291,25 @@ print_variable_and_value_data::operator() (const char *print_name,
>    if (language_def (sym->language ())->symbol_printing_suppressed (sym))
>      return;
>  
> -  frame = frame_find_by_id (frame_id);
> -  if (frame == NULL)
> -    {
> -      warning (_("Unable to restore previously selected frame."));
> -      return;
> -    }
> -

This code handles the case where we cannot re-find the frame for
FRAME_ID.  But if we look in frame_info_ptr::reinflate, there's no such
handling.  We just: gdb_assert (m_ptr != nullptr), in the above code it
would be like replacing the `if` block with gdb_assert (frame != NULL);

I think this is a bug in the reinflate logic which has existed since it
was added.

What would actually need to happen to trigger this bug?  In this case I
think something like a Python pretty printer would need to change the
inferior state in such a way that the backtrace changed, and then flush
the frame cache.

I don't think this is something a well written pretty printed should be
doing.  But it is possible, and GDB shouldn't be asserting if it did.

But also, I don't think this issue is limited to your code, so I don't
think this is something you need to fix.

But it might be worth mentioning in the commit message, that with this
change, this isn't a pure refactor, there is a small edge case which
changes behaviour.

Otherwise, this looks great.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew