From patchwork Mon May 20 13:08:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 90484 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 4DFE93858D34 for ; Mon, 20 May 2024 13:10:13 +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 DC54D3858C53 for ; Mon, 20 May 2024 13:08:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DC54D3858C53 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 DC54D3858C53 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=1716210524; cv=none; b=hK77Vzlhs5QkK3Op/ruLicvrRLic0AExXKiO2ibivCw/CfizJoCMoKS4R4zVdSymFSZ3OWNkZrvIfdevj8N+7wKWpxE7v20LSKrwtu66HKCn6ezrg2yvtll2Y6SEMjzXIPGZ2JL8nmqmzc9UkzPNhs0KQokuF/6SFchhv/Vg+hE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716210524; c=relaxed/simple; bh=NgzeSHLteNf6HLoaMgmNgmXNcRCkNmvkHD6FW+By+dQ=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=DcnQGkDGj9AbehCso9CJUViNrAMlQiSE9MzADYE+myt7f6hIAyVxJFzjAFDjB1tuJVEwhOgBr/xN+QT1ZiQaRwPYr/nvpMHGSh8SDyZtLO2VuRzSnQ15CJs9ZxIpZ8p1ldWKuMOamE/4/23CgcnAi42/7TfSnG1QXD9032EHs2A= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1716210520; 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=uLiQRyXwITKF+xL2FN4HcKRegGB/r/oGZkKhd2WMYE4=; b=YqoI2IYclRLz0HJ4vhQkeGr6FbpiJOneq67edME5hZdTHAoh7ymb+E334mzbyMyypddsiw pPlNjL+kSYAi2yC9gUhhVsYYCNr8+P7QMHSpwlBO7vuvohqItdwdW1Xa3RtVxPPc7keWBo 7L8mihbG5quQSq5RUs8ybx730UzskrA= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-35-1bRBW4AZPqm5LEDn8YpFeg-1; Mon, 20 May 2024 09:08:38 -0400 X-MC-Unique: 1bRBW4AZPqm5LEDn8YpFeg-1 Received: by mail-lf1-f70.google.com with SMTP id 2adb3069b0e04-523936877dbso4530646e87.2 for ; Mon, 20 May 2024 06:08:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716210516; x=1716815316; 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=uLiQRyXwITKF+xL2FN4HcKRegGB/r/oGZkKhd2WMYE4=; b=UK0Gytg5J9tQjDaL/gr1jbaCCHNj0n84GzNFyCmtI+sCI0Jg7yweVLIZUfaKwCRlhZ UYmBP7vff9sUfCLg3ZTrn/qMxmc40lnIIkMhxhpGs/TuykmVFjeInIUefwAzvab6/mt4 L5/n/f5u1OJNOx7w/PvDr1Vagwld/Pit7oPZ/r1NW64YxHSUja0zTB7WG4+U/Dmi1rgx xc9VAtR4VvaS3OBjskbpOAxW0pE+24dg8fRI6BtYKU1i9hw/IohlVeNXdpvl0QFXrWyL dNWaXQTFNPAkT4Vr/xAbkyxLlnXhH5igRgxvdjWdpjkAFeQQ9btQddFdB3KWxU5pOgbj 2iBw== X-Gm-Message-State: AOJu0YwAPw6IiWkyoUqNOuTYMacu8d4nwAOVznujCo/j0nEU67y52ucF xUopTlYPp9Fw8l58yU3oRKJoLrIibRtpw500P4FE7AgvoJ0uvv+Me43IS2prz7mKh1KaxRlfYli GX5Kgx0Kjhcb25H0Kor6AX6iDfUoN47j6faJ9g3sby/GZYMSAokooTyp23QIR+X0pu4uaT5Q6RA HQQ1DjCS9rgc9qDqK6nizzW33lxoo58J38avIfE+WJBuQ= X-Received: by 2002:a19:5f5a:0:b0:51e:1146:16fd with SMTP id 2adb3069b0e04-5220fb768c0mr19551594e87.9.1716210515943; Mon, 20 May 2024 06:08:35 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH+BH7f5B1HBfbEYVItKNeZx1J77s17GtFay+F65bpz3pTVkmqwPX3QRPyS8f+c4xc1SQ765Q== X-Received: by 2002:a19:5f5a:0:b0:51e:1146:16fd with SMTP id 2adb3069b0e04-5220fb768c0mr19551564e87.9.1716210515228; Mon, 20 May 2024 06:08:35 -0700 (PDT) Received: from localhost ([31.111.84.240]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-574bcad0362sm11621270a12.20.2024.05.20.06.08.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 May 2024 06:08:34 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv2 4/4] gdb: unify build-id to objfile lookup code Date: Mon, 20 May 2024 14:08:27 +0100 Message-Id: <2bc90e98483e0849b627cd2ca6de5f775e24d702.1716210406.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=-11.8 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 | 24 +++++++++++ .../gdb.debuginfod/solib-with-soname.exp | 32 +++++++++++++- gdb/testsuite/lib/gdb.exp | 7 +++- 7 files changed, 134 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 6e3301e1c8d..b5dee228ca0 100644 --- a/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp +++ b/gdb/testsuite/gdb.debuginfod/corefile-mapped-file.exp @@ -296,6 +296,30 @@ 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 build "mkdir -p [file dirname $build_id_filename]" +remote_exec build "ln -sf $library_backup_filename $build_id_filename" + +clean_restart $binfile + +gdb_test_no_output "set debug-file-directory $debugdir" \ + "set debug-file-directory" + +load_core_file "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 44c4268edd3..cdd9730a22c 100644 --- a/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp +++ b/gdb/testsuite/gdb.debuginfod/solib-with-soname.exp @@ -126,10 +126,19 @@ if {$corefile eq ""} { # If EXPECT_DOWNLOAD is true then we require a line indicating that # the shared library is being downloaded from debuginfod, otherwise # the shared library should not be downloaded. -proc load_exec_and_core_file { expect_warning expect_download testname } { +# +# If DEBUGDIR is not the empty string then 'debug-file-directory' is +# set to the value of DEBUGDIR. +proc load_exec_and_core_file { expect_warning expect_download 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 set saw_download false set saw_generated false @@ -223,6 +232,27 @@ gdb_assert { [lindex $status 0] == 0 } \ load_exec_and_core_file true false \ "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 build \ + "mkdir -p [file dirname $build_id_filename]"] +gdb_assert { [lindex $status 0] == 0 } \ + "create sub-directory within the debug directory" +set status \ + [remote_exec build \ + "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 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