From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id kOZLJOW5wl9zcgAAWB0awg (envelope-from ) for ; Sat, 28 Nov 2020 15:58:13 -0500 Received: by simark.ca (Postfix, from userid 112) id 929121F0AB; Sat, 28 Nov 2020 15:58:13 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 2CBD91E552 for ; Sat, 28 Nov 2020 15:58:13 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 723D23857C6F; Sat, 28 Nov 2020 20:58:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 723D23857C6F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606597092; bh=2t4iThhR8xtVmd2Ya/vwKN2Qge4Wi3usfhlZeC2SVH8=; h=To:Subject:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=niujqNE8yq5tAxAnimE17XQAfmcSXfkbVoWfMX+bvSivMFDsYTBUG6V4U69A0JkLN ggW2vQYi4v5iX18E+QnxDXPugCMu7RYotWjKsOCAqEAs0qyH0GmHy2saNGFelh2rLZ TD8azse3AxnLXxZ14LSngamQxsq2bSdwYdDuqFaE= Received: from mail.sergiodj.net (mail.sergiodj.net [167.114.15.217]) by sourceware.org (Postfix) with ESMTPS id E1A5A3857C6F for ; Sat, 28 Nov 2020 20:58:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E1A5A3857C6F Received: from localhost (bras-base-toroon1016w-grc-37-76-65-22-220.dsl.bell.ca [76.65.22.220]) by mail.sergiodj.net (Postfix) with ESMTPSA id 72136A00BB; Sat, 28 Nov 2020 15:58:08 -0500 (EST) To: Simon Marchi Subject: Re: [PATCH v2] Search for DWZ files in debug-file-directories as well References: <20201114234842.2334396-1-sergiodj@sergiodj.net> <20201119022708.3627287-1-sergiodj@sergiodj.net> X-URL: http://blog.sergiodj.net Date: Sat, 28 Nov 2020 15:58:08 -0500 In-Reply-To: (Simon Marchi's message of "Thu, 26 Nov 2020 11:53:56 -0500") Message-ID: <87zh31yza7.fsf@paluero> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergio Durigan Junior via Gdb-patches Reply-To: Sergio Durigan Junior Cc: Mark Wielaard , gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On Thursday, November 26 2020, Simon Marchi wrote: > On 2020-11-18 9:27 p.m., Sergio Durigan Junior via Gdb-patches wrote: >> Changes from v1: >> >> - Addressed Simon's comments (new comment explaining how we try to >> match for the current debug-file-directory; properly use >> .erase/.insert methods -- keeping in mind that they modify the >> string in-place). >> >> >> When Debian (and Ubuntu) builds its binaries, it (still) doesn't use >> dwz's "--relative" option. This causes their debuginfo files to >> carry a .gnu_debugaltlink section containing a full pathname to the >> DWZ alt debug file, like this: >> >> $ readelf -wk /usr/bin/cat >> Contents of the .gnu_debugaltlink section: >> >> Separate debug info file: /usr/lib/debug/.dwz/x86_64-linux-gnu/coreutils.debug >> Build-ID (0x14 bytes): >> ee 76 5d 71 97 37 ce 46 99 44 32 bb e8 a9 1a ef 99 96 88 db >> >> Contents of the .gnu_debuglink section: >> >> Separate debug info file: 06d3bee37b8c7e67b31cb2689cb351102ae73b.debug >> CRC value: 0x53267655 >> >> This usually works OK, because most of the debuginfo files installed >> via apt will be present in /usr/lib/debug anyway. However, imagine >> the following scenario: >> >> - You are using /usr/bin/cat, it crashes on you and generates a >> corefile. >> >> - You don't want/need to "apt install" the debuginfo file for >> coreutils from the repositories. Instead, you already have the >> debuginfo files in a separate directory (e.g., $HOME/dbgsym). >> >> - You start GDB and "set debug-file-directory $HOME/dbgsym". > > Should the above read $HOME/dbgsym/usr/lib/debug to be consistent with > the example below? Sure. >> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c >> index 3c59826291..c462b9bb2c 100644 >> --- a/gdb/dwarf2/read.c >> +++ b/gdb/dwarf2/read.c >> @@ -2190,7 +2190,7 @@ locate_dwz_sections (bfd *abfd, asection *sectp, dwz_file *dwz_file) >> struct dwz_file * >> dwarf2_get_dwz_file (dwarf2_per_bfd *per_bfd) >> { >> - const char *filename; >> + std::string filename; >> bfd_size_type buildid_len_arg; >> size_t buildid_len; >> bfd_byte *buildid; >> @@ -2216,19 +2216,17 @@ dwarf2_get_dwz_file (dwarf2_per_bfd *per_bfd) >> >> filename = data.get (); > > Declare the filename variable here. Done. >> >> - std::string abs_storage; >> - if (!IS_ABSOLUTE_PATH (filename)) >> + if (!IS_ABSOLUTE_PATH (filename.c_str ())) >> { >> gdb::unique_xmalloc_ptr abs >> = gdb_realpath (bfd_get_filename (per_bfd->obfd)); >> >> - abs_storage = ldirname (abs.get ()) + SLASH_STRING + filename; >> - filename = abs_storage.c_str (); >> + filename = ldirname (abs.get ()) + SLASH_STRING + filename; >> } >> >> /* First try the file name given in the section. If that doesn't >> work, try to use the build-id instead. */ >> - gdb_bfd_ref_ptr dwz_bfd (gdb_bfd_open (filename, gnutarget)); >> + gdb_bfd_ref_ptr dwz_bfd (gdb_bfd_open (filename.c_str (), gnutarget)); >> if (dwz_bfd != NULL) >> { >> if (!build_id_verify (dwz_bfd.get (), buildid_len, buildid)) >> @@ -2238,6 +2236,69 @@ dwarf2_get_dwz_file (dwarf2_per_bfd *per_bfd) >> if (dwz_bfd == NULL) >> dwz_bfd = build_id_to_debug_bfd (buildid_len, buildid); >> >> + if (dwz_bfd == nullptr) >> + { >> + /* If the user has provided us with different >> + debug-file-directories, we can try them in order. */ >> + size_t dwz_pos = filename.find ("/.dwz/"); >> + >> + if (dwz_pos != std::string::npos) >> + { >> + std::vector> debugdir_vec >> + = dirnames_to_char_ptr_vec (debug_file_directory); >> + >> + for (const gdb::unique_xmalloc_ptr &debugdir : debugdir_vec) >> + { >> + /* The idea is to iterate over the >> + debug-file-directories provided by the user and >> + replace the hard-coded path in the "filename" by each >> + debug-file-directory. >> + >> + For example, suppose that filename is: >> + >> + /usr/lib/debug/.dwz/foo.dwz >> + >> + And suppose that we have "$HOME/bar" as the >> + debug-file-directory. We would then adjust filename >> + to look like: >> + >> + $HOME/bar/.dwz/foo.dwz >> + >> + which would hopefully allow us to find the alt debug >> + file. */ >> + std::string ddir = debugdir.get (); >> + >> + /* Check whether the beginning of FILENAME is DDIR. If >> + it is, then we are dealing with a file which we >> + already attempted to open before, so we just skip it >> + and continue processing the reamining >> + debug-file-directories. */ >> + if (filename.size () > ddir.size () >> + && filename.compare (0, ddir.size (), ddir) == 0) >> + continue; > > Corner case, but what if file name is /usr/lib/abcde/.dwz/foo.dwz and > ddir is /usr/lib/abc? We will wrongfully skip that debug dir I guess? > > Filesystem paths are hard :(. Is there some "is parent of" function we > can use? Worst case, we skip that check and do an unnecessary attempt. So, this case is only problematic if the debug-file-directory in question doesn't end with DIR_SEPARATOR. What I can do is check whether ddir ends with DIR_SEPARATOR, and if not, I can add one. This would make sure that we always check for the full directory name, instead of the incomplete path. >> + >> + /* Replace FILENAME's default debug-file-directory with >> + DDIR. */ >> + std::string new_filename = filename; >> + new_filename.erase (0, dwz_pos); >> + new_filename.insert (0, ddir); > > I think it would be more readable and efficient (less bytes copied > around) to do just build the new_filename string with something like: > > std::string new_filename = debugdir + &filename[dwz_pos]; > > This is untested, but I think you'll get the point. &filename[dwz_pos] > could also be put into a variable with a meaningful name, for clarity > (though I can't think of a good name right now). It is more efficient, but using .erase + .insert can be more readable. But OK, I don't have a strong opinion here. > And we probably don't need the allocated `ddir` std::string, you > construct it but never really use it (you could just refer to debugdir). I'll need it for the new version of the code, which will implement the idea I gave above (always guaranteeing that ddir ends with DIR_SEPARATOR). >> + >> + dwz_bfd = gdb_bfd_open (new_filename.c_str (), gnutarget); >> + >> + if (dwz_bfd != nullptr) >> + { >> + if (!build_id_verify (dwz_bfd.get (), buildid_len, buildid)) >> + { >> + dwz_bfd.reset (nullptr); > > Just ".reset ()". OK. >> + continue; >> + } >> + /* Found it. */ >> + break; >> + } > > Try to minimize the indentation levels where possible, by using early > returns or continues. For example, here: > > if (dwz_bfd == nullptr) > continue; > > if (!build_id_verify (...)) > { > dwz_bfd.reset (); > continue; > } > > /* Found it. */ > break; Done. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible https://sergiodj.net/