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 807EB3889830 for ; Tue, 17 Aug 2021 13:59:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 807EB3889830 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 17HDwLr1031207 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 17 Aug 2021 09:58:26 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 17HDwLr1031207 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 B5FF51E4A3; Tue, 17 Aug 2021 09:58:20 -0400 (EDT) Subject: Re: [PATCH 2/3] gdb: Add soname to build-id mapping for corefiles To: Lancelot SIX , Aaron Merey Cc: gdb-patches@sourceware.org, tom@tromey.com References: <20210812042406.75637-1-amerey@redhat.com> <20210812042406.75637-3-amerey@redhat.com> <20210815145147.ap32fq6wji34wjyf@lsix-M11x-R2> From: Simon Marchi Message-ID: <6d314528-224a-3e5e-2d4b-070ea776823d@polymtl.ca> Date: Tue, 17 Aug 2021 09:58:20 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210815145147.ap32fq6wji34wjyf@lsix-M11x-R2> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 17 Aug 2021 13:58:21 +0000 X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: Tue, 17 Aug 2021 13:59:41 -0000 I did not do a full review, since Lancelot's comments were already a good start. I just added some complements to his comments. >> /* See arch-utils.h. */ >> void >> -default_read_core_file_mappings (struct gdbarch *gdbarch, >> - struct bfd *cbfd, >> - gdb::function_view >> - pre_loop_cb, >> - gdb::function_view> - ULONGEST start, >> - ULONGEST end, >> - ULONGEST file_ofs, >> - const char *filename)> >> - loop_cb) >> +default_read_core_file_mappings >> + (struct gdbarch *gdbarch, >> + struct bfd *cbfd, >> + gdb::function_view pre_loop_cb, >> + gdb::function_view> + ULONGEST start, >> + ULONGEST end, >> + ULONGEST file_ofs, >> + const char *filename, >> + const bfd_build_id *build_id)> >> + loop_cb) > > It looks like 'loop_cb' could go on the previous line. > > If the type of the function callbacks are too big, I guess it could be > possible to give them a name before declaring the function. Something > like > > using loop_cb_ftype = gdb::function_view; Agreed, and giving a name to the function type helps readability too. >> @@ -282,6 +282,16 @@ core_target::build_file_mappings () >> >> /* Set target_section fields. */ >> m_core_file_mappings.emplace_back (start, end, sec); >> + >> + /* If this is a bfd of a shared library, record its soname >> + and build id. */ >> + if (build_id != nullptr) >> + { >> + gdb::optional soname = gdb_bfd_read_elf_soname (bfd); >> + if (soname) >> + current_program_space->set_cbfd_soname_build_id (soname->data (), >> + build_id); > > Here, since set_cbfd_soname_build_id's first argument is a std::string, > you could just use '*soname' instead of 'soname->data ()'. Since the caller (here) doesn't need to keep the string, I would suggest `std::move (*soname)` (see below). >> @@ -358,6 +359,41 @@ print_program_space (struct ui_out *uiout, int requested) >> } >> } >> >> +/* See progspace.h. */ >> + >> +void >> +program_space::set_cbfd_soname_build_id (std::string soname, > > This parameter could be 'std::string const &' or... > >> + const bfd_build_id *build_id) >> +{ >> + std::string build_id_hex = build_id_to_string (build_id); >> + cbfd_soname_to_build_id[soname] = build_id_hex; > > ... use 'std::move (soname)' here. > > I guess the more 'usual' approach would be to have the argument as a > const reference (but to be honest, the implication of calling one more > ctor and copying the soname is negligible, to say the least). Since set_cbfd_soname_build_id needs to keep its own copy of the object, I think it's better to take it by value like this, and have the caller std::move its copy if it doesn't need to keep it around. And have set_cbfd_soname_build_id std::move it into the map: cbfd_soname_to_build_id[std::move (soname)] = build_id_hex; This way, soname is never copied if it doesn't need to. Taking by const-reference means that set_cbfd_soname_build_id will have no choice but to copy the string into the map. And in fact, I would suggest doing: cbfd_soname_to_build_id[std::move (soname)] = build_id_to_string (build_id); ... to avoid copying the build_id_hex string. Alternatively, you could do: std::string build_id_hex = build_id_to_string (build_id); cbfd_soname_to_build_id[std::move (soname)] = std::move (build_id_hex); ... but I think the first version is fine. >> + >> + return; > > I am not sure if the GNU coding standard says something about this, but > 'return;' as the last statement of a void function is redundant. Indeed. >> +} >> + >> +/* See progspace.h. */ >> + >> +const char * >> +program_space::get_cbfd_soname_build_id (const char *soname) > > With set_cbfd_soname_build_id using a std::string, I would find it more > consistent to use std::string here also. Any reason not to use it I > missed? > > You could use 'basename (soname.c_str ())' bellow. > > The return type could also be 'const std::string *' (the map stores > std::string internally), but keeping a const char * is pretty similar. We wouldn't want to return an std::string, that would make an unnecessary copy. If returning a `const char *` doesn't make life difficult for the caller, I think it's perfectly fine. Previously, I would have suggested returning string_view, because that abstracts how the string is stored. But that isn't right, since the string_view interface doesn't require the view to be null-terminated (which we usually need). We would need something like cstring_view instead: http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1402r0.pdf But in the mean time, I think `const char *` is fine. >> +gdb_bfd_read_elf_soname (struct bfd *bfd) >> +{ >> + gdb_assert (bfd != nullptr); >> + >> + gdb_bfd_ref_ptr abfd = gdb_bfd_open (bfd->filename, gnutarget); >> + >> + if (abfd == nullptr) >> + return gdb::optional (); >> + >> + /* Check that bfd is an ET_DYN ELF file. */ >> + bfd_check_format (abfd.get (), bfd_object); I asked this in my previous review, still applies here: What's the point of calling bfd_check_format without checking the result? It looks like a function without side-effects. >> + if (!(bfd_get_file_flags (abfd.get ()) & DYNAMIC)) >> + return gdb::optional (); >> + >> + /* Determine soname of shared library. If found map soname to build-id. */ >> + CORE_ADDR idx; >> + if (!gdb_bfd_scan_elf_dyntag (DT_SONAME, abfd.get (), &idx, nullptr)) >> + return gdb::optional (); >> + >> + struct bfd_section *dynstr = bfd_get_section_by_name (abfd.get (), ".dynstr"); >> + if (dynstr == nullptr) >> + return gdb::optional (); >> + >> + /* Read the soname from the string table. */ >> + gdb::byte_vector dynstr_buf; >> + if (!gdb_bfd_get_full_section_contents (abfd.get (), dynstr, &dynstr_buf)) >> + return gdb::optional (); >> + >> + return gdb::optional ((char *)dynstr_buf.data () + idx); Do we need bounds checking to protect against bad ELF files? What if DT_SONAME points outside .dynstr's size? And ideally that a null-terminator is found somewhere between idx and the end of the section. Otherwise, I could craft a .dynstr section that does not contain a null terminator, that would make GDB read out of bounds. > This will not change much, but you could cast to 'const char *' (this > is the type the std::string constructor expects). And to shorten things up a bit, you can drop "gdb::optional": return std::string ((const char *) dynstr_buf.data () + idx); While at it, instead of returning "gdb::optional ()" on failure, you can simply "return {}". Simon