[4/5] gdb: remove print_sys_errmsg

Message ID fe17339ce952869e985c250299b5d3d8d3316648.1695824275.git.aburgess@redhat.com
State New
Headers
Series Fix using an exec file with target: prefix |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_check--master-arm fail Testing failed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Testing failed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed

Commit Message

Andrew Burgess Sept. 27, 2023, 2:22 p.m. UTC
  This started with me running into this comment in symfile.c:

  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
              styled_string (file_name_style.style (), filename));

In this particular case I think I disagree with the comment; I think
the output should be a warning rather than just a message printed to
gdb_stdout, I think when the executable, or some other objfile that is
currently being debugged, disappears from disk, this is likely an
unexpected situation, and worth warning the user about.

So, in theory, I could just call print_sys_errmsg and remove the
comment, but that would mean loosing the filename styling in the
output... so in the end I remove the comment and updated the code to
call warning.

But that got me looking at print_sys_errmsg and how it's used.

Currently the function takes a string and an errno, and prints, to
stderr, the string followed by the result of calling strerror on the
errno.

In some places the string passed to print_sys_errmsg is just a
filename, and this is used when something goes wrong.  In these cases,
I think calling warning rather than gdb_printf to gdb_stderr, would be
better, and in fact, in a couple of places we manually print a
"warning" prefix, and then call print_sys_errmsg.  And so, for these
users I have added a new function warning_filename_and_errno, which
takes a filename, which is printed with styling, and an errno, which
is passed through strerror and the resulting string printed.  This new
function calls warning to print its output.  I then updated some of
the print_sys_errmsg users to use this new function.

Some other users of print_sys_errmsg are also emitting what is clearly
a warning, however, the string being passed in is more than just a
filename, so the new warning_filename_and_errno function can't be
used, it would style the whole string.  For these users I have
switched to calling warning directly, this allows me to style the
warning message correctly.

Finally, in inflow.c there is one last call to print_sys_errmsg, in
this case I just inlined the definition of print_sys_errmsg.  This is
a really weird case, as after printing this message GDB just does a
hard exit.  This is pretty old code, dating back to the initial GDB
import, I guess it should be updated to call error() maybe, but I'm
reluctant to make this change as part of this commit, just in case
there's some reason why we can't throw an error at this point.

With that done there are now no users of print_sys_errmsg, and so the
old function can be removed.

While I was doing all of the above I added some additional filename
styling in soure.c, this is in an else block where the if contained
the print_sys_errmsg call, so these felt related.

And finally, while I was updating the uses of print_sys_errmsg in
procfs.c, I noticed that we used a static errmsg buffer to format some
error strings.  As the above changes got rid of one of the users of
errmsg I also removed the other two users, and the static buffer.
---
 gdb/inflow.c      |  3 ++-
 gdb/main.c        |  7 +------
 gdb/procfs.c      | 17 ++++++++++-------
 gdb/source.c      | 15 ++++-----------
 gdb/symfile.c     |  5 ++---
 gdb/utils.c       |  9 ++++-----
 gdb/utils.h       |  8 +++++++-
 gdb/windows-nat.c |  2 +-
 8 files changed, 31 insertions(+), 35 deletions(-)
  

Comments

Andrew Burgess Sept. 28, 2023, 12:06 p.m. UTC | #1
The posted version of this patch didn't include some test fixes -- I
pulled this series out of another series I had locally, and I'd
committed the test fixes into a patch that never made it into this
version by mistake.

Here's an updated patch that contains the two test fixes to take account
of the output changes from this patch.

Thanks,
Andrew

---

commit bcc3a21e682e3c109b120db0ef020b48d1933e62
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sun Sep 24 12:37:40 2023 +0100

    gdb: remove print_sys_errmsg
    
    This started with me running into this comment in symfile.c:
    
      /* FIXME, should use print_sys_errmsg but it's not filtered.  */
      gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
                  styled_string (file_name_style.style (), filename));
    
    In this particular case I think I disagree with the comment; I think
    the output should be a warning rather than just a message printed to
    gdb_stdout, I think when the executable, or some other objfile that is
    currently being debugged, disappears from disk, this is likely an
    unexpected situation, and worth warning the user about.
    
    So, in theory, I could just call print_sys_errmsg and remove the
    comment, but that would mean loosing the filename styling in the
    output... so in the end I remove the comment and updated the code to
    call warning.
    
    But that got me looking at print_sys_errmsg and how it's used.
    
    Currently the function takes a string and an errno, and prints, to
    stderr, the string followed by the result of calling strerror on the
    errno.
    
    In some places the string passed to print_sys_errmsg is just a
    filename, and this is used when something goes wrong.  In these cases,
    I think calling warning rather than gdb_printf to gdb_stderr, would be
    better, and in fact, in a couple of places we manually print a
    "warning" prefix, and then call print_sys_errmsg.  And so, for these
    users I have added a new function warning_filename_and_errno, which
    takes a filename, which is printed with styling, and an errno, which
    is passed through strerror and the resulting string printed.  This new
    function calls warning to print its output.  I then updated some of
    the print_sys_errmsg users to use this new function.
    
    Some other users of print_sys_errmsg are also emitting what is clearly
    a warning, however, the string being passed in is more than just a
    filename, so the new warning_filename_and_errno function can't be
    used, it would style the whole string.  For these users I have
    switched to calling warning directly, this allows me to style the
    warning message correctly.
    
    Finally, in inflow.c there is one last call to print_sys_errmsg, in
    this case I just inlined the definition of print_sys_errmsg.  This is
    a really weird case, as after printing this message GDB just does a
    hard exit.  This is pretty old code, dating back to the initial GDB
    import, I guess it should be updated to call error() maybe, but I'm
    reluctant to make this change as part of this commit, just in case
    there's some reason why we can't throw an error at this point.
    
    With that done there are now no users of print_sys_errmsg, and so the
    old function can be removed.
    
    While I was doing all of the above I added some additional filename
    styling in soure.c, this is in an else block where the if contained
    the print_sys_errmsg call, so these felt related.
    
    And finally, while I was updating the uses of print_sys_errmsg in
    procfs.c, I noticed that we used a static errmsg buffer to format some
    error strings.  As the above changes got rid of one of the users of
    errmsg I also removed the other two users, and the static buffer.
    
    There were a couple of tests that depended on the existing output
    message format that needed updating.  In one case we gained an extra
    'warning: ' prefix, and in the other 'Warning: ' becomes 'warning: ',
    I think in both cases the new output is an improvement.

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..095c5f03672 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -766,7 +766,8 @@ check_syscall (const char *msg, int result)
 {
   if (result < 0)
     {
-      print_sys_errmsg (msg, errno);
+      gdb_printf (gdb_stderr, "%s:%s.\n", msg,
+		  safe_strerror (errno));
       _exit (1);
     }
 }
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb20..1ca0fdeee1b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir)
   struct stat st;
 
   if (stat (new_datadir, &st) < 0)
-    {
-      int save_errno = errno;
-
-      gdb_printf (gdb_stderr, "Warning: ");
-      print_sys_errmsg (new_datadir, save_errno);
-    }
+    warning_filename_and_errno (new_datadir, errno);
   else if (!S_ISDIR (st.st_mode))
     warning (_("%ps is not a directory."),
 	     styled_string (file_name_style.style (), new_datadir));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 706ccf0965c..48e9f3dd4b5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -46,6 +46,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/buildargv.h"
+#include "cli/cli-style.h"
 
 /* This module provides the interface between GDB and the
    /proc file system, which is used on many versions of Unix
@@ -558,7 +559,7 @@ enum { NOKILL, KILL };
 static void
 dead_procinfo (procinfo *pi, const char *msg, int kill_p)
 {
-  print_sys_errmsg (pi->pathname, errno);
+  warning_filename_and_errno (pi->pathname, errno);
   if (kill_p == KILL)
     kill (pi->pid, SIGKILL);
 
@@ -594,18 +595,20 @@ static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  print_sys_errmsg (errmsg.c_str (), saved_errno);
+  warning ("procfs: %s line %d, %ps: %s",
+	   func, line, styled_string (file_name_style.style (),
+				      pi->pathname),
+	   safe_strerror (saved_errno));
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  perror_with_name (errmsg.c_str (), saved_errno);
+  error ("procfs: %s line %d, %ps: %s",
+	 func, line, styled_string (file_name_style.style (),
+				    pi->pathname),
+	 safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
diff --git a/gdb/source.c b/gdb/source.c
index 5bdd729be8b..f648adc4520 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	     a directory/etc, then having them in the path should be
 	     harmless.  */
 	  if (stat (name, &st) < 0)
-	    {
-	      int save_errno = errno;
-
-	      gdb_printf (gdb_stderr, "Warning: ");
-	      print_sys_errmsg (name, save_errno);
-	    }
+	    warning_filename_and_errno (name, errno);
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
 	    warning (_("%ps is not a directory."),
 		     styled_string (file_name_style.style (), name));
@@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
       if (!(flags & PRINT_SOURCE_LINES_NOERROR))
 	{
 	  const char *filename = symtab_to_filename_for_display (s);
-	  int len = strlen (filename) + 100;
-	  char *name = (char *) alloca (len);
-
-	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errcode);
+	  warning (_("%d\t%ps: %s"), line,
+		   styled_string (file_name_style.style (), filename),
+		   safe_strerror (errcode));
 	}
       else
 	{
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 145621f3c67..10074e281bd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2483,9 +2483,8 @@ reread_symbols (int from_tty)
       int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
-	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (), filename));
+	  warning (_("`%ps' has disappeared; keeping its symbols."),
+		   styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index 4a9302fb9b7..0588cb35d87 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -392,7 +392,7 @@ proc test_catch_syscall_fail_nodatadir {} {
 	# Make sure GDB doesn't load the syscalls xml from the system
 	# data directory.
 	gdb_test "set data-directory /the/path/to/nowhere" \
-	    "Warning: /the/path/to/nowhere: .*"
+	    "warning: /the/path/to/nowhere: .*"
 
 	# Testing to see if we receive a warning when calling "catch
 	# syscall" without XML support (without datadir).
@@ -658,7 +658,7 @@ proc do_syscall_tests_without_xml {} {
     # Make sure GDB doesn't load the syscalls xml from the system data
     # directory.
     gdb_test "set data-directory /the/path/to/nowhere" \
-	"Warning: /the/path/to/nowhere: .*"
+	"warning: /the/path/to/nowhere: .*"
 
     # Let's test if we can catch syscalls without XML support.
     # We should succeed, but GDB is not supposed to print syscall names.
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
index 7e28931be22..07aa6afbee7 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
@@ -167,7 +167,7 @@ if { $psymtabs_p } {
 }
 
 gdb_test "l imported_unit.c:1" \
-    "1\timported_unit.c: No such file or directory\."
+    "warning: 1\timported_unit.c: No such file or directory"
 
 gdb_test "info source" "\r\nCurrent source file is imported_unit.c\r\n.*" \
     "info source for imported_unit.c"
diff --git a/gdb/utils.c b/gdb/utils.c
index 2f545337cd4..a191d26a007 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -619,14 +619,13 @@ perror_warning_with_name (const char *string)
   warning (_("%s"), combined.c_str ());
 }
 
-/* Print the system error message for ERRCODE, and also mention STRING
-   as the file name for which the error was encountered.  */
+/* See utils.h.  */
 
 void
-print_sys_errmsg (const char *string, int errcode)
+warning_filename_and_errno (const char *filename, int saved_errno)
 {
-  const char *err = safe_strerror (errcode);
-  gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
+  warning (_("%ps: %s"), styled_string (file_name_style.style (), filename),
+	   safe_strerror (saved_errno));
 }
 
 /* Control C eventually causes this to be called, at a convenient time.  */
diff --git a/gdb/utils.h b/gdb/utils.h
index c5364fa4b35..f646b300530 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -275,7 +275,13 @@ extern void fprintf_symbol (struct ui_file *, const char *,
 
 extern void perror_warning_with_name (const char *string);
 
-extern void print_sys_errmsg (const char *, int);
+/* Issue a warning formatted as '<filename>: <explanation>', where
+   <filename> is FILENAME with filename styling applied.  As such, don't
+   pass anything more than a filename in this string.  The <explanation>
+   is a string returned from calling safe_strerror(SAVED_ERRNO).  */
+
+extern void warning_filename_and_errno (const char *filename,
+					int saved_errno);
 
 /* Warnings and error messages.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5a897dbfe76..7a139c8d36f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file,
       tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY);
       if (tty < 0)
 	{
-	  print_sys_errmsg (inferior_tty.c_str (), errno);
+	  warning_filename_and_errno (inferior_tty.c_str (), errno);
 	  ostdin = ostdout = ostderr = -1;
 	}
       else
  

Patch

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..095c5f03672 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -766,7 +766,8 @@  check_syscall (const char *msg, int result)
 {
   if (result < 0)
     {
-      print_sys_errmsg (msg, errno);
+      gdb_printf (gdb_stderr, "%s:%s.\n", msg,
+		  safe_strerror (errno));
       _exit (1);
     }
 }
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb20..1ca0fdeee1b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -115,12 +115,7 @@  set_gdb_data_directory (const char *new_datadir)
   struct stat st;
 
   if (stat (new_datadir, &st) < 0)
-    {
-      int save_errno = errno;
-
-      gdb_printf (gdb_stderr, "Warning: ");
-      print_sys_errmsg (new_datadir, save_errno);
-    }
+    warning_filename_and_errno (new_datadir, errno);
   else if (!S_ISDIR (st.st_mode))
     warning (_("%ps is not a directory."),
 	     styled_string (file_name_style.style (), new_datadir));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 706ccf0965c..48e9f3dd4b5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -46,6 +46,7 @@ 
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/buildargv.h"
+#include "cli/cli-style.h"
 
 /* This module provides the interface between GDB and the
    /proc file system, which is used on many versions of Unix
@@ -558,7 +559,7 @@  enum { NOKILL, KILL };
 static void
 dead_procinfo (procinfo *pi, const char *msg, int kill_p)
 {
-  print_sys_errmsg (pi->pathname, errno);
+  warning_filename_and_errno (pi->pathname, errno);
   if (kill_p == KILL)
     kill (pi->pid, SIGKILL);
 
@@ -594,18 +595,20 @@  static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  print_sys_errmsg (errmsg.c_str (), saved_errno);
+  warning ("procfs: %s line %d, %ps: %s",
+	   func, line, styled_string (file_name_style.style (),
+				      pi->pathname),
+	   safe_strerror (saved_errno));
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  perror_with_name (errmsg.c_str (), saved_errno);
+  error ("procfs: %s line %d, %ps: %s",
+	 func, line, styled_string (file_name_style.style (),
+				    pi->pathname),
+	 safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
diff --git a/gdb/source.c b/gdb/source.c
index 5bdd729be8b..f648adc4520 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -587,12 +587,7 @@  add_path (const char *dirname, char **which_path, int parse_separators)
 	     a directory/etc, then having them in the path should be
 	     harmless.  */
 	  if (stat (name, &st) < 0)
-	    {
-	      int save_errno = errno;
-
-	      gdb_printf (gdb_stderr, "Warning: ");
-	      print_sys_errmsg (name, save_errno);
-	    }
+	    warning_filename_and_errno (name, errno);
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
 	    warning (_("%ps is not a directory."),
 		     styled_string (file_name_style.style (), name));
@@ -1341,11 +1336,9 @@  print_source_lines_base (struct symtab *s, int line, int stopline,
       if (!(flags & PRINT_SOURCE_LINES_NOERROR))
 	{
 	  const char *filename = symtab_to_filename_for_display (s);
-	  int len = strlen (filename) + 100;
-	  char *name = (char *) alloca (len);
-
-	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errcode);
+	  warning (_("%d\t%ps: %s"), line,
+		   styled_string (file_name_style.style (), filename),
+		   safe_strerror (errcode));
 	}
       else
 	{
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 145621f3c67..10074e281bd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2483,9 +2483,8 @@  reread_symbols (int from_tty)
       int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
-	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (), filename));
+	  warning (_("`%ps' has disappeared; keeping its symbols."),
+		   styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
diff --git a/gdb/utils.c b/gdb/utils.c
index 2f545337cd4..a191d26a007 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -619,14 +619,13 @@  perror_warning_with_name (const char *string)
   warning (_("%s"), combined.c_str ());
 }
 
-/* Print the system error message for ERRCODE, and also mention STRING
-   as the file name for which the error was encountered.  */
+/* See utils.h.  */
 
 void
-print_sys_errmsg (const char *string, int errcode)
+warning_filename_and_errno (const char *filename, int saved_errno)
 {
-  const char *err = safe_strerror (errcode);
-  gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
+  warning (_("%ps: %s"), styled_string (file_name_style.style (), filename),
+	   safe_strerror (saved_errno));
 }
 
 /* Control C eventually causes this to be called, at a convenient time.  */
diff --git a/gdb/utils.h b/gdb/utils.h
index c5364fa4b35..f646b300530 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -275,7 +275,13 @@  extern void fprintf_symbol (struct ui_file *, const char *,
 
 extern void perror_warning_with_name (const char *string);
 
-extern void print_sys_errmsg (const char *, int);
+/* Issue a warning formatted as '<filename>: <explanation>', where
+   <filename> is FILENAME with filename styling applied.  As such, don't
+   pass anything more than a filename in this string.  The <explanation>
+   is a string returned from calling safe_strerror(SAVED_ERRNO).  */
+
+extern void warning_filename_and_errno (const char *filename,
+					int saved_errno);
 
 /* Warnings and error messages.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5a897dbfe76..7a139c8d36f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2625,7 +2625,7 @@  windows_nat_target::create_inferior (const char *exec_file,
       tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY);
       if (tty < 0)
 	{
-	  print_sys_errmsg (inferior_tty.c_str (), errno);
+	  warning_filename_and_errno (inferior_tty.c_str (), errno);
 	  ostdin = ostdout = ostderr = -1;
 	}
       else