[PATCH v2 0/2] gdb: Fix open_source_file error handling

[PATCH v2 0/2] gdb: Fix open_source_file error handling

[Aaron Merey]

v1: https://sourceware.org/pipermail/gdb-patches/2023-February/196529.html

v2 implements the following suggestions:
https://sourceware.org/pipermail/gdb-patches/2023-February/196693.html

Aaron Merey (2):
  Move implementation of perror_with_name to gdbsupport
  gdb/source: Fix open_source_file error handling

 gdb/debuginfod-support.h | 15 +++++++++------
 gdb/source-cache.c       |  2 +-
 gdb/source.c             | 40 ++++++++++++++++++++++++++++++----------
 gdb/source.h             |  5 +++--
 gdb/utils.c              | 36 ------------------------------------
 gdbserver/utils.cc       | 22 ----------------------
 gdbsupport/errors.cc     | 24 ++++++++++++++++++++++++
 gdbsupport/errors.h      | 13 ++++++++++---
 8 files changed, 77 insertions(+), 80 deletions(-)

-- 
2.39.1

[PATCH 1/2] Move implementation of perror_with_name to gdbsupport

[Aaron Merey]

gdbsupport/errors.h declares perror_with_name and leaves the
implementation to the clients.

However gdb and gdbserver's implementations are essentially the
same, resulting in unnecessary code duplication.

Fix this by implementing perror_with_name in gdbsupport.  Add an
optional parameter for specifying the errno used to generate the
error message.

Also move the implementation of perror_string to gdbsupport since
perror_with_name requires it.
---
 gdb/utils.c          | 36 ------------------------------------
 gdbserver/utils.cc   | 22 ----------------------
 gdbsupport/errors.cc | 24 ++++++++++++++++++++++++
 gdbsupport/errors.h  | 13 ++++++++++---
 4 files changed, 34 insertions(+), 61 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 95adbe58e4a..d763e91d7e1 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -614,42 +614,6 @@ add_internal_problem_command (struct internal_problem *problem)
     }
 }
 
-/* Return a newly allocated string, containing the PREFIX followed
-   by the system error message for errno (separated by a colon).  */
-
-static std::string
-perror_string (const char *prefix)
-{
-  const char *err = safe_strerror (errno);
-  return std::string (prefix) + ": " + err;
-}
-
-/* Print the system error message for errno, and also mention STRING
-   as the file name for which the error was encountered.  Use ERRCODE
-   for the thrown exception.  Then return to command level.  */
-
-static void ATTRIBUTE_NORETURN
-throw_perror_with_name (enum errors errcode, const char *string)
-{
-  std::string combined = perror_string (string);
-
-  /* I understand setting these is a matter of taste.  Still, some people
-     may clear errno but not know about bfd_error.  Doing this here is not
-     unreasonable.  */
-  bfd_set_error (bfd_error_no_error);
-  errno = 0;
-
-  throw_error (errcode, _("%s."), combined.c_str ());
-}
-
-/* See throw_perror_with_name, ERRCODE defaults here to GENERIC_ERROR.  */
-
-void
-perror_with_name (const char *string)
-{
-  throw_perror_with_name (GENERIC_ERROR, string);
-}
-
 /* Same as perror_with_name except that it prints a warning instead
    of throwing an error.  */
 
diff --git a/gdbserver/utils.cc b/gdbserver/utils.cc
index a6f5bd7b600..511364cfed2 100644
--- a/gdbserver/utils.cc
+++ b/gdbserver/utils.cc
@@ -51,28 +51,6 @@ malloc_failure (long size)
   abort_or_exit ();
 }
 
-/* Print the system error message for errno, and also mention STRING
-   as the file name for which the error was encountered.
-   Then return to command level.  */
-
-void
-perror_with_name (const char *string)
-{
-  const char *err;
-  char *combined;
-
-  err = safe_strerror (errno);
-  if (err == NULL)
-    err = "unknown error";
-
-  combined = (char *) alloca (strlen (err) + strlen (string) + 3);
-  strcpy (combined, string);
-  strcat (combined, ": ");
-  strcat (combined, err);
-
-  error ("%s.", combined);
-}
-
 /* Print an error message and return to top level.  */
 
 void
diff --git a/gdbsupport/errors.cc b/gdbsupport/errors.cc
index 566be377e3f..b48ce10eef8 100644
--- a/gdbsupport/errors.cc
+++ b/gdbsupport/errors.cc
@@ -71,6 +71,30 @@ internal_warning_loc (const char *file, int line, const char *fmt, ...)
   va_end (ap);
 }
 
+/* See errors.h.  */
+
+std::string
+perror_string (const char *prefix, int errnum)
+{
+  const char *err;
+
+  if (errnum != 0)
+    err = safe_strerror (errnum);
+  else
+    err = safe_strerror (errno);
+  return std::string (prefix) + ": " + err;
+}
+
+/* See errors.h.  */
+
+void
+perror_with_name (const char *string, int errnum)
+{
+  std::string combined = perror_string (string, errnum);
+
+  error (_("%s."), combined.c_str ());
+}
+
 #if defined (USE_WIN32API) || defined(__CYGWIN__)
 
 /* See errors.h.  */
diff --git a/gdbsupport/errors.h b/gdbsupport/errors.h
index 2304bc1e3d1..20f9152b671 100644
--- a/gdbsupport/errors.h
+++ b/gdbsupport/errors.h
@@ -83,11 +83,18 @@ extern void internal_vwarning (const char *file, int line,
      ATTRIBUTE_PRINTF (3, 0);
 \f
 
+/* Return a newly allocated string, containing the PREFIX followed
+   by the system error message for errno (separated by a colon).
+   If ERRNUM is given, then use it in place of errno.  */
+
+extern std::string perror_string (const char *prefix, int errnum = 0);
+
 /* Like "error", but the error message is constructed by combining
-   STRING with the system error message for errno.  This function does
-   not return.  This function must be provided by the client.  */
+   STRING with the system error message for errno.  If ERRNUM is given,
+   then use it in place of errno.  This function does not return.  */
 
-extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
+extern void perror_with_name (const char *string, int errnum = 0)
+    ATTRIBUTE_NORETURN;
 
 /* Call this function to handle memory allocation failures.  This
    function does not return.  This function must be provided by the
-- 
2.39.1

[PATCH 2/2] gdb/source: Fix open_source_file error handling

[Aaron Merey]

open_source_file relies on errno to communicate the reason for a missing
source file.

open_source_file may also call debuginfod_find_source.  It is possible
for debuginfod_find_source to set errno to a value unrelated to the
reason for a failed download.

This can result in bogus error messages being reported as the reason for
a missing source file.  The following error message should instead be
"No such file or directory":

  Temporary breakpoint 1, 0x00005555556f4de0 in main ()
  (gdb) list
  Downloading source file /usr/src/debug/glibc-2.36-8.fc37.x86_64/elf/<built-in>
  1       /usr/src/debug/glibc-2.36-8.fc37.x86_64/elf/<built-in>: Directory not empty.

Fix this by having open_source_file return a negative errno if it fails
to open a source file.  Use this value to generate the error message
instead of errno.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29999
---
 gdb/debuginfod-support.h | 15 +++++++++------
 gdb/source-cache.c       |  2 +-
 gdb/source.c             | 40 ++++++++++++++++++++++++++++++----------
 gdb/source.h             |  5 +++--
 4 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/gdb/debuginfod-support.h b/gdb/debuginfod-support.h
index 542d1688fa1..633600a79da 100644
--- a/gdb/debuginfod-support.h
+++ b/gdb/debuginfod-support.h
@@ -33,8 +33,9 @@
    source file path is `/my/source/foo.c`, then SRC_PATH should be
    `/my/build/../source/foo.c`.
 
-   If the file is successfully retrieved, its path on the local machine
-   is stored in DESTNAME.  If GDB is not built with debuginfod, this
+   If the file is successfully retrieved, return a file descriptor and store
+   the file's local path in DESTNAME.  If unsuccessful, print an error message
+   and return a negative errno.  If GDB is not built with debuginfod, this
    function returns -ENOSYS.  */
 
 extern scoped_fd
@@ -51,8 +52,9 @@ debuginfod_source_query (const unsigned char *build_id,
    FILENAME should be the name or path of the main binary associated with
    the separate debug info.  It is used for printing messages to the user.
 
-   If the file is successfully retrieved, its path on the local machine
-   is stored in DESTNAME.  If GDB is not built with debuginfod, this
+   If the file is successfully retrieved, return a file descriptor and store
+   the file's local path in DESTNAME.  If unsuccessful, print an error message
+   and return a negative errno.  If GDB is not built with debuginfod, this
    function returns -ENOSYS.  */
 
 extern scoped_fd
@@ -69,8 +71,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
    FILENAME should be the name or path associated with the executable.
    It is used for printing messages to the user.
 
-   If the file is successfully retrieved, its path on the local machine
-   is stored in DESTNAME.  If GDB is not built with debuginfod, this
+   If the file is successfully retrieved, return a file descriptor and store
+   the file's local path in DESTNAME.  If unsuccessful, print an error message
+   and return a negative errno.  If GDB is not built with debuginfod, this
    function returns -ENOSYS.  */
 
 extern scoped_fd debuginfod_exec_query (const unsigned char *build_id,
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index b7410d46293..77b357cb42b 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -95,7 +95,7 @@ source_cache::get_plain_source_lines (struct symtab *s,
 {
   scoped_fd desc (open_source_file (s));
   if (desc.get () < 0)
-    perror_with_name (symtab_to_filename_for_display (s));
+    perror_with_name (symtab_to_filename_for_display (s), -desc.get ());
 
   struct stat st;
   if (fstat (desc.get (), &st) < 0)
diff --git a/gdb/source.c b/gdb/source.c
index be5af180c49..ca0e8d51fbd 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1069,7 +1069,7 @@ find_and_open_source (const char *filename,
      the attempt to read this source file failed.  GDB will then display
      the filename and line number instead.  */
   if (!source_open)
-    return scoped_fd (-1);
+    return scoped_fd (-ECANCELED);
 
   /* Quick way out if we already know its full name.  */
   if (*fullname)
@@ -1160,11 +1160,15 @@ find_and_open_source (const char *filename,
 			OPEN_MODE, fullname);
     }
 
+  /* If the file wasn't found, then openp will have set errno accordingly.  */
+  if (result < 0)
+    result = -errno;
+
   return scoped_fd (result);
 }
 
 /* Open a source file given a symtab S.  Returns a file descriptor or
-   negative number for error.  
+   negative errno for error.
    
    This function is a convenience function to find_and_open_source.  */
 
@@ -1172,7 +1176,7 @@ scoped_fd
 open_source_file (struct symtab *s)
 {
   if (!s)
-    return scoped_fd (-1);
+    return scoped_fd (-EINVAL);
 
   gdb::unique_xmalloc_ptr<char> fullname (s->fullname);
   s->fullname = NULL;
@@ -1200,10 +1204,21 @@ open_source_file (struct symtab *s)
 
 	  /* Query debuginfod for the source file.  */
 	  if (build_id != nullptr && !srcpath.empty ())
-	    fd = debuginfod_source_query (build_id->data,
-					  build_id->size,
-					  srcpath.c_str (),
-					  &fullname);
+	    {
+	      scoped_fd query_fd
+		= debuginfod_source_query (build_id->data,
+					   build_id->size,
+					   srcpath.c_str (),
+					   &fullname);
+
+	      /* Don't return a negative errno from debuginfod_source_query.
+		 It handles the reporting of its own errors.  */
+	      if (query_fd.get () >= 0)
+		{
+		  s->fullname = fullname.release ();
+		  return query_fd;
+		}
+	    }
 	}
     }
 
@@ -1306,6 +1321,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 			 print_source_lines_flags flags)
 {
   bool noprint = false;
+  int errcode = ENOENT;
   int nlines = stopline - line;
   struct ui_out *uiout = current_uiout;
 
@@ -1336,7 +1352,10 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	  scoped_fd desc = open_source_file (s);
 	  last_source_error = desc.get () < 0;
 	  if (last_source_error)
-	    noprint = true;
+	    {
+	      noprint = true;
+	      errcode = -desc.get ();
+	    }
 	}
     }
   else
@@ -1354,7 +1373,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	  char *name = (char *) alloca (len);
 
 	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errno);
+	  print_sys_errmsg (name, errcode);
 	}
       else
 	{
@@ -1627,7 +1646,8 @@ search_command_helper (const char *regex, int from_tty, bool forward)
 
   scoped_fd desc (open_source_file (loc->symtab ()));
   if (desc.get () < 0)
-    perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
+    perror_with_name (symtab_to_filename_for_display (loc->symtab ()),
+		      -desc.get ());
 
   int line = (forward
 	      ? last_line_listed + 1
diff --git a/gdb/source.h b/gdb/source.h
index 61d1bcd883b..dd6f58c579c 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -67,7 +67,8 @@ extern void init_source_path (void);
      The caller is responsible for freeing FULLNAME.
 
    On Failure
-     An invalid file descriptor is returned (the return value is negative).
+     An invalid file descriptor is returned.  The value of this file
+     descriptor is a negative errno indicating the reason for the failure.
      FULLNAME is set to NULL.  */
 extern scoped_fd find_and_open_source (const char *filename,
 				       const char *dirname,
@@ -81,7 +82,7 @@ extern gdb::unique_xmalloc_ptr<char> find_source_or_rewrite
      (const char *filename, const char *dirname);
 
 /* Open a source file given a symtab S.  Returns a file descriptor or
-   negative number for error.  */
+   negative errno indicating the reason for the failure.  */
 extern scoped_fd open_source_file (struct symtab *s);
 
 extern gdb::unique_xmalloc_ptr<char> rewrite_source_path (const char *path);
-- 
2.39.1

Re: [PATCH 1/2] Move implementation of perror_with_name to gdbsupport

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> gdbsupport/errors.h declares perror_with_name and leaves the
Aaron> implementation to the clients.

Aaron> However gdb and gdbserver's implementations are essentially the
Aaron> same, resulting in unnecessary code duplication.

Aaron> Fix this by implementing perror_with_name in gdbsupport.  Add an
Aaron> optional parameter for specifying the errno used to generate the
Aaron> error message.

Aaron> Also move the implementation of perror_string to gdbsupport since
Aaron> perror_with_name requires it.

Thanks, this looks great.

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

Tom

Re: [PATCH 2/2] gdb/source: Fix open_source_file error handling

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Fix this by having open_source_file return a negative errno if it fails
Aaron> to open a source file.  Use this value to generate the error message
Aaron> instead of errno.

Thanks for doing this.

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

Tom

Re: [PATCH 1/2] Move implementation of perror_with_name to gdbsupport

[Aaron Merey]

On Fri, Feb 10, 2023 at 9:39 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> gdbsupport/errors.h declares perror_with_name and leaves the
> Aaron> implementation to the clients.
>
> Aaron> However gdb and gdbserver's implementations are essentially the
> Aaron> same, resulting in unnecessary code duplication.
>
> Aaron> Fix this by implementing perror_with_name in gdbsupport.  Add an
> Aaron> optional parameter for specifying the errno used to generate the
> Aaron> error message.
>
> Aaron> Also move the implementation of perror_string to gdbsupport since
> Aaron> perror_with_name requires it.
>
> Thanks, this looks great.

Thanks, pushed as commit 40dfb28b56fe55a370a

Aaron

Re: [PATCH 2/2] gdb/source: Fix open_source_file error handling

[Aaron Merey]

On Fri, Feb 10, 2023 at 9:41 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> Fix this by having open_source_file return a negative errno if it fails
> Aaron> to open a source file.  Use this value to generate the error message
> Aaron> instead of errno.
>
> Thanks for doing this.

Thanks, pushed as commit 8cc96ee4169f63191

Aaron