[PATCHv2,5/5] gdb: fix reread_symbols when an objfile has target: prefix
Checks
Commit Message
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
Comments
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
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
>>>>> "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
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
>>>>> "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
@@ -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;
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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