[v2] Use styled_string when defering warnings when loading separate debug files
Commit Message
This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
filenames used in the warnings are styled properly now.
---
This version: - adds comments to the new add_warning function
- style filenames only, not the whole warning string
- avoids creating duplicate type definitions
gdb/build-id.c | 21 ++++++++++++++++++---
gdb/build-id.h | 3 ++-
gdb/coffread.c | 6 +++---
gdb/elfread.c | 6 +++---
gdb/symfile.c | 30 +++++++++++++++++++++++++-----
gdb/symfile.h | 5 ++++-
6 files changed, 55 insertions(+), 16 deletions(-)
Comments
Alexandra Hájková via Gdb-patches <gdb-patches@sourceware.org> writes:
> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
> filenames used in the warnings are styled properly now.
> ---
> This version: - adds comments to the new add_warning function
> - style filenames only, not the whole warning string
> - avoids creating duplicate type definitions
>
> gdb/build-id.c | 21 ++++++++++++++++++---
> gdb/build-id.h | 3 ++-
> gdb/coffread.c | 6 +++---
> gdb/elfread.c | 6 +++---
> gdb/symfile.c | 30 +++++++++++++++++++++++++-----
> gdb/symfile.h | 5 ++++-
> 6 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/build-id.c b/gdb/build-id.c
> index 00250c20ae9..6e48ec2e4ae 100644
> --- a/gdb/build-id.c
> +++ b/gdb/build-id.c
> @@ -18,6 +18,7 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> #include "defs.h"
> +#include "cli/cli-style.h"
> #include "bfd.h"
> #include "gdb_bfd.h"
> #include "build-id.h"
> @@ -205,11 +206,23 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
> return build_id_to_bfd_suffix (build_id_len, build_id, "");
> }
>
> +/* Use callbacks to produce the warnings. */
I don't think this comment is very helpful. Something like:
/* Add a callback to VEC that will produce a warning that separate
debug info file FILENAME actually contains no debug info. */
> +
> +static void
> +add_warning (warnings_vec *vec, std::string filename)
Given you are creating a copy of filename within the callback I think
you can pass filename as 'const std::string &filename'.
> +{
> + vec->emplace_back ([filename] () {
> + warning (_("\"%ps\": separate debug info file has no debug info"),
> + styled_string (file_name_style.style (),
> + filename.c_str()));
You dropped the space before '()' here.
> + });
> +}
> +
> /* See build-id.h. */
>
> std::string
> find_separate_debug_file_by_buildid (struct objfile *objfile,
> - std::vector<std::string> *warnings_vector)
> + warnings_vec *vec)
> {
> const struct bfd_build_id *build_id;
>
> @@ -232,8 +245,10 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
> = string_printf (_("\"%s\": separate debug info file has no "
> "debug info"), bfd_get_filename (abfd.get ()));
> if (separate_debug_file_debug)
> - gdb_printf (gdb_stdlog, "%s", msg.c_str ());
> - warnings_vector->emplace_back (std::move (msg));
> + warning (_("\"%ps\": separate debug info file has no debug info"),
> + styled_string (file_name_style.style (),
> + bfd_get_filename (abfd.get ())));
This doesn't seem right. If `separate_debug_file_debug` is true then we
should be producing debug output, not additional warnings. The
gdb_printf call as it was is fine; you just need to get rid of the `msg`
temporary variable.
> + add_warning(vec, bfd_get_filename (abfd.get ()));
Honestly I'd get rid of the add_warning call completely, and just inline
it here, with only one caller I don't think it adds much value, but
that's just my opinion, not a requirement.
> }
> else if (abfd != NULL)
> return std::string (bfd_get_filename (abfd.get ()));
> diff --git a/gdb/build-id.h b/gdb/build-id.h
> index 191720ddf28..addaec82526 100644
> --- a/gdb/build-id.h
> +++ b/gdb/build-id.h
> @@ -21,6 +21,7 @@
> #define BUILD_ID_H
>
> #include "gdb_bfd.h"
> +#include "gdb/symfile.h"
> #include "gdbsupport/rsp-low.h"
>
> /* Locate NT_GNU_BUILD_ID from ABFD and return its content. */
> @@ -58,7 +59,7 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
> approach, then any warnings will be printed. */
>
> extern std::string find_separate_debug_file_by_buildid
> - (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> + (struct objfile *objfile, warnings_vec *vec);
The comment for this function is out of date, it still talks about
WARNINGS_VECTOR, not VEC, and still thinks the vector holds strings.
>
> /* Return an hex-string representation of BUILD_ID. */
>
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index 65d7828e933..c534eb504cf 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -734,7 +734,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
> /* Try to add separate debug file if no symbols table found. */
> if (!objfile->has_partial_symbols ())
> {
> - std::vector<std::string> warnings_vector;
> + warnings_vec warnings_vector;
> std::string debugfile
> = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
>
> @@ -752,8 +752,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
> /* If all the methods to collect the debuginfo failed, print any
> warnings that were collected. */
> if (debugfile.empty () && !warnings_vector.empty ())
> - for (const std::string &w : warnings_vector)
> - warning ("%s", w.c_str ());
> + for (const auto &cb : warnings_vector)
> + cb ();
> }
> }
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index ca684aab57e..d1c231c2f8c 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1213,7 +1213,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
> && objfile->separate_debug_objfile == NULL
> && objfile->separate_debug_objfile_backlink == NULL)
> {
> - std::vector<std::string> warnings_vector;
> + warnings_vec warnings_vector;
>
> std::string debugfile
> = find_separate_debug_file_by_buildid (objfile, &warnings_vector);
> @@ -1265,8 +1265,8 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
> /* If all the methods to collect the debuginfo failed, print
> the warnings, if there're any. */
> if (debugfile.empty () && !has_dwarf2 && !warnings_vector.empty ())
> - for (const std::string &w : warnings_vector)
> - warning ("%s", w.c_str ());
> + for (const auto &cb : warnings_vector)
> + cb ();
> }
>
> return has_dwarf2;
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 373f5592107..2571623dbc8 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1241,10 +1241,25 @@ symbol_file_clear (int from_tty)
>
> bool separate_debug_file_debug = false;
>
> +/* Use callbacks to produce the warnings. */
> +
> +static void
> +add_warning (warnings_vec *vec, std::string filename, std:: string obj_filename)
Again, the comment needs updating, and the strings can be passed as
`const std::string &' I think.
> +{
> + vec->emplace_back ([filename, obj_filename] () {
> + warning (_("the debug information found in \"%ps\""
> + " does not match \"%ps\" (CRC mismatch).\n"),
Should be no `\n` on the end of the warning.
> + styled_string (file_name_style.style (),
> + filename.c_str()),
> + styled_string (file_name_style.style (),
> + obj_filename.c_str()));
You dropped the space before '()' in a couple of places here.
> + });
> +}
> +
> static int
> separate_debug_file_exists (const std::string &name, unsigned long crc,
> struct objfile *parent_objfile,
> - std::vector<std::string> *warnings_vector)
> + warnings_vec *warnings_vector)
> {
> unsigned long file_crc;
> int file_crc_p;
> @@ -1342,8 +1357,13 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
> " does not match \"%s\" (CRC mismatch).\n"),
> name.c_str (), objfile_name (parent_objfile));
> if (separate_debug_file_debug)
> - gdb_printf (gdb_stdlog, "%s", msg.c_str ());
> - warnings_vector->emplace_back (std::move (msg));
> + warning (_("the debug information found in \"%ps\""
> + " does not match \"%ps\" (CRC mismatch).\n"),
> + styled_string (file_name_style.style (),
> + name.c_str()),
> + styled_string (file_name_style.style (),
> + objfile_name (parent_objfile)));
> + add_warning(warnings_vector, name.c_str (), objfile_name (parent_objfile));
Again here, you should use gdb_printf to print the debug output, which
should NOT include styling. And I (personally) would not bother with
the add_warning function.
If you do keep the 'add_warning' then remember - use a space between the
function name and the argument list.
> }
>
> return 0;
> @@ -1386,7 +1406,7 @@ find_separate_debug_file (const char *dir,
> const char *canon_dir,
> const char *debuglink,
> unsigned long crc32, struct objfile *objfile,
> - std::vector<std::string> *warnings_vector)
> + warnings_vec *warnings_vector)
The comment above this function needs updating.
> {
> if (separate_debug_file_debug).
> gdb_printf (gdb_stdlog,
> @@ -1534,7 +1554,7 @@ terminate_after_last_dir_separator (char *path)
>
> std::string
> find_separate_debug_file_by_debuglink
> - (struct objfile *objfile, std::vector<std::string> *warnings_vector)
> + (struct objfile *objfile, warnings_vec *warnings_vector)
> {
> unsigned long crc32;
>
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index b433e2be31a..0cb12176152 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -241,6 +241,9 @@ extern struct objfile *symbol_file_add_from_bfd (const gdb_bfd_ref_ptr &,
> extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
> symfile_add_flags, struct objfile *);
>
> +using warning_cb = std::function<void (void)>;
> +using warnings_vec = std::vector<warning_cb>;
You should add a comment above these types describing what they are for.
> +
> /* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
> Returns pathname, or an empty string.
>
> @@ -248,7 +251,7 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
> WARNINGS_VECTOR, one std::string per warning. */
>
> extern std::string find_separate_debug_file_by_debuglink
> - (struct objfile *objfile, std::vector<std::string> *warnings_vector);
> + (struct objfile *objfile, warnings_vec *warnings_vector);
The comment above this declaration needs updating, it still talks about
the warnings as being strings.
Thanks,
Andrew
>
> /* Build (allocate and populate) a section_addr_info struct from an
> existing section table. */
> --
> 2.39.1
>>>>> "Alexandra" == Alexandra Hájková via Gdb-patches <gdb-patches@sourceware.org> writes:
Alexandra> This improves commit 6647f05df023b63bbe056e9167e9e234172fa2ca, the
Alexandra> filenames used in the warnings are styled properly now.
Alexandra> +/* Use callbacks to produce the warnings. */
Alexandra> +
Alexandra> +static void
Alexandra> +add_warning (warnings_vec *vec, std::string filename)
Alexandra> +{
Alexandra> + vec->emplace_back ([filename] () {
Alexandra> + warning (_("\"%ps\": separate debug info file has no debug info"),
Alexandra> + styled_string (file_name_style.style (),
Alexandra> + filename.c_str()));
Alexandra> + });
This works but it seems like it would be a lot simpler to just use a
ui_file rather than introduce a vector of callback functions that emit
warnings.
Tom
@@ -18,6 +18,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "cli/cli-style.h"
#include "bfd.h"
#include "gdb_bfd.h"
#include "build-id.h"
@@ -205,11 +206,23 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
return build_id_to_bfd_suffix (build_id_len, build_id, "");
}
+/* Use callbacks to produce the warnings. */
+
+static void
+add_warning (warnings_vec *vec, std::string filename)
+{
+ vec->emplace_back ([filename] () {
+ warning (_("\"%ps\": separate debug info file has no debug info"),
+ styled_string (file_name_style.style (),
+ filename.c_str()));
+ });
+}
+
/* See build-id.h. */
std::string
find_separate_debug_file_by_buildid (struct objfile *objfile,
- std::vector<std::string> *warnings_vector)
+ warnings_vec *vec)
{
const struct bfd_build_id *build_id;
@@ -232,8 +245,10 @@ find_separate_debug_file_by_buildid (struct objfile *objfile,
= string_printf (_("\"%s\": separate debug info file has no "
"debug info"), bfd_get_filename (abfd.get ()));
if (separate_debug_file_debug)
- gdb_printf (gdb_stdlog, "%s", msg.c_str ());
- warnings_vector->emplace_back (std::move (msg));
+ warning (_("\"%ps\": separate debug info file has no debug info"),
+ styled_string (file_name_style.style (),
+ bfd_get_filename (abfd.get ())));
+ add_warning(vec, bfd_get_filename (abfd.get ()));
}
else if (abfd != NULL)
return std::string (bfd_get_filename (abfd.get ()));
@@ -21,6 +21,7 @@
#define BUILD_ID_H
#include "gdb_bfd.h"
+#include "gdb/symfile.h"
#include "gdbsupport/rsp-low.h"
/* Locate NT_GNU_BUILD_ID from ABFD and return its content. */
@@ -58,7 +59,7 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len,
approach, then any warnings will be printed. */
extern std::string find_separate_debug_file_by_buildid
- (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+ (struct objfile *objfile, warnings_vec *vec);
/* Return an hex-string representation of BUILD_ID. */
@@ -734,7 +734,7 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
/* Try to add separate debug file if no symbols table found. */
if (!objfile->has_partial_symbols ())
{
- std::vector<std::string> warnings_vector;
+ warnings_vec warnings_vector;
std::string debugfile
= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
@@ -752,8 +752,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
/* If all the methods to collect the debuginfo failed, print any
warnings that were collected. */
if (debugfile.empty () && !warnings_vector.empty ())
- for (const std::string &w : warnings_vector)
- warning ("%s", w.c_str ());
+ for (const auto &cb : warnings_vector)
+ cb ();
}
}
@@ -1213,7 +1213,7 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
&& objfile->separate_debug_objfile == NULL
&& objfile->separate_debug_objfile_backlink == NULL)
{
- std::vector<std::string> warnings_vector;
+ warnings_vec warnings_vector;
std::string debugfile
= find_separate_debug_file_by_buildid (objfile, &warnings_vector);
@@ -1265,8 +1265,8 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
/* If all the methods to collect the debuginfo failed, print
the warnings, if there're any. */
if (debugfile.empty () && !has_dwarf2 && !warnings_vector.empty ())
- for (const std::string &w : warnings_vector)
- warning ("%s", w.c_str ());
+ for (const auto &cb : warnings_vector)
+ cb ();
}
return has_dwarf2;
@@ -1241,10 +1241,25 @@ symbol_file_clear (int from_tty)
bool separate_debug_file_debug = false;
+/* Use callbacks to produce the warnings. */
+
+static void
+add_warning (warnings_vec *vec, std::string filename, std:: string obj_filename)
+{
+ vec->emplace_back ([filename, obj_filename] () {
+ warning (_("the debug information found in \"%ps\""
+ " does not match \"%ps\" (CRC mismatch).\n"),
+ styled_string (file_name_style.style (),
+ filename.c_str()),
+ styled_string (file_name_style.style (),
+ obj_filename.c_str()));
+ });
+}
+
static int
separate_debug_file_exists (const std::string &name, unsigned long crc,
struct objfile *parent_objfile,
- std::vector<std::string> *warnings_vector)
+ warnings_vec *warnings_vector)
{
unsigned long file_crc;
int file_crc_p;
@@ -1342,8 +1357,13 @@ separate_debug_file_exists (const std::string &name, unsigned long crc,
" does not match \"%s\" (CRC mismatch).\n"),
name.c_str (), objfile_name (parent_objfile));
if (separate_debug_file_debug)
- gdb_printf (gdb_stdlog, "%s", msg.c_str ());
- warnings_vector->emplace_back (std::move (msg));
+ warning (_("the debug information found in \"%ps\""
+ " does not match \"%ps\" (CRC mismatch).\n"),
+ styled_string (file_name_style.style (),
+ name.c_str()),
+ styled_string (file_name_style.style (),
+ objfile_name (parent_objfile)));
+ add_warning(warnings_vector, name.c_str (), objfile_name (parent_objfile));
}
return 0;
@@ -1386,7 +1406,7 @@ find_separate_debug_file (const char *dir,
const char *canon_dir,
const char *debuglink,
unsigned long crc32, struct objfile *objfile,
- std::vector<std::string> *warnings_vector)
+ warnings_vec *warnings_vector)
{
if (separate_debug_file_debug)
gdb_printf (gdb_stdlog,
@@ -1534,7 +1554,7 @@ terminate_after_last_dir_separator (char *path)
std::string
find_separate_debug_file_by_debuglink
- (struct objfile *objfile, std::vector<std::string> *warnings_vector)
+ (struct objfile *objfile, warnings_vec *warnings_vector)
{
unsigned long crc32;
@@ -241,6 +241,9 @@ extern struct objfile *symbol_file_add_from_bfd (const gdb_bfd_ref_ptr &,
extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
symfile_add_flags, struct objfile *);
+using warning_cb = std::function<void (void)>;
+using warnings_vec = std::vector<warning_cb>;
+
/* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
Returns pathname, or an empty string.
@@ -248,7 +251,7 @@ extern void symbol_file_add_separate (const gdb_bfd_ref_ptr &, const char *,
WARNINGS_VECTOR, one std::string per warning. */
extern std::string find_separate_debug_file_by_debuglink
- (struct objfile *objfile, std::vector<std::string> *warnings_vector);
+ (struct objfile *objfile, warnings_vec *warnings_vector);
/* Build (allocate and populate) a section_addr_info struct from an
existing section table. */