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

Message ID 20230320145638.1202335-1-ahajkova@redhat.com
State New
Headers
Series [v2] Use styled_string when defering warnings when loading separate debug files |

Commit Message

Alexandra Petlanova Hajkova March 20, 2023, 2:56 p.m. UTC
  This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
filenames used in the warnings are styled properly now.
---
This version uses ui_file rather instead of vector of callback functions that emit warnings.
Addresses various cosmetic issues.

 gdb/build-id.c | 16 ++++++++++------
 gdb/build-id.h |  4 ++--
 gdb/coffread.c |  4 ++--
 gdb/elfread.c  |  4 ++--
 gdb/symfile.c  | 26 ++++++++++++++++----------
 gdb/symfile.h  |  4 ++--
 gdb/ui-file.h  |  7 +++++++
 7 files changed, 41 insertions(+), 24 deletions(-)
  

Comments

Alexandra Petlanova Hajkova March 30, 2023, 8:11 a.m. UTC | #1
Ping

On Mon, Mar 20, 2023 at 3:56 PM Alexandra Hájková via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
> filenames used in the warnings are styled properly now.
> ---
> This version uses ui_file rather instead of vector of callback functions
> that emit warnings.
> Addresses various cosmetic issues.
>
>  gdb/build-id.c | 16 ++++++++++------
>  gdb/build-id.h |  4 ++--
>  gdb/coffread.c |  4 ++--
>  gdb/elfread.c  |  4 ++--
>  gdb/symfile.c  | 26 ++++++++++++++++----------
>  gdb/symfile.h  |  4 ++--
>  gdb/ui-file.h  |  7 +++++++
>  7 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 00250c20ae9..5d5114e2ace 100644
> --- a/gdb/build-id.c
> +++ b/gdb/build-id.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
> */
>
>  #include "defs.h"
> +#include "cli/cli-style.h"
>  #include "bfd.h"
>  #include "gdb_bfd.h"
>  #include "build-id.h"
> @@ -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)
> +                                    std::vector<string_file>
> *warnings_vector)
>  {
>    const struct bfd_build_id *build_id;
>
> @@ -228,12 +229,15 @@ 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
> ()));
> +         string_file warnings (true);
> +         warnings.printf ("\"%ps\":separate debug info file has no debug
> info",
> +                          styled_string (file_name_style.style (),
> +                                         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));
> +           /* Avoid styling for the debug output.  */
> +           gdb_printf (gdb_stdlog, _("%s: separate debug info file has no
> debug info"),
> +                       bfd_get_filename (abfd.get ()));
> +         warnings_vector->emplace_back (std::move (warnings));
>         }
>        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..0c2c4181b4e 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
> +   WARNINGS_VECTOR, one string_file 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.  */
>
>  extern std::string find_separate_debug_file_by_buildid
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> +  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
>
>  /* Return an hex-string representation of BUILD_ID.  */
>
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index e993b17db09..32705c215fd 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -729,7 +729,7 @@ coff_symfile_read (struct objfile *objfile,
> symfile_add_flags symfile_flags)
>    /* Try to add separate debug file if no symbols table found.   */
>    if (!objfile->has_partial_symbols ())
>      {
> -      std::vector<std::string> warnings_vector;
> +      std::vector<string_file> warnings_vector;
>        std::string debugfile
>         = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>
> @@ -747,7 +747,7 @@ 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)
> +       for (const string_file &w : warnings_vector)
>           warning ("%s", w.c_str ());
>      }
>  }
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index b414da9ed21..4bc89d09d65 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1217,7 +1217,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>            && objfile->separate_debug_objfile == NULL
>            && objfile->separate_debug_objfile_backlink == NULL)
>      {
> -      std::vector<std::string> warnings_vector;
> +      std::vector<string_file> warnings_vector;
>
>        std::string debugfile
>         = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
> @@ -1269,7 +1269,7 @@ 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)
> +       for (const string_file &w : warnings_vector)
>           warning ("%s", w.c_str ());
>      }
>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index bb9981a4634..64d29ea71f9 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1244,7 +1244,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)
> +                           std::vector<string_file> *warnings_vector)
>  {
>    unsigned long file_crc;
>    int file_crc_p;
> @@ -1337,13 +1337,19 @@ 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));
> +         string_file warnings (true);
> +         warnings.printf (_("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)));
> +         /* Avoid styling for the debug output.  */
>           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_vector->emplace_back (std::move (warnings));
>         }
>
>        return 0;
> @@ -1379,14 +1385,14 @@ 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_VECTOR, one string_file per warning.  */
>
>  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)
> +                         std::vector<string_file> *warnings_vector)
>  {
>    if (separate_debug_file_debug)
>      gdb_printf (gdb_stdlog,
> @@ -1534,7 +1540,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, std::vector<string_file> *warnings_vector)
>  {
>    unsigned long crc32;
>
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index b433e2be31a..041f6f74f9c 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -245,10 +245,10 @@ extern void symbol_file_add_separate (const
> gdb_bfd_ref_ptr &, const char *,
>     Returns pathname, or an empty string.
>
>     Any warnings generated as part of this lookup are added to
> -   WARNINGS_VECTOR, one std::string per warning.  */
> +   WARNINGS_VECTOR, one string_file per warning.  */
>
>  extern std::string find_separate_debug_file_by_debuglink
> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> +  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
>
>  /* Build (allocate and populate) a section_addr_info struct from an
>     existing section table.  */
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index de24620e247..4444c8adcdb 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -212,6 +212,13 @@ class string_file : public ui_file
>      return *this;
>    }
>
> +  string_file(string_file &&other)
> +    : m_string (std::move (other.c_str ())),
> +    m_term_out (other.m_term_out)
> +  {
> +
> +  }
> +
>    /* Provide a few convenience methods with the same API as the
>       underlying std::string.  */
>    const char *data () const { return m_string.data (); }
> --
> 2.39.1
>
>
  
Alexandra Petlanova Hajkova April 11, 2023, 12:40 p.m. UTC | #2
Ping2

On Thu, Mar 30, 2023 at 10:11 AM Alexandra Petlanova Hajkova <
ahajkova@redhat.com> wrote:

> Ping
>
> On Mon, Mar 20, 2023 at 3:56 PM Alexandra Hájková via Gdb-patches <
> gdb-patches@sourceware.org> wrote:
>
>> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
>> filenames used in the warnings are styled properly now.
>> ---
>> This version uses ui_file rather instead of vector of callback functions
>> that emit warnings.
>> Addresses various cosmetic issues.
>>
>>  gdb/build-id.c | 16 ++++++++++------
>>  gdb/build-id.h |  4 ++--
>>  gdb/coffread.c |  4 ++--
>>  gdb/elfread.c  |  4 ++--
>>  gdb/symfile.c  | 26 ++++++++++++++++----------
>>  gdb/symfile.h  |  4 ++--
>>  gdb/ui-file.h  |  7 +++++++
>>  7 files changed, 41 insertions(+), 24 deletions(-)
>>
>> diff --git a/gdb/build-id.c b/gdb/build-id.c
>> index 00250c20ae9..5d5114e2ace 100644
>> --- a/gdb/build-id.c
>> +++ b/gdb/build-id.c
>> @@ -18,6 +18,7 @@
>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>>  #include "defs.h"
>> +#include "cli/cli-style.h"
>>  #include "bfd.h"
>>  #include "gdb_bfd.h"
>>  #include "build-id.h"
>> @@ -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)
>> +                                    std::vector<string_file>
>> *warnings_vector)
>>  {
>>    const struct bfd_build_id *build_id;
>>
>> @@ -228,12 +229,15 @@ 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
>> ()));
>> +         string_file warnings (true);
>> +         warnings.printf ("\"%ps\":separate debug info file has no debug
>> info",
>> +                          styled_string (file_name_style.style (),
>> +                                         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));
>> +           /* Avoid styling for the debug output.  */
>> +           gdb_printf (gdb_stdlog, _("%s: separate debug info file has
>> no debug info"),
>> +                       bfd_get_filename (abfd.get ()));
>> +         warnings_vector->emplace_back (std::move (warnings));
>>         }
>>        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..0c2c4181b4e 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
>> +   WARNINGS_VECTOR, one string_file 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.  */
>>
>>  extern std::string find_separate_debug_file_by_buildid
>> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
>> +  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
>>
>>  /* Return an hex-string representation of BUILD_ID.  */
>>
>> diff --git a/gdb/coffread.c b/gdb/coffread.c
>> index e993b17db09..32705c215fd 100644
>> --- a/gdb/coffread.c
>> +++ b/gdb/coffread.c
>> @@ -729,7 +729,7 @@ coff_symfile_read (struct objfile *objfile,
>> symfile_add_flags symfile_flags)
>>    /* Try to add separate debug file if no symbols table found.   */
>>    if (!objfile->has_partial_symbols ())
>>      {
>> -      std::vector<std::string> warnings_vector;
>> +      std::vector<string_file> warnings_vector;
>>        std::string debugfile
>>         = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>>
>> @@ -747,7 +747,7 @@ 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)
>> +       for (const string_file &w : warnings_vector)
>>           warning ("%s", w.c_str ());
>>      }
>>  }
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index b414da9ed21..4bc89d09d65 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -1217,7 +1217,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>>            && objfile->separate_debug_objfile == NULL
>>            && objfile->separate_debug_objfile_backlink == NULL)
>>      {
>> -      std::vector<std::string> warnings_vector;
>> +      std::vector<string_file> warnings_vector;
>>
>>        std::string debugfile
>>         = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>> @@ -1269,7 +1269,7 @@ 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)
>> +       for (const string_file &w : warnings_vector)
>>           warning ("%s", w.c_str ());
>>      }
>>
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index bb9981a4634..64d29ea71f9 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -1244,7 +1244,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)
>> +                           std::vector<string_file> *warnings_vector)
>>  {
>>    unsigned long file_crc;
>>    int file_crc_p;
>> @@ -1337,13 +1337,19 @@ 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));
>> +         string_file warnings (true);
>> +         warnings.printf (_("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)));
>> +         /* Avoid styling for the debug output.  */
>>           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_vector->emplace_back (std::move (warnings));
>>         }
>>
>>        return 0;
>> @@ -1379,14 +1385,14 @@ 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_VECTOR, one string_file per warning.  */
>>
>>  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)
>> +                         std::vector<string_file> *warnings_vector)
>>  {
>>    if (separate_debug_file_debug)
>>      gdb_printf (gdb_stdlog,
>> @@ -1534,7 +1540,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, std::vector<string_file> *warnings_vector)
>>  {
>>    unsigned long crc32;
>>
>> diff --git a/gdb/symfile.h b/gdb/symfile.h
>> index b433e2be31a..041f6f74f9c 100644
>> --- a/gdb/symfile.h
>> +++ b/gdb/symfile.h
>> @@ -245,10 +245,10 @@ extern void symbol_file_add_separate (const
>> gdb_bfd_ref_ptr &, const char *,
>>     Returns pathname, or an empty string.
>>
>>     Any warnings generated as part of this lookup are added to
>> -   WARNINGS_VECTOR, one std::string per warning.  */
>> +   WARNINGS_VECTOR, one string_file per warning.  */
>>
>>  extern std::string find_separate_debug_file_by_debuglink
>> -  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
>> +  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
>>
>>  /* Build (allocate and populate) a section_addr_info struct from an
>>     existing section table.  */
>> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
>> index de24620e247..4444c8adcdb 100644
>> --- a/gdb/ui-file.h
>> +++ b/gdb/ui-file.h
>> @@ -212,6 +212,13 @@ class string_file : public ui_file
>>      return *this;
>>    }
>>
>> +  string_file(string_file &&other)
>> +    : m_string (std::move (other.c_str ())),
>> +    m_term_out (other.m_term_out)
>> +  {
>> +
>> +  }
>> +
>>    /* Provide a few convenience methods with the same API as the
>>       underlying std::string.  */
>>    const char *data () const { return m_string.data (); }
>> --
>> 2.39.1
>>
>>
  
Tom Tromey April 17, 2023, 3:17 p.m. UTC | #3
>>>>> "Alexandra" == Alexandra Hájková via Gdb-patches <gdb-patches@sourceware.org> writes:

Alexandra> This version uses ui_file rather instead of vector of
Alexandra> callback functions that emit warnings.

I think instead of a std::vector<string_file>, what I meant was just use
a single string_file.

The caller can make it, using something like:

  string_file file (gdb_stderr->can_emit_style_escape ());

Alexandra> +	  string_file warnings (true);

... since I think 'true' isn't always correct.

Alexandra> +	  warnings.printf ("\"%ps\":separate debug info file has no debug info",
Alexandra> +			   styled_string (file_name_style.style (),
Alexandra> +					  bfd_get_filename (abfd.get ())));

Then this printf can either add a 'warning:' prefix like warning() does,
or we can add an overload of "warning" that takes a ui_file as its first
parameter.

Alexandra> -	for (const std::string &w : warnings_vector)
Alexandra> +	for (const string_file &w : warnings_vector)
Alexandra>  	  warning ("%s", w.c_str ());

The approach I mention seems fine because the caller ends up not doing
anything except possibly printing the strings.

Alexandra> +  string_file(string_file &&other)
Alexandra> +    : m_string (std::move (other.c_str ())),
Alexandra> +    m_term_out (other.m_term_out)
Alexandra> +  {
Alexandra> +
Alexandra> +  }

Then you wouldn't need this, either.  But for future reference I think
this is just:

  string_file (string_file &&other) = default;

thanks,
Tom
  
Alexandra Petlanova Hajkova April 18, 2023, 12:20 p.m. UTC | #4
I think instead of a std::vector<string_file>, what I meant was just use
> a single string_file.
>
> The caller can make it, using something like:
>
>   string_file file (gdb_stderr->can_emit_style_escape ());
>
> Alexandra> +      string_file warnings (true);
>
> ... since I think 'true' isn't always correct.
>

When is it not true?

>
> Alexandra> +      warnings.printf ("\"%ps\":separate debug info file has
> no debug info",
> Alexandra> +                       styled_string (file_name_style.style (),
> Alexandra> +                                      bfd_get_filename
> (abfd.get ())));
>
> Then this printf can either add a 'warning:' prefix like warning() does,
> or we can add an overload of "warning" that takes a ui_file as its first
> parameter.
>

So you would prefer me to use a single string_file instead of a vector of
those. That means I'll have just a single string and I'll be appending each
new warning to it? I used a vector because I don't know how to easily split
the warnings when printing them. Do you mean I should add an overload of a
"warning" that would be capable of splitting strings? But why is such a
solution better then simply using a vector of string_file?

Thank you for the review,
Alexandra
  
Tom Tromey April 21, 2023, 2:07 p.m. UTC | #5
Alexandra> +      string_file warnings (true);
>> 
>> ... since I think 'true' isn't always correct.
>> 

Alexandra> When is it not true?

I suspect it would be the case if output is redirected or if styling is
disabled globally.

Alexandra> So you would prefer me to use a single string_file instead of a vector of
Alexandra> those. That means I'll have just a single string and I'll be appending each
Alexandra> new warning to it? I used a vector because I don't know how to easily split
Alexandra> the warnings when printing them. Do you mean I should add an overload of a
Alexandra> "warning" that would be capable of splitting strings? But why is such a
Alexandra> solution better then simply using a vector of string_file?

If you feel very strongly about it, I will OK it.  I don't really care
that much.  It just seems to me that there's no reason to construct all
these objects -- a vector of string files -- when the goal is just to
collect some output and maybe print it.  Collecting output like this is
really what a string_file is for.

Adding an overload of warning -- it's up to you, I don't mind either
way.

Printing the output in the end is:

  string_file blah;
  ...
  if (something)
     gdb_puts (gdb_stderr, blah.str ().c_str ());

or something like that.

Tom
  

Patch

diff --git a/gdb/build-id.c b/gdb/build-id.c
index 00250c20ae9..5d5114e2ace 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -18,6 +18,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "cli/cli-style.h"
 #include "bfd.h"
 #include "gdb_bfd.h"
 #include "build-id.h"
@@ -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)
+				     std::vector<string_file> *warnings_vector)
 {
   const struct bfd_build_id *build_id;
 
@@ -228,12 +229,15 @@  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 ()));
+	  string_file warnings (true);
+	  warnings.printf ("\"%ps\":separate debug info file has no debug info",
+			   styled_string (file_name_style.style (),
+					  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));
+	    /* Avoid styling for the debug output.  */
+	    gdb_printf (gdb_stdlog, _("%s: separate debug info file has no debug info"),
+			bfd_get_filename (abfd.get ()));
+	  warnings_vector->emplace_back (std::move (warnings));
 	}
       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..0c2c4181b4e 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
+   WARNINGS_VECTOR, one string_file 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.  */
 
 extern std::string find_separate_debug_file_by_buildid
-  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
 
 /* Return an hex-string representation of BUILD_ID.  */
 
diff --git a/gdb/coffread.c b/gdb/coffread.c
index e993b17db09..32705c215fd 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -729,7 +729,7 @@  coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
   /* Try to add separate debug file if no symbols table found.   */
   if (!objfile->has_partial_symbols ())
     {
-      std::vector<std::string> warnings_vector;
+      std::vector<string_file> warnings_vector;
       std::string debugfile
 	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
 
@@ -747,7 +747,7 @@  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)
+	for (const string_file &w : warnings_vector)
 	  warning ("%s", w.c_str ());
     }
 }
diff --git a/gdb/elfread.c b/gdb/elfread.c
index b414da9ed21..4bc89d09d65 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1217,7 +1217,7 @@  elf_symfile_read_dwarf2 (struct objfile *objfile,
 	   && objfile->separate_debug_objfile == NULL
 	   && objfile->separate_debug_objfile_backlink == NULL)
     {
-      std::vector<std::string> warnings_vector;
+      std::vector<string_file> warnings_vector;
 
       std::string debugfile
 	= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
@@ -1269,7 +1269,7 @@  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)
+	for (const string_file &w : warnings_vector)
 	  warning ("%s", w.c_str ());
     }
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index bb9981a4634..64d29ea71f9 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1244,7 +1244,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)
+			    std::vector<string_file> *warnings_vector)
 {
   unsigned long file_crc;
   int file_crc_p;
@@ -1337,13 +1337,19 @@  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));
+	  string_file warnings (true);
+	  warnings.printf (_("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)));
+	  /* Avoid styling for the debug output.  */
 	  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_vector->emplace_back (std::move (warnings));
 	}
 
       return 0;
@@ -1379,14 +1385,14 @@  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_VECTOR, one string_file per warning.  */
 
 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)
+			  std::vector<string_file> *warnings_vector)
 {
   if (separate_debug_file_debug)
     gdb_printf (gdb_stdlog,
@@ -1534,7 +1540,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, std::vector<string_file> *warnings_vector)
 {
   unsigned long crc32;
 
diff --git a/gdb/symfile.h b/gdb/symfile.h
index b433e2be31a..041f6f74f9c 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -245,10 +245,10 @@  extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
    Returns pathname, or an empty string.
 
    Any warnings generated as part of this lookup are added to
-   WARNINGS_VECTOR, one std::string per warning.  */
+   WARNINGS_VECTOR, one string_file per warning.  */
 
 extern std::string find_separate_debug_file_by_debuglink
-  (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+  (struct objfile *objfile, std::vector<string_file> *warnings_vector);
 
 /* Build (allocate and populate) a section_addr_info struct from an
    existing section table.  */
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index de24620e247..4444c8adcdb 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -212,6 +212,13 @@  class string_file : public ui_file
     return *this;
   }
 
+  string_file(string_file &&other)
+    : m_string (std::move (other.c_str ())),
+    m_term_out (other.m_term_out)
+  {
+
+  }
+
   /* Provide a few convenience methods with the same API as the
      underlying std::string.  */
   const char *data () const { return m_string.data (); }