public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/source: Fix open_source_file error handling
@ 2023-01-28  3:37 Aaron Merey
  2023-02-01 19:45 ` Keith Seitz
  2023-02-08 17:44 ` Tom Tromey
  0 siblings, 2 replies; 6+ messages in thread
From: Aaron Merey @ 2023-01-28  3:37 UTC (permalink / raw)
  To: gdb-patches; +Cc: simark, Aaron Merey

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

open_source_file also may 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 include 
"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. Also add new function throw_sys_errmsg for reporting
this value.

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

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index b7410d46293..9759fabd45e 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));
+    throw_sys_errmsg (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..158b59fd613 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 ()));
+    throw_sys_errmsg (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);
diff --git a/gdb/utils.c b/gdb/utils.c
index 95adbe58e4a..0a5c6a70531 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
   gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
 }
 
+/* Same as perror_with_name except that ERRCODE specifies the error
+   instead of errno.  */
+
+void
+throw_sys_errmsg (const char *string, int errcode)
+{
+  const char *err = safe_strerror (errcode);
+  std::string combined = std::string (string) + ": " + err;
+
+  bfd_set_error (bfd_error_no_error);
+  errno = 0;
+
+  throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
+}
+
 /* Control C eventually causes this to be called, at a convenient time.  */
 
 void
diff --git a/gdb/utils.h b/gdb/utils.h
index 7865812998e..54a1bc4e476 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -278,6 +278,8 @@ 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);
+
+extern void throw_sys_errmsg (const char *, int) ATTRIBUTE_NORETURN;
 \f
 /* Warnings and error messages.  */
 
-- 
2.39.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gdb/source: Fix open_source_file error handling
  2023-01-28  3:37 [PATCH] gdb/source: Fix open_source_file error handling Aaron Merey
@ 2023-02-01 19:45 ` Keith Seitz
  2023-02-02  1:38   ` Aaron Merey
  2023-02-02 10:29   ` Bruno Larsen
  2023-02-08 17:44 ` Tom Tromey
  1 sibling, 2 replies; 6+ messages in thread
From: Keith Seitz @ 2023-02-01 19:45 UTC (permalink / raw)
  To: Aaron Merey, gdb-patches

On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
> 
> 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. Also add new function throw_sys_errmsg for reporting
> this value.

Thanks for the patch. This LGTM [warning: I am not a maintainer], but I just
have two observations (nits?).

> diff --git a/gdb/source.c b/gdb/source.c
> index be5af180c49..158b59fd613 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -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.  */

I would prefer to see a comment on this behavior with debuginfod_source_query's
description. [Repeating here is fine IMO.]

> +	      if (query_fd.get () >= 0)
> +		{
> +		  s->fullname = fullname.release ();
> +		  return query_fd;
> +		}
> +	    }
>   	}
>       }
>   
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 95adbe58e4a..0a5c6a70531 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
>     gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
>   }
>   
> +/* Same as perror_with_name except that ERRCODE specifies the error
> +   instead of errno.  */
> +

Should this comment be moved to the decl in utils.h? In such
cases, we normally use "See utils.h." as a comment at the
definition.

> +void
> +throw_sys_errmsg (const char *string, int errcode)
> +{
> +  const char *err = safe_strerror (errcode);
> +  std::string combined = std::string (string) + ": " + err;
> +
> +  bfd_set_error (bfd_error_no_error);
> +  errno = 0;
> +
> +  throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
> +}
> +
>   /* Control C eventually causes this to be called, at a convenient time.  */
>   
>   void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 7865812998e..54a1bc4e476 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -278,6 +278,8 @@ 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);
> +
> +extern void throw_sys_errmsg (const char *, int) ATTRIBUTE_NORETURN;
>   \f
>   /* Warnings and error messages.  */

<paranoid>FWIW, I've regression tested this on x86_64 both unix/-m64 and
native-extended-gdbserver/-m64. There were no issues.</paranoid>

Keith


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] gdb/source: Fix open_source_file error handling
  2023-02-01 19:45 ` Keith Seitz
@ 2023-02-02  1:38   ` Aaron Merey
  2023-02-08 17:53     ` Tom Tromey
  2023-02-02 10:29   ` Bruno Larsen
  1 sibling, 1 reply; 6+ messages in thread
From: Aaron Merey @ 2023-02-02  1:38 UTC (permalink / raw)
  To: keiths; +Cc: gdb-patches, Aaron Merey

Hi Keith,

On Wed, Feb 1, 2023 at 2:45 PM Keith Seitz <keiths@redhat.com> wrote:
>
> On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
> > +           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.  */
>
> I would prefer to see a comment on this behavior with debuginfod_source_query's
> description. [Repeating here is fine IMO.]

I updated the debuginfod_*_query descriptions in debuginfod-support.h.

> > --- a/gdb/utils.c
> > +++ b/gdb/utils.c
> > @@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
> >     gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
> >   }
> >   
> > +/* Same as perror_with_name except that ERRCODE specifies the error
> > +   instead of errno.  */
> > +
>
> Should this comment be moved to the decl in utils.h? In such
> cases, we normally use "See utils.h." as a comment at the
> definition.

Done.

Thanks,
Aaron

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

open_source_file also may 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 include
"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. Also add new function throw_sys_errmsg to facilitate
reporting this value.

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 +++--
 gdb/utils.c              | 14 ++++++++++++++
 gdb/utils.h              | 10 ++++++++++
 6 files changed, 67 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..9759fabd45e 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));
+    throw_sys_errmsg (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..158b59fd613 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 ()));
+    throw_sys_errmsg (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);
diff --git a/gdb/utils.c b/gdb/utils.c
index 95adbe58e4a..263d0925278 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -670,6 +670,20 @@ print_sys_errmsg (const char *string, int errcode)
   gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
 }
 
+/* See utils.h.  */
+
+void
+throw_sys_errmsg (const char *string, int errcode)
+{
+  const char *err = safe_strerror (errcode);
+  std::string combined = std::string (string) + ": " + err;
+
+  bfd_set_error (bfd_error_no_error);
+  errno = 0;
+
+  throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
+}
+
 /* Control C eventually causes this to be called, at a convenient time.  */
 
 void
diff --git a/gdb/utils.h b/gdb/utils.h
index 7865812998e..ef305122505 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -278,6 +278,16 @@ 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);
+
+/* Print the system error message for ERRCODE, and also mention STRING
+   as the file name for which the error was encountered.  Then return
+   to command level.
+
+   Same as perror_with_name except that ERRCODE specifies the error
+   instead of errno.  */
+
+extern void throw_sys_errmsg (const char *string, int errcode)
+  ATTRIBUTE_NORETURN;
 \f
 /* Warnings and error messages.  */
 
-- 
2.39.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gdb/source: Fix open_source_file error handling
  2023-02-01 19:45 ` Keith Seitz
  2023-02-02  1:38   ` Aaron Merey
@ 2023-02-02 10:29   ` Bruno Larsen
  1 sibling, 0 replies; 6+ messages in thread
From: Bruno Larsen @ 2023-02-02 10:29 UTC (permalink / raw)
  To: Keith Seitz, Aaron Merey, gdb-patches

On 01/02/2023 20:45, Keith Seitz via Gdb-patches wrote:
> On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
>>
>> 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. Also add new function throw_sys_errmsg for reporting
>> this value.
>
> Thanks for the patch. This LGTM [warning: I am not a maintainer], but 
> I just
> have two observations (nits?). 

Friendly reminder that this is exactly what reviewed-by and approved-by 
tags were introduced to solve :-)

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gdb/source: Fix open_source_file error handling
  2023-01-28  3:37 [PATCH] gdb/source: Fix open_source_file error handling Aaron Merey
  2023-02-01 19:45 ` Keith Seitz
@ 2023-02-08 17:44 ` Tom Tromey
  1 sibling, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2023-02-08 17:44 UTC (permalink / raw)
  To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey, simark

>>>>> "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. Also add new function throw_sys_errmsg for reporting
Aaron> this value.

I wonder if it's guaranteed somewhere that the errno values are always
positive.
 
Aaron> +/* Same as perror_with_name except that ERRCODE specifies the error
Aaron> +   instead of errno.  */
Aaron> +
Aaron> +void
Aaron> +throw_sys_errmsg (const char *string, int errcode)

IMO probably better to just add a parameter to perror_with_name that
defaults to 0, meaning use errno.

However, if you want to go this route, a function that always throws
must be marked ATTRIBUTE_NORETURN.

Tom

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gdb/source: Fix open_source_file error handling
  2023-02-02  1:38   ` Aaron Merey
@ 2023-02-08 17:53     ` Tom Tromey
  0 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2023-02-08 17:53 UTC (permalink / raw)
  To: Aaron Merey via Gdb-patches; +Cc: keiths, Aaron Merey

Ok, I re-read the patch and see you did use ATTRIBUTE_NORETURN.  Sorry
about that.  And adding an optional argument means changing gdbserver as
well... IMO this is fine but up to you.

Though it seems to me that:

Aaron> +  bfd_set_error (bfd_error_no_error);
Aaron> +  errno = 0;

... these two lines are probably leftovers from ancient times, when the
gdb exception mechanism was much less robust.  They should not be in new
code, and TBH probably the best route forward is to remove them from
utils.c, and in fact merge the definition of perror_with_name into
gdbsupport, since AFAICT it's really identical between gdb and
gdbserver.

Tom

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-08 17:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-28  3:37 [PATCH] gdb/source: Fix open_source_file error handling Aaron Merey
2023-02-01 19:45 ` Keith Seitz
2023-02-02  1:38   ` Aaron Merey
2023-02-08 17:53     ` Tom Tromey
2023-02-02 10:29   ` Bruno Larsen
2023-02-08 17:44 ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).