[PATCH] gdb/tui: return std::string from tui_get_function_from_frame

[PATCH] gdb/tui: return std::string from tui_get_function_from_frame

[Andrew Burgess]

Update tui_get_function_from_frame to return a std::string rather than
a pointer into a static buffer.

The value returned from tui_get_function_from_frame is passed to
tui_location_tracker::set_location, which already stores the data in a
std::string; this just moves the string creation earlier.

I don't think there was anything particularly wrong with the old code,
but I'm not a huge fan of returning data in static buffers unless
there's a really good reason, and it doesn't feel like there's a
really good reason in this case.

The current approach in tui_get_function_from_frame is to call
print_address_symbolic, and then to pull the function name from the
result.  There is an argument that this approach could be improved,
but I've not done that in this commit, nor do I plan to do that any
time soon.  As such the new code should do exactly what the old code
did.

There should be no user visible changes after this commit.
---
 gdb/tui/tui-location.c |  6 ++----
 gdb/tui/tui-location.h |  2 +-
 gdb/tui/tui-status.c   | 33 ++++++++++++++++-----------------
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/gdb/tui/tui-location.c b/gdb/tui/tui-location.c
index 9527ae50c60..003c022f1e8 100644
--- a/gdb/tui/tui-location.c
+++ b/gdb/tui/tui-location.c
@@ -29,17 +29,15 @@ tui_location_tracker tui_location;
 bool
 tui_location_tracker::set_location (struct gdbarch *gdbarch,
 				    const struct symtab_and_line &sal,
-				    const char *procname)
+				    std::string procname)
 {
-  gdb_assert (procname != nullptr);
-
   bool location_changed_p = set_fullname (sal.symtab);
   location_changed_p |= procname != m_proc_name;
   location_changed_p |= sal.line != m_line_no;
   location_changed_p |= sal.pc != m_addr;
   location_changed_p |= gdbarch != m_gdbarch;
 
-  m_proc_name = procname;
+  m_proc_name = std::move (procname);
   m_line_no = sal.line;
   m_addr = sal.pc;
   m_gdbarch = gdbarch;
diff --git a/gdb/tui/tui-location.h b/gdb/tui/tui-location.h
index 6b663f1df9b..5cf29c3716b 100644
--- a/gdb/tui/tui-location.h
+++ b/gdb/tui/tui-location.h
@@ -32,7 +32,7 @@ struct tui_location_tracker
      and false otherwise.  */
   bool set_location (struct gdbarch *gdbarch,
 		     const struct symtab_and_line &sal,
-		     const char *procname);
+		     std::string procname);
 
   /* Update the current location with the with the provided argument.
      Return true if any of the fields actually changed, otherwise false.  */
diff --git a/gdb/tui/tui-status.c b/gdb/tui/tui-status.c
index b58bf9aa990..3d252503da0 100644
--- a/gdb/tui/tui-status.c
+++ b/gdb/tui/tui-status.c
@@ -207,10 +207,9 @@ tui_status_window::make_status_line () const
 /* Get a printable name for the function at the address.  The symbol
    name is demangled if demangling is turned on.  Returns a pointer to
    a static area holding the result.  */
-static char*
+static std::string
 tui_get_function_from_frame (const frame_info_ptr &fi)
 {
-  static char name[256];
   string_file stream;
 
   print_address_symbolic (get_frame_arch (fi), get_frame_pc (fi),
@@ -219,20 +218,20 @@ tui_get_function_from_frame (const frame_info_ptr &fi)
   /* Use simple heuristics to isolate the function name.  The symbol
      can be demangled and we can have function parameters.  Remove
      them because the status line is too short to display them.  */
-  const char *d = stream.c_str ();
-  if (*d == '<')
-    d++;
-  strncpy (name, d, sizeof (name) - 1);
-  name[sizeof (name) - 1] = 0;
+  std::string name = stream.release ();
+  if (name.front () == '<')
+    name = name.substr (1);
+
+  size_t pos = name.find('(');
+  if (pos == std::string::npos)
+    pos = name.find('>');
+  if (pos != std::string::npos)
+    name.erase (pos);
+
+  pos = name.find('+');
+  if (pos != std::string::npos)
+    name.erase (pos);
 
-  char *p = strchr (name, '(');
-  if (!p)
-    p = strchr (name, '>');
-  if (p)
-    *p = 0;
-  p = strchr (name, '+');
-  if (p)
-    *p = 0;
   return name;
 }
 
@@ -270,7 +269,7 @@ tui_show_frame_info (const frame_info_ptr &fi)
     {
       symtab_and_line sal = find_frame_sal (fi);
 
-      const char *func_name;
+      std::string func_name;
       std::optional<CORE_ADDR> tmp_pc = get_frame_pc_if_available (fi);
       /* find_frame_sal does not always set PC, but we want to ensure
 	 that it is available in the SAL.  */
@@ -284,7 +283,7 @@ tui_show_frame_info (const frame_info_ptr &fi)
 
       struct gdbarch *gdbarch = get_frame_arch (fi);
       status_changed_p
-	= tui_location.set_location (gdbarch, sal, func_name);
+	= tui_location.set_location (gdbarch, sal, std::move (func_name));
 
       /* If the status information has not changed, then frame information has
 	 not changed.  If frame information has not changed, then the windows'

base-commit: 93172549b7397c526b3622e16484909028d81e41
-- 
2.25.4

[PATCH] gdb/tui: return std::string from tui_get_function_from_frame

[Andrew Burgess]

In v2:

  - Added a '!name.empty ()' check to tui_get_function_from_frame.  I
    forgot to check this in prior to sending V1.  Sorry about that.

Thanks,
Andrew

---


Update tui_get_function_from_frame to return a std::string rather than
a pointer into a static buffer.

The value returned from tui_get_function_from_frame is passed to
tui_location_tracker::set_location, which already stores the data in a
std::string; this just moves the string creation earlier.

I don't think there was anything particularly wrong with the old code,
but I'm not a huge fan of returning data in static buffers unless
there's a really good reason, and it doesn't feel like there's a
really good reason in this case.

The current approach in tui_get_function_from_frame is to call
print_address_symbolic, and then to pull the function name from the
result.  There is an argument that this approach could be improved,
but I've not done that in this commit, nor do I plan to do that any
time soon.  As such the new code should do exactly what the old code
did.

There should be no user visible changes after this commit.
---
 gdb/tui/tui-location.c |  6 ++----
 gdb/tui/tui-location.h |  2 +-
 gdb/tui/tui-status.c   | 33 ++++++++++++++++-----------------
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/gdb/tui/tui-location.c b/gdb/tui/tui-location.c
index 9527ae50c60..003c022f1e8 100644
--- a/gdb/tui/tui-location.c
+++ b/gdb/tui/tui-location.c
@@ -29,17 +29,15 @@ tui_location_tracker tui_location;
 bool
 tui_location_tracker::set_location (struct gdbarch *gdbarch,
 				    const struct symtab_and_line &sal,
-				    const char *procname)
+				    std::string procname)
 {
-  gdb_assert (procname != nullptr);
-
   bool location_changed_p = set_fullname (sal.symtab);
   location_changed_p |= procname != m_proc_name;
   location_changed_p |= sal.line != m_line_no;
   location_changed_p |= sal.pc != m_addr;
   location_changed_p |= gdbarch != m_gdbarch;
 
-  m_proc_name = procname;
+  m_proc_name = std::move (procname);
   m_line_no = sal.line;
   m_addr = sal.pc;
   m_gdbarch = gdbarch;
diff --git a/gdb/tui/tui-location.h b/gdb/tui/tui-location.h
index 6b663f1df9b..5cf29c3716b 100644
--- a/gdb/tui/tui-location.h
+++ b/gdb/tui/tui-location.h
@@ -32,7 +32,7 @@ struct tui_location_tracker
      and false otherwise.  */
   bool set_location (struct gdbarch *gdbarch,
 		     const struct symtab_and_line &sal,
-		     const char *procname);
+		     std::string procname);
 
   /* Update the current location with the with the provided argument.
      Return true if any of the fields actually changed, otherwise false.  */
diff --git a/gdb/tui/tui-status.c b/gdb/tui/tui-status.c
index b58bf9aa990..e80b0bb5da2 100644
--- a/gdb/tui/tui-status.c
+++ b/gdb/tui/tui-status.c
@@ -207,10 +207,9 @@ tui_status_window::make_status_line () const
 /* Get a printable name for the function at the address.  The symbol
    name is demangled if demangling is turned on.  Returns a pointer to
    a static area holding the result.  */
-static char*
+static std::string
 tui_get_function_from_frame (const frame_info_ptr &fi)
 {
-  static char name[256];
   string_file stream;
 
   print_address_symbolic (get_frame_arch (fi), get_frame_pc (fi),
@@ -219,20 +218,20 @@ tui_get_function_from_frame (const frame_info_ptr &fi)
   /* Use simple heuristics to isolate the function name.  The symbol
      can be demangled and we can have function parameters.  Remove
      them because the status line is too short to display them.  */
-  const char *d = stream.c_str ();
-  if (*d == '<')
-    d++;
-  strncpy (name, d, sizeof (name) - 1);
-  name[sizeof (name) - 1] = 0;
+  std::string name = stream.release ();
+  if (!name.empty () && name.front () == '<')
+    name = name.erase (0, 1);
+
+  size_t pos = name.find('(');
+  if (pos == std::string::npos)
+    pos = name.find('>');
+  if (pos != std::string::npos)
+    name.erase (pos);
+
+  pos = name.find('+');
+  if (pos != std::string::npos)
+    name.erase (pos);
 
-  char *p = strchr (name, '(');
-  if (!p)
-    p = strchr (name, '>');
-  if (p)
-    *p = 0;
-  p = strchr (name, '+');
-  if (p)
-    *p = 0;
   return name;
 }
 
@@ -270,7 +269,7 @@ tui_show_frame_info (const frame_info_ptr &fi)
     {
       symtab_and_line sal = find_frame_sal (fi);
 
-      const char *func_name;
+      std::string func_name;
       std::optional<CORE_ADDR> tmp_pc = get_frame_pc_if_available (fi);
       /* find_frame_sal does not always set PC, but we want to ensure
 	 that it is available in the SAL.  */
@@ -284,7 +283,7 @@ tui_show_frame_info (const frame_info_ptr &fi)
 
       struct gdbarch *gdbarch = get_frame_arch (fi);
       status_changed_p
-	= tui_location.set_location (gdbarch, sal, func_name);
+	= tui_location.set_location (gdbarch, sal, std::move (func_name));
 
       /* If the status information has not changed, then frame information has
 	 not changed.  If frame information has not changed, then the windows'

base-commit: 93172549b7397c526b3622e16484909028d81e41
-- 
2.25.4

Re: [PATCH] gdb/tui: return std::string from tui_get_function_from_frame

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> I don't think there was anything particularly wrong with the old code,
Andrew> but I'm not a huge fan of returning data in static buffers unless
Andrew> there's a really good reason, and it doesn't feel like there's a
Andrew> really good reason in this case.

Definitely agree.  I feel like most such spots are artifacts of the C
days, where the lack of RAII often meant trying to find a shortcut.

Andrew>  /* Get a printable name for the function at the address.  The symbol
Andrew>     name is demangled if demangling is turned on.  Returns a pointer to
Andrew>     a static area holding the result.  */
Andrew> -static char*
Andrew> +static std::string
Andrew>  tui_get_function_from_frame (const frame_info_ptr &fi)

I think the comment needs a minor update.

Andrew> +  size_t pos = name.find('(');
Andrew> +  if (pos == std::string::npos)
Andrew> +    pos = name.find('>');
...
Andrew> +  if (pos != std::string::npos)
Andrew> +    name.erase (pos);
Andrew> +
Andrew> +  pos = name.find('+');

Some missing space-before-paren.

Otherwise looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb/tui: return std::string from tui_get_function_from_frame

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> I don't think there was anything particularly wrong with the old code,
> Andrew> but I'm not a huge fan of returning data in static buffers unless
> Andrew> there's a really good reason, and it doesn't feel like there's a
> Andrew> really good reason in this case.
>
> Definitely agree.  I feel like most such spots are artifacts of the C
> days, where the lack of RAII often meant trying to find a shortcut.
>
> Andrew>  /* Get a printable name for the function at the address.  The symbol
> Andrew>     name is demangled if demangling is turned on.  Returns a pointer to
> Andrew>     a static area holding the result.  */
> Andrew> -static char*
> Andrew> +static std::string
> Andrew>  tui_get_function_from_frame (const frame_info_ptr &fi)
>
> I think the comment needs a minor update.
>
> Andrew> +  size_t pos = name.find('(');
> Andrew> +  if (pos == std::string::npos)
> Andrew> +    pos = name.find('>');
> ...
> Andrew> +  if (pos != std::string::npos)
> Andrew> +    name.erase (pos);
> Andrew> +
> Andrew> +  pos = name.find('+');
>
> Some missing space-before-paren.
>
> Otherwise looks good.
> Approved-By: Tom Tromey <tom@tromey.com>

Fixed all the issues, and pushed.

Thanks,
Andrew