Re: [PATCH] Use styled_string when defering warnings when loading separate debug files

[Andrew Burgess <aburgess@redhat.com>]

Alexandra Hájková via Gdb-patches <gdb-patches@sourceware.org> writes:

> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
> filenames used in the warnings are styled properly now.
> ---
>  gdb/build-id.c | 19 ++++++++++++++++---
>  gdb/build-id.h |  5 ++++-
>  gdb/coffread.c |  6 +++---
>  gdb/elfread.c  |  6 +++---
>  gdb/symfile.c  | 21 ++++++++++++++++-----
>  gdb/symfile.h  |  5 ++++-
>  6 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 00250c20ae9..74605bb702c 100644
> --- a/gdb/build-id.c
> +++ b/gdb/build-id.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "cli/cli-style.h"
>  #include "bfd.h"
>  #include "gdb_bfd.h"
>  #include "build-id.h"
> @@ -205,11 +206,21 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>    return build_id_to_bfd_suffix (build_id_len, build_id, "");
>  }
>  
> +static void
> +add_warning (warnings_vec *vec, std::string msg)

This function should have a comment just above it.

> +{
> +  vec->emplace_back ([msg] () {
> +		     gdb_printf (gdb_stdlog, "%ps",
> +				 styled_string (file_name_style. style (),
> +						msg.c_str ()));
> +		     });
> +}
> +

This is going to style the whole of MSG with the file_name_style, this
isn't what you want....

>  /* See build-id.h.  */
>  
>  std::string
>  find_separate_debug_file_by_buildid (struct objfile *objfile,
> -				     std::vector<std::string> *warnings_vector)
> +				     warnings_vec *vec)
>  {
>    const struct bfd_build_id *build_id;
>  
> @@ -232,8 +243,10 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
>  	    = string_printf (_("\"%s\": separate debug info file has no "
>  			       "debug info"), bfd_get_filename (abfd.get ()));
>  	  if (separate_debug_file_debug)
> -	    gdb_printf (gdb_stdlog, "%s", msg.c_str ());
> -	  warnings_vector->emplace_back (std::move (msg));
> +	    gdb_printf (gdb_stdlog, "%ps",
> +			styled_string (file_name_style. style (),
> +				       msg.c_str ()));

... again here you're styling the whole string, which is wrong.  Also,
we shouldn't be styling strings (or we don't traditionally) style
strings in debug output.

> +	  add_warning(vec, msg);

You need something like:

         warnings->emplace_back ([msg] () {
           warning (_("\"%ps\": separate debug info file has no debug info"),
                    styled_string (file_name_style.style (),
                                   bfd_get_filename (abfd.get ())));

Which will just style the filename part of the output.


>  	}
>        else if (abfd != NULL)
>  	return std::string (bfd_get_filename (abfd.get ()));
> diff --git a/gdb/build-id.h b/gdb/build-id.h
> index 191720ddf28..8e72e6ecff6 100644
> --- a/gdb/build-id.h
> +++ b/gdb/build-id.h
> @@ -47,6 +47,9 @@ extern gdb_bfd_ref_ptr build_id_to_debug_bfd (size_t build_id_len,
>  extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
>  					     const bfd_byte *build_id);
>  
> +using warning_cb = std::function<void (void)>;
> +using warnings_vec = std::vector<warning_cb>;
> +
>  /* Find the separate debug file for OBJFILE, by using the build-id
>     associated with OBJFILE's BFD.  If successful, returns the file name for the
>     separate debug file, otherwise, return an empty string.
> @@ -58,7 +61,7 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
>     approach, then any warnings will be printed.  */
>  
>  extern std::string find_separate_debug_file_by_buildid
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> +  (struct objfile *objfile, warnings_vec *vec);
>  
>  /* Return an hex-string representation of BUILD_ID.  */
>  
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index 65d7828e933..c534eb504cf 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -734,7 +734,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>    /* Try to add separate debug file if no symbols table found.   */
>    if (!objfile->has_partial_symbols ())
>      {
> -      std::vector<std::string> warnings_vector;
> +      warnings_vec warnings_vector;
>        std::string debugfile
>  	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>  
> @@ -752,8 +752,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>        /* If all the methods to collect the debuginfo failed, print any
>  	 warnings that were collected.  */
>        if (debugfile.empty () && !warnings_vector.empty ())
> -	for (const std::string &w : warnings_vector)
> -	  warning ("%s", w.c_str ());
> +	for (const auto &cb : warnings_vector)
> +	  cb ();
>      }
>  }
>  
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index ca684aab57e..d1c231c2f8c 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1213,7 +1213,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>  	   && objfile->separate_debug_objfile == NULL
>  	   && objfile->separate_debug_objfile_backlink == NULL)
>      {
> -      std::vector<std::string> warnings_vector;
> +      warnings_vec warnings_vector;
>  
>        std::string debugfile
>  	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
> @@ -1265,8 +1265,8 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>        /* If all the methods to collect the debuginfo failed, print
>  	 the warnings, if there're any. */
>        if (debugfile.empty () && !has_dwarf2 && !warnings_vector.empty ())
> -	for (const std::string &w : warnings_vector)
> -	  warning ("%s", w.c_str ());
> +	for (const auto &cb : warnings_vector)
> +	  cb ();
>      }
>  
>    return has_dwarf2;
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 373f5592107..1fcb5fd5ae4 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1241,10 +1241,20 @@ symbol_file_clear (int from_tty)
>  
>  bool separate_debug_file_debug = false;
>  
> +static void
> +add_warning (warnings_vec *vec, std::string msg)
> +{
> +  vec->emplace_back ([msg] () {
> +		     gdb_printf (gdb_stdlog, "warning: %ps",
> +				 styled_string (file_name_style. style (),
> +						msg.c_str ()));
> +		     });
> +}
> +
>  static int
>  separate_debug_file_exists (const std::string &name, unsigned long crc,
>  			    struct objfile *parent_objfile,
> -			    std::vector<std::string> *warnings_vector)
> +			    warnings_vec *warnings_vector)
>  {
>    unsigned long file_crc;
>    int file_crc_p;
> @@ -1342,8 +1352,9 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
>  			       " does not match \"%s\" (CRC mismatch).\n"),
>  			     name.c_str (), objfile_name (parent_objfile));
>  	  if (separate_debug_file_debug)
> -	    gdb_printf (gdb_stdlog, "%s", msg.c_str ());
> -	  warnings_vector->emplace_back (std::move (msg));
> +	    gdb_printf (gdb_stdlog, "%ps", styled_string (file_name_style. style (),
> +							  msg.c_str ()));
> +	  add_warning(warnings_vector, msg);
>  	}
>  
>        return 0;
> @@ -1386,7 +1397,7 @@ find_separate_debug_file (const char *dir,
>  			  const char *canon_dir,
>  			  const char *debuglink,
>  			  unsigned long crc32, struct objfile *objfile,
> -			  std::vector<std::string> *warnings_vector)
> +			  warnings_vec *warnings_vector)
>  {
>    if (separate_debug_file_debug)
>      gdb_printf (gdb_stdlog,
> @@ -1534,7 +1545,7 @@ terminate_after_last_dir_separator (char *path)
>  
>  std::string
>  find_separate_debug_file_by_debuglink
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector)
> +  (struct objfile *objfile, warnings_vec *warnings_vector)
>  {
>    unsigned long crc32;
>  
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index b433e2be31a..0cb12176152 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -241,6 +241,9 @@ extern struct objfile *symbol_file_add_from_bfd (const gdb_bfd_ref_ptr &,
>  extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
>  				      symfile_add_flags, struct objfile *);
>  
> +using warning_cb = std::function<void (void)>;
> +using warnings_vec = std::vector<warning_cb>;

You should avoid creating duplicate type definitions like this, as this
invites problems where one is updated and not the other.

Pick one location and define the types there, and just make sure the
header file is included where needed.  Of the two locations you've used,
I'd go with symfile.h, as that seems the more general of the two.

Additionally ... comments.  Maybe a single comment above both would be
fine?

Thanks,
Andrew


> +
>  /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
>     Returns pathname, or an empty string.
>  
> @@ -248,7 +251,7 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
>     WARNINGS_VECTOR, one std::string per warning.  */
>  
>  extern std::string find_separate_debug_file_by_debuglink
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> +  (struct objfile *objfile, warnings_vec *warnings_vector);
>  
>  /* Build (allocate and populate) a section_addr_info struct from an
>     existing section table.  */
> -- 
> 2.39.1