[2/3] Consistently use BFD's time
Commit Message
gdb uses the gnulib stat, while BFD does not. This can lead to
inconsistencies between the two, because the gnulib stat adjusts for
timezones.
This patch changes gdb to use bfd_stat when examining a BFD's time.
This avoids the problem; and also opens the door to caching target
BFDs as well.
One possible downside here is that gdb must now create a BFD before
checking the cache.
gdb/ChangeLog
2020-01-14 Tom Tromey <tromey@adacore.com>
* gdb_bfd.c (gdb_bfd_open): Use bfd_stat.
* symfile.c (reread_symbols): Use bfd_stat.
Change-Id: Ie937630a221cbae15809c99327b47c1cbce141c0
---
gdb/ChangeLog | 5 +++++
gdb/gdb_bfd.c | 49 +++++++++++++++++++++++++------------------------
gdb/symfile.c | 5 +----
3 files changed, 31 insertions(+), 28 deletions(-)
Comments
On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey <tromey@adacore.com> wrote:
>
> gdb uses the gnulib stat, while BFD does not. This can lead to
> inconsistencies between the two, because the gnulib stat adjusts for
> timezones.
>
> This patch changes gdb to use bfd_stat when examining a BFD's time.
> This avoids the problem; and also opens the door to caching target
> BFDs as well.
>
> One possible downside here is that gdb must now create a BFD before
> checking the cache.
>
> gdb/ChangeLog
> 2020-01-14 Tom Tromey <tromey@adacore.com>
>
> * gdb_bfd.c (gdb_bfd_open): Use bfd_stat.
> * symfile.c (reread_symbols): Use bfd_stat.
>
> Change-Id: Ie937630a221cbae15809c99327b47c1cbce141c0
> ---
> gdb/ChangeLog | 5 +++++
> gdb/gdb_bfd.c | 49 +++++++++++++++++++++++++------------------------
> gdb/symfile.c | 5 +----
> 3 files changed, 31 insertions(+), 28 deletions(-)
>
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index a1ee7814f32..26968396a15 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -442,8 +442,15 @@ gdb_bfd_open (const char *name, const char *target, int fd)
> }
> }
>
> + /* We open the BFD here so that we can use BFD to get the mtime.
> + This is important because gdb uses the gnulib stat, but BFD does
> + not, and this can lead to differences on some systems. */
Maybe mention that mingw/windows is one of those?
Christian
> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>
> Date: Tue, 14 Jan 2020 14:09:55 -0700
>
> gdb uses the gnulib stat, while BFD does not. This can lead to
> inconsistencies between the two, because the gnulib stat adjusts for
> timezones.
There's one more potential issue with Gnulib's replacement of 'fstat':
it also replaces the definition of 'struct stat', and it does that in
a way that might yield incompatibility between the definition on
<sys/stat.h> the system header and Gnulib's sys/stat.h replacement.
If gdb_bfd.c uses the Gnulib definition of 'struct stat' (as I think
we do everywhere in gdb/), then this replacement might create problems
on MinGW similar to those I reported to the Gnulib list (see the URL I
cited in an earlier message), because bfd_stat uses an incompatible
definition of 'struct stat'.
Of course, given that the Gnulib developers rejected my request not to
override the system definition of 'struct stat', GDB could also ignore
those problems, accepting their judgment.
On 1/15/20 4:07 PM, Eli Zaretskii wrote:
>> From: Tom Tromey <tromey@adacore.com>
>> Cc: Tom Tromey <tromey@adacore.com>
>> Date: Tue, 14 Jan 2020 14:09:55 -0700
>>
>> gdb uses the gnulib stat, while BFD does not. This can lead to
>> inconsistencies between the two, because the gnulib stat adjusts for
>> timezones.
>
> There's one more potential issue with Gnulib's replacement of 'fstat':
> it also replaces the definition of 'struct stat', and it does that in
> a way that might yield incompatibility between the definition on
> <sys/stat.h> the system header and Gnulib's sys/stat.h replacement.
> If gdb_bfd.c uses the Gnulib definition of 'struct stat' (as I think
> we do everywhere in gdb/), then this replacement might create problems
> on MinGW similar to those I reported to the Gnulib list (see the URL I
> cited in an earlier message), because bfd_stat uses an incompatible
> definition of 'struct stat'.
>
> Of course, given that the Gnulib developers rejected my request not to
> override the system definition of 'struct stat', GDB could also ignore
> those problems, accepting their judgment.
I think that we need to:
- #undef stat before including bfd headers.
- Redefine it afterwards back to rpl_stat (*)
- add some kind of wrapper around bfd_stat (like maybe called gdb_bfd_stat)
that handles the mismatch. Something like:
int
gdb_bfd_stat (bfd *abfd, struct stat *statbuf)
{
#undef stat
struct stat st;
int res = bfd_stat (abfd, &st);
if (res != 0)
return res;
#define COPY(FIELD) statbuf->FIELD = st.FIELD
COPY (st_dev);
// ... copy over all relevant fields ...
#undef COPY
#define stat rpl_stat
}
(*) there's probably some '#ifdef _GL...' we can check to know
whether we need to do this.
Thanks,
Pedro Alves
On Thu, Jan 16, 2020 at 3:37 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 1/15/20 4:07 PM, Eli Zaretskii wrote:
> >> From: Tom Tromey <tromey@adacore.com>
> >> Cc: Tom Tromey <tromey@adacore.com>
> >> Date: Tue, 14 Jan 2020 14:09:55 -0700
> >>
> >> gdb uses the gnulib stat, while BFD does not. This can lead to
> >> inconsistencies between the two, because the gnulib stat adjusts for
> >> timezones.
> >
> > There's one more potential issue with Gnulib's replacement of 'fstat':
> > it also replaces the definition of 'struct stat', and it does that in
> > a way that might yield incompatibility between the definition on
> > <sys/stat.h> the system header and Gnulib's sys/stat.h replacement.
> > If gdb_bfd.c uses the Gnulib definition of 'struct stat' (as I think
> > we do everywhere in gdb/), then this replacement might create problems
> > on MinGW similar to those I reported to the Gnulib list (see the URL I
> > cited in an earlier message), because bfd_stat uses an incompatible
> > definition of 'struct stat'.
> >
> > Of course, given that the Gnulib developers rejected my request not to
> > override the system definition of 'struct stat', GDB could also ignore
> > those problems, accepting their judgment.
>
> I think that we need to:
>
> - #undef stat before including bfd headers.
> - Redefine it afterwards back to rpl_stat (*)
> - add some kind of wrapper around bfd_stat (like maybe called gdb_bfd_stat)
Wouldn't it be easier to #define GNULIB_NAMESPACE in this file? That
way, the gnulib stuff stays in gnulib:: (or gdb::, or whatever), and
the global ::stat is the system one.
> that handles the mismatch. Something like:
>
> int
> gdb_bfd_stat (bfd *abfd, struct stat *statbuf)
> {
> #undef stat
> struct stat st;
>
> int res = bfd_stat (abfd, &st);
> if (res != 0)
> return res;
>
> #define COPY(FIELD) statbuf->FIELD = st.FIELD
> COPY (st_dev);
> // ... copy over all relevant fields ...
> #undef COPY
>
> #define stat rpl_stat
> }
>
> (*) there's probably some '#ifdef _GL...' we can check to know
> whether we need to do this.
>
> Thanks,
> Pedro Alves
>
On 1/16/20 8:46 PM, Christian Biesinger via gdb-patches wrote:
> On Thu, Jan 16, 2020 at 3:37 PM Pedro Alves <palves@redhat.com> wrote:
>> I think that we need to:
>>
>> - #undef stat before including bfd headers.
>> - Redefine it afterwards back to rpl_stat (*)
>> - add some kind of wrapper around bfd_stat (like maybe called gdb_bfd_stat)
>
> Wouldn't it be easier to #define GNULIB_NAMESPACE in this file? That
> way, the gnulib stuff stays in gnulib:: (or gdb::, or whatever), and
> the global ::stat is the system one.
Good idea. I suspect it's better to do it in an isolated file than
in the whole symfile.c of whatever the problem was found, so that we
don't have to sprinkle the namespace prefix around. I.e., in a file
that only implements gdb_bfd_stat. At least, until have use a namespace
everywhere. We could also poison bfd_stat so that we're sure nothing
in gdb uses it other than the wrapper.
Thanks,
Pedro Alves
> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 16 Jan 2020 20:37:28 +0000
>
> #define COPY(FIELD) statbuf->FIELD = st.FIELD
> COPY (st_dev);
> // ... copy over all relevant fields ...
> #undef COPY
Copying fields will work, but it would need some care. For example,
the Gnulib replacement of 'struct stat' redefines st_size and st_mtime
to wider data types, so copying from the Gnulib definition to the
system definition might overflow.
Thanks.
> From: Christian Biesinger <cbiesinger@google.com>
> Date: Thu, 16 Jan 2020 15:46:52 -0500
> Cc: Eli Zaretskii <eliz@gnu.org>, Tom Tromey <tromey@adacore.com>,
> gdb-patches <gdb-patches@sourceware.org>
>
> > - #undef stat before including bfd headers.
> > - Redefine it afterwards back to rpl_stat (*)
> > - add some kind of wrapper around bfd_stat (like maybe called gdb_bfd_stat)
>
> Wouldn't it be easier to #define GNULIB_NAMESPACE in this file? That
> way, the gnulib stuff stays in gnulib:: (or gdb::, or whatever), and
> the global ::stat is the system one.
That would work, of course, but is there a chance we store some fields
of 'struct stat' in other data structures or objects, and then use
them elsewhere, for example for comparison with values received from
the Gnulib's 'stat' or 'fstat'?
And in any case, we will have to have a prominent commentary
explaining why we do such strange things.
I wonder whether a better way is not to import the Gnulib 'stat' and
'fstat' modules at all. Are they required by other Gnulib modules,
and if so, by which ones?
Thanks.
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
Eli> I wonder whether a better way is not to import the Gnulib 'stat' and
Eli> 'fstat' modules at all. Are they required by other Gnulib modules,
Eli> and if so, by which ones?
I am wondering this as well. While I think we can track down the bad
spots -- either calling the "wrong" stat or incorrectly comparing values
from the different stats (the patches I sent probably accomplish the
latter already) -- it seems fragile to rely on this.
Tom
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
Eli> I wonder whether a better way is not to import the Gnulib 'stat' and
Eli> 'fstat' modules at all. Are they required by other Gnulib modules,
Eli> and if so, by which ones?
Tom> I am wondering this as well. While I think we can track down the bad
Tom> spots -- either calling the "wrong" stat or incorrectly comparing values
Tom> from the different stats (the patches I sent probably accomplish the
Tom> latter already) -- it seems fragile to rely on this.
It came in originally in a patch I sent and one that Yao sent:
https://sourceware.org/ml/gdb-patches/2013-11/msg00502.html
https://sourceware.org/ml/gdb-patches/2014-11/msg00654.html
I thought maybe removing these workarounds would be ok, but it seems
like it can't be done: when I remove stat and lstat from
update-gnulib.sh, they still appear.
Maybe --avoid=stat will work.
Tom
> From: Tom Tromey <tromey@adacore.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, Christian Biesinger <cbiesinger@google.com>, palves@redhat.com, gdb-patches@sourceware.org
> Date: Fri, 17 Jan 2020 13:55:57 -0700
>
> >>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
>
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> Eli> I wonder whether a better way is not to import the Gnulib 'stat' and
> Eli> 'fstat' modules at all. Are they required by other Gnulib modules,
> Eli> and if so, by which ones?
>
> Tom> I am wondering this as well. While I think we can track down the bad
> Tom> spots -- either calling the "wrong" stat or incorrectly comparing values
> Tom> from the different stats (the patches I sent probably accomplish the
> Tom> latter already) -- it seems fragile to rely on this.
>
> It came in originally in a patch I sent and one that Yao sent:
>
> https://sourceware.org/ml/gdb-patches/2013-11/msg00502.html
This one removed gdb_stat.h, which had replacements for the S_IS*
macros, something that should be easy to bring back, and doesn't
really justify replacing the functions and the struct's themselves.
> https://sourceware.org/ml/gdb-patches/2014-11/msg00654.html
This seems to be about using 'lstat' freely. But I see only one call
to 'lstat' in our sources -- in symfile.c. So maybe having our own
replacement, or even calling 'lstat' conditioned by an #ifdef, would
be a good-enough solution.
> I thought maybe removing these workarounds would be ok, but it seems
> like it can't be done: when I remove stat and lstat from
> update-gnulib.sh, they still appear.
>
> Maybe --avoid=stat will work.
I guess this means some other Gnulib module pulls in 'stat', in which
case --avoid=stat is the way to try to avoid it, yes. (My guess is
that the 'largefile' module causes 'stat' to be pulled in.)
@@ -442,8 +442,15 @@ gdb_bfd_open (const char *name, const char *target, int fd)
}
}
+ /* We open the BFD here so that we can use BFD to get the mtime.
+ This is important because gdb uses the gnulib stat, but BFD does
+ not, and this can lead to differences on some systems. */
+ abfd = bfd_fopen (name, target, FOPEN_RB, fd);
+ if (abfd == NULL)
+ return NULL;
+
search.filename = name;
- if (fstat (fd, &st) < 0)
+ if (bfd_stat (abfd, &st) < 0)
{
/* Weird situation here. */
search.mtime = 0;
@@ -461,38 +468,32 @@ gdb_bfd_open (const char *name, const char *target, int fd)
/* Note that this must compute the same result as hash_bfd. */
hash = htab_hash_string (name);
- /* Note that we cannot use htab_find_slot_with_hash here, because
- opening the BFD may fail; and this would violate hashtab
- invariants. */
- abfd = (struct bfd *) htab_find_with_hash (gdb_bfd_cache, &search, hash);
- if (bfd_sharing && abfd != NULL)
+
+ if (bfd_sharing)
{
- if (debug_bfd_cache)
- fprintf_unfiltered (gdb_stdlog,
- "Reusing cached bfd %s for %s\n",
- host_address_to_string (abfd),
- bfd_get_filename (abfd));
- close (fd);
- return gdb_bfd_ref_ptr::new_reference (abfd);
+ slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, INSERT);
+ if (*slot != nullptr)
+ {
+ bfd_close (abfd);
+ abfd = (bfd *) *slot;
+ if (debug_bfd_cache)
+ fprintf_unfiltered (gdb_stdlog,
+ "Reusing cached bfd %s for %s\n",
+ host_address_to_string (abfd),
+ bfd_get_filename (abfd));
+ close (fd);
+ return gdb_bfd_ref_ptr::new_reference (abfd);
+ }
+ else
+ *slot = abfd;
}
- abfd = bfd_fopen (name, target, FOPEN_RB, fd);
- if (abfd == NULL)
- return NULL;
-
if (debug_bfd_cache)
fprintf_unfiltered (gdb_stdlog,
"Creating new bfd %s for %s\n",
host_address_to_string (abfd),
bfd_get_filename (abfd));
- if (bfd_sharing)
- {
- slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, INSERT);
- gdb_assert (!*slot);
- *slot = abfd;
- }
-
/* It's important to pass the already-computed mtime here, rather
than, say, calling gdb_bfd_ref_ptr::new_reference here. BFD by
default will "stat" the file each time bfd_get_mtime is called --
@@ -2456,10 +2456,7 @@ reread_symbols (void)
`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. */
- if (objfile->obfd->my_archive)
- res = stat (objfile->obfd->my_archive->filename, &new_statbuf);
- else
- res = stat (objfile_name (objfile), &new_statbuf);
+ res = bfd_stat (objfile->obfd, &new_statbuf);
if (res != 0)
{
/* FIXME, should use print_sys_errmsg but it's not filtered. */