From patchwork Sat Jan 25 22:55:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 37542 Received: (qmail 32335 invoked by alias); 25 Jan 2020 22:56:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 32122 invoked by uid 89); 25 Jan 2020 22:56:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=hell, H*RU:sk:host86-, HX-Spam-Relays-External:sk:host86-, H*r:sk:host86- X-HELO: mail-wm1-f52.google.com Received: from mail-wm1-f52.google.com (HELO mail-wm1-f52.google.com) (209.85.128.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 25 Jan 2020 22:56:00 +0000 Received: by mail-wm1-f52.google.com with SMTP id p17so3210525wmb.0 for ; Sat, 25 Jan 2020 14:56:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id; bh=WrAf2bIfwZeESp23tkFjhVnbw63vh3gHL1uAgOA/MtU=; b=XbT4+X1HsPvAHaGty2FYt0DGnagVFzOu/J4HLd7SyZpyPuiaDh2pflSTk/9STt/ShR bq1U/ttyR7HAhcjTULR6IJmbI++jhFsoslgG7EXxwVXOZ8Y7GUkb3Pg8SCO54QBIUOzb Q5YoS7AYcMd0CnXjYC8emAa4dbsYWFvNF5DKTIq4T+Hk6JAxnTdzc7wJ8es3dloJZ608 1l446txML0/22Ou0l78dzboJuFbcfaOcMeUArytuCeFnGx+VNJC7BRZ2gwjOyw3PZqIg 6Vt6QmpvFgnin9CbfCYDzlV4IskGaACNxA68dTsD+QVaEMCzYVgyjCIYt/b3xYnysaCd Hu7A== Return-Path: Received: from localhost (host86-191-239-73.range86-191.btcentralplus.com. [86.191.239.73]) by smtp.gmail.com with ESMTPSA id b18sm12781779wru.50.2020.01.25.14.55.57 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 25 Jan 2020 14:55:57 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] gdb: Reinitialize objfile::section_offsets during objfile reload Date: Sat, 25 Jan 2020 22:55:55 +0000 Message-Id: <20200125225555.16846-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes When building and testing with '-D_GLIBCXX_DEBUG=1' I noticed that the test gdb.base/reload.exp was failing. This turns out to be because the objfile::section_offsets vector is not reinitialilzed during the objfile reload process, and in this particular test, GDB ends up indexing outside the bounds of the vector. In order to make this issue show up even if GDB is not compiled with '-D_GLIBCXX_DEBUG=1' I added an assert into elfread.c, then I made some changes in symfile.c:reread_symbols to reinitialilze the vector, after which the problem is resolved. While looking at the code in reread_symbols I noticed a difference between reread_symbols and syms_from_objfile_1 with how we handle the case where objfile->sf is NULL. In the later function we handle this case, while in the former we just assume that objfile->sf is not NULL. I can't see why the NULL case couldn't occur in reread_symbols, however, this would only happen if the user is using srec, ihex, or texhex format BFDs. As such I didn't fancy trying to update reread_symbols to handle this case, so instead I just added an error if this case should arise. This should be better than the current undefined behaviour (crash), and should let us know what has gone wrong if a user ever reports this as an issue. One thing I did wonder about while looking at this fix is whether it would be possible to combine at least parts of syms_from_objfile_1 with the core of reread_symbols. I did have a go at doing this but gave up in the end due to the subtle differences between the two. Still, I think that with some more effort this might be possible, and this could be a nice clean up. gdb/ChangeLog: * elfread.c (record_minimal_symbol): Add an assertion. * symfile.c (reread_symbols): Clear the section_offsets vector, and reinitialize it during reload. Add an error if objfile->sf is NULL. Change-Id: I9d62292641416483a8a7415c7504095bf439fc29 --- gdb/ChangeLog | 7 +++++++ gdb/elfread.c | 4 ++++ gdb/symfile.c | 19 +++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/gdb/elfread.c b/gdb/elfread.c index 453bca527e9..5d0acede078 100644 --- a/gdb/elfread.c +++ b/gdb/elfread.c @@ -218,6 +218,10 @@ record_minimal_symbol (minimal_symbol_reader &reader, if ((bfd_section_flags (bfd_section) & SEC_ALLOC) == SEC_ALLOC) section_index = gdb_bfd_section_index (objfile->obfd, bfd_section); + /* The section_offsets vector should have been initialised by now, and + there should be one entry for each section in objfile. */ + gdb_assert (section_index < objfile->section_offsets.size ()); + struct minimal_symbol *result = reader.record_full (name, copy_name, address, ms_type, section_index); if ((objfile->flags & OBJF_MAINLINE) == 0 diff --git a/gdb/symfile.c b/gdb/symfile.c index f7bada75f35..0da879fd751 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -2553,6 +2553,7 @@ reread_symbols (void) objfile->compunit_symtabs = NULL; objfile->template_symbols = NULL; objfile->static_links.reset (nullptr); + objfile->section_offsets.clear (); /* obstack_init also initializes the obstack so it is empty. We could use obstack_specify_allocation but @@ -2573,6 +2574,16 @@ reread_symbols (void) start over. PR symtab/15885 */ objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd)); + /* In syms_from_objfile_1 after calling objfile_set_sym_fns we + handle the possibility that objfile->sf might be NULL, which + can happen for some obscure objfile formats. We've never + handled the NULL case here before, but */ + if (objfile->sf == nullptr) + error (_("unable to reload object file with format `%s'"), + bfd_get_target (objfile->obfd)); + + gdb_assert (objfile->sf != nullptr); + build_objfile_section_table (objfile); /* What the hell is sym_new_init for, anyway? The concept of @@ -2604,6 +2615,14 @@ reread_symbols (void) objfiles_changed (); + /* Setup the section offsets structure for this objfile. We use + zero section address information here, though it's not clear + this will always be correct. If the user originally loaded + this objfile with non-zero address information then we're + going to loose that here. */ + section_addr_info local_addr; + (*objfile->sf->sym_offsets) (objfile, local_addr); + read_symbols (objfile, 0); if (!objfile_has_symbols (objfile))