[2/5] gdb: merge debug symbol file lookup code from coffread & elfread paths
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
fail
|
Testing failed
|
Commit Message
This commit merges the code that looks for and loads the separate
debug symbol files from coffread.c and elfread.c. The factored out
code is moved into a new objfile::find_and_add_separate_symbol_file()
method.
For the elfread.c path there should be no user visible changes after
this commit.
For the coffread.c path GDB will now attempt to perform a debuginfod
lookup for the missing debug information, assuming that GDB can find a
build-id in the COFF file.
I don't know if COFF files can include a build-id, but I the existing
coffread.c code already includes a call to
find_separate_debug_file_by_build-id, so I know that it is at least OK
for GDB to ask a COFF file for a build-id. If the COFF file doesn't
include a build-id then the debuginfod lookup code will not trigger
and the new code is harmless.
If the COFF file does include a build-id, then we're going to end up
asking debuginfod for the debug file. As build-ids should be unique,
this should be harmless, even if debuginfod doesn't contain any
suitable debug data, it just costs us one debuginfod lookup, so I'm
not too worried about this for now.
I don't have access to a COFF target right now, so beyond compiling
it, the coffread.c changes are completely untested.
---
gdb/coffread.c | 24 ++---------------
gdb/elfread.c | 57 +++------------------------------------
gdb/objfiles.h | 10 +++++++
gdb/symfile-debug.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 81 insertions(+), 76 deletions(-)
Comments
On 18/10/2023 11:53, Andrew Burgess wrote:
> This commit merges the code that looks for and loads the separate
> debug symbol files from coffread.c and elfread.c. The factored out
> code is moved into a new objfile::find_and_add_separate_symbol_file()
> method.
>
> For the elfread.c path there should be no user visible changes after
> this commit.
>
> For the coffread.c path GDB will now attempt to perform a debuginfod
> lookup for the missing debug information, assuming that GDB can find a
> build-id in the COFF file.
>
> I don't know if COFF files can include a build-id, but I the existing
This is at least possible for PE/COFF.
(I wrote the code to add support for this back in 2015, see commit
c74f7d1c6c5a968330208757f476c67a4bb66643)
> coffread.c code already includes a call to
> find_separate_debug_file_by_build-id, so I know that it is at least OK
> for GDB to ask a COFF file for a build-id. If the COFF file doesn't
> include a build-id then the debuginfod lookup code will not trigger
> and the new code is harmless.
>
> If the COFF file does include a build-id, then we're going to end up
> asking debuginfod for the debug file. As build-ids should be unique,
> this should be harmless, even if debuginfod doesn't contain any
> suitable debug data, it just costs us one debuginfod lookup, so I'm
> not too worried about this for now.
But yes, as you say, should be harmless.
> I don't have access to a COFF target right now, so beyond compiling
> it, the coffread.c changes are completely untested.
Jon Turney <jon.turney@dronecode.org.uk> writes:
> On 18/10/2023 11:53, Andrew Burgess wrote:
>> This commit merges the code that looks for and loads the separate
>> debug symbol files from coffread.c and elfread.c. The factored out
>> code is moved into a new objfile::find_and_add_separate_symbol_file()
>> method.
>>
>> For the elfread.c path there should be no user visible changes after
>> this commit.
>>
>> For the coffread.c path GDB will now attempt to perform a debuginfod
>> lookup for the missing debug information, assuming that GDB can find a
>> build-id in the COFF file.
>>
>> I don't know if COFF files can include a build-id, but I the existing
>
> This is at least possible for PE/COFF.
>
> (I wrote the code to add support for this back in 2015, see commit
> c74f7d1c6c5a968330208757f476c67a4bb66643)
Thanks for the link. I figured they probably could based on the code,
but I didn't dig too hard as I didn't think it really mattered for this
change.
Thanks,
Andrew
>
>> coffread.c code already includes a call to
>> find_separate_debug_file_by_build-id, so I know that it is at least OK
>> for GDB to ask a COFF file for a build-id. If the COFF file doesn't
>> include a build-id then the debuginfod lookup code will not trigger
>> and the new code is harmless.
>>
>> If the COFF file does include a build-id, then we're going to end up
>> asking debuginfod for the debug file. As build-ids should be unique,
>> this should be harmless, even if debuginfod doesn't contain any
>> suitable debug data, it just costs us one debuginfod lookup, so I'm
>> not too worried about this for now.
>
> But yes, as you say, should be harmless.
>
>> I don't have access to a COFF target right now, so beyond compiling
>> it, the coffread.c changes are completely untested.
@@ -40,8 +40,6 @@
#include "coff-pe-read.h"
-#include "build-id.h"
-
/* The objfile we are currently reading. */
static struct objfile *coffread_objfile;
@@ -729,26 +727,8 @@ coff_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
&& objfile->separate_debug_objfile == NULL
&& objfile->separate_debug_objfile_backlink == NULL)
{
- deferred_warnings warnings;
- std::string debugfile
- = find_separate_debug_file_by_buildid (objfile, &warnings);
-
- if (debugfile.empty ())
- debugfile
- = find_separate_debug_file_by_debuglink (objfile, &warnings);
-
- if (!debugfile.empty ())
- {
- gdb_bfd_ref_ptr debug_bfd (symfile_bfd_open (debugfile.c_str ()));
-
- symbol_file_add_separate (debug_bfd, debugfile.c_str (),
- symfile_flags, objfile);
- }
- /* If all the methods to collect the debuginfo failed, print any
- warnings that were collected, this is a no-op if there are no
- warnings. */
- if (debugfile.empty ())
- warnings.emit ();
+ if (objfile->find_and_add_separate_symbol_file (symfile_flags))
+ gdb_assert (objfile->separate_debug_objfile != nullptr);
}
}
@@ -41,14 +41,12 @@
#include "regcache.h"
#include "bcache.h"
#include "gdb_bfd.h"
-#include "build-id.h"
#include "location.h"
#include "auxv.h"
#include "mdebugread.h"
#include "ctfread.h"
#include "gdbsupport/gdb_string_view.h"
#include "gdbsupport/scoped_fd.h"
-#include "debuginfod-support.h"
#include "dwarf2/public.h"
#include "cli/cli-cmds.h"
@@ -1218,59 +1216,10 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
&& objfile->separate_debug_objfile == NULL
&& objfile->separate_debug_objfile_backlink == NULL)
{
- deferred_warnings warnings;
-
- std::string debugfile
- = find_separate_debug_file_by_buildid (objfile, &warnings);
-
- if (debugfile.empty ())
- debugfile = find_separate_debug_file_by_debuglink (objfile, &warnings);
-
- if (!debugfile.empty ())
- {
- gdb_bfd_ref_ptr debug_bfd
- (symfile_bfd_open_no_error (debugfile.c_str ()));
-
- if (debug_bfd != nullptr)
- symbol_file_add_separate (debug_bfd, debugfile.c_str (),
- symfile_flags, objfile);
- }
+ if (objfile->find_and_add_separate_symbol_file (symfile_flags))
+ gdb_assert (objfile->separate_debug_objfile != nullptr);
else
- {
- has_dwarf2 = false;
- const struct bfd_build_id *build_id
- = build_id_bfd_get (objfile->obfd.get ());
- const char *filename = bfd_get_filename (objfile->obfd.get ());
-
- if (build_id != nullptr)
- {
- gdb::unique_xmalloc_ptr<char> symfile_path;
- scoped_fd fd (debuginfod_debuginfo_query (build_id->data,
- build_id->size,
- filename,
- &symfile_path));
-
- if (fd.get () >= 0)
- {
- /* File successfully retrieved from server. */
- gdb_bfd_ref_ptr debug_bfd
- (symfile_bfd_open_no_error (symfile_path.get ()));
-
- if (debug_bfd != nullptr
- && build_id_verify (debug_bfd.get (), build_id->size,
- build_id->data))
- {
- symbol_file_add_separate (debug_bfd, symfile_path.get (),
- symfile_flags, objfile);
- has_dwarf2 = true;
- }
- }
- }
- }
- /* 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 ();
+ has_dwarf2 = false;
}
return has_dwarf2;
@@ -513,6 +513,16 @@ struct objfile
bool has_partial_symbols ();
+ /* Look for a separate debug symbol file for this objfile, make use of
+ build-id, debug-link, and debuginfod as necessary. If a suitable
+ separate debug symbol file is found then it is loaded using a call to
+ symbol_file_add_separate (SYMFILE_FLAGS is passed through unmodified
+ to this call) and this function returns true. If no suitable separate
+ debug symbol file is found and loaded then this function returns
+ false. */
+
+ bool find_and_add_separate_symbol_file (symfile_add_flags symfile_flags);
+
/* Return true if this objfile has any unexpanded symbols. A return
value of false indicates either, that this objfile has all its
symbols fully expanded (i.e. fully read in), or that this objfile has
@@ -35,6 +35,8 @@
#include "block.h"
#include "filenames.h"
#include "cli/cli-style.h"
+#include "build-id.h"
+#include "debuginfod-support.h"
/* We need to save a pointer to the real symbol functions.
Plus, the debug versions are malloc'd because we have to NULL out the
@@ -558,6 +560,70 @@ objfile::require_partial_symbols (bool verbose)
}
}
+/* See objfiles.h. */
+
+bool
+objfile::find_and_add_separate_symbol_file (symfile_add_flags symfile_flags)
+{
+ bool has_dwarf2 = true;
+
+ deferred_warnings warnings;
+
+ std::string debugfile
+ = find_separate_debug_file_by_buildid (this, &warnings);
+
+ if (debugfile.empty ())
+ debugfile = find_separate_debug_file_by_debuglink (this, &warnings);
+
+ if (!debugfile.empty ())
+ {
+ gdb_bfd_ref_ptr debug_bfd
+ (symfile_bfd_open_no_error (debugfile.c_str ()));
+
+ if (debug_bfd != nullptr)
+ symbol_file_add_separate (debug_bfd, debugfile.c_str (),
+ symfile_flags, this);
+ }
+ else
+ {
+ has_dwarf2 = false;
+ const struct bfd_build_id *build_id
+ = build_id_bfd_get (this->obfd.get ());
+ const char *filename = bfd_get_filename (this->obfd.get ());
+
+ if (build_id != nullptr)
+ {
+ gdb::unique_xmalloc_ptr<char> symfile_path;
+ scoped_fd fd (debuginfod_debuginfo_query (build_id->data,
+ build_id->size,
+ filename,
+ &symfile_path));
+
+ if (fd.get () >= 0)
+ {
+ /* File successfully retrieved from server. */
+ gdb_bfd_ref_ptr debug_bfd
+ (symfile_bfd_open_no_error (symfile_path.get ()));
+
+ if (debug_bfd != nullptr
+ && build_id_verify (debug_bfd.get (), build_id->size,
+ build_id->data))
+ {
+ symbol_file_add_separate (debug_bfd, symfile_path.get (),
+ symfile_flags, this);
+ has_dwarf2 = true;
+ }
+ }
+ }
+ }
+ /* 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;
+}
+
/* Debugging version of struct sym_probe_fns. */