[PATCH 0/3] Fix gdb's BFD cache

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] Fix gdb's BFD cache

Tom Tromey-4
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


Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] Avoid hash table corruption in gdb_bfd.c

Tom Tromey-4
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  <[hidden email]>

        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

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Consistently use BFD's time

Tom Tromey-4
In reply to this post by Tom Tromey-4
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Further simplify gdb BFD caching

Tom Tromey-4
In reply to this post by Tom Tromey-4
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] Avoid hash table corruption in gdb_bfd.c

Sourceware - gdb-patches mailing list
In reply to this post by Tom Tromey-4
On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey <[hidden email]> 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  <[hidden email]>
>
>         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
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Sourceware - gdb-patches mailing list
In reply to this post by Tom Tromey-4
On Tue, Jan 14, 2020 at 4:10 PM Tom Tromey <[hidden email]> 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  <[hidden email]>
>
>         * 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Eli Zaretskii
In reply to this post by Tom Tromey-4
> From: Tom Tromey <[hidden email]>
> Cc: Tom Tromey <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
On 1/15/20 4:07 PM, Eli Zaretskii wrote:

>> From: Tom Tromey <[hidden email]>
>> Cc: Tom Tromey <[hidden email]>
>> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Sourceware - gdb-patches mailing list
On Thu, Jan 16, 2020 at 3:37 PM Pedro Alves <[hidden email]> wrote:

>
> On 1/15/20 4:07 PM, Eli Zaretskii wrote:
> >> From: Tom Tromey <[hidden email]>
> >> Cc: Tom Tromey <[hidden email]>
> >> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
On 1/16/20 8:46 PM, Christian Biesinger via gdb-patches wrote:
> On Thu, Jan 16, 2020 at 3:37 PM Pedro Alves <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Eli Zaretskii
In reply to this post by Pedro Alves-7
> Cc: [hidden email]
> From: Pedro Alves <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Eli Zaretskii
In reply to this post by Sourceware - gdb-patches mailing list
> From: Christian Biesinger <[hidden email]>
> Date: Thu, 16 Jan 2020 15:46:52 -0500
> Cc: Eli Zaretskii <[hidden email]>, Tom Tromey <[hidden email]>,
> gdb-patches <[hidden email]>
>
> > - #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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Tom Tromey-4
>>>>> "Eli" == Eli Zaretskii <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Tom Tromey-4
>>>>> "Tom" == Tom Tromey <[hidden email]> writes:

>>>>> "Eli" == Eli Zaretskii <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Eli Zaretskii
> From: Tom Tromey <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  Christian Biesinger <[hidden email]>,  [hidden email],  [hidden email]
> Date: Fri, 17 Jan 2020 13:55:57 -0700
>
> >>>>> "Tom" == Tom Tromey <[hidden email]> writes:
>
> >>>>> "Eli" == Eli Zaretskii <[hidden email]> 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.)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
On 1/18/20 7:58 AM, Eli Zaretskii wrote:

>> From: Tom Tromey <[hidden email]>
>> Cc: Eli Zaretskii <[hidden email]>,  Christian Biesinger <[hidden email]>,  [hidden email],  [hidden email]
>> Date: Fri, 17 Jan 2020 13:55:57 -0700
>>
>>>>>>> "Tom" == Tom Tromey <[hidden email]> writes:
>>
>>>>>>> "Eli" == Eli Zaretskii <[hidden email]> 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 <[hidden email]>
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


0002-Fix-gnulib-s-lstat-replacement-in-C-namespace-mode.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Eli Zaretskii
In reply to this post by Pedro Alves-7
> Cc: [hidden email], [hidden email]
> From: Pedro Alves <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
On 1/20/20 5:28 PM, Eli Zaretskii wrote:

>> Cc: [hidden email], [hidden email]
>> From: Pedro Alves <[hidden email]>
>> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3] Consistently use BFD's time

Pedro Alves-7
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

123