* [PATCH 1/2] Move implementation of perror_with_name to gdbsupport
2023-02-10 1:59 [PATCH v2 0/2] gdb: Fix open_source_file error handling Aaron Merey
@ 2023-02-10 1:59 ` Aaron Merey
2023-02-10 14:39 ` Tom Tromey
2023-02-10 1:59 ` [PATCH 2/2] gdb/source: Fix open_source_file error handling Aaron Merey
1 sibling, 1 reply; 7+ messages in thread
From: Aaron Merey @ 2023-02-10 1:59 UTC (permalink / raw)
To: gdb-patches; +Cc: tom, keiths, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 2/2] gdb/source: Fix open_source_file error handling
2023-02-10 1:59 [PATCH v2 0/2] gdb: Fix open_source_file error handling Aaron Merey
2023-02-10 1:59 ` [PATCH 1/2] Move implementation of perror_with_name to gdbsupport Aaron Merey
@ 2023-02-10 1:59 ` Aaron Merey
2023-02-10 14:41 ` Tom Tromey
1 sibling, 1 reply; 7+ messages in thread
From: Aaron Merey @ 2023-02-10 1:59 UTC (permalink / raw)
To: gdb-patches; +Cc: tom, keiths, 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
^ permalink raw reply [flat|nested] 7+ messages in thread