From patchwork Fri May 17 14:19:00 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 90380 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B41E13849AE2 for ; Fri, 17 May 2024 14:20:07 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id A3622384AB75 for ; Fri, 17 May 2024 14:19:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A3622384AB75 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org A3622384AB75 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715955565; cv=none; b=b3MEkIjxoJy1u+kdQzczYFYZdyhwx5YcYxZNCPXbce459rpvg5F4pkrcZQEDyhVdUohHh7iDVGtODp1SU/znFoWJFtH5Lh3ugG4zqO6vldXJSlaCds3PHsmch0QhcbAQRniV165NwvJhuBP9sCccpqsYWfdmiUTtvXXpo8W62Bg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1715955565; c=relaxed/simple; bh=A3K/n+38E1UwO9qQZRWea6cttrDWKIRWun7gY3VdFa0=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=rxgta2scV9lJLIx3ed2nw6Wj35dij4/OwElLbPgIJUjbCs+jUZHMc5JjcDm0eEd42hpughG0W6fkGTu2b7i19PNG8oUVIsc7XIdu0wXVlZVL27C8yo80w3mU3guzWMG4Dys6/Cy1iZfeanKTuJVvAnS3NwDBFsoDoHi5R3wwUMs= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715955559; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FdNEb3FrFaMjZn2IK4QEilsFytE8MDVQjyLnI54nPqk=; b=b6yTinYyqtSv/i2NtkgF11oyP+a+XOWSLWW28Mie764WzecDz3jRsreomUI+f7Af89M2op kClc5Z3fyqpc4e/BoFpeHl4OlyUVLZO7W9Tbl+uzP39j7IG4+UFJsSCN43nZibHQC9m7bK FR0pVfhZw8IQfZ7FbwLNbcggIe64G84= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-133-6IY5A5-eN2e_6Y2K55Z_Lw-1; Fri, 17 May 2024 10:19:17 -0400 X-MC-Unique: 6IY5A5-eN2e_6Y2K55Z_Lw-1 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-34f1b148725so4203504f8f.2 for ; Fri, 17 May 2024 07:19:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715955556; x=1716560356; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=FdNEb3FrFaMjZn2IK4QEilsFytE8MDVQjyLnI54nPqk=; b=qaHEl3zJk1TxM2etNCSzFUrGpmPTFTF1x9WaxaucbFeT2ylUqbKBIb8fj9vKMy8JXE rIQ+Egm0AZTYn+6TYGnQSDqvk4VuPH1wC+nEhsAXR7tSFzOEhN7HpdoWbuPnthNObDA8 jMSAqWugCeRRZZAkoI4tQH8TzSOKRREG40eV+41h3rUqYSnivkjViDtVvszpWv6gvaF8 BxiAFRE057NKn+K8tRdm8dJzNF9ex4vyatNZrD+yDoervNqTPHvkQ8ggINFLtdq6DJXa KQG3LIlgVDoNVLqoM683U4J27i+dNq7H3EBfj6KE5MfK/7TUYlVwnMohY9Bi2TdctRoc YUuA== X-Gm-Message-State: AOJu0YwNDq9gncBL9QdPM4l6mDQnvLdnbra0dlYozUFXA4qhY+QTfXVq xAAkHup1HNp2OHENagcnEHoQsuYLB+bPsUSXkCRC1AaUTynFKR+Oh0Pxryjw6V9TDBZBObgC4Ev bEcXsCH6zjY7zqW/HV2VDmQ9FlkhnD1WEai/ecsG53v25ul8MJDw03jxwF/mSfMELabgi5dFUtx IHiX9WjzjqOxkTW0DwAcIep4XK4QKt7hf14vOl09wNwjs= X-Received: by 2002:a5d:4537:0:b0:351:cb0a:5da9 with SMTP id ffacd0b85a97d-351cb0a5e27mr7161090f8f.54.1715955555824; Fri, 17 May 2024 07:19:15 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGhBQgEF07hC7gQENYTROUrLdKKeh2JuM/vOp11posgeAvQBQmoQIrHq5eLsvvvaaN4j19zzA== X-Received: by 2002:a5d:4537:0:b0:351:cb0a:5da9 with SMTP id ffacd0b85a97d-351cb0a5e27mr7161063f8f.54.1715955554969; Fri, 17 May 2024 07:19:14 -0700 (PDT) Received: from localhost ([31.111.84.240]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3502baad042sm21802755f8f.80.2024.05.17.07.19.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 May 2024 07:19:14 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 4/4] gdb: unify build-id to objfile lookup code Date: Fri, 17 May 2024 15:19:00 +0100 Message-Id: <2fe048273cfc7f0d3581b9abcff46a73e37899db.1715955328.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org There are 3 places where we currently call debuginfod_exec_query to lookup an objfile for a given build-id. In one of these places we first call build_id_to_exec_bfd which also looks up an objfile given a build-id, but this function looks on disk for a symlink in the .build-id/ sub-directory (within the debug-file-directory). I can't think of any reason why we shouldn't call build_id_to_exec_bfd before every call to debuginfod_exec_query. So, in this commit I have added a new function in build-id.c, find_exec_by_build_id, this function calls build_id_to_exec_bfd, and if that fails, then calls debuginfod_exec_query. Everywhere we call debuginfod_exec_query is updated to call the new function, and in locate_exec_from_corefile_build_id, the existing call to build_id_to_exec_bfd is removed as calling find_exec_by_build_id does this for us. One slight weird thing is in core_target::build_file_mappings, here we call find_exec_by_build_id which returns a gdb_bfd_ref_ptr for the opened file, however we immediately reopen the file as "binary". The reason for this is that all the bfds opened in ::build_file_mappings need to be opened as "binary" (see the function comments for why). I did consider passing a target type into find_exec_by_build_id, which could then be forwarded to build_id_to_exec_bfd and used to open the BFD as "binary", however, if you follow the call chain you'll end up in build_id_to_debug_bfd_1, where we actually open the bfd. Notice in here that we call build_id_verify to double check the build-id of the file we found, this requires that the bfd not be opened as "binary". What this means is that we always have to first open the bfd using the gnutarget target type (for the build-id check), and then we would have to reopen it as "binary". There seems little point pushing the reopen logic into find_exec_by_build_id, so we just do this in the ::build_file_mappings function. I've extended the tests to cover the two cases which actually changed in this commit. --- gdb/build-id.c | 42 ++++++++++++++++++- gdb/build-id.h | 21 ++++++---- gdb/corelow.c | 40 ++++++------------ gdb/solib.c | 22 ++++------ .../gdb.debuginfod/corefile-mapped-file.exp | 31 ++++++++++++++ .../gdb.debuginfod/solib-with-soname.exp | 28 ++++++++++++- gdb/testsuite/lib/gdb.exp | 7 +++- 7 files changed, 137 insertions(+), 54 deletions(-) diff --git a/gdb/build-id.c b/gdb/build-id.c index 41667d5e5cf..27642b58d56 100644 --- a/gdb/build-id.c +++ b/gdb/build-id.c @@ -26,6 +26,8 @@ #include "filenames.h" #include "gdbcore.h" #include "cli/cli-style.h" +#include "gdbsupport/scoped_fd.h" +#include "debuginfod-support.h" /* See build-id.h. */ @@ -198,9 +200,11 @@ build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id) return build_id_to_bfd_suffix (build_id_len, build_id, ".debug"); } -/* See build-id.h. */ +/* Find and open a BFD for an executable file given a build-id. If no BFD + can be found, return NULL. The returned reference to the BFD must be + released by the caller. */ -gdb_bfd_ref_ptr +static gdb_bfd_ref_ptr 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, ""); @@ -243,3 +247,37 @@ find_separate_debug_file_by_buildid (struct objfile *objfile, return std::string (); } + +/* See build-id.h. */ + +gdb_bfd_ref_ptr +find_exec_by_build_id (const bfd_build_id *build_id, + const char *expected_filename) +{ + /* Try to find the executable (or shared object) by looking for a + (sym)link on disk from the build-id to the object file. */ + gdb_bfd_ref_ptr abfd = build_id_to_exec_bfd (build_id->size, + build_id->data); + + if (abfd != nullptr) + return abfd; + + /* Attempt to query debuginfod for the executable. */ + gdb::unique_xmalloc_ptr path; + scoped_fd fd = debuginfod_exec_query (build_id->data, build_id->size, + expected_filename, &path); + if (fd.get () >= 0) + { + abfd = gdb_bfd_open (path.get (), gnutarget); + + if (abfd == nullptr) + warning (_("\"%ps\" from debuginfod cannot be opened as bfd: %s"), + styled_string (file_name_style.style (), path.get ()), + gdb_bfd_errmsg (bfd_get_error (), nullptr).c_str ()); + else if (!build_id_verify (abfd.get (), build_id->size, + build_id->data)) + abfd = nullptr; + } + + return abfd; +} diff --git a/gdb/build-id.h b/gdb/build-id.h index c5f20f8782e..3df122a0cbf 100644 --- a/gdb/build-id.h +++ b/gdb/build-id.h @@ -40,13 +40,6 @@ extern int build_id_verify (bfd *abfd, extern gdb_bfd_ref_ptr build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id); -/* Find and open a BFD for an executable file given a build-id. If no BFD - can be found, return NULL. The returned reference to the BFD must be - released by the caller. */ - -extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len, - const bfd_byte *build_id); - /* Find the separate debug file for OBJFILE, by using the build-id associated with OBJFILE's BFD. If successful, returns the file name for the separate debug file, otherwise, return an empty string. @@ -60,6 +53,20 @@ extern gdb_bfd_ref_ptr build_id_to_exec_bfd (size_t build_id_len, extern std::string find_separate_debug_file_by_buildid (struct objfile *objfile, deferred_warnings *warnings); +/* Find an executable (or shared library) that matches BUILD_ID. This is + done by first checking in the debug-file-directory for a .build-id/ + sub-directory, and looking for a symlink in there that points to the + required file. + + If that doesn't find us a file then we call to debuginfod to see if it + can provide the required file. + + EXPECTED_FILENAME is used in output messages from debuginfod, this + should be the file we were looking for but couldn't find. */ + +extern gdb_bfd_ref_ptr find_exec_by_build_id (const bfd_build_id *build_id, + const char *expected_filename); + /* Return an hex-string representation of BUILD_ID. */ static inline std::string diff --git a/gdb/corelow.c b/gdb/corelow.c index 85bc3c26bea..6975719c1f2 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -47,7 +47,6 @@ #include "gdbsupport/pathstuff.h" #include "gdbsupport/scoped_fd.h" #include "gdbsupport/x86-xstate.h" -#include "debuginfod-support.h" #include #include #include "cli/cli-cmds.h" @@ -407,13 +406,19 @@ core_target::build_file_mappings () || !bfd_check_format (abfd.get (), bfd_object)) && file_data.build_id != nullptr) { - expanded_fname = nullptr; - debuginfod_exec_query (file_data.build_id->data, - file_data.build_id->size, - filename.c_str (), &expanded_fname); - if (expanded_fname != nullptr) + abfd = find_exec_by_build_id (file_data.build_id, + filename.c_str ()); + + if (abfd != nullptr) { + /* The find_exec_by_build_id will have opened ABFD using the + GNUTARGET global bfd type, however, we need the bfd opened + as the binary type (see the function's header comment), so + now we reopen ABFD with the desired binary type. */ + expanded_fname + = make_unique_xstrdup (bfd_get_filename (abfd.get ())); struct bfd *b = bfd_openr (expanded_fname.get (), "binary"); + gdb_assert (b != nullptr); abfd = gdb_bfd_ref_ptr::new_reference (b); } } @@ -774,28 +779,7 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty) return; gdb_bfd_ref_ptr execbfd - = build_id_to_exec_bfd (build_id->size, build_id->data); - - if (execbfd == nullptr) - { - /* Attempt to query debuginfod for the executable. */ - gdb::unique_xmalloc_ptr execpath; - scoped_fd fd = debuginfod_exec_query (build_id->data, build_id->size, - abfd->filename, &execpath); - - if (fd.get () >= 0) - { - execbfd = gdb_bfd_open (execpath.get (), gnutarget); - - if (execbfd == nullptr) - warning (_("\"%s\" from debuginfod cannot be opened as bfd: %s"), - execpath.get (), - gdb_bfd_errmsg (bfd_get_error (), nullptr).c_str ()); - else if (!build_id_verify (execbfd.get (), build_id->size, - build_id->data)) - execbfd.reset (nullptr); - } - } + = find_exec_by_build_id (build_id, abfd->filename); if (execbfd != nullptr) { diff --git a/gdb/solib.c b/gdb/solib.c index c39dfbcc78e..3292f361176 100644 --- a/gdb/solib.c +++ b/gdb/solib.c @@ -45,7 +45,6 @@ #include "gdb_bfd.h" #include "gdbsupport/filestuff.h" #include "gdbsupport/scoped_fd.h" -#include "debuginfod-support.h" #include "source.h" #include "cli/cli-style.h" @@ -826,20 +825,15 @@ solib_map_sections (solib &so) abfd = nullptr; if (abfd == nullptr) - { - scoped_fd fd = debuginfod_exec_query - (expected_build_id->data, expected_build_id->size, - so.so_name.c_str (), &filename); + abfd = find_exec_by_build_id (expected_build_id, + so.so_name.c_str ()); - if (fd.get () >= 0) - abfd = ops->bfd_open (filename.get ()); - else if (mismatch) - { - warning (_ ("Build-id of %ps does not match core file."), - styled_string (file_name_style.style (), - filename.get ())); - abfd = nullptr; - } + if (abfd == nullptr && mismatch) + { + warning (_ ("Build-id of %ps does not match core file."), + styled_string (file_name_style.style (), + filename.get ())); + abfd = nullptr; } } } diff --git a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp index c789be87ba3..553d68e4332 100644 --- a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp +++ b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp @@ -230,6 +230,37 @@ set ptr_value [read_ptr_value] gdb_assert { $ptr_value eq "unavailable" } \ "check value of pointer is unavailable with library file missing" +# Now symlink the .build-id/xx/xxx...xxx filename within the debug +# directory to library we just moved aside. Restart GDB and setup the +# debug-file-directory before loading the core file. +# +# GDB should lookup the file to map via the build-id link in the +# .build-id/ directory. +set debugdir [standard_output_file "debugdir"] +set build_id_filename \ + $debugdir/[build_id_debug_filename_get $library_backup_filename ""] + +remote_exec host "mkdir -p [file dirname $build_id_filename]" +remote_exec host "ln -sf $library_backup_filename $build_id_filename" + +clean_restart $binfile + +gdb_test_no_output "set debug-file-directory $debugdir" \ + "set debug-file-directory" + +gdb_test "core-file $corefile" \ + [multi_line \ + "\\\[\[^\r\n\]+\\\]" \ + "Core was generated by \[^\r\n\]+" \ + "Program terminated with signal SIGSEGV, Segmentation fault\\." \ + "#0 main \\(\\) at \[^\r\n\]+" \ + "$decimal\\s+\[^\r\n\]+/\\* Undefined behaviour here\\. \\*/"] \ + "load corefile, lookup in debug-file-directory" + +set ptr_value [read_ptr_value] +gdb_assert { $ptr_value == $ptr_expected_value } \ + "check value of pointer variable from core-file, lookup in debug-file-directory" + # Build a new version of the shared library, keep the library the same size, # but change the contents so the build-id changes. Then restart GDB and load # the core-file again. GDB should spot that the build-id for the shared diff --git a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp index dfc78923436..9d311be8ebc 100644 --- a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp +++ b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp @@ -106,10 +106,15 @@ if {$corefile eq ""} { # unable to load the shared library symbols, otherwise, EXPECT_WARNING # is false and we expect no warnings about loading shared library # symbols. -proc load_exec_and_core_file { expect_warning testname } { +proc load_exec_and_core_file { expect_warning testname {debugdir ""}} { with_test_prefix $testname { clean_restart $::binfile + if { $debugdir ne "" } { + gdb_test_no_output "set debug-file-directory $debugdir" \ + "set debug directory" + } + set saw_warning false gdb_test_multiple "core-file $::corefile" "load core file" { -re "^core-file \[^\r\n\]+\r\n" { @@ -205,6 +210,27 @@ gdb_assert { [lindex $status 0] == 0 } \ load_exec_and_core_file true \ "load core file, libfoo_1.so removed" +# Symlink the .build-id/xx/xxx...xxx filename within the debug +# directory to LIBRARY_1_BACKUP_FILENAME, now when we restart GDB it +# should find the missing library within the debug directory. +set debugdir [standard_output_file "debugdir"] +set build_id_filename \ + $debugdir/[build_id_debug_filename_get $library_1_backup_filename ""] +set status \ + [remote_exec host \ + "mkdir -p [file dirname $build_id_filename]"] +gdb_assert { [lindex $status 0] == 0 } \ + "create sub-directory within the debug directory" +set status \ + [remote_exec host \ + "ln -sf $library_1_backup_filename $build_id_filename"] +gdb_assert { [lindex $status 0] == 0 } \ + "create symlink within the debug directory " + +load_exec_and_core_file false \ + "load core file, find libfoo_1.so through debug-file-directory" \ + $debugdir + # Setup a debuginfod server which can serve the original shared # library file. if {![allow_debuginfod_tests]} { diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index c958ff18d2a..e369b0be96a 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -8019,14 +8019,17 @@ proc get_build_id { filename } { # Return the build-id hex string (usually 160 bits as 40 hex characters) # converted to the form: .build-id/ab/cdef1234...89.debug +# +# The '.debug' suffix can be changed by passing the SUFFIX argument. +# # Return "" if no build-id found. -proc build_id_debug_filename_get { filename } { +proc build_id_debug_filename_get { filename {suffix ".debug"} } { set data [get_build_id $filename] if { $data == "" } { return "" } regsub {^..} $data {\0/} data - return ".build-id/${data}.debug" + return ".build-id/${data}${suffix}" } # DEST should be a file compiled with debug information. This proc