From: Aaron Merey <amerey@redhat.com>
To: keiths@redhat.com
Cc: gdb-patches@sourceware.org, Aaron Merey <amerey@redhat.com>
Subject: [PATCH] gdb/source: Fix open_source_file error handling
Date: Wed, 1 Feb 2023 20:38:53 -0500 [thread overview]
Message-ID: <20230202013853.119354-1-amerey@redhat.com> (raw)
In-Reply-To: <b5604d39-918d-5d72-cbab-1629ec2e0c11@redhat.com>
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
next prev parent reply other threads:[~2023-02-02 1:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-28 3:37 Aaron Merey
2023-02-01 19:45 ` Keith Seitz
2023-02-02 1:38 ` Aaron Merey [this message]
2023-02-08 17:53 ` Tom Tromey
2023-02-02 10:29 ` Bruno Larsen
2023-02-08 17:44 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230202013853.119354-1-amerey@redhat.com \
--to=amerey@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=keiths@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).