[1/4] gdb: use mapped file information to improve debuginfod text
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
When opening a core-file GDB is able to use debuginfod to download the
executable that matches the core-file if GDB can find a build-id for
the executable in the core-file.
In this case GDB calls debuginfod_exec_query to download the
executable and GDB prints a message like:
Downloading executable for /path/to/core-file...
which makes sense in that case.
For a long time GDB has also had the ability to download memory-mapped
files and shared libraries when opening a core-file. However, recent
commits have made these cases more likely to trigger, which is a good
thing, but the messaging from GDB in these cases is not ideal. When
downloading a memory-mapped file GDB prints:
Downloading executable for /path/to/memory-mapped-file
And for a shared library:
Downloading executable for /path/to/libfoo.so
These last two messages could, I think, be improved.
I propose making two changes. First, I suggest instead of using
/path/to/core-file in the first case, we use the name of the
executable that GDB is fetching. This makes the messaging consistent
in that we print the name of the file we're fetching rather than the
name of the file we're fetching something for.
I further propose that we replace 'executable for' with the more
generic word 'file'. The messages will then become:
Downloading file /path/to/exec-file...
Downloading file /path/to/memory-mapped-file...
Downloading file /path/to/libfoo.so...
I think these messages are clearer than what we used to have, and they
are consistent in that we name the thing being downloaded in all
cases.
There is one tiny problem. The first case relies on GDB knowing the
name of the executable it wants to download. The only place we can
currently get that from is, I think, the memory-mapped file list.
[ ASIDE: There is `bfd_core_file_failing_command` which reports the
executable and argument list from the core file, but this
information is not ideal for this task. First, the executable and
arguments are merged into a single string, and second, the string is
a relatively short, fixed length string, so the executable name is
often truncated. For these reasons I don't consider fetching the
executable name using this bfd function as a solution. ]
We do have to consider the case that the core file does not have any
mapped file information. This shouldn't ever be the case for a Linux
target, but it's worth considering.
[ ASIDE: I mention Linux specifically because this only becomes a
problem if we try to do a lookup via debuginfod, which requires that
we have build-ids available. Linux has special support for
embedding build-ids into the core file, but I'm not sure if other
kernels do this. ]
For the unlikely edge case of a core-file that has build-ids, but
doesn't have any mapped file information then I propose that we
synthesis a filename like: 'with build-id xxxxxx'. We would then see
a message like:
Downloading file with build-id xxxxxx...
Where 'xxxxxx' would be replaced by the actual build-id.
This isn't ideal, but I think is good enough, and, as I said, I think
this case is not going to be hit very often, or maybe at all.
We already had some tests that emitted two of the above messages,
which I've updated, these cover the mapped-file and shared library
case.
The message about downloading the exec for the core-file is actually
really hard to trigger now as usually the exec will also appear in the
memory-mapped file list and GDB will download the file at this stage.
Then when GDB needs the executable for loading the symbols it'll ask
debuginfod, and debuginfod will find the file in its cache, and so no
message will be printed.
If anyone has any ideas about how to trigger this case then I'm happy
to add additional tests.
---
gdb/corelow.c | 59 ++++++++++++++++++-
gdb/debuginfod-support.c | 4 +-
.../gdb.debuginfod/corefile-mapped-file.exp | 2 +-
.../gdb.debuginfod/solib-with-soname.exp | 2 +-
4 files changed, 60 insertions(+), 7 deletions(-)
Comments
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
Andrew> For the unlikely edge case of a core-file that has build-ids, but
Andrew> doesn't have any mapped file information then I propose that we
Andrew> synthesis a filename like: 'with build-id xxxxxx'. We would then see
Andrew> a message like:
Andrew> Downloading file with build-id xxxxxx...
Andrew> Where 'xxxxxx' would be replaced by the actual build-id.
Andrew> This isn't ideal, but I think is good enough, and, as I said, I think
Andrew> this case is not going to be hit very often, or maybe at all.
I actually think this is better, because it is more precise about what
gdb is doing.
Anyway this looks fine to me.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
@@ -255,6 +255,27 @@ class core_target final : public process_stratum_target
return m_mapped_file_info.lookup (filename, addr);
}
+ /* Return a string containing the expected executable filename obtained
+ from the mapped file information within the core file. The filename
+ returned will be for the mapped file whose ELF headers are mapped at
+ the lowest address (i.e. which GDB encounters first).
+
+ If no suitable filename can be found then the returned string will be
+ empty.
+
+ If there are no build-ids embedded into the core file then the
+ returned string will be empty.
+
+ If a non-empty string is returned then there is no guarantee that the
+ named file exists on disk, or if it does exist on disk, then the
+ on-disk file might have a different build-id to the desired
+ build-id. */
+ const std::string &
+ expected_exec_filename () const
+ {
+ return m_expected_exec_filename;
+ }
+
private: /* per-core data */
/* Get rid of the core inferior. */
@@ -289,6 +310,11 @@ class core_target final : public process_stratum_target
/* FIXME: kettenis/20031023: Eventually this field should
disappear. */
struct gdbarch *m_core_gdbarch = NULL;
+
+ /* If not empty then this contains the name of the executable discovered
+ when processing the memory-mapped file information. This will only
+ be set if we find a mapped with a suitable build-id. */
+ std::string m_expected_exec_filename;
};
core_target::core_target ()
@@ -418,11 +444,23 @@ core_target::build_file_mappings ()
}
});
+ /* Get the build-id of the core file. */
+ const bfd_build_id *core_build_id
+ = build_id_bfd_get (current_program_space->core_bfd ());
+
for (const auto &iter : mapped_files)
{
const std::string &filename = iter.first;
const mapped_file &file_data = iter.second;
+ /* If this mapped file has the same build-id as was discovered for
+ the core-file itself, then we assume this is the main
+ executable. Record the filename as we can use this later. */
+ if (file_data.build_id != nullptr
+ && m_expected_exec_filename.empty ()
+ && build_id_equal (file_data.build_id, core_build_id))
+ m_expected_exec_filename = filename;
+
/* Use exec_file_find() to do sysroot expansion. It'll
also strip the potential sysroot "target:" prefix. If
there is no sysroot, an equivalent (possibly more
@@ -832,14 +870,29 @@ rename_vmcore_idle_reg_sections (bfd *abfd, inferior *inf)
BFD ABFD. */
static void
-locate_exec_from_corefile_build_id (bfd *abfd, int from_tty)
+locate_exec_from_corefile_build_id (bfd *abfd, core_target *target,
+ int from_tty)
{
const bfd_build_id *build_id = build_id_bfd_get (abfd);
if (build_id == nullptr)
return;
+ /* The filename used for the find_objfile_by_build_id call. */
+ std::string filename;
+
+ if (!target->expected_exec_filename ().empty ())
+ filename = target->expected_exec_filename ();
+ else
+ {
+ /* We didn't find an executable name from the mapped file
+ information, so as a stand-in build a string based on the
+ build-id. */
+ std::string build_id_hex_str = bin2hex (build_id->data, build_id->size);
+ filename = string_printf ("with build-id %s", build_id_hex_str.c_str ());
+ }
+
gdb_bfd_ref_ptr execbfd
- = find_objfile_by_build_id (build_id, abfd->filename);
+ = find_objfile_by_build_id (build_id, filename.c_str ());
if (execbfd != nullptr)
{
@@ -972,7 +1025,7 @@ core_target_open (const char *arg, int from_tty)
if (current_program_space->exec_bfd () == nullptr)
locate_exec_from_corefile_build_id (current_program_space->core_bfd (),
- from_tty);
+ target, from_tty);
post_create_inferior (from_tty);
@@ -409,7 +409,7 @@ debuginfod_exec_query (const unsigned char *build_id,
std::optional<target_terminal::scoped_restore_terminal_state> term_state;
{
- user_data data ("executable for", filename);
+ user_data data ("file", filename);
debuginfod_set_user_data (c, &data);
if (target_supports_terminal_ours ())
@@ -423,7 +423,7 @@ debuginfod_exec_query (const unsigned char *build_id,
debuginfod_set_user_data (c, nullptr);
}
- print_outcome (fd.get (), "executable for", filename);
+ print_outcome (fd.get (), "file", filename);
if (fd.get () >= 0)
destname->reset (dname);
@@ -370,7 +370,7 @@ with_debuginfod_env $cache {
gdb_load $binfile
load_core_file "load corefile, download library from debuginfod" \
- "Downloading\[^\r\n\]* executable for [string_to_regexp $library_filename]\\.\\.\\."
+ "Downloading\[^\r\n\]* file [string_to_regexp $library_filename]\\.\\.\\."
set ptr_value [read_ptr_value]
gdb_assert { $ptr_value == $ptr_expected_value } \
@@ -161,7 +161,7 @@ proc load_exec_and_core_file { expect_warning expect_download testname \
set saw_warning true
exp_continue
}
- -re "^Downloading executable for \[^\r\n\]+/libfoo_1\\.so\\.\\.\\.\r\n" {
+ -re "^Downloading file \[^\r\n\]+/libfoo_1\\.so\\.\\.\\.\r\n" {
set saw_download true
exp_continue
}