* [PATCH 0/3] Fix gdb's BFD cache @ 2020-01-14 21:10 Tom Tromey 2020-01-14 21:10 ` [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c Tom Tromey ` (2 more replies) 0 siblings, 3 replies; 48+ messages in thread From: Tom Tromey @ 2020-01-14 21:10 UTC (permalink / raw) To: gdb-patches Earlier I brought up an issue with gnulib's stat: https://sourceware.org/ml/gdb-patches/2020-01/msg00379.html Here's the patches I wrote to solve this. They worked for me on my test case, which was simply running anything on Windows. Let me know what you think. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c 2020-01-14 21:10 [PATCH 0/3] Fix gdb's BFD cache Tom Tromey @ 2020-01-14 21:10 ` Tom Tromey 2020-01-14 22:26 ` Christian Biesinger via gdb-patches 2020-01-14 21:10 ` [PATCH 2/3] Consistently use BFD's time Tom Tromey 2020-01-14 22:13 ` [PATCH 3/3] Further simplify gdb BFD caching Tom Tromey 2 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-01-14 21:10 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey gdb caches BFDs that come from ordinary files. This code turns out to have a bug where the hash table can become corrupted, causing gdb to crash. When gdb_bfd_open opens the BFD, it uses fstat to get the BFD's mtime. This is used when inserting the entry into gdb_bfd_cache. Then, the function creates the gdb_bfd_data object as a side effect of calling new_reference. This object is used when finding objects in the hash table, and its constructor uses bfd_get_mtime. So, if the file changes between the time the BFD is put into the cache and the time that this object is created, the hash table will be incorrect. When the BFD is later deleted, its entry in the hash table will not be found, and at this point the hash table will point to invalid memory. This patch fixes the bug by ensuring that the mtime that is used for insertion is also used when creating the gdb_bfd_data. gdb/ChangeLog 2020-01-14 Tom Tromey <tromey@adacore.com> PR win32/25302: * gdb_bfd.c (gdb_bfd_data): Add "mt" parameter. (gdb_bfd_init_data): New function. (gdb_bfd_open, gdb_bfd_ref): Use gdb_bfd_init_data. Change-Id: I649ef7060651ac12188fa99d6ae1546caec93594 --- gdb/ChangeLog | 7 +++++++ gdb/gdb_bfd.c | 50 +++++++++++++++++++++++++++++++++++--------------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 5a6dee2d51a..a1ee7814f32 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -59,8 +59,8 @@ static htab_t all_bfds; struct gdb_bfd_data { - gdb_bfd_data (bfd *abfd) - : mtime (bfd_get_mtime (abfd)), + gdb_bfd_data (bfd *abfd, time_t mt) + : mtime (mt), size (bfd_get_size (abfd)), relocation_computed (0), needs_relocations (0), @@ -376,6 +376,30 @@ gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, return result; } +/* A helper function to initialize the data that gdb attaches to each + BFD. */ + +static void +gdb_bfd_init_data (struct bfd *abfd, time_t mtime) +{ + struct gdb_bfd_data *gdata; + void **slot; + + gdb_assert (bfd_usrdata (abfd) == nullptr); + + /* Ask BFD to decompress sections in bfd_get_full_section_contents. */ + abfd->flags |= BFD_DECOMPRESS; + + gdata = new gdb_bfd_data (abfd, mtime); + bfd_set_usrdata (abfd, gdata); + bfd_alloc_data (abfd); + + /* This is the first we've seen it, so add it to the hash table. */ + slot = htab_find_slot (all_bfds, abfd, INSERT); + gdb_assert (slot && !*slot); + *slot = abfd; +} + /* See gdb_bfd.h. */ gdb_bfd_ref_ptr @@ -469,7 +493,14 @@ gdb_bfd_open (const char *name, const char *target, int fd) *slot = abfd; } - return gdb_bfd_ref_ptr::new_reference (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 -- + and since we already entered it into the hash table using this + mtime, if the file changed at the wrong moment, the race would + lead to a hash table corruption. */ + gdb_bfd_init_data (abfd, search.mtime); + return gdb_bfd_ref_ptr (abfd); } /* A helper function that releases any section data attached to the @@ -522,7 +553,6 @@ void gdb_bfd_ref (struct bfd *abfd) { struct gdb_bfd_data *gdata; - void **slot; if (abfd == NULL) return; @@ -541,17 +571,7 @@ gdb_bfd_ref (struct bfd *abfd) return; } - /* Ask BFD to decompress sections in bfd_get_full_section_contents. */ - abfd->flags |= BFD_DECOMPRESS; - - gdata = new gdb_bfd_data (abfd); - bfd_set_usrdata (abfd, gdata); - bfd_alloc_data (abfd); - - /* This is the first we've seen it, so add it to the hash table. */ - slot = htab_find_slot (all_bfds, abfd, INSERT); - gdb_assert (slot && !*slot); - *slot = abfd; + gdb_bfd_init_data (abfd, bfd_get_mtime (abfd)); } /* See gdb_bfd.h. */ -- 2.21.1 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c 2020-01-14 21:10 ` [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c Tom Tromey @ 2020-01-14 22:26 ` Christian Biesinger via gdb-patches 0 siblings, 0 replies; 48+ messages in thread From: Christian Biesinger via gdb-patches @ 2020-01-14 22:26 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey <tromey@adacore.com> wrote: > > gdb caches BFDs that come from ordinary files. This code turns out to > have a bug where the hash table can become corrupted, causing gdb to > crash. > > When gdb_bfd_open opens the BFD, it uses fstat to get the BFD's mtime. > This is used when inserting the entry into gdb_bfd_cache. Then, the > function creates the gdb_bfd_data object as a side effect of calling > new_reference. This object is used when finding objects in the hash > table, and its constructor uses bfd_get_mtime. So, if the file > changes between the time the BFD is put into the cache and the time > that this object is created, the hash table will be incorrect. When > the BFD is later deleted, its entry in the hash table will not be > found, and at this point the hash table will point to invalid memory. > > This patch fixes the bug by ensuring that the mtime that is used for > insertion is also used when creating the gdb_bfd_data. > > gdb/ChangeLog > 2020-01-14 Tom Tromey <tromey@adacore.com> > > PR win32/25302: > * gdb_bfd.c (gdb_bfd_data): Add "mt" parameter. > (gdb_bfd_init_data): New function. > (gdb_bfd_open, gdb_bfd_ref): Use gdb_bfd_init_data. > > Change-Id: I649ef7060651ac12188fa99d6ae1546caec93594 > --- > gdb/ChangeLog | 7 +++++++ > gdb/gdb_bfd.c | 50 +++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 42 insertions(+), 15 deletions(-) > > diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c > index 5a6dee2d51a..a1ee7814f32 100644 > --- a/gdb/gdb_bfd.c > +++ b/gdb/gdb_bfd.c > @@ -59,8 +59,8 @@ static htab_t all_bfds; > > struct gdb_bfd_data > { > - gdb_bfd_data (bfd *abfd) > - : mtime (bfd_get_mtime (abfd)), > + gdb_bfd_data (bfd *abfd, time_t mt) > + : mtime (mt), > size (bfd_get_size (abfd)), > relocation_computed (0), > needs_relocations (0), > @@ -376,6 +376,30 @@ gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, > return result; > } > > +/* A helper function to initialize the data that gdb attaches to each > + BFD. */ > + > +static void > +gdb_bfd_init_data (struct bfd *abfd, time_t mtime) > +{ > + struct gdb_bfd_data *gdata; > + void **slot; Any reason not to move these down to where they are used? > + gdb_assert (bfd_usrdata (abfd) == nullptr); > + > + /* Ask BFD to decompress sections in bfd_get_full_section_contents. */ > + abfd->flags |= BFD_DECOMPRESS; > + > + gdata = new gdb_bfd_data (abfd, mtime); > + bfd_set_usrdata (abfd, gdata); > + bfd_alloc_data (abfd); > + > + /* This is the first we've seen it, so add it to the hash table. */ > + slot = htab_find_slot (all_bfds, abfd, INSERT); > + gdb_assert (slot && !*slot); > + *slot = abfd; > +} > + > /* See gdb_bfd.h. */ > > gdb_bfd_ref_ptr > @@ -469,7 +493,14 @@ gdb_bfd_open (const char *name, const char *target, int fd) > *slot = abfd; > } > > - return gdb_bfd_ref_ptr::new_reference (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 -- > + and since we already entered it into the hash table using this > + mtime, if the file changed at the wrong moment, the race would > + lead to a hash table corruption. */ > + gdb_bfd_init_data (abfd, search.mtime); > + return gdb_bfd_ref_ptr (abfd); > } > > /* A helper function that releases any section data attached to the > @@ -522,7 +553,6 @@ void > gdb_bfd_ref (struct bfd *abfd) > { > struct gdb_bfd_data *gdata; > - void **slot; > > if (abfd == NULL) > return; > @@ -541,17 +571,7 @@ gdb_bfd_ref (struct bfd *abfd) > return; > } > > - /* Ask BFD to decompress sections in bfd_get_full_section_contents. */ > - abfd->flags |= BFD_DECOMPRESS; > - > - gdata = new gdb_bfd_data (abfd); > - bfd_set_usrdata (abfd, gdata); > - bfd_alloc_data (abfd); > - > - /* This is the first we've seen it, so add it to the hash table. */ > - slot = htab_find_slot (all_bfds, abfd, INSERT); > - gdb_assert (slot && !*slot); > - *slot = abfd; > + gdb_bfd_init_data (abfd, bfd_get_mtime (abfd)); > } > > /* See gdb_bfd.h. */ > -- > 2.21.1 > ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 2/3] Consistently use BFD's time 2020-01-14 21:10 [PATCH 0/3] Fix gdb's BFD cache Tom Tromey 2020-01-14 21:10 ` [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c Tom Tromey @ 2020-01-14 21:10 ` Tom Tromey 2020-01-14 23:17 ` Christian Biesinger via gdb-patches 2020-01-15 17:51 ` Eli Zaretskii 2020-01-14 22:13 ` [PATCH 3/3] Further simplify gdb BFD caching Tom Tromey 2 siblings, 2 replies; 48+ messages in thread From: Tom Tromey @ 2020-01-14 21:10 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey 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. */ + 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 -- diff --git a/gdb/symfile.c b/gdb/symfile.c index f7bada75f35..18995351ed3 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -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. */ -- 2.21.1 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-14 21:10 ` [PATCH 2/3] Consistently use BFD's time Tom Tromey @ 2020-01-14 23:17 ` Christian Biesinger via gdb-patches 2020-01-15 17:51 ` Eli Zaretskii 1 sibling, 0 replies; 48+ messages in thread From: Christian Biesinger via gdb-patches @ 2020-01-14 23:17 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-14 21:10 ` [PATCH 2/3] Consistently use BFD's time Tom Tromey 2020-01-14 23:17 ` Christian Biesinger via gdb-patches @ 2020-01-15 17:51 ` Eli Zaretskii 2020-01-16 20:47 ` Pedro Alves 2020-06-18 14:14 ` Tom Tromey 1 sibling, 2 replies; 48+ messages in thread From: Eli Zaretskii @ 2020-01-15 17:51 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-15 17:51 ` Eli Zaretskii @ 2020-01-16 20:47 ` Pedro Alves 2020-01-16 21:58 ` Christian Biesinger via gdb-patches ` (2 more replies) 2020-06-18 14:14 ` Tom Tromey 1 sibling, 3 replies; 48+ messages in thread From: Pedro Alves @ 2020-01-16 20:47 UTC (permalink / raw) To: Eli Zaretskii, Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-16 20:47 ` Pedro Alves @ 2020-01-16 21:58 ` Christian Biesinger via gdb-patches 2020-01-16 22:31 ` Pedro Alves 2020-01-17 8:48 ` Eli Zaretskii 2020-01-17 7:57 ` Eli Zaretskii 2020-04-01 20:20 ` Tom Tromey 2 siblings, 2 replies; 48+ messages in thread From: Christian Biesinger via gdb-patches @ 2020-01-16 21:58 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, Tom Tromey, gdb-patches 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 > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-16 21:58 ` Christian Biesinger via gdb-patches @ 2020-01-16 22:31 ` Pedro Alves 2020-01-17 8:48 ` Eli Zaretskii 1 sibling, 0 replies; 48+ messages in thread From: Pedro Alves @ 2020-01-16 22:31 UTC (permalink / raw) To: Christian Biesinger; +Cc: Eli Zaretskii, Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-16 21:58 ` Christian Biesinger via gdb-patches 2020-01-16 22:31 ` Pedro Alves @ 2020-01-17 8:48 ` Eli Zaretskii 2020-01-17 18:32 ` Tom Tromey 1 sibling, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-01-17 8:48 UTC (permalink / raw) To: Christian Biesinger; +Cc: palves, tromey, gdb-patches > 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-17 8:48 ` Eli Zaretskii @ 2020-01-17 18:32 ` Tom Tromey 2020-01-17 21:03 ` Tom Tromey 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-01-17 18:32 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Christian Biesinger, palves, tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-17 18:32 ` Tom Tromey @ 2020-01-17 21:03 ` Tom Tromey 2020-01-18 11:07 ` Eli Zaretskii 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-01-17 21:03 UTC (permalink / raw) To: Tom Tromey; +Cc: Eli Zaretskii, Christian Biesinger, palves, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-17 21:03 ` Tom Tromey @ 2020-01-18 11:07 ` Eli Zaretskii 2020-01-20 15:52 ` Pedro Alves 0 siblings, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-01-18 11:07 UTC (permalink / raw) To: Tom Tromey; +Cc: cbiesinger, palves, gdb-patches > 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.) ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-18 11:07 ` Eli Zaretskii @ 2020-01-20 15:52 ` Pedro Alves 2020-01-20 15:53 ` Pedro Alves ` (3 more replies) 0 siblings, 4 replies; 48+ messages in thread From: Pedro Alves @ 2020-01-20 15:52 UTC (permalink / raw) To: Eli Zaretskii, Tom Tromey; +Cc: cbiesinger, gdb-patches [-- Attachment #1: Type: text/plain, Size: 8320 bytes --] On 1/18/20 7:58 AM, Eli Zaretskii wrote: >> 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.) I'm not sure about that solution -- won't --avoid=stat mean that we disable any stat gnulib fix for all platforms, instead of just for Windows? We only have one lstat call, but we also use fstat, for example, and I assume that these *stat modules in gnulib are all intertwined. Also, we may only have one lstat call nowadays, but who knows about the future. I played with a workaround along the lines of what I was suggesting earlier, and I couldn't make it work in the forms discussed previously. I did come up with a workaround though, it's just different. I noticed that gnulib's sys/stat.h replacement starts with a way to bypass it: #if defined __need_system_sys_stat_h /* Special invocation convention. */ #include_next <sys/stat.h> #else /* Normal invocation convention. */ #ifndef _GL_SYS_STAT_H So I think we can take advantage of that to be able to make sure that when we include "bfd.h", its functions are declared using the system's stat, which is the same version that bfd is built against. See prototype patch below, particularly common-types.h, which the place where we include bfd.h for the first time. It builds with my mingw cross compiler, the remaining issue would be looking more in detail to the to_sys_stat conversion function. I've also attached a gnulib fix for an issue I ran into, which will need to go upstream. From 3666298dcbdb817dbd5603dd2073e5788c67cac1 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 20 Jan 2020 15:40:54 +0000 Subject: [PATCH 1/2] Handle different "struct stat" between GDB and BFD --- gdb/defs.h | 1 - gdb/gdb_bfd.c | 45 +++++++++++++++++++++++++++++++++++++++++---- gdb/gdb_bfd.h | 2 +- gdb/jit.c | 4 ++-- gdb/symfile.c | 2 +- gdbsupport/common-types.h | 13 +++++++++++++ 6 files changed, 58 insertions(+), 9 deletions(-) diff --git a/gdb/defs.h b/gdb/defs.h index 1ad52feb1f8..f38b9dc6ff5 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -34,7 +34,6 @@ #undef PACKAGE_TARNAME #include <config.h> -#include "bfd.h" #include <sys/types.h> #include <limits.h> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 5a6dee2d51a..82b6a6bbaa2 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -66,7 +66,7 @@ struct gdb_bfd_data needs_relocations (0), crc_computed (0) { - struct stat buf; + sys_stat buf; if (bfd_stat (abfd, &buf) == 0) { @@ -355,24 +355,61 @@ gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream) return 0; } +/* Convert between a struct stat (potentially a gnulib replacement) + and a sys_stat (the system's struct stat). */ + +static sys_stat +to_sys_stat (struct stat *st) +{ + sys_stat sst {}; + +#define COPY(FIELD) \ + sst.FIELD = st->FIELD + + COPY (st_dev); + COPY (st_ino); + COPY (st_mode); + COPY (st_nlink); + COPY (st_uid); + COPY (st_gid); + COPY (st_rdev); + + /* Check for overflow? */ + COPY (st_size); + + // Should probably check _GL_WINDOWS_STAT_TIMESPEC and refer to + // st_atim, etc. instead. + COPY (st_atime); + COPY (st_mtime); + COPY (st_ctime); + +#undef COPY + + return sst; +} + /* Wrapper for target_fileio_fstat suitable for passing as the STAT_FUNC argument to gdb_bfd_openr_iovec. */ static int gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, - struct stat *sb) + sys_stat *sb) { int fd = *(int *) stream; int target_errno; int result; - result = target_fileio_fstat (fd, sb, &target_errno); + struct stat st; + + result = target_fileio_fstat (fd, &st, &target_errno); if (result == -1) { errno = fileio_errno_to_host (target_errno); bfd_set_error (bfd_error_system_call); } + *sb = to_sys_stat (&st); + return result; } @@ -818,7 +855,7 @@ gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)) + sys_stat *sb)) { bfd *result = bfd_openr_iovec (filename, target, open_func, open_closure, diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h index 9b1e292bf18..f0ad4814a80 100644 --- a/gdb/gdb_bfd.h +++ b/gdb/gdb_bfd.h @@ -154,7 +154,7 @@ gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)); + sys_stat *sb)); /* A wrapper for bfd_openr_next_archived_file that initializes the gdb-specific reference count. */ diff --git a/gdb/jit.c b/gdb/jit.c index eeaab70bfe0..976f8555250 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -124,11 +124,11 @@ mem_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf, /* For statting the file, we only support the st_size attribute. */ static int -mem_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb) +mem_bfd_iovec_stat (struct bfd *abfd, void *stream, sys_stat *sb) { struct target_buffer *buffer = (struct target_buffer*) stream; - memset (sb, 0, sizeof (struct stat)); + memset (sb, 0, sizeof (sys_stat)); sb->st_size = buffer->size; return 0; } diff --git a/gdb/symfile.c b/gdb/symfile.c index f7bada75f35..65d342a53dd 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1260,7 +1260,7 @@ separate_debug_file_exists (const std::string &name, unsigned long crc, { unsigned long file_crc; int file_crc_p; - struct stat parent_stat, abfd_stat; + sys_stat parent_stat, abfd_stat; int verified_as_different; /* Find a separate debug info file as if symbols would be present in diff --git a/gdbsupport/common-types.h b/gdbsupport/common-types.h index 61099b4e25d..8c136212c80 100644 --- a/gdbsupport/common-types.h +++ b/gdbsupport/common-types.h @@ -32,8 +32,21 @@ typedef unsigned long long ULONGEST; #else /* GDBSERVER */ +/* Gnulib may replace struct stat with its own version. Bfd does not + use gnulib, so when we call into bfd, we must use the system struct + stat. */ +#define __need_system_sys_stat_h 1 + +#include <sys/stat.h> + #include "bfd.h" +typedef struct stat sys_stat; + +#undef __need_system_sys_stat_h + +#include <sys/stat.h> + /* * A byte from the program being debugged. */ typedef bfd_byte gdb_byte; base-commit: 26916852e189323593102561f5e3e2137b892dec -- 2.14.5 [-- Attachment #2: 0002-Fix-gnulib-s-lstat-replacement-in-C-namespace-mode.patch --] [-- Type: text/x-patch, Size: 3622 bytes --] From 224fa934579ca3a1292c2e6a0377aaa9bfecc645 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 20 Jan 2020 15:40:55 +0000 Subject: [PATCH 2/2] Fix gnulib's lstat replacement in C++ namespace mode Fixes: unittests/string_view-selftests.c: In member function 'gnulib::_gl_lstat_wrapper::operator gnulib::_gl_lstat_wrapper::type() const': unittests/string_view-selftests.c:11432:22: error: expected primary-expression before ';' token return ::rpl_stat; ^ The problem is that the lstat replacement depends on the stat (function) declaration, which is only declared afterwards. The fix is simply to declare lstat after stat. --- gnulib/import/sys_stat.in.h | 66 ++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/gnulib/import/sys_stat.in.h b/gnulib/import/sys_stat.in.h index 9ddd1a8d004..537917b6a51 100644 --- a/gnulib/import/sys_stat.in.h +++ b/gnulib/import/sys_stat.in.h @@ -536,40 +536,6 @@ _GL_WARN_ON_USE (lchmod, "lchmod is unportable - " #endif -#if @GNULIB_LSTAT@ -# if ! @HAVE_LSTAT@ -/* mingw does not support symlinks, therefore it does not have lstat. But - without links, stat does just fine. */ -# if !(defined __cplusplus && defined GNULIB_NAMESPACE) -# define lstat stat -# endif -_GL_CXXALIAS_RPL_1 (lstat, stat, int, (const char *name, struct stat *buf)); -# elif @REPLACE_LSTAT@ -# if !(defined __cplusplus && defined GNULIB_NAMESPACE) -# undef lstat -# define lstat rpl_lstat -# endif -_GL_FUNCDECL_RPL (lstat, int, (const char *name, struct stat *buf) - _GL_ARG_NONNULL ((1, 2))); -_GL_CXXALIAS_RPL (lstat, int, (const char *name, struct stat *buf)); -# else -_GL_CXXALIAS_SYS (lstat, int, (const char *name, struct stat *buf)); -# endif -# if @HAVE_LSTAT@ -_GL_CXXALIASWARN (lstat); -# endif -#elif @GNULIB_OVERRIDES_STRUCT_STAT@ -# undef lstat -# define lstat lstat_used_without_requesting_gnulib_module_lstat -#elif defined GNULIB_POSIXCHECK -# undef lstat -# if HAVE_RAW_DECL_LSTAT -_GL_WARN_ON_USE (lstat, "lstat is unportable - " - "use gnulib module lstat for portability"); -# endif -#endif - - #if @REPLACE_MKDIR@ # if !(defined __cplusplus && defined GNULIB_NAMESPACE) # undef mkdir @@ -781,6 +747,38 @@ _GL_WARN_ON_USE (stat, "stat is unportable - " # endif #endif +#if @GNULIB_LSTAT@ +# if ! @HAVE_LSTAT@ +/* mingw does not support symlinks, therefore it does not have lstat. But + without links, stat does just fine. */ +# if !(defined __cplusplus && defined GNULIB_NAMESPACE) +# define lstat stat +# endif +_GL_CXXALIAS_RPL_1 (lstat, stat, int, (const char *name, struct stat *buf)); +# elif @REPLACE_LSTAT@ +# if !(defined __cplusplus && defined GNULIB_NAMESPACE) +# undef lstat +# define lstat rpl_lstat +# endif +_GL_FUNCDECL_RPL (lstat, int, (const char *name, struct stat *buf) + _GL_ARG_NONNULL ((1, 2))); +_GL_CXXALIAS_RPL (lstat, int, (const char *name, struct stat *buf)); +# else +_GL_CXXALIAS_SYS (lstat, int, (const char *name, struct stat *buf)); +# endif +# if @HAVE_LSTAT@ +_GL_CXXALIASWARN (lstat); +# endif +#elif @GNULIB_OVERRIDES_STRUCT_STAT@ +# undef lstat +# define lstat lstat_used_without_requesting_gnulib_module_lstat +#elif defined GNULIB_POSIXCHECK +# undef lstat +# if HAVE_RAW_DECL_LSTAT +_GL_WARN_ON_USE (lstat, "lstat is unportable - " + "use gnulib module lstat for portability"); +# endif +#endif #if @GNULIB_UTIMENSAT@ /* Use the rpl_ prefix also on Solaris <= 9, because on Solaris 9 our utimensat -- 2.14.5 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 15:52 ` Pedro Alves @ 2020-01-20 15:53 ` Pedro Alves 2020-01-20 20:50 ` Eli Zaretskii ` (2 subsequent siblings) 3 siblings, 0 replies; 48+ messages in thread From: Pedro Alves @ 2020-01-20 15:53 UTC (permalink / raw) To: Eli Zaretskii, Tom Tromey; +Cc: cbiesinger, gdb-patches On 1/20/20 3:48 PM, Pedro Alves wrote: > So I think we can take advantage of that to be able to make sure that > when we include "bfd.h", its functions are declared using the system's > stat, which is the same version that bfd is built against. > See prototype patch below, particularly common-types.h, which the > place where we include bfd.h for the first time. > > It builds with my mingw cross compiler, the remaining issue would be > looking more in detail to the to_sys_stat conversion function. > > I've also attached a gnulib fix for an issue I ran into, which will > need to go upstream. Forgot to say that I put this on the users/palves/stat branch if you'd like to try it & poke at it. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 15:52 ` Pedro Alves 2020-01-20 15:53 ` Pedro Alves @ 2020-01-20 20:50 ` Eli Zaretskii 2020-01-20 20:58 ` Pedro Alves 2020-01-23 22:05 ` Tom Tromey 2020-06-19 17:51 ` Tom Tromey 3 siblings, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-01-20 20:50 UTC (permalink / raw) To: Pedro Alves; +Cc: tromey, cbiesinger, gdb-patches > Cc: cbiesinger@google.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Mon, 20 Jan 2020 15:48:22 +0000 > > > 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.) > > I'm not sure about that solution -- won't --avoid=stat mean that > we disable any stat gnulib fix for all platforms, instead of just > for Windows? It would, but do we have any known problems with stat and fstat on other platforms? > We only have one lstat call, but we also use fstat, for example, and > I assume that these *stat modules in gnulib are all intertwined. > Also, we may only have one lstat call nowadays, but who knows about > the future. Even having a gdb_lstat for that purpose will be simpler and more future-proof than the whole Gnulib stat module, I think. > I did come up with a workaround though, it's just different. > > I noticed that gnulib's sys/stat.h replacement starts with a way to > bypass it: > > #if defined __need_system_sys_stat_h > /* Special invocation convention. */ > > #include_next <sys/stat.h> > > #else > /* Normal invocation convention. */ > > #ifndef _GL_SYS_STAT_H > > So I think we can take advantage of that to be able to make sure that > when we include "bfd.h", its functions are declared using the system's > stat, which is the same version that bfd is built against. But stat/fstat the functions will still shadow the system ones, would they not? And if they would, doesn't it mean subtle bugs where, e.g., the Gnulib replacement implementations rely on wide-enough st_size, for example, or st_mtime? Also, aren't some of the problems on platforms other than MinGW resolved by the Gnulib sys/stat.h header, as opposed to the replacement implementation of the functions themselves? Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 20:50 ` Eli Zaretskii @ 2020-01-20 20:58 ` Pedro Alves 2020-01-21 15:50 ` Pedro Alves 2020-01-21 17:56 ` Eli Zaretskii 0 siblings, 2 replies; 48+ messages in thread From: Pedro Alves @ 2020-01-20 20:58 UTC (permalink / raw) To: Eli Zaretskii; +Cc: tromey, cbiesinger, gdb-patches On 1/20/20 5:28 PM, Eli Zaretskii wrote: >> Cc: cbiesinger@google.com, gdb-patches@sourceware.org >> From: Pedro Alves <palves@redhat.com> >> Date: Mon, 20 Jan 2020 15:48:22 +0000 >> >>> 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.) >> >> I'm not sure about that solution -- won't --avoid=stat mean that >> we disable any stat gnulib fix for all platforms, instead of just >> for Windows? > > It would, but do we have any known problems with stat and fstat on > other platforms? There's the list of known problems in the gnulib pages: https://www.gnu.org/software/gnulib/manual/html_node/fstat.html https://www.gnu.org/software/gnulib/manual/html_node/stat.html https://www.gnu.org/software/gnulib/manual/html_node/lstat.html > >> We only have one lstat call, but we also use fstat, for example, and >> I assume that these *stat modules in gnulib are all intertwined. >> Also, we may only have one lstat call nowadays, but who knows about >> the future. > > Even having a gdb_lstat for that purpose will be simpler and more > future-proof than the whole Gnulib stat module, I think. I think one could use that argument for any piece of gnulib in isolation. But fighting against use of some particular modules ends up creating a larger burden over time IMO. I'd rather avoid doing that if possible. > >> I did come up with a workaround though, it's just different. >> >> I noticed that gnulib's sys/stat.h replacement starts with a way to >> bypass it: >> >> #if defined __need_system_sys_stat_h >> /* Special invocation convention. */ >> >> #include_next <sys/stat.h> >> >> #else >> /* Normal invocation convention. */ >> >> #ifndef _GL_SYS_STAT_H >> >> So I think we can take advantage of that to be able to make sure that >> when we include "bfd.h", its functions are declared using the system's >> stat, which is the same version that bfd is built against. > > But stat/fstat the functions will still shadow the system ones, would > they not? Let's look at the patch: --- c/gdbsupport/common-types.h +++ w/gdbsupport/common-types.h @@ -32,8 +32,21 @@ typedef unsigned long long ULONGEST; #else /* GDBSERVER */ +/* Gnulib may replace struct stat with its own version. Bfd does not + use gnulib, so when we call into bfd, we must use the system struct + stat. */ +#define __need_system_sys_stat_h 1 + +#include <sys/stat.h> + #include "bfd.h" +typedef struct stat sys_stat; + +#undef __need_system_sys_stat_h + +#include <sys/stat.h> Currently, without that patch, because of the gnulib struct stat replacement, when we include bfd.h, we end up with the following function declared, if you expand the preprocessor macros: extern int bfd_stat (bfd *, struct rpl_stat *); This is incorrect, because that bfd function was not defined that way. It is instead written as: extern int bfd_stat (bfd *, struct stat *); Given the stat replacement, we pass it a rpl_stat pointer, when it is in reality expecting a "struct stat" one (or whatever that expands in the system headers). So we get undefined behavior at run time. By defining __need_system_sys_stat_h just before bfd.h is included, we're sure that bfd's bfd_stat is declared with the system's stat, just like when bfd itself was compiled. extern int bfd_stat (bfd *, struct stat *); We undef __need_system_sys_stat_h again just after including "bfd.h", so that the gdb uses the gnulib struct stat. But we also create the sys_stat typedef so that we can refer to the system's stat type after __need_system_sys_stat_h is undefined and gnulib's stat replacement is visible. So the *stat functions will be shadowed by the gnulib ones within gdb, yes. But we also get a compile error if we call bfd_stat with a masked struct stat: binutils-gdb/src/gdb/gdb_bfd.c: In constructor 'gdb_bfd_data::gdb_bfd_data(bfd*)': binutils-gdb/src/gdb/gdb_bfd.c:72:29: error: cannot convert 'rpl_stat*' to '_stat64*' for argument '2' to 'int bfd_stat(bfd*, _stat64*)' if (bfd_stat (abfd, &buf) == 0) ^ > And if they would, doesn't it mean subtle bugs where, e.g., > the Gnulib replacement implementations rely on wide-enough st_size, > for example, or st_mtime? I'm not sure what you mean here. When the replacement implementations are called, they're passed a replaced struct stat pointer too. It's only while the bfd.h header is being compiled that the gnulib replacements aren't visible. > Also, aren't some of the problems on platforms other than MinGW > resolved by the Gnulib sys/stat.h header, as opposed to the > replacement implementation of the functions themselves? Some yes, but not all. But it's the sys/stat.h header replacement that redefines struct stat, so I don't see the point you're making. Now, the solution I came up with is reusable for any other library we may end up depending on that is built with a different struct stat and requires passing a struct stat in one of its entry pointers. However, with all that said, bfd is always built along with gdb, so we have a higher degree of control. Maybe a better solution for this is really to make sure that bfd is compiled with largefile support, as suggested in the bug-gnulib discussion. So far, I was under the impression that you're reaching the GNULIB_defined_struct_stat code, where gnulib defines its own struct stat. But reading your description of the bug in gnulib again, I see you're actually getting stat defined to _stati64. So perhaps what we need is to enable largefile support by default on bfd for mingw, to force use of the 64-bit stat? Does the original issue go away if you configure with --enable-largefile? Maybe we need a mingw-specific tweak in gdb's src/config/largefile.m4? Looking my mingw-w64's stat.h, I see: #if defined(_FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS == 64) #ifdef _USE_32BIT_TIME_T #define stat _stat32i64 #define fstat _fstat32i64 #else #define stat _stat64 #define fstat _fstat64 #endif #endif So if BFD is compiled with _FILE_OFFSET_BITS == 64 and _USE_32BIT_TIME_T is not defined, the mismatch ends up going away? Is there a reason we _wouldn't_ want to enable largefile support in bfd? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 20:58 ` Pedro Alves @ 2020-01-21 15:50 ` Pedro Alves 2020-01-21 19:38 ` Eli Zaretskii 2020-01-21 17:56 ` Eli Zaretskii 1 sibling, 1 reply; 48+ messages in thread From: Pedro Alves @ 2020-01-21 15:50 UTC (permalink / raw) To: Eli Zaretskii; +Cc: tromey, cbiesinger, gdb-patches On 1/20/20 8:50 PM, Pedro Alves wrote: > So if BFD is compiled with _FILE_OFFSET_BITS == 64 and > _USE_32BIT_TIME_T is not defined, the mismatch ends up going > away? Is there a reason we _wouldn't_ want to enable largefile > support in bfd? I'm looking at this some more, and am trying to understand what is really going on. I can't seem to reproduce your original issue, I think because I'm using (32-bit) mingw-w64, while the issue with 32-bit size_t happen on (32-bit) mingw.org instead. Correct? But re-reading your description of the problem on bug-gnulib, I think I get it. BFD already uses AC_SYS_LARGEFILE, wrapped in ACX_LARGEFILE (config/largefile.m4) due to a Solaris issue. Actually, the whole tree uses that -- ld, binutils, bfd, gdb, etc., even our toplevel gnulib/ directory's configure.ac calls it. But then, the gnulib/import/ So maybe the best to do here is to import gnulib with --avoid=largefile, and let the ACX_LARGEFILE in gnulib/'s top configure handle the enabling largefile support in sync with all other top level dirs. I tried that, and confirmed that _FILE_OFFSET_BITS=64 still ends up in gnulib's config.h. The build then fails inside gnulib for me on 32-bit mingw-w64, maybe there's a bug that needs to be fixed, but I'd think this _should_ work. See the users/palves/gnulib-largefile branch. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-21 15:50 ` Pedro Alves @ 2020-01-21 19:38 ` Eli Zaretskii 0 siblings, 0 replies; 48+ messages in thread From: Eli Zaretskii @ 2020-01-21 19:38 UTC (permalink / raw) To: Pedro Alves; +Cc: tromey, cbiesinger, gdb-patches > Cc: tromey@adacore.com, cbiesinger@google.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Tue, 21 Jan 2020 13:47:28 +0000 > > On 1/20/20 8:50 PM, Pedro Alves wrote: > > > So if BFD is compiled with _FILE_OFFSET_BITS == 64 and > > _USE_32BIT_TIME_T is not defined, the mismatch ends up going > > away? Is there a reason we _wouldn't_ want to enable largefile > > support in bfd? > > I'm looking at this some more, and am trying to understand > what is really going on. I can't seem to reproduce your original > issue, I think because I'm using (32-bit) mingw-w64, while the issue > with 32-bit size_t happen on (32-bit) mingw.org instead. Correct? It isn't only st_size, it's also the width of the st_?time fields. > So maybe the best to do here is to import gnulib with > --avoid=largefile, and let the ACX_LARGEFILE in gnulib/'s > top configure handle the enabling largefile support in sync > with all other top level dirs. I tried that, > and confirmed that _FILE_OFFSET_BITS=64 still ends up in > gnulib's config.h. The build then fails inside gnulib > for me on 32-bit mingw-w64, maybe there's a bug that needs > to be fixed, but I'd think this _should_ work. AFAIU, largefile is intentionally requested in all MinGW builds, the Gnulib developers explicitly said that was their intent. And largefile then causes the replacement of st_size etc. in struct stat, for consistency. MinGW64 doesn't need all that stuff at all, because their defaults are already set to use 64-bit st_size and 64-bit time_t fields. That's because MinGW64 tossed support for Windows version before Vista long ago, and therefore the incompatible changes Microsoft introduced into MSVCRT.DLL from Vista onwards are of no importance for MinGW64 (and the fragment from their stat.h you've shown is by now just useless ballast that is AFAIU never used in MinGW64). But mingw.org's MinGW still supports the older Windows versions, and the only sane way of doing that is to use 32-bit time_t, which basically means one needs to use the regular implementation of stat, not _stati64 or any other variety. Moreover, using _stati64 instead of stat, as MinGW64 does by default (and Gnulib forces the same on any other MinGW build), is a non-starter for me, because _stati64 didn't exist before Vista. So a GDB built that way will simply refuse to run on any older system, even if you build it on a Windows box that does have _stati64. Having said all that, let me explicitly say that I don't want to put a drag on GDB development, and therefore will not insist that solutions for this kind of problems must compile cleanly with my MinGW toolchain. That is why I never bothered to say anything here about the struct stat problem, and only posted that to the Gnulib list. I know how to hack the build to make it DTRT for mingw.org's MinGW; I did that with the pretest, as soon as I found out the reason for the breakage (in a nutshell, remote debugging with gdbserver was completely broken: GDB would crash as soon as you run the remote target). As long as the problem was limited to mingw.org's MinGW, I kept silence here, and only broke that silence because Tom uncovered what seems to be a similar problem, but one that affects MinGW64 as well. Bottom line: if you have a satisfactory solution for MinGW64, go for it regardless of what it will mean for the MinGW I use; I don't want to complicate this stuff any more than it already is, given that the Gnulib developers rejected what I consider the simplest and the most reliable solution (which would have seamlessly satisfied both varieties of MinGW). Now that I'm aware of the problems with the Gnulib stat replacements, I can work around that very easily. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 20:58 ` Pedro Alves 2020-01-21 15:50 ` Pedro Alves @ 2020-01-21 17:56 ` Eli Zaretskii 1 sibling, 0 replies; 48+ messages in thread From: Eli Zaretskii @ 2020-01-21 17:56 UTC (permalink / raw) To: Pedro Alves; +Cc: tromey, cbiesinger, gdb-patches > Cc: tromey@adacore.com, cbiesinger@google.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Mon, 20 Jan 2020 20:50:17 +0000 > > >> I'm not sure about that solution -- won't --avoid=stat mean that > >> we disable any stat gnulib fix for all platforms, instead of just > >> for Windows? > > > > It would, but do we have any known problems with stat and fstat on > > other platforms? > > There's the list of known problems in the gnulib pages: > > https://www.gnu.org/software/gnulib/manual/html_node/fstat.html This one is only about Solaris problems with st_mtime etc. (MSVC problems are of no concern to us.) > https://www.gnu.org/software/gnulib/manual/html_node/stat.html Is the trailing slash issue important for GDB? > https://www.gnu.org/software/gnulib/manual/html_node/lstat.html Likewise. All in all, the gain sounds small to me, if at all. > >> We only have one lstat call, but we also use fstat, for example, and > >> I assume that these *stat modules in gnulib are all intertwined. > >> Also, we may only have one lstat call nowadays, but who knows about > >> the future. > > > > Even having a gdb_lstat for that purpose will be simpler and more > > future-proof than the whole Gnulib stat module, I think. > > I think one could use that argument for any piece of gnulib in > isolation. I wouldn't say "any piece", there are some functions there with very non-trivial guts. But yes, the impact of Gnulib IME is mostly important where it provides missing functions, not where it replaces existing ones. > But fighting against use of some particular modules ends up creating > a larger burden over time IMO. I'd rather avoid doing that if > possible. Sure, this goes without saying. I was just considering this as one alternative, since we don't seem to have a much better one. Or do we? > extern int bfd_stat (bfd *, struct rpl_stat *); > > This is incorrect, because that bfd function was not defined > that way. It is instead written as: > > extern int bfd_stat (bfd *, struct stat *); > > Given the stat replacement, we pass it a rpl_stat pointer, when it > is in reality expecting a "struct stat" one (or whatever that expands > in the system headers). So we get undefined behavior at run time. > > By defining __need_system_sys_stat_h just before bfd.h is included, > we're sure that bfd's bfd_stat is declared with the system's > stat, just like when bfd itself was compiled. > > extern int bfd_stat (bfd *, struct stat *); > > We undef __need_system_sys_stat_h again just after including > "bfd.h", so that the gdb uses the gnulib struct stat. But we > also create the sys_stat typedef so that we can refer to the > system's stat type after __need_system_sys_stat_h is undefined > and gnulib's stat replacement is visible. > > So the *stat functions will be shadowed by the gnulib ones > within gdb, yes. But we also get a compile error if we > call bfd_stat with a masked struct stat: OK, I see. This would work, of course, but IME solutions based on specific order of inclusion of header files are fragile, and break easily, because header files tend to include other header files and break the order you carefully observed. But if there's no better alternative, perhaps this is the way to go. > > Also, aren't some of the problems on platforms other than MinGW > > resolved by the Gnulib sys/stat.h header, as opposed to the > > replacement implementation of the functions themselves? > > Some yes, but not all. But it's the sys/stat.h header replacement > that redefines struct stat, so I don't see the point you're making. And you are sure that including first the system's stat.h and then the Gnulib's version of it will never cause any compilation problems? Also, if GDB uses values based on what bfd_stat retrieves, then these values might be different from what the Gnulib replacement stat retrieves in GDB's own code for the same file (due to size of the fields), right? Are we sure we never compare those expecting them to match for the same file? > So perhaps what we need is to enable largefile support by > default on bfd for mingw, to force use of the 64-bit stat? > Does the original issue go away if you configure > with --enable-largefile? Maybe we need a mingw-specific > tweak in gdb's src/config/largefile.m4? > > Looking my mingw-w64's stat.h, I see: > > #if defined(_FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS == 64) > #ifdef _USE_32BIT_TIME_T > #define stat _stat32i64 > #define fstat _fstat32i64 > #else > #define stat _stat64 > #define fstat _fstat64 > #endif > #endif > > So if BFD is compiled with _FILE_OFFSET_BITS == 64 and > _USE_32BIT_TIME_T is not defined, the mismatch ends up going > away? Is there a reason we _wouldn't_ want to enable largefile > support in bfd? That's a non-starter for me, as I will explain in response to your further message. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 15:52 ` Pedro Alves 2020-01-20 15:53 ` Pedro Alves 2020-01-20 20:50 ` Eli Zaretskii @ 2020-01-23 22:05 ` Tom Tromey 2020-06-19 17:51 ` Tom Tromey 3 siblings, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-01-23 22:05 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, Tom Tromey, cbiesinger, gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> I'm not sure about that solution -- won't --avoid=stat mean that Pedro> we disable any stat gnulib fix for all platforms, instead of just Pedro> for Windows? Yeah. Actually, there is one more option for us, which is to patch gnulib in-tree. We already have the machinery to do this. Pedro> So I think we can take advantage of that to be able to make sure that Pedro> when we include "bfd.h", its functions are declared using the system's Pedro> stat, which is the same version that bfd is built against. Pedro> See prototype patch below, particularly common-types.h, which the Pedro> place where we include bfd.h for the first time. Pedro> It builds with my mingw cross compiler, the remaining issue would be Pedro> looking more in detail to the to_sys_stat conversion function. This looks reasonably promising to me. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-20 15:52 ` Pedro Alves ` (2 preceding siblings ...) 2020-01-23 22:05 ` Tom Tromey @ 2020-06-19 17:51 ` Tom Tromey 3 siblings, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-06-19 17:51 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, Tom Tromey, cbiesinger, gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> I've also attached a gnulib fix for an issue I ran into, which will Pedro> need to go upstream. I didn't seem to need this. I wonder if I'm doing something wrong..? Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-16 20:47 ` Pedro Alves 2020-01-16 21:58 ` Christian Biesinger via gdb-patches @ 2020-01-17 7:57 ` Eli Zaretskii 2020-04-01 20:20 ` Tom Tromey 2 siblings, 0 replies; 48+ messages in thread From: Eli Zaretskii @ 2020-01-17 7:57 UTC (permalink / raw) To: Pedro Alves; +Cc: tromey, gdb-patches > 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-16 20:47 ` Pedro Alves 2020-01-16 21:58 ` Christian Biesinger via gdb-patches 2020-01-17 7:57 ` Eli Zaretskii @ 2020-04-01 20:20 ` Tom Tromey 2 siblings, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-04-01 20:20 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, Tom Tromey, gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: [ system stat -vs- gnulib stat ] Pedro> I think that we need to: Pedro> - #undef stat before including bfd headers. Pedro> - Redefine it afterwards back to rpl_stat (*) Pedro> - add some kind of wrapper around bfd_stat (like maybe called gdb_bfd_stat) Pedro> that handles the mismatch. Something like: I read the thread again but I found it hard to figure out which alternative of the ones presented is preferred. Could you tell me which one it is? I'd like to update these patches and fix the caching bug as well. thanks, Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-01-15 17:51 ` Eli Zaretskii 2020-01-16 20:47 ` Pedro Alves @ 2020-06-18 14:14 ` Tom Tromey 2020-06-18 15:04 ` Eli Zaretskii 1 sibling, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-06-18 14:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Tom Tromey, gdb-patches, Pedro Alves >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes: >> 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. Eli> There's one more potential issue with Gnulib's replacement of 'fstat': Eli> it also replaces the definition of 'struct stat', and it does that in Eli> a way that might yield incompatibility between the definition on Eli> <sys/stat.h> the system header and Gnulib's sys/stat.h replacement. I am looking into this issue again for the upcoming gdb 10 release. Looking at the gnulib in the gdb tree, as far as I can tell, it doesn't actually override struct stat. The code is all there: /* Optionally, override 'struct stat' on native Windows. */ #if @GNULIB_OVERRIDES_STRUCT_STAT@ [...] ... however, this is only ever set to 0 in our tree: $ git grep 'GNULIB_OVERRIDES_STRUCT_STAT.*=' -- gnulib/import/ gnulib/import/Makefile.in:GNULIB_OVERRIDES_STRUCT_STAT = @GNULIB_OVERRIDES_STRUCT_STAT@ gnulib/import/m4/sys_stat_h.m4: GNULIB_OVERRIDES_STRUCT_STAT=0; AC_SUBST([GNULIB_OVERRIDES_STRUCT_STAT]) (I chose a tighter grep for the purposes of posting, but in reality I looked at all mentions of GNULIB_OVERRIDES_STRUCT_STAT.) So, I think this is maybe not an issue. The timezone thing, however, remains an issue. It seems dangerous to me that we would have two implementations of stat that would give different answers. It's too easy, IMO, to accidentally compare values from one with values from the other -- in fact, that's what lead to the patches in this thread I'm replying to. Based on this, my inclination is to patch our copy of gnulib to avoid replacing stat on Windows. I'm looking into how to do this. Much of the rest of this series will still be needed. In particular it unearths a real race in the BFD cache. Let me know what you think. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 14:14 ` Tom Tromey @ 2020-06-18 15:04 ` Eli Zaretskii 2020-06-18 16:00 ` Tom Tromey 0 siblings, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-06-18 15:04 UTC (permalink / raw) To: Tom Tromey; +Cc: tromey, gdb-patches, palves > From: Tom Tromey <tom@tromey.com> > Cc: Tom Tromey <tromey@adacore.com>, gdb-patches@sourceware.org, > Pedro Alves <palves@redhat.com> > Date: Thu, 18 Jun 2020 08:14:18 -0600 > > Eli> There's one more potential issue with Gnulib's replacement of 'fstat': > Eli> it also replaces the definition of 'struct stat', and it does that in > Eli> a way that might yield incompatibility between the definition on > Eli> <sys/stat.h> the system header and Gnulib's sys/stat.h replacement. > > I am looking into this issue again for the upcoming gdb 10 release. > > Looking at the gnulib in the gdb tree, as far as I can tell, it doesn't > actually override struct stat. The code is all there: > > /* Optionally, override 'struct stat' on native Windows. */ > #if @GNULIB_OVERRIDES_STRUCT_STAT@ > [...] > > ... however, this is only ever set to 0 in our tree: > > $ git grep 'GNULIB_OVERRIDES_STRUCT_STAT.*=' -- gnulib/import/ > gnulib/import/Makefile.in:GNULIB_OVERRIDES_STRUCT_STAT = @GNULIB_OVERRIDES_STRUCT_STAT@ > gnulib/import/m4/sys_stat_h.m4: GNULIB_OVERRIDES_STRUCT_STAT=0; AC_SUBST([GNULIB_OVERRIDES_STRUCT_STAT]) > > (I chose a tighter grep for the purposes of posting, but in reality I > looked at all mentions of GNULIB_OVERRIDES_STRUCT_STAT.) > > So, I think this is maybe not an issue. I meant a different part of Gnulib's sys/stat.h: /* Large File Support on native Windows. */ #if @WINDOWS_64_BIT_ST_SIZE@ # define stat _stati64 #endif This will also redirect 'struct stat' to 'struct _stati64', which has a 64-bit st_size and a 64-bit st_mtime (and other times) members. And by default @WINDOWS_64_BIT_ST_SIZE@ evaluates to 1 in the MinGW build. This might not be a problem when building with MinGW64, but my GDB builds are 32-bit, and I want to use the default version of 'struct stat', where both the st_size and the time-related members are 32-bit wide. Therefore, I had a problem with the above redirection. In case you need to refresh your memory, I explained all that in more detail here: https://sourceware.org/pipermail/gdb-patches/2020-January/164882.html > The timezone thing, however, remains an issue. It seems dangerous to me > that we would have two implementations of stat that would give different > answers. It's too easy, IMO, to accidentally compare values from one > with values from the other -- in fact, that's what lead to the patches > in this thread I'm replying to. I agree. > Based on this, my inclination is to patch our copy of gnulib to avoid > replacing stat on Windows. I'm looking into how to do this. AFAICT, gnulib/import/m4/stat.m4 does that for MinGW unconditionally: case "$host_os" in mingw*) dnl On this platform, the original stat() returns st_atime, st_mtime, dnl st_ctime values that are affected by the time zone. REPLACE_STAT=1 ;; So the way to avoid the replacement is to modify this test, right? Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 15:04 ` Eli Zaretskii @ 2020-06-18 16:00 ` Tom Tromey 2020-06-18 17:27 ` Eli Zaretskii 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-06-18 16:00 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Tom Tromey, tromey, gdb-patches, palves >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes: Eli> I meant a different part of Gnulib's sys/stat.h: Thanks. Eli> AFAICT, gnulib/import/m4/stat.m4 does that for MinGW unconditionally: Eli> case "$host_os" in Eli> mingw*) Eli> dnl On this platform, the original stat() returns st_atime, st_mtime, Eli> dnl st_ctime values that are affected by the time zone. Eli> REPLACE_STAT=1 Eli> ;; Eli> So the way to avoid the replacement is to modify this test, right? I tried that, but unfortunately I think it didn't really work -- gnulib still builds and uses _gl_convert_FILETIME_to_POSIX and _gl_fstat_by_handle. I considered two other approaches: 1. Link BFD and binutils against gnulib. This seems like a pain because at least Fedora installs libbfd.so, so we'd either have to arrange to build a gnulib.so (I guess under some other name) or get the gnulib objects into the shared libbfd... ugh. 2. Use --avoid=stat --avoid=fstat. So far this seems like the best approach. Pedro pointed out that this means we won't get any gnulib fixes for other bugs in this area. However, given gdb's relatively minimal needs from stat, and given the fact that gnulib is introducing other bugs, this seems like an acceptable tradeoff to me. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 16:00 ` Tom Tromey @ 2020-06-18 17:27 ` Eli Zaretskii 2020-06-18 17:32 ` Pedro Alves 0 siblings, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-06-18 17:27 UTC (permalink / raw) To: Tom Tromey; +Cc: tromey, gdb-patches, palves > From: Tom Tromey <tom@tromey.com> > Cc: Tom Tromey <tom@tromey.com>, tromey@adacore.com, > gdb-patches@sourceware.org, palves@redhat.com > Date: Thu, 18 Jun 2020 10:00:57 -0600 > > 2. Use --avoid=stat --avoid=fstat. So far this seems like the best > approach. Pedro pointed out that this means we won't get any gnulib > fixes for other bugs in this area. However, given gdb's relatively > minimal needs from stat, and given the fact that gnulib is > introducing other bugs, this seems like an acceptable tradeoff to me. If that works, I think it's an okay solution. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 17:27 ` Eli Zaretskii @ 2020-06-18 17:32 ` Pedro Alves 2020-06-18 17:54 ` Eli Zaretskii 2020-06-18 17:57 ` Tom Tromey 0 siblings, 2 replies; 48+ messages in thread From: Pedro Alves @ 2020-06-18 17:32 UTC (permalink / raw) To: Eli Zaretskii, Tom Tromey; +Cc: gdb-patches, tromey On 6/18/20 6:27 PM, Eli Zaretskii wrote: >> From: Tom Tromey <tom@tromey.com> >> Cc: Tom Tromey <tom@tromey.com>, tromey@adacore.com, >> gdb-patches@sourceware.org, palves@redhat.com >> Date: Thu, 18 Jun 2020 10:00:57 -0600 >> >> 2. Use --avoid=stat --avoid=fstat. So far this seems like the best >> approach. Pedro pointed out that this means we won't get any gnulib >> fixes for other bugs in this area. However, given gdb's relatively >> minimal needs from stat, and given the fact that gnulib is >> introducing other bugs, this seems like an acceptable tradeoff to me. > > If that works, I think it's an okay solution. I disagree. It's not only GDB's stat usage that counts, it's the other gnulib modules that depend on the stat module fixes too. Also, that approach disables the stat module for all systems, not just mingw. I think it's just bad policy to disable a module like that. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 17:32 ` Pedro Alves @ 2020-06-18 17:54 ` Eli Zaretskii 2020-06-19 12:02 ` Pedro Alves 2020-06-18 17:57 ` Tom Tromey 1 sibling, 1 reply; 48+ messages in thread From: Eli Zaretskii @ 2020-06-18 17:54 UTC (permalink / raw) To: Pedro Alves; +Cc: tom, gdb-patches, tromey > Cc: gdb-patches@sourceware.org, tromey@adacore.com > From: Pedro Alves <palves@redhat.com> > Date: Thu, 18 Jun 2020 18:32:42 +0100 > > >> 2. Use --avoid=stat --avoid=fstat. So far this seems like the best > >> approach. Pedro pointed out that this means we won't get any gnulib > >> fixes for other bugs in this area. However, given gdb's relatively > >> minimal needs from stat, and given the fact that gnulib is > >> introducing other bugs, this seems like an acceptable tradeoff to me. > > > > If that works, I think it's an okay solution. > > I disagree. It's not only GDB's stat usage that counts, it's the > other gnulib modules that depend on the stat module fixes too. Also, > that approach disables the stat module for all systems, not just > mingw. I think it's just bad policy to disable a module like that. What would you suggest instead? ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 17:54 ` Eli Zaretskii @ 2020-06-19 12:02 ` Pedro Alves 2020-06-19 12:13 ` Eli Zaretskii 2020-06-19 17:09 ` Tom Tromey 0 siblings, 2 replies; 48+ messages in thread From: Pedro Alves @ 2020-06-19 12:02 UTC (permalink / raw) To: Eli Zaretskii; +Cc: tom, tromey, gdb-patches On 6/18/20 6:54 PM, Eli Zaretskii wrote: >> Cc: gdb-patches@sourceware.org, tromey@adacore.com >> From: Pedro Alves <palves@redhat.com> >> Date: Thu, 18 Jun 2020 18:32:42 +0100 >> >>>> 2. Use --avoid=stat --avoid=fstat. So far this seems like the best >>>> approach. Pedro pointed out that this means we won't get any gnulib >>>> fixes for other bugs in this area. However, given gdb's relatively >>>> minimal needs from stat, and given the fact that gnulib is >>>> introducing other bugs, this seems like an acceptable tradeoff to me. >>> >>> If that works, I think it's an okay solution. >> >> I disagree. It's not only GDB's stat usage that counts, it's the >> other gnulib modules that depend on the stat module fixes too. Also, >> that approach disables the stat module for all systems, not just >> mingw. I think it's just bad policy to disable a module like that. > > What would you suggest instead? > I had presented two different ways forward back when this was originally discussed: [Handle different "struct stat" between GDB and BFD] [users/palves/stat branch] https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00608.html [use gnulib --avoid=largefile, let ACX_LARGEFILE handle enabling largefile support consistently throughout the tree] [users/palves/gnulib-largefile branch] https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00637.html Back then IIRC, I was thinking that the --avoid=largefile solution was the best one, but for some reason, the gnulib build fails with that, which seemed like a gnulib bug. Since you then said back then you didn't really care for a solution to this problem, I ended up forgetting about this whole issue and many of the details. :-/ Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 12:02 ` Pedro Alves @ 2020-06-19 12:13 ` Eli Zaretskii 2020-06-19 17:09 ` Tom Tromey 1 sibling, 0 replies; 48+ messages in thread From: Eli Zaretskii @ 2020-06-19 12:13 UTC (permalink / raw) To: Pedro Alves; +Cc: tom, tromey, gdb-patches > Cc: tom@tromey.com, tromey@adacore.com, gdb-patches@sourceware.org > From: Pedro Alves <palves@redhat.com> > Date: Fri, 19 Jun 2020 13:02:28 +0100 > > I had presented two different ways forward back when this was originally > discussed: > > [Handle different "struct stat" between GDB and BFD] > [users/palves/stat branch] > https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00608.html > > [use gnulib --avoid=largefile, let ACX_LARGEFILE handle enabling largefile support consistently > throughout the tree] > [users/palves/gnulib-largefile branch] > https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00637.html > > Back then IIRC, I was thinking that the --avoid=largefile solution > was the best one, but for some reason, the gnulib build fails with that, > which seemed like a gnulib bug. So maybe we should report the latter issue to Gnulib folks, because the --avoid=largefile solution was also proposed to me by Paul Eggert and Bruno Haible at the time: https://lists.gnu.org/archive/html/bug-gnulib/2019-12/msg00216.html > Since you then said back then you didn't really care for a solution > to this problem, I ended up forgetting about this whole issue and > many of the details. :-/ I still "don't care", in the sense that if this is only a problem for the mingw.org's MinGW build of GDB, I can solve it locally, and don't want to be the reason for the GDB development being at an impasse. I responded to Tom's message because I interpreted it as meaning that Tom was working on a problem whose effects are much wider, and because Tom explicitly asked for my opinion. IOW, I'm okay with your proposals as well, as long as they work for the affected systems. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 12:02 ` Pedro Alves 2020-06-19 12:13 ` Eli Zaretskii @ 2020-06-19 17:09 ` Tom Tromey 2020-06-19 20:24 ` Tom Tromey 1 sibling, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-06-19 17:09 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, tromey, gdb-patches Pedro> [Handle different "struct stat" between GDB and BFD] Pedro> [users/palves/stat branch] Pedro> https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00608.html Pedro> [use gnulib --avoid=largefile, let ACX_LARGEFILE handle enabling largefile support consistently Pedro> throughout the tree] Pedro> [users/palves/gnulib-largefile branch] Pedro> https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00637.html Both of these have the "different mtime" problem though. But, I suppose we can try the auditing approach and see what happens. I rebased the stat branch and I'll integrate my patches into it today. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 17:09 ` Tom Tromey @ 2020-06-19 20:24 ` Tom Tromey 2020-06-19 23:05 ` Pedro Alves 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-06-19 20:24 UTC (permalink / raw) To: Tom Tromey; +Cc: Pedro Alves, Eli Zaretskii, gdb-patches Tom> I rebased the stat branch and I'll integrate my patches into it today. I couldn't make this work. The previous patch worked because, at the time, common-defs.h could include bfd.h relatively early. That's no longer the case, and after trying a number of things I found that perhaps this double inclusion interferes with some system headers somehow. I'm not sure what to do. Maybe I can try namespacing just sys/stat.h? Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 20:24 ` Tom Tromey @ 2020-06-19 23:05 ` Pedro Alves 2020-07-21 19:39 ` Tom Tromey 2020-07-28 19:31 ` Tom Tromey 0 siblings, 2 replies; 48+ messages in thread From: Pedro Alves @ 2020-06-19 23:05 UTC (permalink / raw) To: Tom Tromey; +Cc: Eli Zaretskii, gdb-patches On 6/19/20 9:24 PM, Tom Tromey wrote: > Tom> I rebased the stat branch and I'll integrate my patches into it today. > > I couldn't make this work. The previous patch worked because, at the > time, common-defs.h could include bfd.h relatively early. That's no > longer the case, and after trying a number of things I found that > perhaps this double inclusion interferes with some system headers > somehow. > I'm not sure what to do. Maybe I can try namespacing just sys/stat.h? Can you clarify what do you see not work? I tried rebasing it here, and it seems to work. It builds cleanly on GNU/Linux and mingw32-w64 (both -m64 and -m32). I've pasted the resulting patch below. I've also added an #error in case something pulls in sys/stat.h before we get to defs.h. I think that if that happens due to some include in common-defs.h pulling sys/stat.h (say, libiberty.h ever exports some API using struct stat), then it would likely be exposing a problem similar to the bfd.h one, in that we should be calling such an API with the stat that libiberty was built against. Hopefully that won't happen -- external interfaces using "struct stat" are a really bad idea, as this whole thread shows... I think I'd still like to try to understand why the --avoid=largefile solution doesn't work though. From fd8d185820e572dcff6fbfe2e65cd5b79cef86f3 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Fri, 19 Jun 2020 23:09:10 +0100 Subject: [PATCH] Handle different "struct stat" between GDB and BFD --- gdb/defs.h | 19 +++++++++++++++++++ gdb/gdb_bfd.c | 45 +++++++++++++++++++++++++++++++++++++++++---- gdb/gdb_bfd.h | 2 +- gdb/jit.c | 4 ++-- gdb/symfile.c | 2 +- 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/gdb/defs.h b/gdb/defs.h index a75511158a4..b8723dccd12 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -34,8 +34,27 @@ #undef PACKAGE_TARNAME #include <config.h> + +/* Gnulib may replace struct stat with its own version. Bfd does not + use gnulib, so when we call into bfd, we must use the system struct + stat. */ + +#ifdef _GL_SYS_STAT_H +# error "gnulib's <sys/stat.h> included too early" +#endif + +#define __need_system_sys_stat_h 1 + +#include <sys/stat.h> + #include "bfd.h" +typedef struct stat sys_stat; + +#undef __need_system_sys_stat_h + +#include <sys/stat.h> + #include <sys/types.h> #include <limits.h> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 25e0178a8b8..6026a6b7dbe 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -66,7 +66,7 @@ struct gdb_bfd_data needs_relocations (0), crc_computed (0) { - struct stat buf; + sys_stat buf; if (bfd_stat (abfd, &buf) == 0) { @@ -361,24 +361,61 @@ gdb_bfd_iovec_fileio_close (struct bfd *abfd, void *stream) return 0; } +/* Convert between a struct stat (potentially a gnulib replacement) + and a sys_stat (the system's struct stat). */ + +static sys_stat +to_sys_stat (struct stat *st) +{ + sys_stat sst {}; + +#define COPY(FIELD) \ + sst.FIELD = st->FIELD + + COPY (st_dev); + COPY (st_ino); + COPY (st_mode); + COPY (st_nlink); + COPY (st_uid); + COPY (st_gid); + COPY (st_rdev); + + /* Check for overflow? */ + COPY (st_size); + + // Should probably check _GL_WINDOWS_STAT_TIMESPEC and refer to + // st_atim, etc. instead. + COPY (st_atime); + COPY (st_mtime); + COPY (st_ctime); + +#undef COPY + + return sst; +} + /* Wrapper for target_fileio_fstat suitable for passing as the STAT_FUNC argument to gdb_bfd_openr_iovec. */ static int gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, - struct stat *sb) + sys_stat *sb) { int fd = *(int *) stream; int target_errno; int result; - result = target_fileio_fstat (fd, sb, &target_errno); + struct stat st; + + result = target_fileio_fstat (fd, &st, &target_errno); if (result == -1) { errno = fileio_errno_to_host (target_errno); bfd_set_error (bfd_error_system_call); } + *sb = to_sys_stat (&st); + return result; } @@ -826,7 +863,7 @@ gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)) + sys_stat *sb)) { bfd *result = bfd_openr_iovec (filename, target, open_func, open_closure, diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h index a941b79fe73..8eb44f5ea20 100644 --- a/gdb/gdb_bfd.h +++ b/gdb/gdb_bfd.h @@ -158,7 +158,7 @@ gdb_bfd_ref_ptr gdb_bfd_openr_iovec (const char *filename, const char *target, void *stream), int (*stat_func) (struct bfd *abfd, void *stream, - struct stat *sb)); + sys_stat *sb)); /* A wrapper for bfd_openr_next_archived_file that initializes the gdb-specific reference count. */ diff --git a/gdb/jit.c b/gdb/jit.c index 1b5ef46469e..bb59dafb126 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -124,11 +124,11 @@ mem_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf, /* For statting the file, we only support the st_size attribute. */ static int -mem_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb) +mem_bfd_iovec_stat (struct bfd *abfd, void *stream, sys_stat *sb) { struct target_buffer *buffer = (struct target_buffer*) stream; - memset (sb, 0, sizeof (struct stat)); + memset (sb, 0, sizeof (sys_stat)); sb->st_size = buffer->size; return 0; } diff --git a/gdb/symfile.c b/gdb/symfile.c index b29f864b373..a00f04697ad 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1257,7 +1257,7 @@ separate_debug_file_exists (const std::string &name, unsigned long crc, { unsigned long file_crc; int file_crc_p; - struct stat parent_stat, abfd_stat; + sys_stat parent_stat, abfd_stat; int verified_as_different; /* Find a separate debug info file as if symbols would be present in base-commit: 87f83f20023bf366c14ec4e0fd307948d96caaee -- 2.14.5 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 23:05 ` Pedro Alves @ 2020-07-21 19:39 ` Tom Tromey 2020-07-28 19:31 ` Tom Tromey 1 sibling, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-07-21 19:39 UTC (permalink / raw) To: Pedro Alves via Gdb-patches; +Cc: Tom Tromey, Pedro Alves >>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes: Pedro> Can you clarify what do you see not work? I tried rebasing it Pedro> here, and it seems to work. I don't remember any more. It doesn't seem important anyway. Pedro> I've also added an #error in case something pulls in sys/stat.h Pedro> before we get to defs.h. I think that if that happens due to Pedro> some include in common-defs.h pulling sys/stat.h (say, Pedro> libiberty.h ever exports some API using struct stat), then it Pedro> would likely be exposing a problem similar to the bfd.h one, in Pedro> that we should be calling such an API with the stat that Pedro> libiberty was built against. I think we still have the problem where we must to be careful to avoid comparing mtime-from-bfd against mtime-from-gnulib. I think this does occur in gdb. In particular in source-cache.c: if (mtime && mtime < st.st_mtime) warning (_("Source file is more recent than executable.")); mtime comes from BFD, but st.st_mtime comes from gnulib. Pedro> Hopefully that won't Pedro> happen -- external interfaces using "struct stat" are a really bad idea, Pedro> as this whole thread shows... I think it's more a self-caused problem, in that gnulib assumes that it can freely replace anything, but on the other hand gdb relies on libraries that do not follow this same model. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-19 23:05 ` Pedro Alves 2020-07-21 19:39 ` Tom Tromey @ 2020-07-28 19:31 ` Tom Tromey 2020-08-13 12:15 ` Tom de Vries 1 sibling, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-07-28 19:31 UTC (permalink / raw) To: Pedro Alves via Gdb-patches; +Cc: Tom Tromey, Pedro Alves >>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes: Pedro> Can you clarify what do you see not work? I tried rebasing it Pedro> here, and it seems to work. It builds cleanly on GNU/Linux and Pedro> mingw32-w64 (both -m64 and -m32). I've pasted the resulting patch below. What if we apply this patch, but also apply a patch to our gnulib to avoid the timezone offsetting? We kept the code in gnulib/update-gnulib.sh to make this pretty easy to do, and we could try to work with upstream gnulib to make this feature optional (hopefully eliminating the need for our patch). This would avoid the mtime comparison problem. I don't know how to solve that another way, at least not without changing BFD to also use gnulib -- which seems to come with its own risks. Once we solve this somehow, I can rebase and update the original series here. This is one of the final blockers to the 10.1 branch, so it would be good to reach some kind of resolution soon. thanks, Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-07-28 19:31 ` Tom Tromey @ 2020-08-13 12:15 ` Tom de Vries 2020-08-14 23:40 ` Joel Brobecker 0 siblings, 1 reply; 48+ messages in thread From: Tom de Vries @ 2020-08-13 12:15 UTC (permalink / raw) To: Tom Tromey, Pedro Alves via Gdb-patches; +Cc: Pedro Alves, Joel Brobecker On 7/28/20 9:31 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes: > > Pedro> Can you clarify what do you see not work? I tried rebasing it > Pedro> here, and it seems to work. It builds cleanly on GNU/Linux and > Pedro> mingw32-w64 (both -m64 and -m32). I've pasted the resulting patch below. > > What if we apply this patch, but also apply a patch to our gnulib to > avoid the timezone offsetting? We kept the code in > gnulib/update-gnulib.sh to make this pretty easy to do, and we could try > to work with upstream gnulib to make this feature optional (hopefully > eliminating the need for our patch). > > This would avoid the mtime comparison problem. I don't know how to > solve that another way, at least not without changing BFD to also use > gnulib -- which seems to come with its own risks. > > Once we solve this somehow, I can rebase and update the original series > here. > > This is one of the final blockers to the 10.1 branch, so it would be > good to reach some kind of resolution soon. > Hi, [ it's been a bit more that two weeks since the last email on this, so I'm pinging this. ] Joel wrote as summary here ( https://sourceware.org/pipermail/gdb-patches/2020-August/171176.html ) : ... Hannes proposed, as a workaround, a hack involving undefining/redefining the fstat macro. The way it was proposed is not stricly portable as I understand it (it uses an extension), so we wouldn't be able to use it as is. But I'm starting to warm to the idea of doing something like that on the branch, while we take the time we need to discuss the proper approach. ... Is there any indication by now which way we want to go with this for the 10.1 branch? Thanks, - Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-08-13 12:15 ` Tom de Vries @ 2020-08-14 23:40 ` Joel Brobecker 2020-08-23 16:09 ` Joel Brobecker 0 siblings, 1 reply; 48+ messages in thread From: Joel Brobecker @ 2020-08-14 23:40 UTC (permalink / raw) To: Tom de Vries; +Cc: Tom Tromey, Pedro Alves via Gdb-patches, Pedro Alves > > Pedro> Can you clarify what do you see not work? I tried rebasing it > > Pedro> here, and it seems to work. It builds cleanly on GNU/Linux and > > Pedro> mingw32-w64 (both -m64 and -m32). I've pasted the resulting patch below. > > > > What if we apply this patch, but also apply a patch to our gnulib to > > avoid the timezone offsetting? We kept the code in > > gnulib/update-gnulib.sh to make this pretty easy to do, and we could try > > to work with upstream gnulib to make this feature optional (hopefully > > eliminating the need for our patch). > > > > This would avoid the mtime comparison problem. I don't know how to > > solve that another way, at least not without changing BFD to also use > > gnulib -- which seems to come with its own risks. > > > > Once we solve this somehow, I can rebase and update the original series > > here. > > > > This is one of the final blockers to the 10.1 branch, so it would be > > good to reach some kind of resolution soon. > > > > Hi, > > [ it's been a bit more that two weeks since the last email on this, so > I'm pinging this. ] > > Joel wrote as summary here ( > https://sourceware.org/pipermail/gdb-patches/2020-August/171176.html ) : > ... > Hannes proposed, as a workaround, a hack involving > undefining/redefining the fstat macro. The way it was proposed > is not stricly portable as I understand it (it uses an extension), > so we wouldn't be able to use it as is. But I'm starting to > warm to the idea of doing something like that on the branch, > while we take the time we need to discuss the proper approach. > ... > > Is there any indication by now which way we want to go with this for the > 10.1 branch? For the short-term solution, I think the best compromise is to patch gnulib like Tom suggests. We have the infrastructure to do that and maintain the patch for as long as it takes to discuss the proper solution (personally, I think the only way forward for the better solution will require a live discussion; I am happy to help set that up if people would find it useful). Please let us know if you guys think there is a better short term solution. Otherwise, let's ask Tom to submit a patch that patches gnulib, and start creating the GDB 10 pre-release. -- Joel ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-08-14 23:40 ` Joel Brobecker @ 2020-08-23 16:09 ` Joel Brobecker 2020-08-23 23:32 ` Pedro Alves 0 siblings, 1 reply; 48+ messages in thread From: Joel Brobecker @ 2020-08-23 16:09 UTC (permalink / raw) To: Tom de Vries; +Cc: Pedro Alves, Pedro Alves via Gdb-patches, Tom Tromey > For the short-term solution, I think the best compromise is to patch > gnulib like Tom suggests. We have the infrastructure to do that and > maintain the patch for as long as it takes to discuss the proper > solution (personally, I think the only way forward for the better > solution will require a live discussion; I am happy to help set that > up if people would find it useful). I discussed it live with Tom. We don't have the time to work on this right now, so we'll send a patch in a week or so. Given that everyone has had a a lot of time to provide feedback on that thread, and given the fact that we want to fix this before we branch, I don't think we want to wait too long before getting that patch in -- assuming we are still short on a better long term solution. -- Joel ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-08-23 16:09 ` Joel Brobecker @ 2020-08-23 23:32 ` Pedro Alves 2020-08-24 20:04 ` Joel Brobecker 0 siblings, 1 reply; 48+ messages in thread From: Pedro Alves @ 2020-08-23 23:32 UTC (permalink / raw) To: Joel Brobecker, Tom de Vries; +Cc: Tom Tromey, Pedro Alves via Gdb-patches On 8/23/20 5:09 PM, Joel Brobecker wrote: >> For the short-term solution, I think the best compromise is to patch >> gnulib like Tom suggests. We have the infrastructure to do that and >> maintain the patch for as long as it takes to discuss the proper >> solution (personally, I think the only way forward for the better >> solution will require a live discussion; I am happy to help set that >> up if people would find it useful). > > I discussed it live with Tom. We don't have the time to work on this > right now, so we'll send a patch in a week or so. Given that everyone > has had a a lot of time to provide feedback on that thread, and given > the fact that we want to fix this before we branch, I don't think we > want to wait too long before getting that patch in -- assuming we are > still short on a better long term solution. > FWIW, I've tried to look at this more than once the past weeks, including today, hoping I'd find some nice solution, but all I got was a headache. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-08-23 23:32 ` Pedro Alves @ 2020-08-24 20:04 ` Joel Brobecker 2020-09-02 14:45 ` Tom Tromey 0 siblings, 1 reply; 48+ messages in thread From: Joel Brobecker @ 2020-08-24 20:04 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom de Vries, Tom Tromey, Pedro Alves via Gdb-patches Hi Pedro, > >> For the short-term solution, I think the best compromise is to patch > >> gnulib like Tom suggests. We have the infrastructure to do that and > >> maintain the patch for as long as it takes to discuss the proper > >> solution (personally, I think the only way forward for the better > >> solution will require a live discussion; I am happy to help set that > >> up if people would find it useful). > > > > I discussed it live with Tom. We don't have the time to work on this > > right now, so we'll send a patch in a week or so. Given that everyone > > has had a a lot of time to provide feedback on that thread, and given > > the fact that we want to fix this before we branch, I don't think we > > want to wait too long before getting that patch in -- assuming we are > > still short on a better long term solution. > > > > FWIW, I've tried to look at this more than once the past weeks, > including today, hoping I'd find some nice solution, but all I got > was a headache. Yeah, cannot say I am surprised :-(, although I was thinking you might be in posessions of some superpowers that made you able to find a solution I couldn't. The fact that we have this hybrid situation where GDB uses gnulib but one of its depedencies doesn't makes the use of stat intractable, and if even you are having difficulties with it, I think our only realistic options are to take a more radical approach, either: - Stop using the stat module from gnulib (which I think we said we want to keep); or - Patch the gnulib implementation for now to block the incompatible behavior. The idea with this approach is that we'd try to work with gnulib maintainers to see if we could agree on a way to make the incompatibility conditional on something. That would be our way to eliminate this local change. - Something else? -- Joel ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-08-24 20:04 ` Joel Brobecker @ 2020-09-02 14:45 ` Tom Tromey 2020-09-02 14:59 ` Joel Brobecker 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-09-02 14:45 UTC (permalink / raw) To: Joel Brobecker; +Cc: Pedro Alves, Pedro Alves via Gdb-patches, Tom Tromey >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes: Joel> I think our only realistic options are to take a more radical approach, Joel> either: Joel> - Stop using the stat module from gnulib (which I think we said Joel> we want to keep); or Joel> - Patch the gnulib implementation for now to block the incompatible Joel> behavior. [...] Joel> - Something else? One remaining option is to change BFD to use gnulib as well. This is workable as long as gdb never tries to use an out-of-tree library that uses struct stat (or a stat-derived time_t) as part of its API. To my knowledge gdb is currently safe on this front. I'd guess it seems likely to remain safe too, though of course one can't be certain. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-09-02 14:45 ` Tom Tromey @ 2020-09-02 14:59 ` Joel Brobecker 0 siblings, 0 replies; 48+ messages in thread From: Joel Brobecker @ 2020-09-02 14:59 UTC (permalink / raw) To: Tom Tromey; +Cc: Pedro Alves, Pedro Alves via Gdb-patches, Tom Tromey > Joel> I think our only realistic options are to take a more radical approach, > Joel> either: > > Joel> - Stop using the stat module from gnulib (which I think we said > Joel> we want to keep); or > > Joel> - Patch the gnulib implementation for now to block the incompatible > Joel> behavior. > [...] > Joel> - Something else? > > One remaining option is to change BFD to use gnulib as well. > > This is workable as long as gdb never tries to use an out-of-tree > library that uses struct stat (or a stat-derived time_t) as part of its > API. To my knowledge gdb is currently safe on this front. I'd guess it > seems likely to remain safe too, though of course one can't be certain. Indeed. IIRC, this is something that was briefly discussed with the binutils team. They were open to the idea, but on the other hand, I think they needed a bit more information about the expected gains they could hope for. We can pursue this again with them if we'd like and we think this could be decided and implemented relatively quickly. My feeling is that this is not a trivial move, and thus would deserve a bit of discussion before moving forward. Doing that just before we branch seems a bit riskier. Perhaps we could first go with the local gnulib patch, and then discuss with binutils about them adopting gnulib. PS: Do we have an idea how binutils depending on gnulib might affect binutils standalone distribution (e.g. the binutils libs provided by the various distros)? -- Joel ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 2/3] Consistently use BFD's time 2020-06-18 17:32 ` Pedro Alves 2020-06-18 17:54 ` Eli Zaretskii @ 2020-06-18 17:57 ` Tom Tromey 1 sibling, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-06-18 17:57 UTC (permalink / raw) To: Pedro Alves; +Cc: Eli Zaretskii, Tom Tromey, gdb-patches, tromey Pedro> I disagree. It's not only GDB's stat usage that counts, it's the Pedro> other gnulib modules that depend on the stat module fixes too. Also, Pedro> that approach disables the stat module for all systems, not just Pedro> mingw. I think it's just bad policy to disable a module like that. The reason I came to this conclusion is that the list of fixes doesn't seem to include anything super important; while using it introduces bugs that are difficult to work around, or even audit for. In particular, we'll have to be careful to never compare mtime-from-gnulib against mtime-from-BFD. Here's the list from the version of gnulib we're using. Portability problems fixed by Gnulib: * On platforms where 'off_t' is a 32-bit type, 'stat' may not correctly report the size of files or block devices larger than 2 GB. (Cf. 'AC_SYS_LARGEFILE'.) * On Linux/x86 and Linux/x86_64, applications compiled in 32-bit mode cannot access files that happen to have a 64-bit inode number. This can occur with file systems such as XFS (typically on large disks) and NFS. (Cf. 'AC_SYS_LARGEFILE'.) * The 'st_atime', 'st_ctime', 'st_mtime' fields are affected by the current time zone and by the DST flag of the current time zone on some platforms: mingw, MSVC 14 (when the environment variable 'TZ' is set). * On MSVC 14, this function fails with error 'ENOENT' on files such as 'C:\pagefile.sys' and on directories such as 'C:\System Volume Information'. * On some platforms, 'stat("link-to-file/",buf)' succeeds instead of failing with 'ENOTDIR'. FreeBSD 7.2, AIX 7.1, Solaris 9, mingw64. * On some platforms, 'stat(".",buf)' and 'stat("./",buf)' give different results: mingw, MSVC 14. * On Solaris 11.4, when this function yields a timestamp with a nonpositive 'tv_sec' value, 'tv_nsec' might be in the range -1000000000..-1, representing a negative nanoseconds offset from 'tv_sec'. I think the first one is probably not important to gdb; the second is already working; and the third is a bug. The rest just don't seem very relevant. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 3/3] Further simplify gdb BFD caching 2020-01-14 21:10 [PATCH 0/3] Fix gdb's BFD cache Tom Tromey 2020-01-14 21:10 ` [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c Tom Tromey 2020-01-14 21:10 ` [PATCH 2/3] Consistently use BFD's time Tom Tromey @ 2020-01-14 22:13 ` Tom Tromey 2020-01-23 22:30 ` Tom Tromey 2 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-01-14 22:13 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This patch wasn't necessary to fix any bug, but while working on the previous patches I noticed that gdb was calling bfd_stat one extra time, and that it would be simple to rearrange the code to avoid this extra call. This also changes gdb so that it no longer caches a BFD if the stat fails, which seems preferable. gdb/ChangeLog 2020-01-14 Tom Tromey <tromey@adacore.com> * gdb_bfd.c (gdb_bfd_data): Remove "mt" parameter, add "st" parameter. Don't call bfd_stat. (gdb_bfd_init_data): Replace "mt" parameter with "st". (gdb_bfd_open): Rearrange and update. (gdb_bfd_ref): Call bfd_stat. Change-Id: I48f13f09f59bde49191cb40ee88ed3e18fa7c7d9 --- gdb/ChangeLog | 8 ++++++ gdb/gdb_bfd.c | 69 ++++++++++++++++++++++----------------------------- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c index 26968396a15..d64630b2362 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -59,26 +59,16 @@ static htab_t all_bfds; struct gdb_bfd_data { - gdb_bfd_data (bfd *abfd, time_t mt) - : mtime (mt), - size (bfd_get_size (abfd)), + /* Note that if ST is nullptr, then we simply fill in zeroes. */ + gdb_bfd_data (bfd *abfd, struct stat *st) + : mtime (st == nullptr ? 0 : st->st_mtime), + size (st == nullptr ? 0 : st->st_size), + inode (st == nullptr ? 0 : st->st_ino), + device_id (st == nullptr ? 0 : st->st_dev), relocation_computed (0), needs_relocations (0), crc_computed (0) { - struct stat buf; - - if (bfd_stat (abfd, &buf) == 0) - { - inode = buf.st_ino; - device_id = buf.st_dev; - } - else - { - /* The stat failed. */ - inode = 0; - device_id = 0; - } } ~gdb_bfd_data () @@ -380,7 +370,7 @@ gdb_bfd_iovec_fileio_fstat (struct bfd *abfd, void *stream, BFD. */ static void -gdb_bfd_init_data (struct bfd *abfd, time_t mtime) +gdb_bfd_init_data (struct bfd *abfd, struct stat *st) { struct gdb_bfd_data *gdata; void **slot; @@ -390,7 +380,7 @@ gdb_bfd_init_data (struct bfd *abfd, time_t mtime) /* Ask BFD to decompress sections in bfd_get_full_section_contents. */ abfd->flags |= BFD_DECOMPRESS; - gdata = new gdb_bfd_data (abfd, mtime); + gdata = new gdb_bfd_data (abfd, st); bfd_set_usrdata (abfd, gdata); bfd_alloc_data (abfd); @@ -405,10 +395,7 @@ gdb_bfd_init_data (struct bfd *abfd, time_t mtime) gdb_bfd_ref_ptr gdb_bfd_open (const char *name, const char *target, int fd) { - hashval_t hash; - void **slot; bfd *abfd; - struct gdb_bfd_cache_search search; struct stat st; if (is_target_filename (name)) @@ -428,10 +415,6 @@ gdb_bfd_open (const char *name, const char *target, int fd) name += strlen (TARGET_SYSROOT_PREFIX); } - if (gdb_bfd_cache == NULL) - gdb_bfd_cache = htab_create_alloc (1, hash_bfd, eq_bfd, NULL, - xcalloc, xfree); - if (fd == -1) { fd = gdb_open_cloexec (name, O_RDONLY | O_BINARY, 0); @@ -449,29 +432,31 @@ gdb_bfd_open (const char *name, const char *target, int fd) if (abfd == NULL) return NULL; - search.filename = name; + struct stat *st_ptr = &st; if (bfd_stat (abfd, &st) < 0) { - /* Weird situation here. */ - search.mtime = 0; - search.size = 0; - search.inode = 0; - search.device_id = 0; + /* Weird situation here -- don't cache if we can't stat. */ + st_ptr = nullptr; } - else + + if (bfd_sharing && st_ptr != nullptr) { + if (gdb_bfd_cache == NULL) + gdb_bfd_cache = htab_create_alloc (1, hash_bfd, eq_bfd, NULL, + xcalloc, xfree); + + struct gdb_bfd_cache_search search; + search.filename = name; search.mtime = st.st_mtime; search.size = st.st_size; search.inode = st.st_ino; search.device_id = st.st_dev; - } - /* Note that this must compute the same result as hash_bfd. */ - hash = htab_hash_string (name); + /* Note that this must compute the same result as hash_bfd. */ + hashval_t hash = htab_hash_string (name); - if (bfd_sharing) - { - slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, INSERT); + void **slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, + INSERT); if (*slot != nullptr) { bfd_close (abfd); @@ -500,7 +485,7 @@ gdb_bfd_open (const char *name, const char *target, int fd) and since we already entered it into the hash table using this mtime, if the file changed at the wrong moment, the race would lead to a hash table corruption. */ - gdb_bfd_init_data (abfd, search.mtime); + gdb_bfd_init_data (abfd, st_ptr); return gdb_bfd_ref_ptr (abfd); } @@ -572,7 +557,11 @@ gdb_bfd_ref (struct bfd *abfd) return; } - gdb_bfd_init_data (abfd, bfd_get_mtime (abfd)); + struct stat st, *st_ptr = &st; + if (bfd_stat (abfd, &st) < 0) + st_ptr = nullptr; + + gdb_bfd_init_data (abfd, st_ptr); } /* See gdb_bfd.h. */ -- 2.21.1 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/3] Further simplify gdb BFD caching 2020-01-14 22:13 ` [PATCH 3/3] Further simplify gdb BFD caching Tom Tromey @ 2020-01-23 22:30 ` Tom Tromey 2020-09-02 18:45 ` Tom Tromey 0 siblings, 1 reply; 48+ messages in thread From: Tom Tromey @ 2020-01-23 22:30 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches >>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes: Tom> gdb/ChangeLog Tom> 2020-01-14 Tom Tromey <tromey@adacore.com> Tom> * gdb_bfd.c (gdb_bfd_data): Remove "mt" parameter, add "st" Tom> parameter. Don't call bfd_stat. Tom> (gdb_bfd_init_data): Replace "mt" parameter with "st". Tom> (gdb_bfd_open): Rearrange and update. Tom> (gdb_bfd_ref): Call bfd_stat. This probably has to be merged into an earlier patch, because the eq_bfd function checks these other fields as well. I may end up just squashing the whole series, we'll see... but I won't do anything until the stat problem has a solution. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/3] Further simplify gdb BFD caching 2020-01-23 22:30 ` Tom Tromey @ 2020-09-02 18:45 ` Tom Tromey 0 siblings, 0 replies; 48+ messages in thread From: Tom Tromey @ 2020-09-02 18:45 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey, gdb-patches Tom> gdb/ChangeLog Tom> 2020-01-14 Tom Tromey <tromey@adacore.com> Tom> * gdb_bfd.c (gdb_bfd_data): Remove "mt" parameter, add "st" Tom> parameter. Don't call bfd_stat. Tom> (gdb_bfd_init_data): Replace "mt" parameter with "st". Tom> (gdb_bfd_open): Rearrange and update. Tom> (gdb_bfd_ref): Call bfd_stat. Tom> This probably has to be merged into an earlier patch, because the eq_bfd Tom> function checks these other fields as well. I may end up just squashing Tom> the whole series, we'll see... but I won't do anything until the stat Tom> problem has a solution. I've merged patches #1 and #3 from this series. I think the result is correct regardless of how the stat issue is fixed, so soon I'm going to submit it separately. Tom ^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2020-09-02 18:45 UTC | newest] Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-14 21:10 [PATCH 0/3] Fix gdb's BFD cache Tom Tromey 2020-01-14 21:10 ` [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c Tom Tromey 2020-01-14 22:26 ` Christian Biesinger via gdb-patches 2020-01-14 21:10 ` [PATCH 2/3] Consistently use BFD's time Tom Tromey 2020-01-14 23:17 ` Christian Biesinger via gdb-patches 2020-01-15 17:51 ` Eli Zaretskii 2020-01-16 20:47 ` Pedro Alves 2020-01-16 21:58 ` Christian Biesinger via gdb-patches 2020-01-16 22:31 ` Pedro Alves 2020-01-17 8:48 ` Eli Zaretskii 2020-01-17 18:32 ` Tom Tromey 2020-01-17 21:03 ` Tom Tromey 2020-01-18 11:07 ` Eli Zaretskii 2020-01-20 15:52 ` Pedro Alves 2020-01-20 15:53 ` Pedro Alves 2020-01-20 20:50 ` Eli Zaretskii 2020-01-20 20:58 ` Pedro Alves 2020-01-21 15:50 ` Pedro Alves 2020-01-21 19:38 ` Eli Zaretskii 2020-01-21 17:56 ` Eli Zaretskii 2020-01-23 22:05 ` Tom Tromey 2020-06-19 17:51 ` Tom Tromey 2020-01-17 7:57 ` Eli Zaretskii 2020-04-01 20:20 ` Tom Tromey 2020-06-18 14:14 ` Tom Tromey 2020-06-18 15:04 ` Eli Zaretskii 2020-06-18 16:00 ` Tom Tromey 2020-06-18 17:27 ` Eli Zaretskii 2020-06-18 17:32 ` Pedro Alves 2020-06-18 17:54 ` Eli Zaretskii 2020-06-19 12:02 ` Pedro Alves 2020-06-19 12:13 ` Eli Zaretskii 2020-06-19 17:09 ` Tom Tromey 2020-06-19 20:24 ` Tom Tromey 2020-06-19 23:05 ` Pedro Alves 2020-07-21 19:39 ` Tom Tromey 2020-07-28 19:31 ` Tom Tromey 2020-08-13 12:15 ` Tom de Vries 2020-08-14 23:40 ` Joel Brobecker 2020-08-23 16:09 ` Joel Brobecker 2020-08-23 23:32 ` Pedro Alves 2020-08-24 20:04 ` Joel Brobecker 2020-09-02 14:45 ` Tom Tromey 2020-09-02 14:59 ` Joel Brobecker 2020-06-18 17:57 ` Tom Tromey 2020-01-14 22:13 ` [PATCH 3/3] Further simplify gdb BFD caching Tom Tromey 2020-01-23 22:30 ` Tom Tromey 2020-09-02 18:45 ` 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).