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

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

[Alexandra Hájková]

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)
+{
+  vec->emplace_back ([msg] () {
+		     gdb_printf (gdb_stdlog, "%ps",
+				 styled_string (file_name_style. style (),
+						msg.c_str ()));
+		     });
+}
+
 /* 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 ()));
+	  add_warning(vec, msg);
 	}
       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>;
+
 /* 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

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

[Andrew Burgess]

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

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

[Alexandra Hájková]

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.  */
+
+static void
+add_warning (warnings_vec *vec, 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()));
+		     });
+}
+
 /* 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 ())));
+	  add_warning(vec, bfd_get_filename (abfd.get ()));
 	}
       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);
 
 /* 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)
+{
+  vec->emplace_back ([filename, obj_filename] () {
+		     warning (_("the debug information found in \"%ps\""
+				" does not match \"%ps\" (CRC mismatch).\n"),
+			      styled_string (file_name_style.style (),
+					     filename.c_str()),
+			      styled_string (file_name_style.style (),
+					     obj_filename.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 +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));
 	}
 
       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)
 {
   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>;
+
 /* 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

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

[Andrew Burgess]

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

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

[Tom Tromey]

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

Alexandra> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
Alexandra> filenames used in the warnings are styled properly now.
 
Alexandra> +/* Use callbacks to produce the warnings.  */
Alexandra> +
Alexandra> +static void
Alexandra> +add_warning (warnings_vec *vec, std::string filename)
Alexandra> +{
Alexandra> +  vec->emplace_back ([filename] () {
Alexandra> +		     warning (_("\"%ps\": separate debug info file has no debug info"),
Alexandra> +			      styled_string (file_name_style.style (),
Alexandra> +					     filename.c_str()));
Alexandra> +		     });

This works but it seems like it would be a lot simpler to just use a
ui_file rather than introduce a vector of callback functions that emit
warnings.

Tom