[PATCH] Lock bfd_stat and bfd_get_mtime

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> PR gdb/31713 points out some races when using the background DWARF
> reader.
>
> This particular patch fixes some of these, namely the ones when using
> the sim.  In this case, the 'load' command calls reopen_exec_file,
> which calls bfd_stat, which introduces a race.
>
> BFD only locks globals -- concurrent use of a single BFD must be
> handled by the application.  To this end, this patch adds locked
> wrappers for bfd_stat and bfd_get_mtime.
>
> I couldn't reproduce these data races but the original reporter tested
> the patch and confirms that it helps.

Thanks.  This looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713
> ---
>  gdb/corefile.c  |  6 +++---
>  gdb/exec.c      |  2 +-
>  gdb/gdb_bfd.c   | 26 ++++++++++++++++++++++++++
>  gdb/gdb_bfd.h   | 11 +++++++++++
>  gdb/machoread.c |  3 ++-
>  gdb/objfiles.c  |  2 +-
>  gdb/symfile.c   |  6 +++---
>  7 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index f6ec3cd5ca1..be0f1075488 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -51,7 +51,7 @@ reopen_exec_file (void)
>  
>    /* If the timestamp of the exec file has changed, reopen it.  */
>    struct stat st;
> -  int res = bfd_stat (exec_bfd, &st);
> +  int res = gdb_bfd_stat (exec_bfd, &st);
>  
>    if (res == 0
>        && current_program_space->ebfd_mtime != 0
> @@ -70,8 +70,8 @@ validate_files (void)
>        if (!core_file_matches_executable_p (current_program_space->core_bfd (),
>  					   current_program_space->exec_bfd ()))
>  	warning (_("core file may not match specified executable file."));
> -      else if (bfd_get_mtime (current_program_space->exec_bfd ())
> -	       > bfd_get_mtime (current_program_space->core_bfd ()))
> +      else if (gdb_bfd_get_mtime (current_program_space->exec_bfd ())
> +	       > gdb_bfd_get_mtime (current_program_space->core_bfd ()))
>  	warning (_("exec file is newer than core file."));
>      }
>  }
> diff --git a/gdb/exec.c b/gdb/exec.c
> index 82d9266b7e3..73280ad59e7 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -483,7 +483,7 @@ exec_file_attach (const char *filename, int from_tty)
>  	= build_section_table (current_program_space->exec_bfd ());
>  
>        current_program_space->ebfd_mtime
> -	= bfd_get_mtime (current_program_space->exec_bfd ());
> +	= gdb_bfd_get_mtime (current_program_space->exec_bfd ());
>  
>        validate_files ();
>  
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 330d9182803..e0171871b76 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -1140,6 +1140,32 @@ gdb_bfd_get_full_section_contents (bfd *abfd, asection *section,
>  				   section_size);
>  }
>  
> +/* See gdb_bfd.h.  */
> +
> +int
> +gdb_bfd_stat (bfd *abfd, struct stat *sbuf)
> +{
> +#if CXX_STD_THREAD
> +  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
> +  std::lock_guard<std::mutex> guard (gdata->per_bfd_mutex);
> +#endif
> +
> +  return bfd_stat (abfd, sbuf);
> +}
> +
> +/* See gdb_bfd.h.  */
> +
> +long
> +gdb_bfd_get_mtime (bfd *abfd)
> +{
> +#if CXX_STD_THREAD
> +  gdb_bfd_data *gdata = (gdb_bfd_data *) bfd_usrdata (abfd);
> +  std::lock_guard<std::mutex> guard (gdata->per_bfd_mutex);
> +#endif
> +
> +  return bfd_get_mtime (abfd);
> +}
> +
>  #define AMBIGUOUS_MESS1	".\nMatching formats:"
>  #define AMBIGUOUS_MESS2	\
>    ".\nUse \"set gnutarget format-name\" to specify the format."
> diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
> index 331570d769c..a93d9aa92e8 100644
> --- a/gdb/gdb_bfd.h
> +++ b/gdb/gdb_bfd.h
> @@ -251,6 +251,17 @@ gdb_bfd_sections (const gdb_bfd_ref_ptr &abfd)
>    return gdb_bfd_section_range (abfd->sections);
>  };
>  
> +/* A wrapper for bfd_stat that acquires the per-BFD lock on ABFD.  */
> +
> +extern int gdb_bfd_stat (bfd *abfd, struct stat *sbuf)
> +  ATTRIBUTE_WARN_UNUSED_RESULT;
> +
> +/* A wrapper for bfd_get_mtime that acquires the per-BFD lock on
> +   ABFD.  */
> +
> +extern long gdb_bfd_get_mtime (bfd *abfd)
> +  ATTRIBUTE_WARN_UNUSED_RESULT;
> +
>  /* A wrapper for bfd_errmsg to produce a more helpful error message
>     in the case of bfd_error_file_ambiguously recognized.
>     MATCHING, if non-NULL, is the corresponding argument to
> diff --git a/gdb/machoread.c b/gdb/machoread.c
> index ef6cf660629..ac764c09496 100644
> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -434,7 +434,8 @@ macho_add_oso_symfile (oso_el *oso, const gdb_bfd_ref_ptr &abfd,
>        return;
>      }
>  
> -  if (abfd->my_archive == NULL && oso->mtime != bfd_get_mtime (abfd.get ()))
> +  if (abfd->my_archive == nullptr
> +      && oso->mtime != gdb_bfd_get_mtime (abfd.get ()))
>      {
>        warning (_("`%s': file time stamp mismatch."), oso->name);
>        return;
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 555195dc61f..db9a704f333 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -337,7 +337,7 @@ objfile::objfile (gdb_bfd_ref_ptr bfd_, program_space *pspace,
>  
>    if (obfd != nullptr)
>      {
> -      mtime = bfd_get_mtime (obfd.get ());
> +      mtime = gdb_bfd_get_mtime (obfd.get ());
>  
>        /* Build section table.  */
>        build_objfile_section_table (this);
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 4386cdce5e7..aa3076555c9 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1267,9 +1267,9 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
>       numbers will never set st_ino to zero, this is merely an
>       optimization, so we do not need to worry about false negatives.  */
>  
> -  if (bfd_stat (abfd.get (), &abfd_stat) == 0
> +  if (gdb_bfd_stat (abfd.get (), &abfd_stat) == 0
>        && abfd_stat.st_ino != 0
> -      && bfd_stat (parent_objfile->obfd.get (), &parent_stat) == 0)
> +      && gdb_bfd_stat (parent_objfile->obfd.get (), &parent_stat) == 0)
>      {
>        if (abfd_stat.st_dev == parent_stat.st_dev
>  	  && abfd_stat.st_ino == parent_stat.st_ino)
> @@ -2488,7 +2488,7 @@ reread_symbols (int from_tty)
>  	continue;
>  
>        struct stat new_statbuf;
> -      int res = bfd_stat (objfile->obfd.get (), &new_statbuf);
> +      int res = gdb_bfd_stat (objfile->obfd.get (), &new_statbuf);
>        if (res != 0)
>  	{
>  	  /* If this object is from an archive (what you usually create
> -- 
> 2.46.1