* [PATCH 0/5] Fix using an exec file with target: prefix @ 2023-09-27 14:22 Andrew Burgess 2023-09-27 14:22 ` [PATCH 1/5] gdb: some additional filename styling Andrew Burgess ` (5 more replies) 0 siblings, 6 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess I spotted that if a user specifies an executable with a target: prefix, e.g.: (gdb) file target:/path/to/exec Then GDB _almost_ does what we'd want. This is definitely suppsosed to work as there's some code in place to support this .... but there's also a bug. Patch #5 fixes this issue. Patches #1 to #4 are various bits of cleanup and refactoring that fell out while I was working on patch #5. --- Andrew Burgess (5): gdb: some additional filename styling gdb: use archive name in warning when appropriate gdb: remove use of a static buffer for building error strings gdb: remove print_sys_errmsg gdb: fix reread_symbols when an objfile has target: prefix gdb/inflow.c | 3 +- gdb/main.c | 7 +- gdb/procfs.c | 32 ++-- gdb/source.c | 18 +- gdb/symfile.c | 30 ++- gdb/target.c | 15 ++ gdb/target.h | 39 ++++ gdb/testsuite/gdb.server/target-exec-file.c | 22 +++ gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++ gdb/utils.c | 9 +- gdb/utils.h | 8 +- gdb/windows-nat.c | 2 +- 12 files changed, 313 insertions(+), 46 deletions(-) create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8 -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/5] gdb: some additional filename styling 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess @ 2023-09-27 14:22 ` Andrew Burgess 2023-09-27 14:22 ` [PATCH 2/5] gdb: use archive name in warning when appropriate Andrew Burgess ` (4 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Fix up another couple of places where we can apply filename styling. --- gdb/source.c | 3 ++- gdb/symfile.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/gdb/source.c b/gdb/source.c index 9c701e866a6..5bdd729be8b 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -594,7 +594,8 @@ add_path (const char *dirname, char **which_path, int parse_separators) print_sys_errmsg (name, save_errno); } else if ((st.st_mode & S_IFMT) != S_IFDIR) - warning (_("%s is not a directory."), name); + warning (_("%ps is not a directory."), + styled_string (file_name_style.style (), name)); } append: diff --git a/gdb/symfile.c b/gdb/symfile.c index 76b5e1b8fe7..19cf9c911f9 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2482,8 +2482,9 @@ reread_symbols (int from_tty) if (res != 0) { /* FIXME, should use print_sys_errmsg but it's not filtered. */ - gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"), - objfile_name (objfile)); + gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), + styled_string (file_name_style.style (), + objfile_name (objfile))); continue; } new_modtime = new_statbuf.st_mtime; -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/5] gdb: use archive name in warning when appropriate 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess 2023-09-27 14:22 ` [PATCH 1/5] gdb: some additional filename styling Andrew Burgess @ 2023-09-27 14:22 ` Andrew Burgess 2023-09-27 14:22 ` [PATCH 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess ` (3 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess While working on some other patch I noticed that in reread_symbols there is a diagnostic message that can be printed, and in some cases we might use the wrong filename in the message. The code in question is checking to see if an objfile has changed on disk, we do this by stat-ing the on disk file and checking the mtime. If this file has been removed from disk then we print a message that the file has been removed, however, if the objfile is within an archive then we stat the archive itself, but then warn that the component within the archive has disappeared. I think it makes more sense to say that the archive has disappeared. The last related commit is this one: commit 02aeec7bde8ec8a04d14a5637e75f1c6ab899e23 Date: Tue Apr 27 21:01:30 2010 +0000 Check library name rather than member name when rereading symbols. Though this just makes the code to stat the archive unconditional, the code in question existed before this commit. However, the above commit doesn't include any tests, and seems to indicate that the problem being addressed was seen on Darwin. I'm not sure how to setup a test where GDB is using an objfile from within an archive, and so there's no tests for this commit... ... but if someone can let me know how I can setup a suitable test, please let me know and I'll try to get something working. --- gdb/symfile.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/gdb/symfile.c b/gdb/symfile.c index 19cf9c911f9..145621f3c67 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2459,7 +2459,6 @@ reread_symbols (int from_tty) { long new_modtime; struct stat new_statbuf; - int res; std::vector<struct objfile *> new_objfiles; for (objfile *objfile : current_program_space->objfiles ()) @@ -2475,16 +2474,18 @@ reread_symbols (int from_tty) `ar', often called a `static library' on most systems, though a `shared library' on AIX is also an archive), then you should stat on the archive name, not member name. */ + const char *filename; if (objfile->obfd->my_archive) - res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf); + filename = bfd_get_filename (objfile->obfd->my_archive); else - res = stat (objfile_name (objfile), &new_statbuf); + filename = objfile_name (objfile); + + int res = stat (filename, &new_statbuf); if (res != 0) { /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), - styled_string (file_name_style.style (), - objfile_name (objfile))); + styled_string (file_name_style.style (), filename)); continue; } new_modtime = new_statbuf.st_mtime; -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/5] gdb: remove use of a static buffer for building error strings 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess 2023-09-27 14:22 ` [PATCH 1/5] gdb: some additional filename styling Andrew Burgess 2023-09-27 14:22 ` [PATCH 2/5] gdb: use archive name in warning when appropriate Andrew Burgess @ 2023-09-27 14:22 ` Andrew Burgess 2023-09-27 14:22 ` [PATCH 4/5] gdb: remove print_sys_errmsg Andrew Burgess ` (2 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess I noticed in procfs.c that we use a static buffer for building error strings when we could easily use std::string and string_printf to achieve the same result, this commit does that. I ran into this while performing a further refactor/cleanup that will be presented in a later patch in this series, and thought this was worth splitting out into its own patch. As far as I can tell, only Solaris uses procfs.c, so I did a test build on a Solaris machine, and I don't believe that I've broken anything. There should be no user visible changes after this commit. --- gdb/procfs.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/gdb/procfs.c b/gdb/procfs.c index 9443b074483..706ccf0965c 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -255,8 +255,6 @@ typedef struct procinfo { int threads_valid: 1; } procinfo; -static char errmsg[128]; /* shared error msg buffer */ - /* Function prototypes for procinfo module: */ static procinfo *find_procinfo_or_die (int pid, int tid); @@ -595,17 +593,19 @@ static void proc_resume (procinfo *pi, ptid_t scope_ptid, static void proc_warn (procinfo *pi, const char *func, int line) { - xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s", - func, line, pi->pathname); - print_sys_errmsg (errmsg, errno); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); + print_sys_errmsg (errmsg.c_str (), saved_errno); } static void proc_error (procinfo *pi, const char *func, int line) { - xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s", - func, line, pi->pathname); - perror_with_name (errmsg); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); + perror_with_name (errmsg.c_str (), saved_errno); } /* Updates the status struct in the procinfo. There is a 'valid' @@ -1805,11 +1805,12 @@ do_attach (ptid_t ptid) if (!open_procinfo_files (pi, FD_CTL)) { - gdb_printf (gdb_stderr, "procfs:%d -- ", __LINE__); - xsnprintf (errmsg, sizeof (errmsg), - "do_attach: couldn't open /proc file for process %d", - ptid.pid ()); - dead_procinfo (pi, errmsg, NOKILL); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs:%d -- do_attach: couldn't open /proc " + "file for process %d", __LINE__, ptid.pid ()); + errno = saved_errno; + dead_procinfo (pi, errmsg.c_str (), NOKILL); } /* Stop the process (if it isn't already stopped). */ -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5] gdb: remove print_sys_errmsg 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess ` (2 preceding siblings ...) 2023-09-27 14:22 ` [PATCH 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess @ 2023-09-27 14:22 ` Andrew Burgess 2023-09-28 12:06 ` Andrew Burgess 2023-09-27 14:22 ` [PATCH 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess 5 siblings, 1 reply; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess This started with me running into this comment in symfile.c: /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), styled_string (file_name_style.style (), filename)); In this particular case I think I disagree with the comment; I think the output should be a warning rather than just a message printed to gdb_stdout, I think when the executable, or some other objfile that is currently being debugged, disappears from disk, this is likely an unexpected situation, and worth warning the user about. So, in theory, I could just call print_sys_errmsg and remove the comment, but that would mean loosing the filename styling in the output... so in the end I remove the comment and updated the code to call warning. But that got me looking at print_sys_errmsg and how it's used. Currently the function takes a string and an errno, and prints, to stderr, the string followed by the result of calling strerror on the errno. In some places the string passed to print_sys_errmsg is just a filename, and this is used when something goes wrong. In these cases, I think calling warning rather than gdb_printf to gdb_stderr, would be better, and in fact, in a couple of places we manually print a "warning" prefix, and then call print_sys_errmsg. And so, for these users I have added a new function warning_filename_and_errno, which takes a filename, which is printed with styling, and an errno, which is passed through strerror and the resulting string printed. This new function calls warning to print its output. I then updated some of the print_sys_errmsg users to use this new function. Some other users of print_sys_errmsg are also emitting what is clearly a warning, however, the string being passed in is more than just a filename, so the new warning_filename_and_errno function can't be used, it would style the whole string. For these users I have switched to calling warning directly, this allows me to style the warning message correctly. Finally, in inflow.c there is one last call to print_sys_errmsg, in this case I just inlined the definition of print_sys_errmsg. This is a really weird case, as after printing this message GDB just does a hard exit. This is pretty old code, dating back to the initial GDB import, I guess it should be updated to call error() maybe, but I'm reluctant to make this change as part of this commit, just in case there's some reason why we can't throw an error at this point. With that done there are now no users of print_sys_errmsg, and so the old function can be removed. While I was doing all of the above I added some additional filename styling in soure.c, this is in an else block where the if contained the print_sys_errmsg call, so these felt related. And finally, while I was updating the uses of print_sys_errmsg in procfs.c, I noticed that we used a static errmsg buffer to format some error strings. As the above changes got rid of one of the users of errmsg I also removed the other two users, and the static buffer. --- gdb/inflow.c | 3 ++- gdb/main.c | 7 +------ gdb/procfs.c | 17 ++++++++++------- gdb/source.c | 15 ++++----------- gdb/symfile.c | 5 ++--- gdb/utils.c | 9 ++++----- gdb/utils.h | 8 +++++++- gdb/windows-nat.c | 2 +- 8 files changed, 31 insertions(+), 35 deletions(-) diff --git a/gdb/inflow.c b/gdb/inflow.c index 767cfd02c48..095c5f03672 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -766,7 +766,8 @@ check_syscall (const char *msg, int result) { if (result < 0) { - print_sys_errmsg (msg, errno); + gdb_printf (gdb_stderr, "%s:%s.\n", msg, + safe_strerror (errno)); _exit (1); } } diff --git a/gdb/main.c b/gdb/main.c index cf46f6acb20..1ca0fdeee1b 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir) struct stat st; if (stat (new_datadir, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (new_datadir, save_errno); - } + warning_filename_and_errno (new_datadir, errno); else if (!S_ISDIR (st.st_mode)) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), new_datadir)); diff --git a/gdb/procfs.c b/gdb/procfs.c index 706ccf0965c..48e9f3dd4b5 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -46,6 +46,7 @@ #include "gdbsupport/scoped_fd.h" #include "gdbsupport/pathstuff.h" #include "gdbsupport/buildargv.h" +#include "cli/cli-style.h" /* This module provides the interface between GDB and the /proc file system, which is used on many versions of Unix @@ -558,7 +559,7 @@ enum { NOKILL, KILL }; static void dead_procinfo (procinfo *pi, const char *msg, int kill_p) { - print_sys_errmsg (pi->pathname, errno); + warning_filename_and_errno (pi->pathname, errno); if (kill_p == KILL) kill (pi->pid, SIGKILL); @@ -594,18 +595,20 @@ static void proc_warn (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - print_sys_errmsg (errmsg.c_str (), saved_errno); + warning ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } static void proc_error (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - perror_with_name (errmsg.c_str (), saved_errno); + error ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } /* Updates the status struct in the procinfo. There is a 'valid' diff --git a/gdb/source.c b/gdb/source.c index 5bdd729be8b..f648adc4520 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators) a directory/etc, then having them in the path should be harmless. */ if (stat (name, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (name, save_errno); - } + warning_filename_and_errno (name, errno); else if ((st.st_mode & S_IFMT) != S_IFDIR) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), name)); @@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline, if (!(flags & PRINT_SOURCE_LINES_NOERROR)) { const char *filename = symtab_to_filename_for_display (s); - int len = strlen (filename) + 100; - char *name = (char *) alloca (len); - - xsnprintf (name, len, "%d\t%s", line, filename); - print_sys_errmsg (name, errcode); + warning (_("%d\t%ps: %s"), line, + styled_string (file_name_style.style (), filename), + safe_strerror (errcode)); } else { diff --git a/gdb/symfile.c b/gdb/symfile.c index 145621f3c67..10074e281bd 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2483,9 +2483,8 @@ reread_symbols (int from_tty) int res = stat (filename, &new_statbuf); if (res != 0) { - /* FIXME, should use print_sys_errmsg but it's not filtered. */ - gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), - styled_string (file_name_style.style (), filename)); + warning (_("`%ps' has disappeared; keeping its symbols."), + styled_string (file_name_style.style (), filename)); continue; } new_modtime = new_statbuf.st_mtime; diff --git a/gdb/utils.c b/gdb/utils.c index 2f545337cd4..a191d26a007 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -619,14 +619,13 @@ perror_warning_with_name (const char *string) warning (_("%s"), combined.c_str ()); } -/* Print the system error message for ERRCODE, and also mention STRING - as the file name for which the error was encountered. */ +/* See utils.h. */ void -print_sys_errmsg (const char *string, int errcode) +warning_filename_and_errno (const char *filename, int saved_errno) { - const char *err = safe_strerror (errcode); - gdb_printf (gdb_stderr, "%s: %s.\n", string, err); + warning (_("%ps: %s"), styled_string (file_name_style.style (), filename), + safe_strerror (saved_errno)); } /* Control C eventually causes this to be called, at a convenient time. */ diff --git a/gdb/utils.h b/gdb/utils.h index c5364fa4b35..f646b300530 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -275,7 +275,13 @@ 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); +/* Issue a warning formatted as '<filename>: <explanation>', where + <filename> is FILENAME with filename styling applied. As such, don't + pass anything more than a filename in this string. The <explanation> + is a string returned from calling safe_strerror(SAVED_ERRNO). */ + +extern void warning_filename_and_errno (const char *filename, + int saved_errno); \f /* Warnings and error messages. */ diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 5a897dbfe76..7a139c8d36f 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file, tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY); if (tty < 0) { - print_sys_errmsg (inferior_tty.c_str (), errno); + warning_filename_and_errno (inferior_tty.c_str (), errno); ostdin = ostdout = ostderr = -1; } else -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] gdb: remove print_sys_errmsg 2023-09-27 14:22 ` [PATCH 4/5] gdb: remove print_sys_errmsg Andrew Burgess @ 2023-09-28 12:06 ` Andrew Burgess 0 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 12:06 UTC (permalink / raw) To: gdb-patches The posted version of this patch didn't include some test fixes -- I pulled this series out of another series I had locally, and I'd committed the test fixes into a patch that never made it into this version by mistake. Here's an updated patch that contains the two test fixes to take account of the output changes from this patch. Thanks, Andrew --- commit bcc3a21e682e3c109b120db0ef020b48d1933e62 Author: Andrew Burgess <aburgess@redhat.com> Date: Sun Sep 24 12:37:40 2023 +0100 gdb: remove print_sys_errmsg This started with me running into this comment in symfile.c: /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), styled_string (file_name_style.style (), filename)); In this particular case I think I disagree with the comment; I think the output should be a warning rather than just a message printed to gdb_stdout, I think when the executable, or some other objfile that is currently being debugged, disappears from disk, this is likely an unexpected situation, and worth warning the user about. So, in theory, I could just call print_sys_errmsg and remove the comment, but that would mean loosing the filename styling in the output... so in the end I remove the comment and updated the code to call warning. But that got me looking at print_sys_errmsg and how it's used. Currently the function takes a string and an errno, and prints, to stderr, the string followed by the result of calling strerror on the errno. In some places the string passed to print_sys_errmsg is just a filename, and this is used when something goes wrong. In these cases, I think calling warning rather than gdb_printf to gdb_stderr, would be better, and in fact, in a couple of places we manually print a "warning" prefix, and then call print_sys_errmsg. And so, for these users I have added a new function warning_filename_and_errno, which takes a filename, which is printed with styling, and an errno, which is passed through strerror and the resulting string printed. This new function calls warning to print its output. I then updated some of the print_sys_errmsg users to use this new function. Some other users of print_sys_errmsg are also emitting what is clearly a warning, however, the string being passed in is more than just a filename, so the new warning_filename_and_errno function can't be used, it would style the whole string. For these users I have switched to calling warning directly, this allows me to style the warning message correctly. Finally, in inflow.c there is one last call to print_sys_errmsg, in this case I just inlined the definition of print_sys_errmsg. This is a really weird case, as after printing this message GDB just does a hard exit. This is pretty old code, dating back to the initial GDB import, I guess it should be updated to call error() maybe, but I'm reluctant to make this change as part of this commit, just in case there's some reason why we can't throw an error at this point. With that done there are now no users of print_sys_errmsg, and so the old function can be removed. While I was doing all of the above I added some additional filename styling in soure.c, this is in an else block where the if contained the print_sys_errmsg call, so these felt related. And finally, while I was updating the uses of print_sys_errmsg in procfs.c, I noticed that we used a static errmsg buffer to format some error strings. As the above changes got rid of one of the users of errmsg I also removed the other two users, and the static buffer. There were a couple of tests that depended on the existing output message format that needed updating. In one case we gained an extra 'warning: ' prefix, and in the other 'Warning: ' becomes 'warning: ', I think in both cases the new output is an improvement. diff --git a/gdb/inflow.c b/gdb/inflow.c index 767cfd02c48..095c5f03672 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -766,7 +766,8 @@ check_syscall (const char *msg, int result) { if (result < 0) { - print_sys_errmsg (msg, errno); + gdb_printf (gdb_stderr, "%s:%s.\n", msg, + safe_strerror (errno)); _exit (1); } } diff --git a/gdb/main.c b/gdb/main.c index cf46f6acb20..1ca0fdeee1b 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir) struct stat st; if (stat (new_datadir, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (new_datadir, save_errno); - } + warning_filename_and_errno (new_datadir, errno); else if (!S_ISDIR (st.st_mode)) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), new_datadir)); diff --git a/gdb/procfs.c b/gdb/procfs.c index 706ccf0965c..48e9f3dd4b5 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -46,6 +46,7 @@ #include "gdbsupport/scoped_fd.h" #include "gdbsupport/pathstuff.h" #include "gdbsupport/buildargv.h" +#include "cli/cli-style.h" /* This module provides the interface between GDB and the /proc file system, which is used on many versions of Unix @@ -558,7 +559,7 @@ enum { NOKILL, KILL }; static void dead_procinfo (procinfo *pi, const char *msg, int kill_p) { - print_sys_errmsg (pi->pathname, errno); + warning_filename_and_errno (pi->pathname, errno); if (kill_p == KILL) kill (pi->pid, SIGKILL); @@ -594,18 +595,20 @@ static void proc_warn (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - print_sys_errmsg (errmsg.c_str (), saved_errno); + warning ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } static void proc_error (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - perror_with_name (errmsg.c_str (), saved_errno); + error ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } /* Updates the status struct in the procinfo. There is a 'valid' diff --git a/gdb/source.c b/gdb/source.c index 5bdd729be8b..f648adc4520 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators) a directory/etc, then having them in the path should be harmless. */ if (stat (name, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (name, save_errno); - } + warning_filename_and_errno (name, errno); else if ((st.st_mode & S_IFMT) != S_IFDIR) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), name)); @@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline, if (!(flags & PRINT_SOURCE_LINES_NOERROR)) { const char *filename = symtab_to_filename_for_display (s); - int len = strlen (filename) + 100; - char *name = (char *) alloca (len); - - xsnprintf (name, len, "%d\t%s", line, filename); - print_sys_errmsg (name, errcode); + warning (_("%d\t%ps: %s"), line, + styled_string (file_name_style.style (), filename), + safe_strerror (errcode)); } else { diff --git a/gdb/symfile.c b/gdb/symfile.c index 145621f3c67..10074e281bd 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2483,9 +2483,8 @@ reread_symbols (int from_tty) int res = stat (filename, &new_statbuf); if (res != 0) { - /* FIXME, should use print_sys_errmsg but it's not filtered. */ - gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), - styled_string (file_name_style.style (), filename)); + warning (_("`%ps' has disappeared; keeping its symbols."), + styled_string (file_name_style.style (), filename)); continue; } new_modtime = new_statbuf.st_mtime; diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp index 4a9302fb9b7..0588cb35d87 100644 --- a/gdb/testsuite/gdb.base/catch-syscall.exp +++ b/gdb/testsuite/gdb.base/catch-syscall.exp @@ -392,7 +392,7 @@ proc test_catch_syscall_fail_nodatadir {} { # Make sure GDB doesn't load the syscalls xml from the system # data directory. gdb_test "set data-directory /the/path/to/nowhere" \ - "Warning: /the/path/to/nowhere: .*" + "warning: /the/path/to/nowhere: .*" # Testing to see if we receive a warning when calling "catch # syscall" without XML support (without datadir). @@ -658,7 +658,7 @@ proc do_syscall_tests_without_xml {} { # Make sure GDB doesn't load the syscalls xml from the system data # directory. gdb_test "set data-directory /the/path/to/nowhere" \ - "Warning: /the/path/to/nowhere: .*" + "warning: /the/path/to/nowhere: .*" # Let's test if we can catch syscalls without XML support. # We should succeed, but GDB is not supposed to print syscall names. diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp index 7e28931be22..07aa6afbee7 100644 --- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp @@ -167,7 +167,7 @@ if { $psymtabs_p } { } gdb_test "l imported_unit.c:1" \ - "1\timported_unit.c: No such file or directory\." + "warning: 1\timported_unit.c: No such file or directory" gdb_test "info source" "\r\nCurrent source file is imported_unit.c\r\n.*" \ "info source for imported_unit.c" diff --git a/gdb/utils.c b/gdb/utils.c index 2f545337cd4..a191d26a007 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -619,14 +619,13 @@ perror_warning_with_name (const char *string) warning (_("%s"), combined.c_str ()); } -/* Print the system error message for ERRCODE, and also mention STRING - as the file name for which the error was encountered. */ +/* See utils.h. */ void -print_sys_errmsg (const char *string, int errcode) +warning_filename_and_errno (const char *filename, int saved_errno) { - const char *err = safe_strerror (errcode); - gdb_printf (gdb_stderr, "%s: %s.\n", string, err); + warning (_("%ps: %s"), styled_string (file_name_style.style (), filename), + safe_strerror (saved_errno)); } /* Control C eventually causes this to be called, at a convenient time. */ diff --git a/gdb/utils.h b/gdb/utils.h index c5364fa4b35..f646b300530 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -275,7 +275,13 @@ 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); +/* Issue a warning formatted as '<filename>: <explanation>', where + <filename> is FILENAME with filename styling applied. As such, don't + pass anything more than a filename in this string. The <explanation> + is a string returned from calling safe_strerror(SAVED_ERRNO). */ + +extern void warning_filename_and_errno (const char *filename, + int saved_errno); \f /* Warnings and error messages. */ diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 5a897dbfe76..7a139c8d36f 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file, tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY); if (tty < 0) { - print_sys_errmsg (inferior_tty.c_str (), errno); + warning_filename_and_errno (inferior_tty.c_str (), errno); ostdin = ostdout = ostderr = -1; } else ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess ` (3 preceding siblings ...) 2023-09-27 14:22 ` [PATCH 4/5] gdb: remove print_sys_errmsg Andrew Burgess @ 2023-09-27 14:22 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-27 14:22 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess When using a remote target, it is possible to tell GDB that the executable to be debugged is located on the remote machine, like this: (gdb) target extended-remote :54321 ... snip ... (gdb) file target:/tmp/hello.x Reading /tmp/hello.x from remote target... warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead. Reading /tmp/hello.x from remote target... Reading symbols from target:/tmp/hello.x... (gdb) So far so good. However, when we try to start the inferior we run into a small problem: (gdb) set remote exec-file /tmp/hello.x (gdb) start `target:/tmp/hello.x' has disappeared; keeping its symbols. Temporary breakpoint 1 at 0x401198: file /tmp/hello.c, line 18. Starting program: target:/tmp/hello.x ... snip ... Temporary breakpoint 1, main () at /tmp/hello.c:18 18 printf ("Hello World\n"); (gdb) Notice this line: `target:/tmp/hello.x' has disappeared; keeping its symbols. That's wrong, the executable hasn't been removed, GDB just doesn't know how to check if the remote file has changed, and so falls back to assuming that the file has been removed. In this commit I add support to reread_symbols for stat-ing remote files. With this in place GDB no longer complains when using a remote executable file. --- gdb/symfile.c | 19 +- gdb/target.c | 15 ++ gdb/target.h | 39 ++++ gdb/testsuite/gdb.server/target-exec-file.c | 22 +++ gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++ 5 files changed, 268 insertions(+), 1 deletion(-) create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp diff --git a/gdb/symfile.c b/gdb/symfile.c index 10074e281bd..b21c8ead225 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2480,7 +2480,24 @@ reread_symbols (int from_tty) else filename = objfile_name (objfile); - int res = stat (filename, &new_statbuf); + int res; + if (is_target_filename (filename)) + { + fileio_error target_errno; + + scoped_target_fileio_open fd (nullptr, + (filename + + strlen (TARGET_SYSROOT_PREFIX)), + FILEIO_O_RDONLY, 0, false, + &target_errno); + if (fd.get () == -1) + res = -1; + else + res = target_fileio_fstat (fd.get (), &new_statbuf, + &target_errno); + } + else + res = stat (filename, &new_statbuf); if (res != 0) { warning (_("`%ps' has disappeared; keeping its symbols."), diff --git a/gdb/target.c b/gdb/target.c index 8cb4fa1736d..0d1608bb5fe 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -54,6 +54,7 @@ #include "target-connection.h" #include "valprint.h" #include "cli/cli-decode.h" +#include "cli/cli-style.h" static void generic_tls_error (void) ATTRIBUTE_NORETURN; @@ -3577,6 +3578,20 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename) return gdb::unique_xmalloc_ptr<char> (bufstr); } +/* See target.h. */ + +scoped_target_fileio_open::~scoped_target_fileio_open () +{ + if (m_fd != -1) + { + fileio_error target_errno; + if (target_fileio_close (m_fd, &target_errno) != 0) + warning (_("failed to close %ps: %s"), + styled_string (file_name_style.style (), + m_filename.c_str ()), + safe_strerror (fileio_error_to_host (target_errno))); + } +} static int default_region_ok_for_hw_watchpoint (struct target_ops *self, diff --git a/gdb/target.h b/gdb/target.h index 936ae79219c..5712adc367d 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -2235,6 +2235,45 @@ extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc with EIO. */ extern void fileio_handles_invalidate_target (target_ops *targ); +/* A class that performs a target_fileio_open within a scope. On leaving + the scope target_fileio_close is called. + + If the close fails then a warning is issued. */ +struct scoped_target_fileio_open +{ + /* Call target_fileio_open passing all the arguments through. See + target_fileio_open for the meaning of all arguments. */ + scoped_target_fileio_open (struct inferior *inf, + const char *filename, int flags, + int mode, bool warn_if_slow, + fileio_error *target_errno) + : m_filename (filename) + { + m_fd = target_fileio_open (inf, filename, flags, mode, warn_if_slow, + target_errno); + } + + /* Close the file that was opened in the constructor, issue a warning if + the close fails. */ + ~scoped_target_fileio_open (); + + /* Return the file descriptor for the opened file. This can only be used + with target_fileio_* calls. This will return -1 if the open failed. */ + int get () const + { + return m_fd; + } + +private: + /* The filename that we opened. Stored so we can give a warning if the + close fails for any reason. */ + std::string m_filename; + + /* The target file descriptor for the opened file, or -1 if the open + failed for any reason. */ + int m_fd = -1; +}; + /* Tracepoint-related operations. */ extern void target_trace_init (); diff --git a/gdb/testsuite/gdb.server/target-exec-file.c b/gdb/testsuite/gdb.server/target-exec-file.c new file mode 100644 index 00000000000..3a264f239ed --- /dev/null +++ b/gdb/testsuite/gdb.server/target-exec-file.c @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2023 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +main () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.server/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp new file mode 100644 index 00000000000..9260df8b88d --- /dev/null +++ b/gdb/testsuite/gdb.server/target-exec-file.exp @@ -0,0 +1,174 @@ +# This testcase is part of GDB, the GNU debugger. + +# Copyright 2023 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test GDB's handling of using a file with a 'target:' prefix as the +# executable file. This test includes checking what happens when the +# file on the target system changes and GDB needs to reload it. + +load_lib gdbserver-support.exp + +require allow_gdbserver_tests !use_gdb_stub + +standard_testfile + +if { [build_executable "failed to prepare" $testfile $srcfile debug] } { + return -1 +} + +clean_restart + +# Some boards specifically set the sysroot to the empty string to +# avoid copying files from the target. But for this test we do want +# to copy files from the target, so set the sysroot back to 'target:'. +# +# This is fine so long as we're not using a board file that sets the +# sysroot to something else -- but none of the standard boards do +# this, and plenty of other tests mess with the sysroot, so I guess we +# don't worry about that too much. +gdb_test "set sysroot target:" ".*" + +# Make sure we're disconnected, in case we're testing with an +# extended-remote board, therefore already connected. +gdb_test "disconnect" ".*" + +# Ensure the executable is on the target. +set target_exec [gdb_remote_download target $binfile] + +# We're going to be restarting the inferior. Lets ask GDB not to +# prompt us if this is the right thing to do. +gdb_test_no_output "set confirm off" + +# Start gdbserver, but always in extended-remote mode, and then +# connect to it from GDB. +set res [gdbserver_start "--multi" $target_exec] +set gdbserver_protocol "extended-remote" +set gdbserver_gdbport [lindex $res 1] +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport + +# Issue a 'file' command and parse the output. We look for a couple +# of specific things to ensure that we are correctly reading the exec +# from the remote target. +set saw_read_of_remote_exec false +set saw_read_of_syms_from_exec false +gdb_test_multiple "file target:$target_exec" "run file command" { + -re "^file target:\[^\r\n\]+\r\n" { + exp_continue + } + + -re "^Reading (\[^\r\n\]+) from remote target\\.\\.\\.\r\n" { + set filename $expect_out(1,string) + if { $filename eq $target_exec } { + set saw_read_of_remote_exec true + } + exp_continue + } + + -re "^warning: File transfers from remote targets can be slow\[^\r\n\]+\r\n" { + exp_continue + } + + -re "^Reading symbols from target:(\[^\r\n\]+)\\.\\.\\.\r\n" { + set filename $expect_out(1,string) + if { $filename eq $target_exec } { + set saw_read_of_syms_from_exec true + } + exp_continue + } + + -re "^Expanding full symbols from \[^\r\n\]+\r\n" { + exp_continue + } + + -re "^$gdb_prompt $" { + pass $gdb_test_name + } +} + +gdb_assert { $saw_read_of_remote_exec } \ + "exec was read from the remote target" + +gdb_assert { $saw_read_of_syms_from_exec } \ + "symbols were read from remote exec file" + +# Start the inferior (with the 'start' command), use TESTNAME for any +# pass/fail calls. EXPECT_REREAD should be true or false and +# indicates if we expect to too a line like: +# +# `FILE' has changed; re-reading symbols. +proc start_inferior { testname expect_reread } { + with_test_prefix $testname { + if { [gdb_start_cmd] < 0 } { + fail "start command" + return -1 + } + + set saw_reread false + gdb_test_multiple "" "stopped at main" { + -re "^start\\s*\r\n" { + exp_continue + } + -re "^`\[^\r\n\]+' has changed; re-reading symbols\\.\r\n" { + set saw_reread true + exp_continue + } + -re "^Reading \[^\r\n\]+ from remote target\\.\\.\\.\r\n" { + exp_continue + } + -re "^Expanding full symbols from \[^\r\n\]+\\.\\.\\.\r\n" { + exp_continue + } + -re "^Temporary breakpoint $::decimal at $::hex: \[^\r\n\]+\r\n" { + exp_continue + } + -re "^Starting program: \[^\r\n\]+\r\n" { + exp_continue + } + -re "^\\s*\r\n" { + exp_continue + } + -re "^Temporary breakpoint $::decimal, main \\(\\) at .*$::gdb_prompt $" { + pass $testname + } + } + + gdb_assert { $expect_reread == $saw_reread } \ + "check symbol re-read behaviour" + } +} + +# Start the inferior for the first time. The symbols were already +# read from the file when the 'file' command was used, we should not +# see the symbols re-read now. +start_inferior "start inferior the first time" false + +# Re-start the inferior. The executable is unchanged so we should not +# see the symbol file being re-read. +start_inferior "start inferior a second time" false + +# Delay for a short while so, when we touch the exec, we know the +# timestamp will change. +sleep 1 +set res [remote_exec target "touch $target_exec"] +set status [lindex $res 0] +if { $status != 0 } { + fail "touching executable on target" + return -1 +} + +# Start the inferior again, we expect to see the symbols being re-read +# from the remote file. +start_inferior "start inferior a third time" true -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 0/5] Fix using an exec file with target: prefix 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess ` (4 preceding siblings ...) 2023-09-27 14:22 ` [PATCH 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 1/5] gdb: some additional filename styling Andrew Burgess ` (5 more replies) 5 siblings, 6 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess In V2: - Fixed a test regression in patch #4, - Patch #5 has been completely changed. Tom Tromey pointed me to a previous series of his which (a) never got merged, and (b) included a much better solution to this problem. I've pulled out just the part I need to resolve the issue in that patch. --- Andrew Burgess (5): gdb: some additional filename styling gdb: use archive name in warning when appropriate gdb: remove use of a static buffer for building error strings gdb: remove print_sys_errmsg gdb: fix reread_symbols when an objfile has target: prefix gdb/inflow.c | 3 +- gdb/main.c | 7 +- gdb/procfs.c | 32 ++-- gdb/source.c | 18 +- gdb/symfile.c | 21 +-- gdb/testsuite/gdb.base/catch-syscall.exp | 4 +- gdb/testsuite/gdb.dwarf2/imported-unit.exp | 2 +- gdb/testsuite/gdb.server/target-exec-file.c | 22 +++ gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++ gdb/utils.c | 9 +- gdb/utils.h | 8 +- gdb/windows-nat.c | 2 +- 12 files changed, 247 insertions(+), 55 deletions(-) create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8 -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 1/5] gdb: some additional filename styling 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 2/5] gdb: use archive name in warning when appropriate Andrew Burgess ` (4 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Fix up another couple of places where we can apply filename styling. --- gdb/source.c | 3 ++- gdb/symfile.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/gdb/source.c b/gdb/source.c index 9c701e866a6..5bdd729be8b 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -594,7 +594,8 @@ add_path (const char *dirname, char **which_path, int parse_separators) print_sys_errmsg (name, save_errno); } else if ((st.st_mode & S_IFMT) != S_IFDIR) - warning (_("%s is not a directory."), name); + warning (_("%ps is not a directory."), + styled_string (file_name_style.style (), name)); } append: diff --git a/gdb/symfile.c b/gdb/symfile.c index 76b5e1b8fe7..19cf9c911f9 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2482,8 +2482,9 @@ reread_symbols (int from_tty) if (res != 0) { /* FIXME, should use print_sys_errmsg but it's not filtered. */ - gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"), - objfile_name (objfile)); + gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), + styled_string (file_name_style.style (), + objfile_name (objfile))); continue; } new_modtime = new_statbuf.st_mtime; -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 2/5] gdb: use archive name in warning when appropriate 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 1/5] gdb: some additional filename styling Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess ` (3 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess While working on some other patch I noticed that in reread_symbols there is a diagnostic message that can be printed, and in some cases we might use the wrong filename in the message. The code in question is checking to see if an objfile has changed on disk, we do this by stat-ing the on disk file and checking the mtime. If this file has been removed from disk then we print a message that the file has been removed, however, if the objfile is within an archive then we stat the archive itself, but then warn that the component within the archive has disappeared. I think it makes more sense to say that the archive has disappeared. The last related commit is this one: commit 02aeec7bde8ec8a04d14a5637e75f1c6ab899e23 Date: Tue Apr 27 21:01:30 2010 +0000 Check library name rather than member name when rereading symbols. Though this just makes the code to stat the archive unconditional, the code in question existed before this commit. However, the above commit doesn't include any tests, and seems to indicate that the problem being addressed was seen on Darwin. I'm not sure how to setup a test where GDB is using an objfile from within an archive, and so there's no tests for this commit... ... but if someone can let me know how I can setup a suitable test, please let me know and I'll try to get something working. --- gdb/symfile.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/gdb/symfile.c b/gdb/symfile.c index 19cf9c911f9..145621f3c67 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2459,7 +2459,6 @@ reread_symbols (int from_tty) { long new_modtime; struct stat new_statbuf; - int res; std::vector<struct objfile *> new_objfiles; for (objfile *objfile : current_program_space->objfiles ()) @@ -2475,16 +2474,18 @@ reread_symbols (int from_tty) `ar', often called a `static library' on most systems, though a `shared library' on AIX is also an archive), then you should stat on the archive name, not member name. */ + const char *filename; if (objfile->obfd->my_archive) - res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf); + filename = bfd_get_filename (objfile->obfd->my_archive); else - res = stat (objfile_name (objfile), &new_statbuf); + filename = objfile_name (objfile); + + int res = stat (filename, &new_statbuf); if (res != 0) { /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), - styled_string (file_name_style.style (), - objfile_name (objfile))); + styled_string (file_name_style.style (), filename)); continue; } new_modtime = new_statbuf.st_mtime; -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 3/5] gdb: remove use of a static buffer for building error strings 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 1/5] gdb: some additional filename styling Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 2/5] gdb: use archive name in warning when appropriate Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 4/5] gdb: remove print_sys_errmsg Andrew Burgess ` (2 subsequent siblings) 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess I noticed in procfs.c that we use a static buffer for building error strings when we could easily use std::string and string_printf to achieve the same result, this commit does that. I ran into this while performing a further refactor/cleanup that will be presented in a later patch in this series, and thought this was worth splitting out into its own patch. As far as I can tell, only Solaris uses procfs.c, so I did a test build on a Solaris machine, and I don't believe that I've broken anything. There should be no user visible changes after this commit. --- gdb/procfs.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/gdb/procfs.c b/gdb/procfs.c index 9443b074483..706ccf0965c 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -255,8 +255,6 @@ typedef struct procinfo { int threads_valid: 1; } procinfo; -static char errmsg[128]; /* shared error msg buffer */ - /* Function prototypes for procinfo module: */ static procinfo *find_procinfo_or_die (int pid, int tid); @@ -595,17 +593,19 @@ static void proc_resume (procinfo *pi, ptid_t scope_ptid, static void proc_warn (procinfo *pi, const char *func, int line) { - xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s", - func, line, pi->pathname); - print_sys_errmsg (errmsg, errno); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); + print_sys_errmsg (errmsg.c_str (), saved_errno); } static void proc_error (procinfo *pi, const char *func, int line) { - xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s", - func, line, pi->pathname); - perror_with_name (errmsg); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); + perror_with_name (errmsg.c_str (), saved_errno); } /* Updates the status struct in the procinfo. There is a 'valid' @@ -1805,11 +1805,12 @@ do_attach (ptid_t ptid) if (!open_procinfo_files (pi, FD_CTL)) { - gdb_printf (gdb_stderr, "procfs:%d -- ", __LINE__); - xsnprintf (errmsg, sizeof (errmsg), - "do_attach: couldn't open /proc file for process %d", - ptid.pid ()); - dead_procinfo (pi, errmsg, NOKILL); + int saved_errno = errno; + std::string errmsg + = string_printf ("procfs:%d -- do_attach: couldn't open /proc " + "file for process %d", __LINE__, ptid.pid ()); + errno = saved_errno; + dead_procinfo (pi, errmsg.c_str (), NOKILL); } /* Stop the process (if it isn't already stopped). */ -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 4/5] gdb: remove print_sys_errmsg 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess ` (2 preceding siblings ...) 2023-09-28 14:00 ` [PATCHv2 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess 2023-10-02 19:13 ` [PATCHv2 0/5] Fix using an exec file with " Tom Tromey 5 siblings, 0 replies; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess This started with me running into this comment in symfile.c: /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), styled_string (file_name_style.style (), filename)); In this particular case I think I disagree with the comment; I think the output should be a warning rather than just a message printed to gdb_stdout, I think when the executable, or some other objfile that is currently being debugged, disappears from disk, this is likely an unexpected situation, and worth warning the user about. So, in theory, I could just call print_sys_errmsg and remove the comment, but that would mean loosing the filename styling in the output... so in the end I remove the comment and updated the code to call warning. But that got me looking at print_sys_errmsg and how it's used. Currently the function takes a string and an errno, and prints, to stderr, the string followed by the result of calling strerror on the errno. In some places the string passed to print_sys_errmsg is just a filename, and this is used when something goes wrong. In these cases, I think calling warning rather than gdb_printf to gdb_stderr, would be better, and in fact, in a couple of places we manually print a "warning" prefix, and then call print_sys_errmsg. And so, for these users I have added a new function warning_filename_and_errno, which takes a filename, which is printed with styling, and an errno, which is passed through strerror and the resulting string printed. This new function calls warning to print its output. I then updated some of the print_sys_errmsg users to use this new function. Some other users of print_sys_errmsg are also emitting what is clearly a warning, however, the string being passed in is more than just a filename, so the new warning_filename_and_errno function can't be used, it would style the whole string. For these users I have switched to calling warning directly, this allows me to style the warning message correctly. Finally, in inflow.c there is one last call to print_sys_errmsg, in this case I just inlined the definition of print_sys_errmsg. This is a really weird case, as after printing this message GDB just does a hard exit. This is pretty old code, dating back to the initial GDB import, I guess it should be updated to call error() maybe, but I'm reluctant to make this change as part of this commit, just in case there's some reason why we can't throw an error at this point. With that done there are now no users of print_sys_errmsg, and so the old function can be removed. While I was doing all of the above I added some additional filename styling in soure.c, this is in an else block where the if contained the print_sys_errmsg call, so these felt related. And finally, while I was updating the uses of print_sys_errmsg in procfs.c, I noticed that we used a static errmsg buffer to format some error strings. As the above changes got rid of one of the users of errmsg I also removed the other two users, and the static buffer. There were a couple of tests that depended on the existing output message format that needed updating. In one case we gained an extra 'warning: ' prefix, and in the other 'Warning: ' becomes 'warning: ', I think in both cases the new output is an improvement. --- gdb/inflow.c | 3 ++- gdb/main.c | 7 +------ gdb/procfs.c | 17 ++++++++++------- gdb/source.c | 15 ++++----------- gdb/symfile.c | 5 ++--- gdb/testsuite/gdb.base/catch-syscall.exp | 4 ++-- gdb/testsuite/gdb.dwarf2/imported-unit.exp | 2 +- gdb/utils.c | 9 ++++----- gdb/utils.h | 8 +++++++- gdb/windows-nat.c | 2 +- 10 files changed, 34 insertions(+), 38 deletions(-) diff --git a/gdb/inflow.c b/gdb/inflow.c index 767cfd02c48..095c5f03672 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -766,7 +766,8 @@ check_syscall (const char *msg, int result) { if (result < 0) { - print_sys_errmsg (msg, errno); + gdb_printf (gdb_stderr, "%s:%s.\n", msg, + safe_strerror (errno)); _exit (1); } } diff --git a/gdb/main.c b/gdb/main.c index cf46f6acb20..1ca0fdeee1b 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir) struct stat st; if (stat (new_datadir, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (new_datadir, save_errno); - } + warning_filename_and_errno (new_datadir, errno); else if (!S_ISDIR (st.st_mode)) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), new_datadir)); diff --git a/gdb/procfs.c b/gdb/procfs.c index 706ccf0965c..48e9f3dd4b5 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -46,6 +46,7 @@ #include "gdbsupport/scoped_fd.h" #include "gdbsupport/pathstuff.h" #include "gdbsupport/buildargv.h" +#include "cli/cli-style.h" /* This module provides the interface between GDB and the /proc file system, which is used on many versions of Unix @@ -558,7 +559,7 @@ enum { NOKILL, KILL }; static void dead_procinfo (procinfo *pi, const char *msg, int kill_p) { - print_sys_errmsg (pi->pathname, errno); + warning_filename_and_errno (pi->pathname, errno); if (kill_p == KILL) kill (pi->pid, SIGKILL); @@ -594,18 +595,20 @@ static void proc_warn (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - print_sys_errmsg (errmsg.c_str (), saved_errno); + warning ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } static void proc_error (procinfo *pi, const char *func, int line) { int saved_errno = errno; - std::string errmsg - = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname); - perror_with_name (errmsg.c_str (), saved_errno); + error ("procfs: %s line %d, %ps: %s", + func, line, styled_string (file_name_style.style (), + pi->pathname), + safe_strerror (saved_errno)); } /* Updates the status struct in the procinfo. There is a 'valid' diff --git a/gdb/source.c b/gdb/source.c index 5bdd729be8b..f648adc4520 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators) a directory/etc, then having them in the path should be harmless. */ if (stat (name, &st) < 0) - { - int save_errno = errno; - - gdb_printf (gdb_stderr, "Warning: "); - print_sys_errmsg (name, save_errno); - } + warning_filename_and_errno (name, errno); else if ((st.st_mode & S_IFMT) != S_IFDIR) warning (_("%ps is not a directory."), styled_string (file_name_style.style (), name)); @@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline, if (!(flags & PRINT_SOURCE_LINES_NOERROR)) { const char *filename = symtab_to_filename_for_display (s); - int len = strlen (filename) + 100; - char *name = (char *) alloca (len); - - xsnprintf (name, len, "%d\t%s", line, filename); - print_sys_errmsg (name, errcode); + warning (_("%d\t%ps: %s"), line, + styled_string (file_name_style.style (), filename), + safe_strerror (errcode)); } else { diff --git a/gdb/symfile.c b/gdb/symfile.c index 145621f3c67..10074e281bd 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2483,9 +2483,8 @@ reread_symbols (int from_tty) int res = stat (filename, &new_statbuf); if (res != 0) { - /* FIXME, should use print_sys_errmsg but it's not filtered. */ - gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"), - styled_string (file_name_style.style (), filename)); + warning (_("`%ps' has disappeared; keeping its symbols."), + styled_string (file_name_style.style (), filename)); continue; } new_modtime = new_statbuf.st_mtime; diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp index 4a9302fb9b7..0588cb35d87 100644 --- a/gdb/testsuite/gdb.base/catch-syscall.exp +++ b/gdb/testsuite/gdb.base/catch-syscall.exp @@ -392,7 +392,7 @@ proc test_catch_syscall_fail_nodatadir {} { # Make sure GDB doesn't load the syscalls xml from the system # data directory. gdb_test "set data-directory /the/path/to/nowhere" \ - "Warning: /the/path/to/nowhere: .*" + "warning: /the/path/to/nowhere: .*" # Testing to see if we receive a warning when calling "catch # syscall" without XML support (without datadir). @@ -658,7 +658,7 @@ proc do_syscall_tests_without_xml {} { # Make sure GDB doesn't load the syscalls xml from the system data # directory. gdb_test "set data-directory /the/path/to/nowhere" \ - "Warning: /the/path/to/nowhere: .*" + "warning: /the/path/to/nowhere: .*" # Let's test if we can catch syscalls without XML support. # We should succeed, but GDB is not supposed to print syscall names. diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp index 7e28931be22..07aa6afbee7 100644 --- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp +++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp @@ -167,7 +167,7 @@ if { $psymtabs_p } { } gdb_test "l imported_unit.c:1" \ - "1\timported_unit.c: No such file or directory\." + "warning: 1\timported_unit.c: No such file or directory" gdb_test "info source" "\r\nCurrent source file is imported_unit.c\r\n.*" \ "info source for imported_unit.c" diff --git a/gdb/utils.c b/gdb/utils.c index 2f545337cd4..a191d26a007 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -619,14 +619,13 @@ perror_warning_with_name (const char *string) warning (_("%s"), combined.c_str ()); } -/* Print the system error message for ERRCODE, and also mention STRING - as the file name for which the error was encountered. */ +/* See utils.h. */ void -print_sys_errmsg (const char *string, int errcode) +warning_filename_and_errno (const char *filename, int saved_errno) { - const char *err = safe_strerror (errcode); - gdb_printf (gdb_stderr, "%s: %s.\n", string, err); + warning (_("%ps: %s"), styled_string (file_name_style.style (), filename), + safe_strerror (saved_errno)); } /* Control C eventually causes this to be called, at a convenient time. */ diff --git a/gdb/utils.h b/gdb/utils.h index c5364fa4b35..f646b300530 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -275,7 +275,13 @@ 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); +/* Issue a warning formatted as '<filename>: <explanation>', where + <filename> is FILENAME with filename styling applied. As such, don't + pass anything more than a filename in this string. The <explanation> + is a string returned from calling safe_strerror(SAVED_ERRNO). */ + +extern void warning_filename_and_errno (const char *filename, + int saved_errno); \f /* Warnings and error messages. */ diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index 5a897dbfe76..7a139c8d36f 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file, tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY); if (tty < 0) { - print_sys_errmsg (inferior_tty.c_str (), errno); + warning_filename_and_errno (inferior_tty.c_str (), errno); ostdin = ostdout = ostderr = -1; } else -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess ` (3 preceding siblings ...) 2023-09-28 14:00 ` [PATCHv2 4/5] gdb: remove print_sys_errmsg Andrew Burgess @ 2023-09-28 14:00 ` Andrew Burgess 2023-09-28 18:17 ` Tom Tromey 2023-10-02 19:13 ` [PATCHv2 0/5] Fix using an exec file with " Tom Tromey 5 siblings, 1 reply; 19+ messages in thread From: Andrew Burgess @ 2023-09-28 14:00 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess, Tom Tromey When using a remote target, it is possible to tell GDB that the executable to be debugged is located on the remote machine, like this: (gdb) target extended-remote :54321 ... snip ... (gdb) file target:/tmp/hello.x Reading /tmp/hello.x from remote target... warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead. Reading /tmp/hello.x from remote target... Reading symbols from target:/tmp/hello.x... (gdb) So far so good. However, when we try to start the inferior we run into a small problem: (gdb) set remote exec-file /tmp/hello.x (gdb) start `target:/tmp/hello.x' has disappeared; keeping its symbols. Temporary breakpoint 1 at 0x401198: file /tmp/hello.c, line 18. Starting program: target:/tmp/hello.x ... snip ... Temporary breakpoint 1, main () at /tmp/hello.c:18 18 printf ("Hello World\n"); (gdb) Notice this line: `target:/tmp/hello.x' has disappeared; keeping its symbols. That's wrong, the executable hasn't been removed, GDB just doesn't know how to check if the remote file has changed, and so falls back to assuming that the file has been removed. In this commit I update reread_symbols to use bfd_stat instead of a direct stat call, this adds support for target: files, and fixes the problem. This change was proposed before in this commit: https://inbox.sourceware.org/gdb-patches/20200114210956.25115-3-tromey@adacore.com/ However, that patch never got merged, and seemed to get stuck discussing issues around gnulib stat vs system stat as used by BFD. I didn't 100% understand the issues discussed in that thread, however, I think the problem with the previous thread related to the changes in gdb_bfd.c, rather than to the change in symfile.c. As such, I think this change might be acceptable, my reasoning is: - the objfile::mtime field is set by a call to bfd_get_mtime (see objfiles.c), which calls bfd_stat under the hood. This will end up using the system stat, - In symfile.c we currently call stat directly, which will call the gnulib stat, which, if I understand the above thread correctly, might give a different result to the system stat in some cases, - By switching to using bfd_stat in symfile.c we should now be consistently calling the system stat, Co-Authored-By: Tom Tromey <tromey@adacore.com> --- gdb/symfile.c | 18 +- gdb/testsuite/gdb.server/target-exec-file.c | 22 +++ gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++ 3 files changed, 203 insertions(+), 11 deletions(-) create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp diff --git a/gdb/symfile.c b/gdb/symfile.c index 10074e281bd..87f8e0a3ea6 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2470,19 +2470,15 @@ reread_symbols (int from_tty) if (objfile->separate_debug_objfile_backlink) continue; - /* If this object is from an archive (what you usually create with - `ar', often called a `static library' on most systems, though - a `shared library' on AIX is also an archive), then you should - stat on the archive name, not member name. */ - const char *filename; - if (objfile->obfd->my_archive) - filename = bfd_get_filename (objfile->obfd->my_archive); - else - filename = objfile_name (objfile); - - int res = stat (filename, &new_statbuf); + int res = bfd_stat (objfile->obfd.get (), &new_statbuf); if (res != 0) { + const char *filename; + if (objfile->obfd->my_archive) + filename = bfd_get_filename (objfile->obfd->my_archive); + else + filename = objfile_name (objfile); + warning (_("`%ps' has disappeared; keeping its symbols."), styled_string (file_name_style.style (), filename)); continue; diff --git a/gdb/testsuite/gdb.server/target-exec-file.c b/gdb/testsuite/gdb.server/target-exec-file.c new file mode 100644 index 00000000000..3a264f239ed --- /dev/null +++ b/gdb/testsuite/gdb.server/target-exec-file.c @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2023 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +main () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.server/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp new file mode 100644 index 00000000000..9260df8b88d --- /dev/null +++ b/gdb/testsuite/gdb.server/target-exec-file.exp @@ -0,0 +1,174 @@ +# This testcase is part of GDB, the GNU debugger. + +# Copyright 2023 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test GDB's handling of using a file with a 'target:' prefix as the +# executable file. This test includes checking what happens when the +# file on the target system changes and GDB needs to reload it. + +load_lib gdbserver-support.exp + +require allow_gdbserver_tests !use_gdb_stub + +standard_testfile + +if { [build_executable "failed to prepare" $testfile $srcfile debug] } { + return -1 +} + +clean_restart + +# Some boards specifically set the sysroot to the empty string to +# avoid copying files from the target. But for this test we do want +# to copy files from the target, so set the sysroot back to 'target:'. +# +# This is fine so long as we're not using a board file that sets the +# sysroot to something else -- but none of the standard boards do +# this, and plenty of other tests mess with the sysroot, so I guess we +# don't worry about that too much. +gdb_test "set sysroot target:" ".*" + +# Make sure we're disconnected, in case we're testing with an +# extended-remote board, therefore already connected. +gdb_test "disconnect" ".*" + +# Ensure the executable is on the target. +set target_exec [gdb_remote_download target $binfile] + +# We're going to be restarting the inferior. Lets ask GDB not to +# prompt us if this is the right thing to do. +gdb_test_no_output "set confirm off" + +# Start gdbserver, but always in extended-remote mode, and then +# connect to it from GDB. +set res [gdbserver_start "--multi" $target_exec] +set gdbserver_protocol "extended-remote" +set gdbserver_gdbport [lindex $res 1] +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport + +# Issue a 'file' command and parse the output. We look for a couple +# of specific things to ensure that we are correctly reading the exec +# from the remote target. +set saw_read_of_remote_exec false +set saw_read_of_syms_from_exec false +gdb_test_multiple "file target:$target_exec" "run file command" { + -re "^file target:\[^\r\n\]+\r\n" { + exp_continue + } + + -re "^Reading (\[^\r\n\]+) from remote target\\.\\.\\.\r\n" { + set filename $expect_out(1,string) + if { $filename eq $target_exec } { + set saw_read_of_remote_exec true + } + exp_continue + } + + -re "^warning: File transfers from remote targets can be slow\[^\r\n\]+\r\n" { + exp_continue + } + + -re "^Reading symbols from target:(\[^\r\n\]+)\\.\\.\\.\r\n" { + set filename $expect_out(1,string) + if { $filename eq $target_exec } { + set saw_read_of_syms_from_exec true + } + exp_continue + } + + -re "^Expanding full symbols from \[^\r\n\]+\r\n" { + exp_continue + } + + -re "^$gdb_prompt $" { + pass $gdb_test_name + } +} + +gdb_assert { $saw_read_of_remote_exec } \ + "exec was read from the remote target" + +gdb_assert { $saw_read_of_syms_from_exec } \ + "symbols were read from remote exec file" + +# Start the inferior (with the 'start' command), use TESTNAME for any +# pass/fail calls. EXPECT_REREAD should be true or false and +# indicates if we expect to too a line like: +# +# `FILE' has changed; re-reading symbols. +proc start_inferior { testname expect_reread } { + with_test_prefix $testname { + if { [gdb_start_cmd] < 0 } { + fail "start command" + return -1 + } + + set saw_reread false + gdb_test_multiple "" "stopped at main" { + -re "^start\\s*\r\n" { + exp_continue + } + -re "^`\[^\r\n\]+' has changed; re-reading symbols\\.\r\n" { + set saw_reread true + exp_continue + } + -re "^Reading \[^\r\n\]+ from remote target\\.\\.\\.\r\n" { + exp_continue + } + -re "^Expanding full symbols from \[^\r\n\]+\\.\\.\\.\r\n" { + exp_continue + } + -re "^Temporary breakpoint $::decimal at $::hex: \[^\r\n\]+\r\n" { + exp_continue + } + -re "^Starting program: \[^\r\n\]+\r\n" { + exp_continue + } + -re "^\\s*\r\n" { + exp_continue + } + -re "^Temporary breakpoint $::decimal, main \\(\\) at .*$::gdb_prompt $" { + pass $testname + } + } + + gdb_assert { $expect_reread == $saw_reread } \ + "check symbol re-read behaviour" + } +} + +# Start the inferior for the first time. The symbols were already +# read from the file when the 'file' command was used, we should not +# see the symbols re-read now. +start_inferior "start inferior the first time" false + +# Re-start the inferior. The executable is unchanged so we should not +# see the symbol file being re-read. +start_inferior "start inferior a second time" false + +# Delay for a short while so, when we touch the exec, we know the +# timestamp will change. +sleep 1 +set res [remote_exec target "touch $target_exec"] +set status [lindex $res 0] +if { $status != 0 } { + fail "touching executable on target" + return -1 +} + +# Start the inferior again, we expect to see the symbols being re-read +# from the remote file. +start_inferior "start inferior a third time" true -- 2.25.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-09-28 14:00 ` [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess @ 2023-09-28 18:17 ` Tom Tromey 2023-09-29 10:20 ` Andrew Burgess 0 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-09-28 18:17 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey Andrew> I didn't 100% understand the issues discussed in that thread, however, Andrew> I think the problem with the previous thread related to the changes in Andrew> gdb_bfd.c, rather than to the change in symfile.c. As such, I think Andrew> this change might be acceptable, my reasoning is: Andrew> - the objfile::mtime field is set by a call to bfd_get_mtime (see Andrew> objfiles.c), which calls bfd_stat under the hood. This will end Andrew> up using the system stat, Andrew> - In symfile.c we currently call stat directly, which will call the Andrew> gnulib stat, which, if I understand the above thread correctly, Andrew> might give a different result to the system stat in some cases, Andrew> - By switching to using bfd_stat in symfile.c we should now be Andrew> consistently calling the system stat, The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be a problem here. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-09-28 18:17 ` Tom Tromey @ 2023-09-29 10:20 ` Andrew Burgess 2023-10-02 14:19 ` Tom Tromey 0 siblings, 1 reply; 19+ messages in thread From: Andrew Burgess @ 2023-09-29 10:20 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches, Tom Tromey Tom Tromey <tromey@adacore.com> writes: > Andrew> I didn't 100% understand the issues discussed in that thread, however, > Andrew> I think the problem with the previous thread related to the changes in > Andrew> gdb_bfd.c, rather than to the change in symfile.c. As such, I think > Andrew> this change might be acceptable, my reasoning is: > > Andrew> - the objfile::mtime field is set by a call to bfd_get_mtime (see > Andrew> objfiles.c), which calls bfd_stat under the hood. This will end > Andrew> up using the system stat, > > Andrew> - In symfile.c we currently call stat directly, which will call the > Andrew> gnulib stat, which, if I understand the above thread correctly, > Andrew> might give a different result to the system stat in some cases, > > Andrew> - By switching to using bfd_stat in symfile.c we should now be > Andrew> consistently calling the system stat, > > The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be > a problem here. OK, but the original mtime is captured via a call to bfd_stat. Isn't the problem when we have two mismatched calls. Using bfd_stat in one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from GDB in another place (a.k.a. gnulib stat/fstat). In this patch I'm proposing that we _consistently_ call bfd_stat. Sure that might disagree with system stat/fstat -- but who cares? So long as the time being calculated and compared to is a BFD time_t result then we should be fine .... or am I really not understanding the problem? Thanks, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-09-29 10:20 ` Andrew Burgess @ 2023-10-02 14:19 ` Tom Tromey 2023-10-02 16:05 ` Andrew Burgess 0 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-10-02 14:19 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: >> The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be >> a problem here. Andrew> OK, but the original mtime is captured via a call to bfd_stat. Andrew> Isn't the problem when we have two mismatched calls. Using bfd_stat in Andrew> one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from Andrew> GDB in another place (a.k.a. gnulib stat/fstat). Andrew> In this patch I'm proposing that we _consistently_ call bfd_stat. Sure Andrew> that might disagree with system stat/fstat -- but who cares? So long as Andrew> the time being calculated and compared to is a BFD time_t result then we Andrew> should be fine .... or am I really not understanding the problem? I think the fstat in gdb_bfd.c means that we will always re-read the debug info, because now gdb_bfd_open will use fstat (and this is what is recorded in the hash table), but reread_symbols will use bfd_stat. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-10-02 14:19 ` Tom Tromey @ 2023-10-02 16:05 ` Andrew Burgess 2023-10-02 17:00 ` Tom Tromey 0 siblings, 1 reply; 19+ messages in thread From: Andrew Burgess @ 2023-10-02 16:05 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey, gdb-patches Tom Tromey <tromey@adacore.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > >>> The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be >>> a problem here. > > Andrew> OK, but the original mtime is captured via a call to bfd_stat. > > Andrew> Isn't the problem when we have two mismatched calls. Using bfd_stat in > Andrew> one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from > Andrew> GDB in another place (a.k.a. gnulib stat/fstat). > > Andrew> In this patch I'm proposing that we _consistently_ call bfd_stat. Sure > Andrew> that might disagree with system stat/fstat -- but who cares? So long as > Andrew> the time being calculated and compared to is a BFD time_t result then we > Andrew> should be fine .... or am I really not understanding the problem? > > I think the fstat in gdb_bfd.c means that we will always re-read the > debug info, because now gdb_bfd_open will use fstat (and this is what is > recorded in the hash table), but reread_symbols will use bfd_stat. I really don't think this is the case. The code in reread_symbols is this: int res; struct stat new_statbuf; if (objfile->obfd->my_archive) res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf); else res = stat (objfile_name (objfile), &new_statbuf); if (res != 0) { /* FIXME, should use print_sys_errmsg but it's not filtered. */ gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"), objfile_name (objfile)); continue; } time_t new_modtime = new_statbuf.st_mtime; if (new_modtime != objfile->mtime) { gdb_printf (_("`%ps' has changed; re-reading symbols.\n"), styled_string (file_name_style.style (), objfile_name (objfile))); ... } So we perform a system 'stat' call and compare this to objfile::mtime. And objfile::mtime is initialised in objfiles.c with this code: if (obfd != nullptr) { mtime = bfd_get_mtime (obfd.get ()); /* Build section table. */ build_objfile_section_table (this); } Where bfd_get_mtime is going to fetch a value by calling bfd_stat, which will either by the system stat (linked into libbfd.a), or will be GDB's remote fileio stat call. The fstat call you are looking at in gdb_bfd.c is I believe only used as part of GDB's internal caching mechanism for BFDs. I can't see anywhere that these mtime values can escape gdb_bfd.c, nor do I see anywhere that these values are compared to values returned by bfd_stat. We know that the value in reread_symbols comes from the system stat. So what I'm missing, is the path by which objfile::mtime gets set from anywhere other that within libbfd.a, such that it might get contaminated by a gnulib stat result. Thanks, Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix 2023-10-02 16:05 ` Andrew Burgess @ 2023-10-02 17:00 ` Tom Tromey 0 siblings, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-10-02 17:00 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: Andrew> We know that the value in reread_symbols comes from the system stat. So Andrew> what I'm missing, is the path by which objfile::mtime gets set from Andrew> anywhere other that within libbfd.a, such that it might get contaminated Andrew> by a gnulib stat result. I was digging some more and I found: commit d706b69e48268ccf3e95fd29b5374ac94c3a507b Author: Tom Tromey <tromey@adacore.com> Date: Tue Sep 8 10:20:44 2020 -0600 Do not adjust mtime timezone on Windows ... where I put in a patch to disable gnulib's stat replacement. So I think your patch is totally fine, and I don't really know why I didn't land my other changes after this. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv2 0/5] Fix using an exec file with target: prefix 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess ` (4 preceding siblings ...) 2023-09-28 14:00 ` [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess @ 2023-10-02 19:13 ` Tom Tromey 5 siblings, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-10-02 19:13 UTC (permalink / raw) To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess >>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes: Andrew> - Patch #5 has been completely changed. Tom Tromey pointed me to a Andrew> previous series of his which (a) never got merged, and (b) Andrew> included a much better solution to this problem. I've pulled out Andrew> just the part I need to resolve the issue in that patch. I looked at my series again, and it's worse than the status quo in some ways; for instance it requires opening a BFD before checking the cache. But, like I mentioned elsewhere, I did seem to fix the gnulib stat problem, which means a lot of that series is obsolete. I think you've picked up the only remaining good bit, which is the use of bfd_stat in reread_symbols. So, I think this series is ok. Approved-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-10-02 19:13 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-27 14:22 [PATCH 0/5] Fix using an exec file with target: prefix Andrew Burgess 2023-09-27 14:22 ` [PATCH 1/5] gdb: some additional filename styling Andrew Burgess 2023-09-27 14:22 ` [PATCH 2/5] gdb: use archive name in warning when appropriate Andrew Burgess 2023-09-27 14:22 ` [PATCH 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess 2023-09-27 14:22 ` [PATCH 4/5] gdb: remove print_sys_errmsg Andrew Burgess 2023-09-28 12:06 ` Andrew Burgess 2023-09-27 14:22 ` [PATCH 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 0/5] Fix using an exec file with " Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 1/5] gdb: some additional filename styling Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 2/5] gdb: use archive name in warning when appropriate Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 3/5] gdb: remove use of a static buffer for building error strings Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 4/5] gdb: remove print_sys_errmsg Andrew Burgess 2023-09-28 14:00 ` [PATCHv2 5/5] gdb: fix reread_symbols when an objfile has target: prefix Andrew Burgess 2023-09-28 18:17 ` Tom Tromey 2023-09-29 10:20 ` Andrew Burgess 2023-10-02 14:19 ` Tom Tromey 2023-10-02 16:05 ` Andrew Burgess 2023-10-02 17:00 ` Tom Tromey 2023-10-02 19:13 ` [PATCHv2 0/5] Fix using an exec file with " 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).