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(-)
@@ -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,
@@ -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)
@@ -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
@@ -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);
@@ -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
@@ -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;
/* Warnings and error messages. */