From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 8464E3858D3C for ; Sun, 14 Nov 2021 02:58:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8464E3858D3C Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 1AE2uwNw007747 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 13 Nov 2021 21:57:04 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 1AE2uwNw007747 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8292C1E813; Sat, 13 Nov 2021 21:56:58 -0500 (EST) Message-ID: <500888d1-2d7d-5f55-2940-95ed5d962961@polymtl.ca> Date: Sat, 13 Nov 2021 21:56:57 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [PATCH 3/3] PR gdb/27570: missing support for debuginfod in core_target::build_file_mappings Content-Language: en-US To: Aaron Merey , gdb-patches@sourceware.org Cc: tom@tromey.com, lsix@lancelotsix.com References: <20211110014755.532490-1-amerey@redhat.com> <20211110014755.532490-4-amerey@redhat.com> From: Simon Marchi In-Reply-To: <20211110014755.532490-4-amerey@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sun, 14 Nov 2021 02:56:58 +0000 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Sun, 14 Nov 2021 02:58:09 -0000 > @@ -416,6 +423,32 @@ locate_exec_from_corefile_build_id (bfd *abfd, int from_tty) > symbol_file_add_main (bfd_get_filename (execbfd.get ()), > symfile_add_flag (from_tty ? SYMFILE_VERBOSE : 0)); > } > + else > + { > + 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 (_("File \"%s\" from debuginfod cannot be opened as bfd"), > + execpath.get ()); > + else if (!build_id_verify (execbfd.get (), build_id->size, > + build_id->data)) > + execbfd.reset (nullptr); > + else > + { > + /* Successful download */ > + exec_file_attach (execpath.get (), from_tty); > + symbol_file_add_main (execpath.get (), > + symfile_add_flag (from_tty ? > + SYMFILE_VERBOSE : 0)); > + } > + } > + } The duplication of the steps taken when we have successfully found a BFD (call exec_file_attach and symbol_file_add_main) annoy me a little bit here. If those steps need to change, it would be easy to update one and forget the other. Can you refactor the code to be something like: gdb_bfd_ref_ptr execbfd = build_id_to_exec_bfd (build_id->size, build_id->data); if (execbfd == nullptr) { // Try to get execbfd using debuginfod, leave execbfd nullptr if // not successful. } if (execbfd != nullptr) { // We found a BFD one way or another, call exec_file_attach and // symbol_file_add_main. } > diff --git a/gdb/gcore.in b/gdb/gcore.in > index 24354a79e27..25b24c3cd3d 100644 > --- a/gdb/gcore.in > +++ b/gdb/gcore.in > @@ -89,6 +89,9 @@ if [ ! -f "$binary_path/@GDB_TRANSFORM_NAME@" ]; then > exit 1 > fi > > +# Prevent unnecessary debuginfod queries during core file generation. > +unset DEBUGINFOD_URLS I think this should be mentioned in the commit log, with an explanation of why there may be queries and why they are unnecessary. > @@ -114,8 +119,10 @@ proc write_dwarf_file {filename buildid {value 99}} { > } > } > > +set corefile "corefile" > + > proc no_url { } { > - global binfile outputdir debugdir > + global binfile corefile outputdir debugdir Use $::corefile to refer to the global variable, rather than using the "global" keyword. > > setenv DEBUGINFOD_URLS "" > > @@ -167,10 +174,20 @@ proc no_url { } { > gdb_test "file ${binfile}_alt.o" \ > ".*could not find '.gnu_debugaltlink'.*" \ > "file [file tail ${binfile}_alt.o]" > + > + # Generate a core file and test that gdb cannot find the executable > + clean_restart ${binfile}2 > + gdb_test "start" "Temporary breakpoint.*" > + gdb_test "generate-core-file $corefile" \ > + "Saved corefile $corefile" Where exactly is that "corefile" file written to? I suppose that it's written to the current working directory, which would be $build/gdb/testsuite. Even though it's temporary (the file is moved right after), the test shouldn't write outside of its output directory. If the user does ^C exactly here, it will leave a random file there. Also, if two tests decide to write a "corefile" file, they could step on each other's toes when running tests in parallel. Simon