Re: [PATCH v2] 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.
> ---
> This version: - adds comments to the new add_warning function
>               - style filenames only, not the whole warning string
> 	      - avoids creating duplicate type definitions
>
>  gdb/build-id.c | 21 ++++++++++++++++++---
>  gdb/build-id.h |  3 ++-
>  gdb/coffread.c |  6 +++---
>  gdb/elfread.c  |  6 +++---
>  gdb/symfile.c  | 30 +++++++++++++++++++++++++-----
>  gdb/symfile.h  |  5 ++++-
>  6 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 00250c20ae9..6e48ec2e4ae 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,23 @@ 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, "");
>  }
>  
> +/* Use callbacks to produce the warnings.  */

I don't think this comment is very helpful.  Something like:

  /* Add a callback to VEC that will produce a warning that separate
     debug info file FILENAME actually contains no debug info.  */

> +
> +static void
> +add_warning (warnings_vec *vec, std::string filename)

Given you are creating a copy of filename within the callback I think
you can pass filename as 'const std::string &filename'.

> +{
> +  vec->emplace_back ([filename] () {
> +		     warning (_("\"%ps\": separate debug info file has no debug info"),
> +			      styled_string (file_name_style.style (),
> +					     filename.c_str()));

You dropped the space before '()' here.

> +		     });
> +}
> +
>  /* 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 +245,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));
> +	    warning (_("\"%ps\": separate debug info file has no debug info"),
> +		     styled_string (file_name_style.style (),
> +				    bfd_get_filename (abfd.get ())));

This doesn't seem right.  If `separate_debug_file_debug` is true then we
should be producing debug output, not additional warnings.  The
gdb_printf call as it was is fine; you just need to get rid of the `msg`
temporary variable.

> +	  add_warning(vec, bfd_get_filename (abfd.get ()));

Honestly I'd get rid of the add_warning call completely, and just inline
it here, with only one caller I don't think it adds much value, but
that's just my opinion, not a requirement.

>  	}
>        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..addaec82526 100644
> --- a/gdb/build-id.h
> +++ b/gdb/build-id.h
> @@ -21,6 +21,7 @@
>  #define BUILD_ID_H
>  
>  #include "gdb_bfd.h"
> +#include "gdb/symfile.h"
>  #include "gdbsupport/rsp-low.h"
>  
>  /* Locate NT_GNU_BUILD_ID from ABFD and return its content.  */
> @@ -58,7 +59,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);

The comment for this function is out of date, it still talks about
WARNINGS_VECTOR, not VEC, and still thinks the vector holds strings.

>  
>  /* 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..2571623dbc8 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1241,10 +1241,25 @@ symbol_file_clear (int from_tty)
>  
>  bool separate_debug_file_debug = false;
>  
> +/* Use callbacks to produce the warnings.  */
> +
> +static void
> +add_warning (warnings_vec *vec, std::string filename, std:: string obj_filename)

Again, the comment needs updating, and the strings can be passed as
`const std::string &' I think.

> +{
> +  vec->emplace_back ([filename, obj_filename] () {
> +		     warning (_("the debug information found in \"%ps\""
> +				" does not match \"%ps\" (CRC mismatch).\n"),

Should be no `\n` on the end of the warning.

> +			      styled_string (file_name_style.style (),
> +					     filename.c_str()),
> +			      styled_string (file_name_style.style (),
> +					     obj_filename.c_str()));

You dropped the space before '()' in a couple of places here.

> +		     });
> +}
> +
>  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 +1357,13 @@ 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));
> +	    warning (_("the debug information found in \"%ps\""
> +		       " does not match \"%ps\" (CRC mismatch).\n"),
> +		     styled_string (file_name_style.style (),
> +				    name.c_str()),
> +		     styled_string (file_name_style.style (),
> +				    objfile_name (parent_objfile)));
> +	  add_warning(warnings_vector, name.c_str (), objfile_name (parent_objfile));

Again here, you should use gdb_printf to print the debug output, which
should NOT include styling.  And I (personally) would not bother with
the add_warning function.

If you do keep the 'add_warning' then remember - use a space between the
function name and the argument list.

>  	}
>  
>        return 0;
> @@ -1386,7 +1406,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)

The comment above this function needs updating.

>  {
>    if (separate_debug_file_debug).
>      gdb_printf (gdb_stdlog,
> @@ -1534,7 +1554,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 add a comment above these types describing what they are for.

> +
>  /* 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);

The comment above this declaration needs updating, it still talks about
the warnings as being strings.

Thanks,
Andrew
>  
>  /* Build (allocate and populate) a section_addr_info struct from an
>     existing section table.  */
> -- 
> 2.39.1