[PATCHv3] gdb: style filenames in separate debug file warnings

[PATCHv3] gdb: style filenames in separate debug file warnings

[Andrew Burgess]

After the commit:

  commit 6647f05df023b63bbe056e9167e9e234172fa2ca
  Date:   Tue Jan 24 18:13:38 2023 +0100

      gdb: defer warnings when loading separate debug files

It was pointed out[1] that the warnings being deferred and then later
emitted lacked styling.  The warnings lacked styling before the above
commit, but it was suggested that the filenames in these warnings
should be styled, and this commit does this.

There were a couple of previous attempts[2][3][4] to solve this
problem, but these all tried to extend the mechanism introduced in the
above commit, the deferred warnings were placed directly into a
std::vector, but now we tried to, when appropriate, style these
warnings.  The review feedback that this approach looked too complex.

So instead, this revision adds a new helper class 'deferred_warnings'
which can be used to collect a set of deferred warnings, and then emit
these deferred warnings later, if needed.  This helper class hides the
complexity, so at the point the deferred warning is created no extra
logic is required.

The deferred_warnings class will style the deferred warnings only if
gdb_stderr supports styling.  GDB's warnings are sent to gdb_stderr,
so this should ensure we only style when expected.

There was also review feedback[5] that all of the warnings should be
bundled into a single string_file, this has not been done.  I feel
pretty strongly that separate warnings should be emitted using
separate "warning" calls.  If we do end up with multiple warnings in
this case they aren't really related, one will be about looking up
debug via .gnu_debuglink, while the other will be about build-id based
lookup.  So I'd really rather keep the warnings separate.

[1] https://inbox.sourceware.org/gdb-patches/87edr9pcku.fsf@tromey.com/
[2] https://inbox.sourceware.org/gdb-patches/20230216195604.2685177-1-ahajkova@redhat.com/
[3] https://inbox.sourceware.org/gdb-patches/20230217123547.2737612-1-ahajkova@redhat.com/
[4] https://inbox.sourceware.org/gdb-patches/20230320145638.1202335-1-ahajkova@redhat.com/
[5] https://inbox.sourceware.org/gdb-patches/87o7nh1g8h.fsf@tromey.com/

Co-Authored-By: Alexandra Hájková <ahajkova@redhat.com>
---
 gdb/build-id.c                                | 14 +++--
 gdb/build-id.h                                | 10 ++--
 gdb/coffread.c                                | 11 ++--
 gdb/elfread.c                                 | 16 +++--
 gdb/symfile.c                                 | 37 ++++++------
 gdb/symfile.h                                 |  9 ++-
 .../build-id-no-debug-warning.exp             | 24 ++++++++
 gdb/testsuite/gdb.debuginfod/crc_mismatch.exp | 33 ++++++++++-
 gdb/ui-file.h                                 |  4 ++
 gdb/utils.h                                   | 58 +++++++++++++++++++
 10 files changed, 166 insertions(+), 50 deletions(-)

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 65b493f0832..f68384f0197 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -26,6 +26,7 @@
 #include "objfiles.h"
 #include "filenames.h"
 #include "gdbcore.h"
+#include "cli/cli-style.h"
 
 /* See build-id.h.  */
 
@@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
 
 std::string
 find_separate_debug_file_by_buildid (struct objfile *objfile,
-				     std::vector<std::string> *warnings_vector)
+				     deferred_warnings *warnings)
 {
   const struct bfd_build_id *build_id;
 
@@ -228,12 +229,13 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
 	  && filename_cmp (bfd_get_filename (abfd.get ()),
 			   objfile_name (objfile)) == 0)
 	{
-	  std::string msg
-	    = 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, "\"%s\": separate debug info file has no "
+			"debug info", bfd_get_filename (abfd.get ()));
+	  warnings->warn (_("\"%ps\": separate debug info file has no "
+			    "debug info"),
+			  styled_string (file_name_style.style (),
+					 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..a5c8b85a442 100644
--- a/gdb/build-id.h
+++ b/gdb/build-id.h
@@ -52,13 +52,13 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
    separate debug file, otherwise, return an empty string.
 
    Any warnings that are generated by the lookup process should be added to
-   WARNINGS_VECTOR, one std::string per warning.  If some other mechanism can
-   be used to lookup the debug information then the warning will not be shown,
-   however, if GDB fails to find suitable debug information using any
-   approach, then any warnings will be printed.  */
+   WARNINGS.  If some other mechanism can be used to lookup the debug
+   information then the warning will not be shown, however, if GDB fails to
+   find suitable debug information using any 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, deferred_warnings *warnings);
 
 /* Return an hex-string representation of BUILD_ID.  */
 
diff --git a/gdb/coffread.c b/gdb/coffread.c
index ff4d4ae5313..135ea3469c8 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -729,13 +729,13 @@ 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;
+      deferred_warnings warnings;
       std::string debugfile
-	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
+	= find_separate_debug_file_by_buildid (objfile, &warnings);
 
       if (debugfile.empty ())
 	debugfile
-	  = find_separate_debug_file_by_debuglink (objfile, &warnings_vector);
+	  = find_separate_debug_file_by_debuglink (objfile, &warnings);
 
       if (!debugfile.empty ())
 	{
@@ -746,9 +746,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 ());
+      if (debugfile.empty () && ! warnings.empty ())
+	warnings.emit ();
     }
 }
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 799e3b914f8..7215fc4f35b 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1224,14 +1224,13 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
 	   && objfile->separate_debug_objfile == NULL
 	   && objfile->separate_debug_objfile_backlink == NULL)
     {
-      std::vector<std::string> warnings_vector;
+      deferred_warnings warnings;
 
       std::string debugfile
-	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
+	= find_separate_debug_file_by_buildid (objfile, &warnings);
 
       if (debugfile.empty ())
-	debugfile = find_separate_debug_file_by_debuglink (objfile,
-							   &warnings_vector);
+	debugfile = find_separate_debug_file_by_debuglink (objfile, &warnings);
 
       if (!debugfile.empty ())
 	{
@@ -1274,11 +1273,10 @@ 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 ());
+      /* If all the methods to collect the debuginfo failed, print the
+	 warnings, if there are any. */
+      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
+	warnings.emit ();
     }
 
   return has_dwarf2;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 3a0aee163ff..0ccaf15e4f6 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1243,7 +1243,7 @@ bool separate_debug_file_debug = false;
 static int
 separate_debug_file_exists (const std::string &name, unsigned long crc,
 			    struct objfile *parent_objfile,
-			    std::vector<std::string> *warnings_vector)
+			    deferred_warnings *warnings)
 {
   unsigned long file_crc;
   int file_crc_p;
@@ -1336,13 +1336,16 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
 
       if (verified_as_different || parent_crc != file_crc)
 	{
-	  std::string msg
-	    = string_printf (_("the debug information found in \"%s\""
-			       " 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, "the debug information found in \"%s\""
+			" does not match \"%s\" (CRC mismatch).\n",
+			name.c_str (), objfile_name (parent_objfile));
+	  warnings->warn (_("the debug information found in \"%ps\""
+			    " does not match \"%ps\" (CRC mismatch)."),
+			  styled_string (file_name_style.style (),
+					 name.c_str ()),
+			  styled_string (file_name_style.style (),
+					 objfile_name (parent_objfile)));
 	}
 
       return 0;
@@ -1385,7 +1388,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)
+			  deferred_warnings *warnings)
 {
   if (separate_debug_file_debug)
     gdb_printf (gdb_stdlog,
@@ -1396,7 +1399,7 @@ find_separate_debug_file (const char *dir,
   std::string debugfile = dir;
   debugfile += debuglink;
 
-  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
+  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
@@ -1405,7 +1408,7 @@ find_separate_debug_file (const char *dir,
   debugfile += "/";
   debugfile += debuglink;
 
-  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
+  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the global debugfile directories.
@@ -1450,8 +1453,7 @@ find_separate_debug_file (const char *dir,
       debugfile += dir_notarget;
       debugfile += debuglink;
 
-      if (separate_debug_file_exists (debugfile, crc32, objfile,
-				      warnings_vector))
+      if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	return debugfile;
 
       const char *base_path = NULL;
@@ -1473,8 +1475,7 @@ find_separate_debug_file (const char *dir,
 	  debugfile += "/";
 	  debugfile += debuglink;
 
-	  if (separate_debug_file_exists (debugfile, crc32, objfile,
-					  warnings_vector))
+	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	    return debugfile;
 
 	  /* If the file is in the sysroot, try using its base path in
@@ -1501,7 +1502,7 @@ find_separate_debug_file (const char *dir,
 	      debugfile += debuglink;
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
-					      warnings_vector))
+					      warnings))
 		return debugfile;
 	    }
 	}
@@ -1533,7 +1534,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, deferred_warnings *warnings)
 {
   uint32_t crc32;
 
@@ -1554,7 +1555,7 @@ find_separate_debug_file_by_debuglink
   std::string debugfile
     = find_separate_debug_file (dir.c_str (), canon_dir.get (),
 				debuglink.get (), crc32, objfile,
-				warnings_vector);
+				warnings);
 
   if (debugfile.empty ())
     {
@@ -1579,7 +1580,7 @@ find_separate_debug_file_by_debuglink
 							debuglink.get (),
 							crc32,
 							objfile,
-							warnings_vector);
+							warnings);
 		}
 	    }
 	}
diff --git a/gdb/symfile.h b/gdb/symfile.h
index 7c800ea6cf9..296fee945c1 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -244,11 +244,14 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
 /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
    Returns pathname, or an empty string.
 
-   Any warnings generated as part of this lookup are added to
-   WARNINGS_VECTOR, one std::string per warning.  */
+   Any warnings generated as part of this lookup are added to WARNINGS.  If
+   some other mechanism can be used to lookup the debug information then
+   the warning will not be shown, however, if GDB fails to find suitable
+   debug information using any approach, then any warnings will be
+   printed.  */
 
 extern std::string find_separate_debug_file_by_debuglink
-  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+  (struct objfile *objfile, deferred_warnings *warnings);
 
 /* Build (allocate and populate) a section_addr_info struct from an
    existing section table.  */
diff --git a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
index b49770f4a5e..148363a7c05 100644
--- a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
+++ b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
@@ -95,6 +95,30 @@ gdb_test "file ${build_id_debug_file}" \
 	 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
     "load test file, expect a warning"
 
+# Do the same thing again, but this time check that the styling is
+# correct.
+with_test_prefix "check styling" {
+    save_vars { env(TERM) } {
+	# We need an ANSI-capable terminal to get styled output.
+	setenv TERM ansi
+
+	clean_restart
+
+	gdb_test_no_output "set debug-file-directory ${debug_file_directory}" \
+	    "set debug-file-directory"
+
+	# Now load the file into GDB, and look for the warning.
+	set debug_file_re [style [string_to_regexp $build_id_debug_file] file]
+
+	gdb_test "file ${build_id_debug_file}" \
+	    [multi_line \
+		 "Reading symbols from $debug_file_re\\.\\.\\." \
+		 "warning: \"$debug_file_re\": separate debug info file has no debug info" \
+		 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
+	    "load test file, expect a warning"
+    }
+}
+
 # Now we should close GDB.
 gdb_exit
 
diff --git a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
index 588fb7e7277..074028eadfd 100644
--- a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
+++ b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
@@ -58,12 +58,39 @@ if {[build_executable crc_mismatch.exp crc_mismatch-2 crc_mismatch-2.c debug] !=
     # to trigger the "CRC mismatch" warning.
     remote_exec build "mv ${debugfile} [standard_output_file crc_mismatch-2.debug]"
 
-    gdb_exit
-    gdb_start
+    clean_restart
 
     set escapedobjdirsubdir [string_to_regexp [standard_output_file {}]]
+    gdb_test "file [standard_output_file crc_mismatch-2]" \
+	[multi_line \
+	     "Reading symbols from ${escapedobjdirsubdir}/crc_mismatch-2\\.\\.\\." \
+	     "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\." \
+	     "\\(No debugging symbols found in .*\\)"] \
+	"CRC mismatch is reported"
+
+    # Do the same thing again, but this time check that the styling is
+    # correct.
+    with_test_prefix "check styling" {
+	save_vars { env(TERM) } {
+	    # We need an ANSI-capable terminal to get styled output.
+	    setenv TERM ansi
+
+	    clean_restart
+
+	    set debug_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2\\.debug" file]
+	    set exe_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2" file]
+
+	    gdb_test "file [standard_output_file crc_mismatch-2]" \
+		[multi_line \
+		     "Reading symbols from ${exe_file_re}\\.\\.\\." \
+		     "warning: the debug information found in \"${debug_file_re}\" does not match \"${exe_file_re}\" \\(CRC mismatch\\)\\." \
+		     "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
+		"CRC mismatch is reported"
+	}
+    }
 
-    gdb_test "file [standard_output_file crc_mismatch-2]" "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\..*\\(No debugging symbols found in .*\\).*" "CRC mismatch is reported"
+    # Restart GDB with styling disabled.
+    clean_restart
 }
 
 # Create CACHE and DB directories ready for debuginfod to use.
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index de24620e247..31f87ffd51d 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -30,6 +30,8 @@ class ui_file
   ui_file ();
   virtual ~ui_file () = 0;
 
+  ui_file (ui_file &&other) = default;
+
   /* Public non-virtual API.  */
 
   void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3);
@@ -177,6 +179,8 @@ class string_file : public ui_file
   explicit string_file (bool term_out) : m_term_out (term_out) {};
   ~string_file () override;
 
+  string_file (string_file &&other) = default;
+
   /* Override ui_file methods.  */
 
   void write (const char *buf, long length_buf) override;
diff --git a/gdb/utils.h b/gdb/utils.h
index 3faac20ec0f..602216a4c03 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -364,4 +364,62 @@ assign_return_if_changed (T &lval, const T &val)
   return true;
 }
 
+/* In some cases GDB needs to try several different solutions to a problem,
+   if any of the solutions work then as far as the user is concerned the
+   problem is solved, and GDB should continue without warnings.  However,
+   if non of the solutions work then GDB should emit any warnings that
+   occurred while trying each possible solution.
+
+   One example of this is locating separate debug info.  There are several
+   different approaches for this; following the .gnu_debuglink, a build-id
+   based lookup, or using debuginfod.  If any works, and debug info is
+   located, then the user doesn't want to see warnings from the earlier
+   approaches that were tried and failed.
+
+   However, GDB should emit all the warnings using separate calls to
+   warning -- this ensures that each warning is formatted on its own line,
+   and that any styling is emitted correctly.
+
+   This class helps with deferring warnings.  Warnings can be added to an
+   instance of this class with the 'warn' function, and all warnings can be
+   emitted with a single call to 'emit'.  */
+
+struct deferred_warnings
+{
+  /* Add a warning to the list of deferred warnings.  */
+  void warn (const char *format, ...)
+  {
+    /* Generate the warning text into a string_file.  We allow the text to
+       be styled only if gdb_stderr allows styling -- warnings are sent to
+       gdb_stderr.  */
+    string_file msg (gdb_stderr->can_emit_style_escape ());
+
+    va_list args;
+    va_start (args, format);
+    msg.vprintf (format, args);
+    va_end (args);
+
+    /* Move the text into the list of deferred warnings.  */
+    m_warnings.emplace_back (std::move (msg));
+  }
+
+  /* Emit all warnings.  */
+  void emit () const
+  {
+    for (const auto &w : m_warnings)
+      warning ("%s", w.c_str ());
+  }
+
+  /* Return true if there are no deferred warnings.  */
+  bool empty () const
+  {
+    return m_warnings.empty ();
+  }
+
+private:
+
+  /* The list of all deferred warnings.  */
+  std::vector<string_file> m_warnings;
+};
+
 #endif /* UTILS_H */

base-commit: bb2bd584f31a25ba1cfe5bdac4d07d8cffe87c3d
-- 
2.25.4

Re: [PATCHv3] gdb: style filenames in separate debug file warnings

[Simon Marchi]

> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>  
>  std::string
>  find_separate_debug_file_by_buildid (struct objfile *objfile,
> -				     std::vector<std::string> *warnings_vector)
> +				     deferred_warnings *warnings)
Reference instead of pointer?

> @@ -1274,11 +1273,10 @@ 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 ());
> +      /* If all the methods to collect the debuginfo failed, print the
> +	 warnings, if there are any. */
> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
> +	warnings.emit ();

The warnings.empty() seems unnecessary to me, since emit will be a no-op
if there are no warnings.

> @@ -1385,7 +1388,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)
> +			  deferred_warnings *warnings)
>  {

The comment above find_separate_debug_file needs to be updated.

> diff --git a/gdb/utils.h b/gdb/utils.h
> index 3faac20ec0f..602216a4c03 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.hOn 7/4/23 11:17, Andrew Burgess via Gdb-patches wrote:
> After the commit:
> 
>   commit 6647f05df023b63bbe056e9167e9e234172fa2ca
>   Date:   Tue Jan 24 18:13:38 2023 +0100
> 
>       gdb: defer warnings when loading separate debug files
> 
> It was pointed out[1] that the warnings being deferred and then later
> emitted lacked styling.  The warnings lacked styling before the above
> commit, but it was suggested that the filenames in these warnings
> should be styled, and this commit does this.
> 
> There were a couple of previous attempts[2][3][4] to solve this
> problem, but these all tried to extend the mechanism introduced in the
> above commit, the deferred warnings were placed directly into a
> std::vector, but now we tried to, when appropriate, style these
> warnings.  The review feedback that this approach looked too complex.
> 
> So instead, this revision adds a new helper class 'deferred_warnings'
> which can be used to collect a set of deferred warnings, and then emit
> these deferred warnings later, if needed.  This helper class hides the
> complexity, so at the point the deferred warning is created no extra
> logic is required.
> 
> The deferred_warnings class will style the deferred warnings only if
> gdb_stderr supports styling.  GDB's warnings are sent to gdb_stderr,
> so this should ensure we only style when expected.
> 
> There was also review feedback[5] that all of the warnings should be
> bundled into a single string_file, this has not been done.  I feel
> pretty strongly that separate warnings should be emitted using
> separate "warning" calls.  If we do end up with multiple warnings in
> this case they aren't really related, one will be about looking up
> debug via .gnu_debuglink, while the other will be about build-id based
> lookup.  So I'd really rather keep the warnings separate.
> 
> [1] https://inbox.sourceware.org/gdb-patches/87edr9pcku.fsf@tromey.com/
> [2] https://inbox.sourceware.org/gdb-patches/20230216195604.2685177-1-ahajkova@redhat.com/
> [3] https://inbox.sourceware.org/gdb-patches/20230217123547.2737612-1-ahajkova@redhat.com/
> [4] https://inbox.sourceware.org/gdb-patches/20230320145638.1202335-1-ahajkova@redhat.com/
> [5] https://inbox.sourceware.org/gdb-patches/87o7nh1g8h.fsf@tromey.com/
> 
> Co-Authored-By: Alexandra Hájková <ahajkova@redhat.com>
> ---
>  gdb/build-id.c                                | 14 +++--
>  gdb/build-id.h                                | 10 ++--
>  gdb/coffread.c                                | 11 ++--
>  gdb/elfread.c                                 | 16 +++--
>  gdb/symfile.c                                 | 37 ++++++------
>  gdb/symfile.h                                 |  9 ++-
>  .../build-id-no-debug-warning.exp             | 24 ++++++++
>  gdb/testsuite/gdb.debuginfod/crc_mismatch.exp | 33 ++++++++++-
>  gdb/ui-file.h                                 |  4 ++
>  gdb/utils.h                                   | 58 +++++++++++++++++++
>  10 files changed, 166 insertions(+), 50 deletions(-)
> 
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 65b493f0832..f68384f0197 100644
> --- a/gdb/build-id.c
> +++ b/gdb/build-id.c
> @@ -26,6 +26,7 @@
>  #include "objfiles.h"
>  #include "filenames.h"
>  #include "gdbcore.h"
> +#include "cli/cli-style.h"
>  
>  /* See build-id.h.  */
>  
> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>  
>  std::string
>  find_separate_debug_file_by_buildid (struct objfile *objfile,
> -				     std::vector<std::string> *warnings_vector)
> +				     deferred_warnings *warnings)
>  {
>    const struct bfd_build_id *build_id;
>  
> @@ -228,12 +229,13 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
>  	  && filename_cmp (bfd_get_filename (abfd.get ()),
>  			   objfile_name (objfile)) == 0)
>  	{
> -	  std::string msg
> -	    = 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, "\"%s\": separate debug info file has no "
> +			"debug info", bfd_get_filename (abfd.get ()));
> +	  warnings->warn (_("\"%ps\": separate debug info file has no "
> +			    "debug info"),
> +			  styled_string (file_name_style.style (),
> +					 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..a5c8b85a442 100644
> --- a/gdb/build-id.h
> +++ b/gdb/build-id.h
> @@ -52,13 +52,13 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
>     separate debug file, otherwise, return an empty string.
>  
>     Any warnings that are generated by the lookup process should be added to
> -   WARNINGS_VECTOR, one std::string per warning.  If some other mechanism can
> -   be used to lookup the debug information then the warning will not be shown,
> -   however, if GDB fails to find suitable debug information using any
> -   approach, then any warnings will be printed.  */
> +   WARNINGS.  If some other mechanism can be used to lookup the debug
> +   information then the warning will not be shown, however, if GDB fails to
> +   find suitable debug information using any 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, deferred_warnings *warnings);
>  
>  /* Return an hex-string representation of BUILD_ID.  */
>  
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index ff4d4ae5313..135ea3469c8 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -729,13 +729,13 @@ 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;
> +      deferred_warnings warnings;
>        std::string debugfile
> -	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
> +	= find_separate_debug_file_by_buildid (objfile, &warnings);
>  
>        if (debugfile.empty ())
>  	debugfile
> -	  = find_separate_debug_file_by_debuglink (objfile, &warnings_vector);
> +	  = find_separate_debug_file_by_debuglink (objfile, &warnings);
>  
>        if (!debugfile.empty ())
>  	{
> @@ -746,9 +746,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 ());
> +      if (debugfile.empty () && ! warnings.empty ())
> +	warnings.emit ();
>      }
>  }
>  
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 799e3b914f8..7215fc4f35b 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1224,14 +1224,13 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>  	   && objfile->separate_debug_objfile == NULL
>  	   && objfile->separate_debug_objfile_backlink == NULL)
>      {
> -      std::vector<std::string> warnings_vector;
> +      deferred_warnings warnings;
>  
>        std::string debugfile
> -	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
> +	= find_separate_debug_file_by_buildid (objfile, &warnings);
>  
>        if (debugfile.empty ())
> -	debugfile = find_separate_debug_file_by_debuglink (objfile,
> -							   &warnings_vector);
> +	debugfile = find_separate_debug_file_by_debuglink (objfile, &warnings);
>  
>        if (!debugfile.empty ())
>  	{
> @@ -1274,11 +1273,10 @@ 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 ());
> +      /* If all the methods to collect the debuginfo failed, print the
> +	 warnings, if there are any. */
> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
> +	warnings.emit ();
>      }
>  
>    return has_dwarf2;
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 3a0aee163ff..0ccaf15e4f6 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1243,7 +1243,7 @@ bool separate_debug_file_debug = false;
>  static int
>  separate_debug_file_exists (const std::string &name, unsigned long crc,
>  			    struct objfile *parent_objfile,
> -			    std::vector<std::string> *warnings_vector)
> +			    deferred_warnings *warnings)
>  {
>    unsigned long file_crc;
>    int file_crc_p;
> @@ -1336,13 +1336,16 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
>  
>        if (verified_as_different || parent_crc != file_crc)
>  	{
> -	  std::string msg
> -	    = string_printf (_("the debug information found in \"%s\""
> -			       " 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, "the debug information found in \"%s\""
> +			" does not match \"%s\" (CRC mismatch).\n",
> +			name.c_str (), objfile_name (parent_objfile));
> +	  warnings->warn (_("the debug information found in \"%ps\""
> +			    " does not match \"%ps\" (CRC mismatch)."),
> +			  styled_string (file_name_style.style (),
> +					 name.c_str ()),
> +			  styled_string (file_name_style.style (),
> +					 objfile_name (parent_objfile)));
>  	}
>  
>        return 0;
> @@ -1385,7 +1388,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)
> +			  deferred_warnings *warnings)
>  {
>    if (separate_debug_file_debug)
>      gdb_printf (gdb_stdlog,
> @@ -1396,7 +1399,7 @@ find_separate_debug_file (const char *dir,
>    std::string debugfile = dir;
>    debugfile += debuglink;
>  
> -  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
> +  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>      return debugfile;
>  
>    /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
> @@ -1405,7 +1408,7 @@ find_separate_debug_file (const char *dir,
>    debugfile += "/";
>    debugfile += debuglink;
>  
> -  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
> +  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>      return debugfile;
>  
>    /* Then try in the global debugfile directories.
> @@ -1450,8 +1453,7 @@ find_separate_debug_file (const char *dir,
>        debugfile += dir_notarget;
>        debugfile += debuglink;
>  
> -      if (separate_debug_file_exists (debugfile, crc32, objfile,
> -				      warnings_vector))
> +      if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>  	return debugfile;
>  
>        const char *base_path = NULL;
> @@ -1473,8 +1475,7 @@ find_separate_debug_file (const char *dir,
>  	  debugfile += "/";
>  	  debugfile += debuglink;
>  
> -	  if (separate_debug_file_exists (debugfile, crc32, objfile,
> -					  warnings_vector))
> +	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>  	    return debugfile;
>  
>  	  /* If the file is in the sysroot, try using its base path in
> @@ -1501,7 +1502,7 @@ find_separate_debug_file (const char *dir,
>  	      debugfile += debuglink;
>  
>  	      if (separate_debug_file_exists (debugfile, crc32, objfile,
> -					      warnings_vector))
> +					      warnings))
>  		return debugfile;
>  	    }
>  	}
> @@ -1533,7 +1534,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, deferred_warnings *warnings)
>  {
>    uint32_t crc32;
>  
> @@ -1554,7 +1555,7 @@ find_separate_debug_file_by_debuglink
>    std::string debugfile
>      = find_separate_debug_file (dir.c_str (), canon_dir.get (),
>  				debuglink.get (), crc32, objfile,
> -				warnings_vector);
> +				warnings);
>  
>    if (debugfile.empty ())
>      {
> @@ -1579,7 +1580,7 @@ find_separate_debug_file_by_debuglink
>  							debuglink.get (),
>  							crc32,
>  							objfile,
> -							warnings_vector);
> +							warnings);
>  		}
>  	    }
>  	}
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index 7c800ea6cf9..296fee945c1 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -244,11 +244,14 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
>  /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
>     Returns pathname, or an empty string.
>  
> -   Any warnings generated as part of this lookup are added to
> -   WARNINGS_VECTOR, one std::string per warning.  */
> +   Any warnings generated as part of this lookup are added to WARNINGS.  If
> +   some other mechanism can be used to lookup the debug information then
> +   the warning will not be shown, however, if GDB fails to find suitable
> +   debug information using any approach, then any warnings will be
> +   printed.  */
>  
>  extern std::string find_separate_debug_file_by_debuglink
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> +  (struct objfile *objfile, deferred_warnings *warnings);
>  
>  /* Build (allocate and populate) a section_addr_info struct from an
>     existing section table.  */
> diff --git a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
> index b49770f4a5e..148363a7c05 100644
> --- a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
> +++ b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
> @@ -95,6 +95,30 @@ gdb_test "file ${build_id_debug_file}" \
>  	 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
>      "load test file, expect a warning"
>  
> +# Do the same thing again, but this time check that the styling is
> +# correct.
> +with_test_prefix "check styling" {
> +    save_vars { env(TERM) } {
> +	# We need an ANSI-capable terminal to get styled output.
> +	setenv TERM ansi
> +
> +	clean_restart
> +
> +	gdb_test_no_output "set debug-file-directory ${debug_file_directory}" \
> +	    "set debug-file-directory"
> +
> +	# Now load the file into GDB, and look for the warning.
> +	set debug_file_re [style [string_to_regexp $build_id_debug_file] file]
> +
> +	gdb_test "file ${build_id_debug_file}" \
> +	    [multi_line \
> +		 "Reading symbols from $debug_file_re\\.\\.\\." \
> +		 "warning: \"$debug_file_re\": separate debug info file has no debug info" \
> +		 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
> +	    "load test file, expect a warning"
> +    }
> +}
> +
>  # Now we should close GDB.
>  gdb_exit
>  
> diff --git a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
> index 588fb7e7277..074028eadfd 100644
> --- a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
> +++ b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
> @@ -58,12 +58,39 @@ if {[build_executable crc_mismatch.exp crc_mismatch-2 crc_mismatch-2.c debug] !=
>      # to trigger the "CRC mismatch" warning.
>      remote_exec build "mv ${debugfile} [standard_output_file crc_mismatch-2.debug]"
>  
> -    gdb_exit
> -    gdb_start
> +    clean_restart
>  
>      set escapedobjdirsubdir [string_to_regexp [standard_output_file {}]]
> +    gdb_test "file [standard_output_file crc_mismatch-2]" \
> +	[multi_line \
> +	     "Reading symbols from ${escapedobjdirsubdir}/crc_mismatch-2\\.\\.\\." \
> +	     "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\." \
> +	     "\\(No debugging symbols found in .*\\)"] \
> +	"CRC mismatch is reported"
> +
> +    # Do the same thing again, but this time check that the styling is
> +    # correct.
> +    with_test_prefix "check styling" {
> +	save_vars { env(TERM) } {
> +	    # We need an ANSI-capable terminal to get styled output.
> +	    setenv TERM ansi
> +
> +	    clean_restart
> +
> +	    set debug_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2\\.debug" file]
> +	    set exe_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2" file]
> +
> +	    gdb_test "file [standard_output_file crc_mismatch-2]" \
> +		[multi_line \
> +		     "Reading symbols from ${exe_file_re}\\.\\.\\." \
> +		     "warning: the debug information found in \"${debug_file_re}\" does not match \"${exe_file_re}\" \\(CRC mismatch\\)\\." \
> +		     "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
> +		"CRC mismatch is reported"
> +	}
> +    }
>  
> -    gdb_test "file [standard_output_file crc_mismatch-2]" "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\..*\\(No debugging symbols found in .*\\).*" "CRC mismatch is reported"
> +    # Restart GDB with styling disabled.
> +    clean_restart
>  }
>  
>  # Create CACHE and DB directories ready for debuginfod to use.
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index de24620e247..31f87ffd51d 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -30,6 +30,8 @@ class ui_file
>    ui_file ();
>    virtual ~ui_file () = 0;
>  
> +  ui_file (ui_file &&other) = default;
> +
>    /* Public non-virtual API.  */
>  
>    void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3);
> @@ -177,6 +179,8 @@ class string_file : public ui_file
>    explicit string_file (bool term_out) : m_term_out (term_out) {};
>    ~string_file () override;
>  
> +  string_file (string_file &&other) = default;
> +
>    /* Override ui_file methods.  */
>  
>    void write (const char *buf, long length_buf) override;
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 3faac20ec0f..602216a4c03 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -364,4 +364,62 @@ assign_return_if_changed (T &lval, const T &val)
>    return true;
>  }
>  
> +/* In some cases GDB needs to try several different solutions to a problem,
> +   if any of the solutions work then as far as the user is concerned the
> +   problem is solved, and GDB should continue without warnings.  However,
> +   if non of the solutions work then GDB should emit any warnings that
> +   occurred while trying each possible solution.
> +
> +   One example of this is locating separate debug info.  There are several
> +   different approaches for this; following the .gnu_debuglink, a build-id
> +   based lookup, or using debuginfod.  If any works, and debug info is
> +   located, then the user doesn't want to see warnings from the earlier
> +   approaches that were tried and failed.
> +
> +   However, GDB should emit all the warnings using separate calls to
> +   warning -- this ensures that each warning is formatted on its own line,
> +   and that any styling is emitted correctly.
> +
> +   This class helps with deferring warnings.  Warnings can be added to an
> +   instance of this class with the 'warn' function, and all warnings can be
> +   emitted with a single call to 'emit'.  */
> +
> +struct deferred_warnings
> +{
> +  /* Add a warning to the list of deferred warnings.  */
> +  void warn (const char *format, ...)
> +  {
> +    /* Generate the warning text into a string_file.  We allow the text to
> +       be styled only if gdb_stderr allows styling -- warnings are sent to
> +       gdb_stderr.  */
> +    string_file msg (gdb_stderr->can_emit_style_escape ());
> +
> +    va_list args;
> +    va_start (args, format);
> +    msg.vprintf (format, args);
> +    va_end (args);
> +
> +    /* Move the text into the list of deferred warnings.  */
> +    m_warnings.emplace_back (std::move (msg));
> +  }
> +
> +  /* Emit all warnings.  */
> +  void emit () const
> +  {
> +    for (const auto &w : m_warnings)
> +      warning ("%s", w.c_str ());
> +  }
> +
> +  /* Return true if there are no deferred warnings.  */
> +  bool empty () const
> +  {
> +    return m_warnings.empty ();
> +  }
> +
> +private:
> +
> +  /* The list of all deferred warnings.  */
> +  std::vector<string_file> m_warnings;
> +};
> +
>  #endif /* UTILS_H */
> 
> base-commit: bb2bd584f31a25ba1cfe5bdac4d07d8cffe87c3d
> -- 
> 2.25.4
> 

> @@ -364,4 +364,62 @@ assign_return_if_changed (T &lval, const T &val)
>    return true;
>  }
>  
> +/* In some cases GDB needs to try several different solutions to a problem,
> +   if any of the solutions work then as far as the user is concerned the
> +   problem is solved, and GDB should continue without warnings.  However,
> +   if non of the solutions work then GDB should emit any warnings that

non -> none

> +   occurred while trying each possible solution.
> +
> +   One example of this is locating separate debug info.  There are several
> +   different approaches for this; following the .gnu_debuglink, a build-id
> +   based lookup, or using debuginfod.  If any works, and debug info is
> +   located, then the user doesn't want to see warnings from the earlier
> +   approaches that were tried and failed.
> +
> +   However, GDB should emit all the warnings using separate calls to
> +   warning -- this ensures that each warning is formatted on its own line,
> +   and that any styling is emitted correctly.
> +
> +   This class helps with deferring warnings.  Warnings can be added to an
> +   instance of this class with the 'warn' function, and all warnings can be
> +   emitted with a single call to 'emit'.  */
> +
> +struct deferred_warnings
> +{
> +  /* Add a warning to the list of deferred warnings.  */
> +  void warn (const char *format, ...)

You can probably put an ATTRIBUTE_PRINTF attribute here.

Simon

Re: [PATCHv3] gdb: style filenames in separate debug file warnings

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

>> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>>  
>>  std::string
>>  find_separate_debug_file_by_buildid (struct objfile *objfile,
>> -				     std::vector<std::string> *warnings_vector)
>> +				     deferred_warnings *warnings)
> Reference instead of pointer?

I thought at one point (maybe I'm miss-remembering) GDB style was to
pass a pointer if an object was modified, and a const reference when an
object was not going to be modified.  I've stuck to that since pretty
much forever for GDB code .... is this not (or was it never) a thing?

I've not changed this for now, but I can do this if that's your
preference, just let me know.

>
>> @@ -1274,11 +1273,10 @@ 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 ());
>> +      /* If all the methods to collect the debuginfo failed, print the
>> +	 warnings, if there are any. */
>> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
>> +	warnings.emit ();
>
> The warnings.empty() seems unnecessary to me, since emit will be a no-op
> if there are no warnings.

Funny you should call this out.  I added the .empty() just prior to
posting this to the mailing list because, when I gave the patches a
final review, it in someway felt more obvious what was going on if there
was an empty check.  But I'm not really tied to it, so I'm going to
remove the empty check.

>
>> @@ -1385,7 +1388,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)
>> +			  deferred_warnings *warnings)
>>  {
>
> The comment above find_separate_debug_file needs to be updated.

Done.

>
>> diff --git a/gdb/utils.h b/gdb/utils.h
>> index 3faac20ec0f..602216a4c03 100644
>> --- a/gdb/utils.h
>> +++ b/gdb/utils.hOn 7/4/23 11:17, Andrew Burgess via Gdb-patches wrote:
>> After the commit:
>> 
>>   commit 6647f05df023b63bbe056e9167e9e234172fa2ca
>>   Date:   Tue Jan 24 18:13:38 2023 +0100
>> 
>>       gdb: defer warnings when loading separate debug files
>> 
>> It was pointed out[1] that the warnings being deferred and then later
>> emitted lacked styling.  The warnings lacked styling before the above
>> commit, but it was suggested that the filenames in these warnings
>> should be styled, and this commit does this.
>> 
>> There were a couple of previous attempts[2][3][4] to solve this
>> problem, but these all tried to extend the mechanism introduced in the
>> above commit, the deferred warnings were placed directly into a
>> std::vector, but now we tried to, when appropriate, style these
>> warnings.  The review feedback that this approach looked too complex.
>> 
>> So instead, this revision adds a new helper class 'deferred_warnings'
>> which can be used to collect a set of deferred warnings, and then emit
>> these deferred warnings later, if needed.  This helper class hides the
>> complexity, so at the point the deferred warning is created no extra
>> logic is required.
>> 
>> The deferred_warnings class will style the deferred warnings only if
>> gdb_stderr supports styling.  GDB's warnings are sent to gdb_stderr,
>> so this should ensure we only style when expected.
>> 
>> There was also review feedback[5] that all of the warnings should be
>> bundled into a single string_file, this has not been done.  I feel
>> pretty strongly that separate warnings should be emitted using
>> separate "warning" calls.  If we do end up with multiple warnings in
>> this case they aren't really related, one will be about looking up
>> debug via .gnu_debuglink, while the other will be about build-id based
>> lookup.  So I'd really rather keep the warnings separate.
>> 
>> [1] https://inbox.sourceware.org/gdb-patches/87edr9pcku.fsf@tromey.com/
>> [2] https://inbox.sourceware.org/gdb-patches/20230216195604.2685177-1-ahajkova@redhat.com/
>> [3] https://inbox.sourceware.org/gdb-patches/20230217123547.2737612-1-ahajkova@redhat.com/
>> [4] https://inbox.sourceware.org/gdb-patches/20230320145638.1202335-1-ahajkova@redhat.com/
>> [5] https://inbox.sourceware.org/gdb-patches/87o7nh1g8h.fsf@tromey.com/
>> 
>> Co-Authored-By: Alexandra Hájková <ahajkova@redhat.com>
>> ---
>>  gdb/build-id.c                                | 14 +++--
>>  gdb/build-id.h                                | 10 ++--
>>  gdb/coffread.c                                | 11 ++--
>>  gdb/elfread.c                                 | 16 +++--
>>  gdb/symfile.c                                 | 37 ++++++------
>>  gdb/symfile.h                                 |  9 ++-
>>  .../build-id-no-debug-warning.exp             | 24 ++++++++
>>  gdb/testsuite/gdb.debuginfod/crc_mismatch.exp | 33 ++++++++++-
>>  gdb/ui-file.h                                 |  4 ++
>>  gdb/utils.h                                   | 58 +++++++++++++++++++
>>  10 files changed, 166 insertions(+), 50 deletions(-)
>> 
>> diff --git a/gdb/build-id.c b/gdb/build-id.c
>> index 65b493f0832..f68384f0197 100644
>> --- a/gdb/build-id.c
>> +++ b/gdb/build-id.c
>> @@ -26,6 +26,7 @@
>>  #include "objfiles.h"
>>  #include "filenames.h"
>>  #include "gdbcore.h"
>> +#include "cli/cli-style.h"
>>  
>>  /* See build-id.h.  */
>>  
>> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>>  
>>  std::string
>>  find_separate_debug_file_by_buildid (struct objfile *objfile,
>> -				     std::vector<std::string> *warnings_vector)
>> +				     deferred_warnings *warnings)
>>  {
>>    const struct bfd_build_id *build_id;
>>  
>> @@ -228,12 +229,13 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
>>  	  && filename_cmp (bfd_get_filename (abfd.get ()),
>>  			   objfile_name (objfile)) == 0)
>>  	{
>> -	  std::string msg
>> -	    = 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, "\"%s\": separate debug info file has no "
>> +			"debug info", bfd_get_filename (abfd.get ()));
>> +	  warnings->warn (_("\"%ps\": separate debug info file has no "
>> +			    "debug info"),
>> +			  styled_string (file_name_style.style (),
>> +					 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..a5c8b85a442 100644
>> --- a/gdb/build-id.h
>> +++ b/gdb/build-id.h
>> @@ -52,13 +52,13 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
>>     separate debug file, otherwise, return an empty string.
>>  
>>     Any warnings that are generated by the lookup process should be added to
>> -   WARNINGS_VECTOR, one std::string per warning.  If some other mechanism can
>> -   be used to lookup the debug information then the warning will not be shown,
>> -   however, if GDB fails to find suitable debug information using any
>> -   approach, then any warnings will be printed.  */
>> +   WARNINGS.  If some other mechanism can be used to lookup the debug
>> +   information then the warning will not be shown, however, if GDB fails to
>> +   find suitable debug information using any 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, deferred_warnings *warnings);
>>  
>>  /* Return an hex-string representation of BUILD_ID.  */
>>  
>> diff --git a/gdb/coffread.c b/gdb/coffread.c
>> index ff4d4ae5313..135ea3469c8 100644
>> --- a/gdb/coffread.c
>> +++ b/gdb/coffread.c
>> @@ -729,13 +729,13 @@ 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;
>> +      deferred_warnings warnings;
>>        std::string debugfile
>> -	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>> +	= find_separate_debug_file_by_buildid (objfile, &warnings);
>>  
>>        if (debugfile.empty ())
>>  	debugfile
>> -	  = find_separate_debug_file_by_debuglink (objfile, &warnings_vector);
>> +	  = find_separate_debug_file_by_debuglink (objfile, &warnings);
>>  
>>        if (!debugfile.empty ())
>>  	{
>> @@ -746,9 +746,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 ());
>> +      if (debugfile.empty () && ! warnings.empty ())
>> +	warnings.emit ();
>>      }
>>  }
>>  
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index 799e3b914f8..7215fc4f35b 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -1224,14 +1224,13 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>>  	   && objfile->separate_debug_objfile == NULL
>>  	   && objfile->separate_debug_objfile_backlink == NULL)
>>      {
>> -      std::vector<std::string> warnings_vector;
>> +      deferred_warnings warnings;
>>  
>>        std::string debugfile
>> -	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>> +	= find_separate_debug_file_by_buildid (objfile, &warnings);
>>  
>>        if (debugfile.empty ())
>> -	debugfile = find_separate_debug_file_by_debuglink (objfile,
>> -							   &warnings_vector);
>> +	debugfile = find_separate_debug_file_by_debuglink (objfile, &warnings);
>>  
>>        if (!debugfile.empty ())
>>  	{
>> @@ -1274,11 +1273,10 @@ 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 ());
>> +      /* If all the methods to collect the debuginfo failed, print the
>> +	 warnings, if there are any. */
>> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
>> +	warnings.emit ();
>>      }
>>  
>>    return has_dwarf2;
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index 3a0aee163ff..0ccaf15e4f6 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -1243,7 +1243,7 @@ bool separate_debug_file_debug = false;
>>  static int
>>  separate_debug_file_exists (const std::string &name, unsigned long crc,
>>  			    struct objfile *parent_objfile,
>> -			    std::vector<std::string> *warnings_vector)
>> +			    deferred_warnings *warnings)
>>  {
>>    unsigned long file_crc;
>>    int file_crc_p;
>> @@ -1336,13 +1336,16 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
>>  
>>        if (verified_as_different || parent_crc != file_crc)
>>  	{
>> -	  std::string msg
>> -	    = string_printf (_("the debug information found in \"%s\""
>> -			       " 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, "the debug information found in \"%s\""
>> +			" does not match \"%s\" (CRC mismatch).\n",
>> +			name.c_str (), objfile_name (parent_objfile));
>> +	  warnings->warn (_("the debug information found in \"%ps\""
>> +			    " does not match \"%ps\" (CRC mismatch)."),
>> +			  styled_string (file_name_style.style (),
>> +					 name.c_str ()),
>> +			  styled_string (file_name_style.style (),
>> +					 objfile_name (parent_objfile)));
>>  	}
>>  
>>        return 0;
>> @@ -1385,7 +1388,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)
>> +			  deferred_warnings *warnings)
>>  {
>>    if (separate_debug_file_debug)
>>      gdb_printf (gdb_stdlog,
>> @@ -1396,7 +1399,7 @@ find_separate_debug_file (const char *dir,
>>    std::string debugfile = dir;
>>    debugfile += debuglink;
>>  
>> -  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
>> +  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>>      return debugfile;
>>  
>>    /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
>> @@ -1405,7 +1408,7 @@ find_separate_debug_file (const char *dir,
>>    debugfile += "/";
>>    debugfile += debuglink;
>>  
>> -  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
>> +  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>>      return debugfile;
>>  
>>    /* Then try in the global debugfile directories.
>> @@ -1450,8 +1453,7 @@ find_separate_debug_file (const char *dir,
>>        debugfile += dir_notarget;
>>        debugfile += debuglink;
>>  
>> -      if (separate_debug_file_exists (debugfile, crc32, objfile,
>> -				      warnings_vector))
>> +      if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>>  	return debugfile;
>>  
>>        const char *base_path = NULL;
>> @@ -1473,8 +1475,7 @@ find_separate_debug_file (const char *dir,
>>  	  debugfile += "/";
>>  	  debugfile += debuglink;
>>  
>> -	  if (separate_debug_file_exists (debugfile, crc32, objfile,
>> -					  warnings_vector))
>> +	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
>>  	    return debugfile;
>>  
>>  	  /* If the file is in the sysroot, try using its base path in
>> @@ -1501,7 +1502,7 @@ find_separate_debug_file (const char *dir,
>>  	      debugfile += debuglink;
>>  
>>  	      if (separate_debug_file_exists (debugfile, crc32, objfile,
>> -					      warnings_vector))
>> +					      warnings))
>>  		return debugfile;
>>  	    }
>>  	}
>> @@ -1533,7 +1534,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, deferred_warnings *warnings)
>>  {
>>    uint32_t crc32;
>>  
>> @@ -1554,7 +1555,7 @@ find_separate_debug_file_by_debuglink
>>    std::string debugfile
>>      = find_separate_debug_file (dir.c_str (), canon_dir.get (),
>>  				debuglink.get (), crc32, objfile,
>> -				warnings_vector);
>> +				warnings);
>>  
>>    if (debugfile.empty ())
>>      {
>> @@ -1579,7 +1580,7 @@ find_separate_debug_file_by_debuglink
>>  							debuglink.get (),
>>  							crc32,
>>  							objfile,
>> -							warnings_vector);
>> +							warnings);
>>  		}
>>  	    }
>>  	}
>> diff --git a/gdb/symfile.h b/gdb/symfile.h
>> index 7c800ea6cf9..296fee945c1 100644
>> --- a/gdb/symfile.h
>> +++ b/gdb/symfile.h
>> @@ -244,11 +244,14 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
>>  /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
>>     Returns pathname, or an empty string.
>>  
>> -   Any warnings generated as part of this lookup are added to
>> -   WARNINGS_VECTOR, one std::string per warning.  */
>> +   Any warnings generated as part of this lookup are added to WARNINGS.  If
>> +   some other mechanism can be used to lookup the debug information then
>> +   the warning will not be shown, however, if GDB fails to find suitable
>> +   debug information using any approach, then any warnings will be
>> +   printed.  */
>>  
>>  extern std::string find_separate_debug_file_by_debuglink
>> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
>> +  (struct objfile *objfile, deferred_warnings *warnings);
>>  
>>  /* Build (allocate and populate) a section_addr_info struct from an
>>     existing section table.  */
>> diff --git a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
>> index b49770f4a5e..148363a7c05 100644
>> --- a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
>> +++ b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
>> @@ -95,6 +95,30 @@ gdb_test "file ${build_id_debug_file}" \
>>  	 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
>>      "load test file, expect a warning"
>>  
>> +# Do the same thing again, but this time check that the styling is
>> +# correct.
>> +with_test_prefix "check styling" {
>> +    save_vars { env(TERM) } {
>> +	# We need an ANSI-capable terminal to get styled output.
>> +	setenv TERM ansi
>> +
>> +	clean_restart
>> +
>> +	gdb_test_no_output "set debug-file-directory ${debug_file_directory}" \
>> +	    "set debug-file-directory"
>> +
>> +	# Now load the file into GDB, and look for the warning.
>> +	set debug_file_re [style [string_to_regexp $build_id_debug_file] file]
>> +
>> +	gdb_test "file ${build_id_debug_file}" \
>> +	    [multi_line \
>> +		 "Reading symbols from $debug_file_re\\.\\.\\." \
>> +		 "warning: \"$debug_file_re\": separate debug info file has no debug info" \
>> +		 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
>> +	    "load test file, expect a warning"
>> +    }
>> +}
>> +
>>  # Now we should close GDB.
>>  gdb_exit
>>  
>> diff --git a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
>> index 588fb7e7277..074028eadfd 100644
>> --- a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
>> +++ b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
>> @@ -58,12 +58,39 @@ if {[build_executable crc_mismatch.exp crc_mismatch-2 crc_mismatch-2.c debug] !=
>>      # to trigger the "CRC mismatch" warning.
>>      remote_exec build "mv ${debugfile} [standard_output_file crc_mismatch-2.debug]"
>>  
>> -    gdb_exit
>> -    gdb_start
>> +    clean_restart
>>  
>>      set escapedobjdirsubdir [string_to_regexp [standard_output_file {}]]
>> +    gdb_test "file [standard_output_file crc_mismatch-2]" \
>> +	[multi_line \
>> +	     "Reading symbols from ${escapedobjdirsubdir}/crc_mismatch-2\\.\\.\\." \
>> +	     "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\." \
>> +	     "\\(No debugging symbols found in .*\\)"] \
>> +	"CRC mismatch is reported"
>> +
>> +    # Do the same thing again, but this time check that the styling is
>> +    # correct.
>> +    with_test_prefix "check styling" {
>> +	save_vars { env(TERM) } {
>> +	    # We need an ANSI-capable terminal to get styled output.
>> +	    setenv TERM ansi
>> +
>> +	    clean_restart
>> +
>> +	    set debug_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2\\.debug" file]
>> +	    set exe_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2" file]
>> +
>> +	    gdb_test "file [standard_output_file crc_mismatch-2]" \
>> +		[multi_line \
>> +		     "Reading symbols from ${exe_file_re}\\.\\.\\." \
>> +		     "warning: the debug information found in \"${debug_file_re}\" does not match \"${exe_file_re}\" \\(CRC mismatch\\)\\." \
>> +		     "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
>> +		"CRC mismatch is reported"
>> +	}
>> +    }
>>  
>> -    gdb_test "file [standard_output_file crc_mismatch-2]" "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\..*\\(No debugging symbols found in .*\\).*" "CRC mismatch is reported"
>> +    # Restart GDB with styling disabled.
>> +    clean_restart
>>  }
>>  
>>  # Create CACHE and DB directories ready for debuginfod to use.
>> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
>> index de24620e247..31f87ffd51d 100644
>> --- a/gdb/ui-file.h
>> +++ b/gdb/ui-file.h
>> @@ -30,6 +30,8 @@ class ui_file
>>    ui_file ();
>>    virtual ~ui_file () = 0;
>>  
>> +  ui_file (ui_file &&other) = default;
>> +
>>    /* Public non-virtual API.  */
>>  
>>    void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3);
>> @@ -177,6 +179,8 @@ class string_file : public ui_file
>>    explicit string_file (bool term_out) : m_term_out (term_out) {};
>>    ~string_file () override;
>>  
>> +  string_file (string_file &&other) = default;
>> +
>>    /* Override ui_file methods.  */
>>  
>>    void write (const char *buf, long length_buf) override;
>> diff --git a/gdb/utils.h b/gdb/utils.h
>> index 3faac20ec0f..602216a4c03 100644
>> --- a/gdb/utils.h
>> +++ b/gdb/utils.h
>> @@ -364,4 +364,62 @@ assign_return_if_changed (T &lval, const T &val)
>>    return true;
>>  }
>>  
>> +/* In some cases GDB needs to try several different solutions to a problem,
>> +   if any of the solutions work then as far as the user is concerned the
>> +   problem is solved, and GDB should continue without warnings.  However,
>> +   if non of the solutions work then GDB should emit any warnings that
>> +   occurred while trying each possible solution.
>> +
>> +   One example of this is locating separate debug info.  There are several
>> +   different approaches for this; following the .gnu_debuglink, a build-id
>> +   based lookup, or using debuginfod.  If any works, and debug info is
>> +   located, then the user doesn't want to see warnings from the earlier
>> +   approaches that were tried and failed.
>> +
>> +   However, GDB should emit all the warnings using separate calls to
>> +   warning -- this ensures that each warning is formatted on its own line,
>> +   and that any styling is emitted correctly.
>> +
>> +   This class helps with deferring warnings.  Warnings can be added to an
>> +   instance of this class with the 'warn' function, and all warnings can be
>> +   emitted with a single call to 'emit'.  */
>> +
>> +struct deferred_warnings
>> +{
>> +  /* Add a warning to the list of deferred warnings.  */
>> +  void warn (const char *format, ...)
>> +  {
>> +    /* Generate the warning text into a string_file.  We allow the text to
>> +       be styled only if gdb_stderr allows styling -- warnings are sent to
>> +       gdb_stderr.  */
>> +    string_file msg (gdb_stderr->can_emit_style_escape ());
>> +
>> +    va_list args;
>> +    va_start (args, format);
>> +    msg.vprintf (format, args);
>> +    va_end (args);
>> +
>> +    /* Move the text into the list of deferred warnings.  */
>> +    m_warnings.emplace_back (std::move (msg));
>> +  }
>> +
>> +  /* Emit all warnings.  */
>> +  void emit () const
>> +  {
>> +    for (const auto &w : m_warnings)
>> +      warning ("%s", w.c_str ());
>> +  }
>> +
>> +  /* Return true if there are no deferred warnings.  */
>> +  bool empty () const
>> +  {
>> +    return m_warnings.empty ();
>> +  }
>> +
>> +private:
>> +
>> +  /* The list of all deferred warnings.  */
>> +  std::vector<string_file> m_warnings;
>> +};
>> +
>>  #endif /* UTILS_H */
>> 
>> base-commit: bb2bd584f31a25ba1cfe5bdac4d07d8cffe87c3d
>> -- 
>> 2.25.4
>> 
>
>> @@ -364,4 +364,62 @@ assign_return_if_changed (T &lval, const T &val)
>>    return true;
>>  }
>>  
>> +/* In some cases GDB needs to try several different solutions to a problem,
>> +   if any of the solutions work then as far as the user is concerned the
>> +   problem is solved, and GDB should continue without warnings.  However,
>> +   if non of the solutions work then GDB should emit any warnings that
>
> non -> none

Fixed.

>
>> +   occurred while trying each possible solution.
>> +
>> +   One example of this is locating separate debug info.  There are several
>> +   different approaches for this; following the .gnu_debuglink, a build-id
>> +   based lookup, or using debuginfod.  If any works, and debug info is
>> +   located, then the user doesn't want to see warnings from the earlier
>> +   approaches that were tried and failed.
>> +
>> +   However, GDB should emit all the warnings using separate calls to
>> +   warning -- this ensures that each warning is formatted on its own line,
>> +   and that any styling is emitted correctly.
>> +
>> +   This class helps with deferring warnings.  Warnings can be added to an
>> +   instance of this class with the 'warn' function, and all warnings can be
>> +   emitted with a single call to 'emit'.  */
>> +
>> +struct deferred_warnings
>> +{
>> +  /* Add a warning to the list of deferred warnings.  */
>> +  void warn (const char *format, ...)
>
> You can probably put an ATTRIBUTE_PRINTF attribute here.

Fixed.

An updated version of the patch is below.

Thanks,
Andrew

---

commit af26bd97ac6507de7903a43339dff4929008f241
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri May 5 14:22:38 2023 +0100

    gdb: style filenames in separate debug file warnings
    
    After the commit:
    
      commit 6647f05df023b63bbe056e9167e9e234172fa2ca
      Date:   Tue Jan 24 18:13:38 2023 +0100
    
          gdb: defer warnings when loading separate debug files
    
    It was pointed out[1] that the warnings being deferred and then later
    emitted lacked styling.  The warnings lacked styling before the above
    commit, but it was suggested that the filenames in these warnings
    should be styled, and this commit does this.
    
    There were a couple of previous attempts[2][3][4] to solve this
    problem, but these all tried to extend the mechanism introduced in the
    above commit, the deferred warnings were placed directly into a
    std::vector, but now we tried to, when appropriate, style these
    warnings.  The review feedback that this approach looked too complex.
    
    So instead, this revision adds a new helper class 'deferred_warnings'
    which can be used to collect a set of deferred warnings, and then emit
    these deferred warnings later, if needed.  This helper class hides the
    complexity, so at the point the deferred warning is created no extra
    logic is required.
    
    The deferred_warnings class will style the deferred warnings only if
    gdb_stderr supports styling.  GDB's warnings are sent to gdb_stderr,
    so this should ensure we only style when expected.
    
    There was also review feedback[5] that all of the warnings should be
    bundled into a single string_file, this has not been done.  I feel
    pretty strongly that separate warnings should be emitted using
    separate "warning" calls.  If we do end up with multiple warnings in
    this case they aren't really related, one will be about looking up
    debug via .gnu_debuglink, while the other will be about build-id based
    lookup.  So I'd really rather keep the warnings separate.
    
    [1] https://inbox.sourceware.org/gdb-patches/87edr9pcku.fsf@tromey.com/
    [2] https://inbox.sourceware.org/gdb-patches/20230216195604.2685177-1-ahajkova@redhat.com/
    [3] https://inbox.sourceware.org/gdb-patches/20230217123547.2737612-1-ahajkova@redhat.com/
    [4] https://inbox.sourceware.org/gdb-patches/20230320145638.1202335-1-ahajkova@redhat.com/
    [5] https://inbox.sourceware.org/gdb-patches/87o7nh1g8h.fsf@tromey.com/
    
    Co-Authored-By: Alexandra Hájková <ahajkova@redhat.com>

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 65b493f0832..f68384f0197 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -26,6 +26,7 @@
 #include "objfiles.h"
 #include "filenames.h"
 #include "gdbcore.h"
+#include "cli/cli-style.h"
 
 /* See build-id.h.  */
 
@@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
 
 std::string
 find_separate_debug_file_by_buildid (struct objfile *objfile,
-				     std::vector<std::string> *warnings_vector)
+				     deferred_warnings *warnings)
 {
   const struct bfd_build_id *build_id;
 
@@ -228,12 +229,13 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
 	  && filename_cmp (bfd_get_filename (abfd.get ()),
 			   objfile_name (objfile)) == 0)
 	{
-	  std::string msg
-	    = 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, "\"%s\": separate debug info file has no "
+			"debug info", bfd_get_filename (abfd.get ()));
+	  warnings->warn (_("\"%ps\": separate debug info file has no "
+			    "debug info"),
+			  styled_string (file_name_style.style (),
+					 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..a5c8b85a442 100644
--- a/gdb/build-id.h
+++ b/gdb/build-id.h
@@ -52,13 +52,13 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
    separate debug file, otherwise, return an empty string.
 
    Any warnings that are generated by the lookup process should be added to
-   WARNINGS_VECTOR, one std::string per warning.  If some other mechanism can
-   be used to lookup the debug information then the warning will not be shown,
-   however, if GDB fails to find suitable debug information using any
-   approach, then any warnings will be printed.  */
+   WARNINGS.  If some other mechanism can be used to lookup the debug
+   information then the warning will not be shown, however, if GDB fails to
+   find suitable debug information using any 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, deferred_warnings *warnings);
 
 /* Return an hex-string representation of BUILD_ID.  */
 
diff --git a/gdb/coffread.c b/gdb/coffread.c
index ff4d4ae5313..33fb2ba1fca 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -729,13 +729,13 @@ 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;
+      deferred_warnings warnings;
       std::string debugfile
-	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
+	= find_separate_debug_file_by_buildid (objfile, &warnings);
 
       if (debugfile.empty ())
 	debugfile
-	  = find_separate_debug_file_by_debuglink (objfile, &warnings_vector);
+	  = find_separate_debug_file_by_debuglink (objfile, &warnings);
 
       if (!debugfile.empty ())
 	{
@@ -745,10 +745,10 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 				    symfile_flags, objfile);
 	}
       /* 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 ());
+	 warnings that were collected, this is a no-op if there are no
+	 warnings.  */
+      if (debugfile.empty ())
+	warnings.emit ();
     }
 }
 
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 7e1bb8db4f3..dbc63e52ed2 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -150,7 +150,7 @@ struct gdb_printing_disassembler : public gdb_disassemble_info
      DIS_INFO pointer is a pointer to a gdb_printing_disassembler object.
      Content is written to the m_stream extracted from DIS_INFO.  */
   static int fprintf_func (void *dis_info, const char *format, ...) noexcept
-    ATTRIBUTE_PRINTF(2,3);
+    ATTRIBUTE_PRINTF (2, 3);
 
   /* Callback used as the disassemble_info's fprintf_styled_func callback.
      The DIS_INFO pointer is a pointer to a gdb_printing_disassembler
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 799e3b914f8..d2662940c97 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1224,14 +1224,13 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
 	   && objfile->separate_debug_objfile == NULL
 	   && objfile->separate_debug_objfile_backlink == NULL)
     {
-      std::vector<std::string> warnings_vector;
+      deferred_warnings warnings;
 
       std::string debugfile
-	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
+	= find_separate_debug_file_by_buildid (objfile, &warnings);
 
       if (debugfile.empty ())
-	debugfile = find_separate_debug_file_by_debuglink (objfile,
-							   &warnings_vector);
+	debugfile = find_separate_debug_file_by_debuglink (objfile, &warnings);
 
       if (!debugfile.empty ())
 	{
@@ -1274,11 +1273,10 @@ 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 ());
+      /* If all the methods to collect the debuginfo failed, print the
+	 warnings, this is a no-op if there are no warnings.  */
+      if (debugfile.empty () && !has_dwarf2)
+	warnings.emit ();
     }
 
   return has_dwarf2;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 3a0aee163ff..d28404070cd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1243,7 +1243,7 @@ bool separate_debug_file_debug = false;
 static int
 separate_debug_file_exists (const std::string &name, unsigned long crc,
 			    struct objfile *parent_objfile,
-			    std::vector<std::string> *warnings_vector)
+			    deferred_warnings *warnings)
 {
   unsigned long file_crc;
   int file_crc_p;
@@ -1336,13 +1336,16 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
 
       if (verified_as_different || parent_crc != file_crc)
 	{
-	  std::string msg
-	    = string_printf (_("the debug information found in \"%s\""
-			       " 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, "the debug information found in \"%s\""
+			" does not match \"%s\" (CRC mismatch).\n",
+			name.c_str (), objfile_name (parent_objfile));
+	  warnings->warn (_("the debug information found in \"%ps\""
+			    " does not match \"%ps\" (CRC mismatch)."),
+			  styled_string (file_name_style.style (),
+					 name.c_str ()),
+			  styled_string (file_name_style.style (),
+					 objfile_name (parent_objfile)));
 	}
 
       return 0;
@@ -1378,14 +1381,17 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
    string.
 
    Any warnings generated as part of the lookup process are added to
-   WARNINGS_VECTOR, one std::string per warning.  */
+   WARNINGS.  If some other mechanism can be used to lookup the debug
+   information then the warning will not be shown, however, if GDB fails to
+   find suitable debug information using any approach, then any warnings
+   will be printed.  */
 
 static std::string
 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)
+			  deferred_warnings *warnings)
 {
   if (separate_debug_file_debug)
     gdb_printf (gdb_stdlog,
@@ -1396,7 +1402,7 @@ find_separate_debug_file (const char *dir,
   std::string debugfile = dir;
   debugfile += debuglink;
 
-  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
+  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
@@ -1405,7 +1411,7 @@ find_separate_debug_file (const char *dir,
   debugfile += "/";
   debugfile += debuglink;
 
-  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings_vector))
+  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the global debugfile directories.
@@ -1450,8 +1456,7 @@ find_separate_debug_file (const char *dir,
       debugfile += dir_notarget;
       debugfile += debuglink;
 
-      if (separate_debug_file_exists (debugfile, crc32, objfile,
-				      warnings_vector))
+      if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	return debugfile;
 
       const char *base_path = NULL;
@@ -1473,8 +1478,7 @@ find_separate_debug_file (const char *dir,
 	  debugfile += "/";
 	  debugfile += debuglink;
 
-	  if (separate_debug_file_exists (debugfile, crc32, objfile,
-					  warnings_vector))
+	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	    return debugfile;
 
 	  /* If the file is in the sysroot, try using its base path in
@@ -1501,7 +1505,7 @@ find_separate_debug_file (const char *dir,
 	      debugfile += debuglink;
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
-					      warnings_vector))
+					      warnings))
 		return debugfile;
 	    }
 	}
@@ -1533,7 +1537,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, deferred_warnings *warnings)
 {
   uint32_t crc32;
 
@@ -1554,7 +1558,7 @@ find_separate_debug_file_by_debuglink
   std::string debugfile
     = find_separate_debug_file (dir.c_str (), canon_dir.get (),
 				debuglink.get (), crc32, objfile,
-				warnings_vector);
+				warnings);
 
   if (debugfile.empty ())
     {
@@ -1579,7 +1583,7 @@ find_separate_debug_file_by_debuglink
 							debuglink.get (),
 							crc32,
 							objfile,
-							warnings_vector);
+							warnings);
 		}
 	    }
 	}
diff --git a/gdb/symfile.h b/gdb/symfile.h
index 7c800ea6cf9..296fee945c1 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -244,11 +244,14 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
 /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
    Returns pathname, or an empty string.
 
-   Any warnings generated as part of this lookup are added to
-   WARNINGS_VECTOR, one std::string per warning.  */
+   Any warnings generated as part of this lookup are added to WARNINGS.  If
+   some other mechanism can be used to lookup the debug information then
+   the warning will not be shown, however, if GDB fails to find suitable
+   debug information using any approach, then any warnings will be
+   printed.  */
 
 extern std::string find_separate_debug_file_by_debuglink
-  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+  (struct objfile *objfile, deferred_warnings *warnings);
 
 /* Build (allocate and populate) a section_addr_info struct from an
    existing section table.  */
diff --git a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
index b49770f4a5e..148363a7c05 100644
--- a/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
+++ b/gdb/testsuite/gdb.debuginfod/build-id-no-debug-warning.exp
@@ -95,6 +95,30 @@ gdb_test "file ${build_id_debug_file}" \
 	 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
     "load test file, expect a warning"
 
+# Do the same thing again, but this time check that the styling is
+# correct.
+with_test_prefix "check styling" {
+    save_vars { env(TERM) } {
+	# We need an ANSI-capable terminal to get styled output.
+	setenv TERM ansi
+
+	clean_restart
+
+	gdb_test_no_output "set debug-file-directory ${debug_file_directory}" \
+	    "set debug-file-directory"
+
+	# Now load the file into GDB, and look for the warning.
+	set debug_file_re [style [string_to_regexp $build_id_debug_file] file]
+
+	gdb_test "file ${build_id_debug_file}" \
+	    [multi_line \
+		 "Reading symbols from $debug_file_re\\.\\.\\." \
+		 "warning: \"$debug_file_re\": separate debug info file has no debug info" \
+		 "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
+	    "load test file, expect a warning"
+    }
+}
+
 # Now we should close GDB.
 gdb_exit
 
diff --git a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
index 588fb7e7277..074028eadfd 100644
--- a/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
+++ b/gdb/testsuite/gdb.debuginfod/crc_mismatch.exp
@@ -58,12 +58,39 @@ if {[build_executable crc_mismatch.exp crc_mismatch-2 crc_mismatch-2.c debug] !=
     # to trigger the "CRC mismatch" warning.
     remote_exec build "mv ${debugfile} [standard_output_file crc_mismatch-2.debug]"
 
-    gdb_exit
-    gdb_start
+    clean_restart
 
     set escapedobjdirsubdir [string_to_regexp [standard_output_file {}]]
+    gdb_test "file [standard_output_file crc_mismatch-2]" \
+	[multi_line \
+	     "Reading symbols from ${escapedobjdirsubdir}/crc_mismatch-2\\.\\.\\." \
+	     "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\." \
+	     "\\(No debugging symbols found in .*\\)"] \
+	"CRC mismatch is reported"
+
+    # Do the same thing again, but this time check that the styling is
+    # correct.
+    with_test_prefix "check styling" {
+	save_vars { env(TERM) } {
+	    # We need an ANSI-capable terminal to get styled output.
+	    setenv TERM ansi
+
+	    clean_restart
+
+	    set debug_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2\\.debug" file]
+	    set exe_file_re [style "${escapedobjdirsubdir}/crc_mismatch-2" file]
+
+	    gdb_test "file [standard_output_file crc_mismatch-2]" \
+		[multi_line \
+		     "Reading symbols from ${exe_file_re}\\.\\.\\." \
+		     "warning: the debug information found in \"${debug_file_re}\" does not match \"${exe_file_re}\" \\(CRC mismatch\\)\\." \
+		     "\\(No debugging symbols found in \[^\r\n\]+\\)"] \
+		"CRC mismatch is reported"
+	}
+    }
 
-    gdb_test "file [standard_output_file crc_mismatch-2]" "warning: the debug information found in \"${escapedobjdirsubdir}/crc_mismatch-2\\.debug\" does not match \"${escapedobjdirsubdir}/crc_mismatch-2\" \\(CRC mismatch\\)\\..*\\(No debugging symbols found in .*\\).*" "CRC mismatch is reported"
+    # Restart GDB with styling disabled.
+    clean_restart
 }
 
 # Create CACHE and DB directories ready for debuginfod to use.
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index de24620e247..31f87ffd51d 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -30,6 +30,8 @@ class ui_file
   ui_file ();
   virtual ~ui_file () = 0;
 
+  ui_file (ui_file &&other) = default;
+
   /* Public non-virtual API.  */
 
   void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3);
@@ -177,6 +179,8 @@ class string_file : public ui_file
   explicit string_file (bool term_out) : m_term_out (term_out) {};
   ~string_file () override;
 
+  string_file (string_file &&other) = default;
+
   /* Override ui_file methods.  */
 
   void write (const char *buf, long length_buf) override;
diff --git a/gdb/utils.h b/gdb/utils.h
index 3faac20ec0f..3733a07684e 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -364,4 +364,56 @@ assign_return_if_changed (T &lval, const T &val)
   return true;
 }
 
+/* In some cases GDB needs to try several different solutions to a problem,
+   if any of the solutions work then as far as the user is concerned the
+   problem is solved, and GDB should continue without warnings.  However,
+   if none of the solutions work then GDB should emit any warnings that
+   occurred while trying each possible solution.
+
+   One example of this is locating separate debug info.  There are several
+   different approaches for this; following the .gnu_debuglink, a build-id
+   based lookup, or using debuginfod.  If any works, and debug info is
+   located, then the user doesn't want to see warnings from the earlier
+   approaches that were tried and failed.
+
+   However, GDB should emit all the warnings using separate calls to
+   warning -- this ensures that each warning is formatted on its own line,
+   and that any styling is emitted correctly.
+
+   This class helps with deferring warnings.  Warnings can be added to an
+   instance of this class with the 'warn' function, and all warnings can be
+   emitted with a single call to 'emit'.  */
+
+struct deferred_warnings
+{
+  /* Add a warning to the list of deferred warnings.  */
+  void warn (const char *format, ...) ATTRIBUTE_PRINTF(2,3)
+  {
+    /* Generate the warning text into a string_file.  We allow the text to
+       be styled only if gdb_stderr allows styling -- warnings are sent to
+       gdb_stderr.  */
+    string_file msg (gdb_stderr->can_emit_style_escape ());
+
+    va_list args;
+    va_start (args, format);
+    msg.vprintf (format, args);
+    va_end (args);
+
+    /* Move the text into the list of deferred warnings.  */
+    m_warnings.emplace_back (std::move (msg));
+  }
+
+  /* Emit all warnings.  */
+  void emit () const
+  {
+    for (const auto &w : m_warnings)
+      warning ("%s", w.c_str ());
+  }
+
+private:
+
+  /* The list of all deferred warnings.  */
+  std::vector<string_file> m_warnings;
+};
+
 #endif /* UTILS_H */

Re: [PATCHv3] gdb: style filenames in separate debug file warnings

[Simon Marchi]

On 7/7/23 11:03, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>>> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>>>  
>>>  std::string
>>>  find_separate_debug_file_by_buildid (struct objfile *objfile,
>>> -				     std::vector<std::string> *warnings_vector)
>>> +				     deferred_warnings *warnings)
>> Reference instead of pointer?
> 
> I thought at one point (maybe I'm miss-remembering) GDB style was to
> pass a pointer if an object was modified, and a const reference when an
> object was not going to be modified.  I've stuck to that since pretty
> much forever for GDB code .... is this not (or was it never) a thing?
> 
> I've not changed this for now, but I can do this if that's your
> preference, just let me know.

I had added it to the wiki for a brief period of time, but removed it
since there was some disagreement (not in the thread, it must have been
offline, on IRC, or in another thread):

https://inbox.sourceware.org/gdb-patches/b31a374788e2b6067786fb8c6c0a3acc@polymtl.ca/

I based myself on the Google style guide at the time, which I thought
had a good rationale.  But they have since changed it to allow non-const
references:

https://github.com/google/styleguide/commit/7a7a2f510efe7d7fc

Since then I've gotten used to it, and use non-const references when
needed, since that's just the C++ way to do it.

>>> @@ -1274,11 +1273,10 @@ 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 ());
>>> +      /* If all the methods to collect the debuginfo failed, print the
>>> +	 warnings, if there are any. */
>>> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
>>> +	warnings.emit ();
>>
>> The warnings.empty() seems unnecessary to me, since emit will be a no-op
>> if there are no warnings.
> 
> Funny you should call this out.  I added the .empty() just prior to
> posting this to the mailing list because, when I gave the patches a
> final review, it in someway felt more obvious what was going on if there
> was an empty check.  But I'm not really tied to it, so I'm going to
> remove the empty check.

No strong feelings here.  But the "if there are any" in your comment
makes it clear to me that there might be no warnings to emit.

> An updated version of the patch is below.

I don't really have time right now to look at the updated patch, but if
all you changed is things I pointed out:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCHv3] gdb: style filenames in separate debug file warnings

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 7/7/23 11:03, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>>> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>>>>  
>>>>  std::string
>>>>  find_separate_debug_file_by_buildid (struct objfile *objfile,
>>>> -				     std::vector<std::string> *warnings_vector)
>>>> +				     deferred_warnings *warnings)
>>> Reference instead of pointer?
>> 
>> I thought at one point (maybe I'm miss-remembering) GDB style was to
>> pass a pointer if an object was modified, and a const reference when an
>> object was not going to be modified.  I've stuck to that since pretty
>> much forever for GDB code .... is this not (or was it never) a thing?
>> 
>> I've not changed this for now, but I can do this if that's your
>> preference, just let me know.
>
> I had added it to the wiki for a brief period of time, but removed it
> since there was some disagreement (not in the thread, it must have been
> offline, on IRC, or in another thread):
>
> https://inbox.sourceware.org/gdb-patches/b31a374788e2b6067786fb8c6c0a3acc@polymtl.ca/
>
> I based myself on the Google style guide at the time, which I thought
> had a good rationale.  But they have since changed it to allow non-const
> references:
>
> https://github.com/google/styleguide/commit/7a7a2f510efe7d7fc
>
> Since then I've gotten used to it, and use non-const references when
> needed, since that's just the C++ way to do it.
>
>>>> @@ -1274,11 +1273,10 @@ 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 ());
>>>> +      /* If all the methods to collect the debuginfo failed, print the
>>>> +	 warnings, if there are any. */
>>>> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
>>>> +	warnings.emit ();
>>>
>>> The warnings.empty() seems unnecessary to me, since emit will be a no-op
>>> if there are no warnings.
>> 
>> Funny you should call this out.  I added the .empty() just prior to
>> posting this to the mailing list because, when I gave the patches a
>> final review, it in someway felt more obvious what was going on if there
>> was an empty check.  But I'm not really tied to it, so I'm going to
>> remove the empty check.
>
> No strong feelings here.  But the "if there are any" in your comment
> makes it clear to me that there might be no warnings to emit.
>
>> An updated version of the patch is below.
>
> I don't really have time right now to look at the updated patch, but if
> all you changed is things I pointed out:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Pushed.

Thanks,
Andrew