From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 4CD593858410 for ; Sun, 13 Aug 2023 04:00:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4CD593858410 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691899225; 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=ViIDbL6K8+CwUWW4J8gTm7+KJclLI45qZ6cs3ILyUBg=; b=BHXch2Yu/Xo5r98x5nf80AgZoTAcm9lB7lciDyEeMxbV7kCQJxFeHWCBeSUELRuPsd/cUn HPTlQ8gS0B5qKUKHjT0K9879O1VbzufQj+Or33klAi2xk5KfoPyzLHlm7nKWinPDqrdjJt rfcb9lUr3QIrSFQ3Z8WQj4TbYvQhBs4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-103-UJnKTzW4PcKyX8jD4kJoYA-1; Sun, 13 Aug 2023 00:00:22 -0400 X-MC-Unique: UJnKTzW4PcKyX8jD4kJoYA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 68BF8800C78; Sun, 13 Aug 2023 04:00:22 +0000 (UTC) Received: from f38-1.lan (unknown [10.22.32.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id 25190492B0F; Sun, 13 Aug 2023 04:00:13 +0000 (UTC) From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Kevin Buettner , Simon Marchi Subject: [PATCH v3 1/2] [symtab/27831] Fix OBJF_MAINLINE assert Date: Sat, 12 Aug 2023 20:48:52 -0700 Message-ID: <20230813035925.3344167-2-kevinb@redhat.com> In-Reply-To: <20230813035925.3344167-1-kevinb@redhat.com> References: <20230813035925.3344167-1-kevinb@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_STOCKGEN,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 List-Id: This commit fixes a bug mentioned by Florian Weimer during the libpthread/ld.so load order discussion from 2021. Florian provided instructions for reproducing the bug here: https://sourceware.org/pipermail/gdb-patches/2021-April/177923.html That particular test does some interesting things involving forks, threads, and thread local storage. Fortunately, none of that is needed to reproduce the problem. I've made a new test case (which is now found in a separate commit) contained in the files gdb.base/add-symbol-file-attach.{c,exp}. The .c file is fairly simple as is the recipe for reproducing the problem. After separately starting the test case and noting the process id, start gdb (w/ no arguments), and do the following to reproduce the assertion failure - for this run, the process id of the separately started add-symbol-file-attach process is 4103218: (gdb) add-symbol-file add-symbol-file-attach add symbol table from file "add-symbol-file-attach" (y or n) y Reading symbols from add-symbol-file-attach... (gdb) attach 4103218 Attaching to process 4103218 Load new symbol table from "/tmp/add-symbol-file-attach"? (y or n) y Reading symbols from /tmp/add-symbol-file-attach... Reading symbols from /lib64/libc.so.6... (No debugging symbols found in /lib64/libc.so.6) Reading symbols from /lib64/ld-linux-x86-64.so.2... (No debugging symbols found in /lib64/ld-linux-x86-64.so.2) 0x00007f502130bf27 in pause () from /lib64/libc.so.6 (gdb) p foo symtab.c:6417: internal-error: CORE_ADDR get_msymbol_address(objfile*, const minimal_symbol*): Assertion `(objf->flags & OBJF_MAINLINE) == 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. The add-symbol-file command causes the symbols to be loaded without the SYMFILE_MAINLINE (and hence the OBJFILE_MAINLINE) flags being set. This, in turn, causes the "maybe_copied" flag to be set for the global symbol (named "foo" in the provided test case). The attach command will cause another objfile to be created, but it will reuse the symtabs from the objfile created by add-symbol-file, leading to a situation in which the OBJFILE_MAINLINE flag will be set for the new (attach-created) objfile, however the "maybe_copied" flag will still be set for the global symbol. Had it been loaded anew, this flag would not be set due to OBJFILE_MAINLINE being set for the objfile. At present, minimal_symbol::value_address looks like this: CORE_ADDR minimal_symbol::value_address (objfile *objfile) const { if (this->maybe_copied (objfile)) return get_msymbol_address (objfile, this); else return (CORE_ADDR (this->unrelocated_address ()) + objfile->section_offsets[this->section_index ()]); } So, we can now see the problem: When the "maybe_copied" flag is set, get_msymbol_address() will be called. However, get_msymbol_address() assumes that it won't be called with the OBF_MAINLINE flag set for the objfile in question. It, in fact, contains an assert() which makes sure that this is the case: gdb_assert ((objf->flags & OBJF_MAINLINE) == 0); (If this assert is removed, then get_msymbol_address() recurses infinitely for the case under consideration.) So, the problem here is that the maybe_copied flag is set for the symbol AND the OBJF_MAINLINE flag is set for the objfile. As noted earlier, this happens due to add-symbol-file being used; this causes the maybe_copied flag to be set. Later, when the attach is performed, OBJF_MAINLINE will be set for that objfile, leading to this unfortunate situation. My first cut at a solution involved adjusting the MSYMBOL_VALUE_ADDRESS macro (which has since been changed to be the method noted above) to include a test of the OBJFILE_MAINLINE flag. However, Simon Marchi, in his review of my patch, suggested a better solution. Simon observed that the 'maybe_copied' flag is (was, after this commit) being set/initialized in record_minimal_symbol() using using the objfile in the context in which the symbol was created. Simon further observed: Today, a single copy is created, as symtabs are shared between objfiles. This means that everything that we store into a symbol must be independent of any objfile. However, the value of the maybe_copied field is dependent on the objfile in the context of which the symbol was created. Meaning that when the symbol is re-used in the context of another objfile, the maybe_copied value is not right in the context of that objfile. So I think it means there isn't a single "is this symbol maybe copied" value, but instead "is this symbol maybe copied, in the context of this given objfile". And the answer is yes or no, depending on whether the objfile is mainline. So maybe_copied should become a method that takes an objfile and returns an answer based on that. [ Simon's full review can be found here: https://sourceware.org/pipermail/gdb-patches/2021-May/178855.html ] Simon also provided a patch which implements this suggestion. The current patch is his work, though I did move and adjust a comment, and, later, performed a rebase. Lastly, I'll note that this is a strange use case. It's far more common to either let gdb figure out which file to load by itself when attaching, i.e. (gdb) attach 4104360 Attaching to process 4104360 Reading symbols from /tmp/add-symbol-file-attach... Reading symbols from /lib64/libc.so.6... (No debugging symbols found in /lib64/libc.so.6) Reading symbols from /lib64/ld-linux-x86-64.so.2... (No debugging symbols found in /lib64/ld-linux-x86-64.so.2) 0x00007fdb1fc33f27 in pause () from /lib64/libc.so.6 (gdb) p foo $1 = 42 ...or to use the "file" command prior to the attach, like this: (gdb) file add-symbol-file-attach Reading symbols from add-symbol-file-attach... (gdb) attach 4104360 Attaching to program: /tmp/add-symbol-file-attach, process 4104360 Reading symbols from /lib64/libc.so.6... (No debugging symbols found in /lib64/libc.so.6) Reading symbols from /lib64/ld-linux-x86-64.so.2... (No debugging symbols found in /lib64/ld-linux-x86-64.so.2) 0x00007fdb1fc33f27 in pause () from /lib64/libc.so.6 Both of these more common scenarios work perfectly fine; using "add-symbol-file" to load the program to which you will attach isn't recommended as a normal use case. That said, it's bad for gdb to assert, hence this fix. Co-Authored-By: Simon Marchi Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27831 --- gdb/elfread.c | 8 +------- gdb/symtab.c | 13 +++++++++++-- gdb/symtab.h | 17 +++++++++-------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/gdb/elfread.c b/gdb/elfread.c index 349a8b75bcc..dbe5951c802 100644 --- a/gdb/elfread.c +++ b/gdb/elfread.c @@ -220,13 +220,7 @@ record_minimal_symbol (minimal_symbol_reader &reader, || bfd_section == bfd_abs_section_ptr) section_index = gdb_bfd_section_index (objfile->obfd.get (), bfd_section); - struct minimal_symbol *result - = reader.record_full (name, copy_name, address, ms_type, section_index); - if ((objfile->flags & OBJF_MAINLINE) == 0 - && (ms_type == mst_data || ms_type == mst_bss)) - result->maybe_copied = 1; - - return result; + return reader.record_full (name, copy_name, address, ms_type, section_index); } /* Read the symbol table of an ELF file. diff --git a/gdb/symtab.c b/gdb/symtab.c index 0117a2a59d7..16c682c81b9 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -435,7 +435,7 @@ compunit_symtab::language () const CORE_ADDR minimal_symbol::value_address (objfile *objfile) const { - if (this->maybe_copied) + if (this->maybe_copied (objfile)) return get_msymbol_address (objfile, this); else return (CORE_ADDR (this->unrelocated_address ()) @@ -467,6 +467,15 @@ minimal_symbol::text_p () const || m_type == mst_file_text; } +/* See symtab.h. */ + +bool +minimal_symbol::maybe_copied (objfile *objfile) const +{ + return ((objfile->flags & OBJF_MAINLINE) == 0 + && (m_type == mst_data || m_type == mst_bss)); +} + /* See whether FILENAME matches SEARCH_NAME using the rule that we advertise to the user. (The manual's description of linespecs describes what we advertise). Returns true if they match, false @@ -6515,7 +6524,7 @@ get_symbol_address (const struct symbol *sym) CORE_ADDR get_msymbol_address (struct objfile *objf, const struct minimal_symbol *minsym) { - gdb_assert (minsym->maybe_copied); + gdb_assert (minsym->maybe_copied (objf)); gdb_assert ((objf->flags & OBJF_MAINLINE) == 0); const char *linkage_name = minsym->linkage_name (); diff --git a/gdb/symtab.h b/gdb/symtab.h index ee4729b14cd..fa77fc16d2f 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -855,14 +855,6 @@ struct minimal_symbol : public general_symbol_info the object file format may not carry that piece of information. */ unsigned int m_has_size : 1; - /* For data symbols only, if this is set, then the symbol might be - subject to copy relocation. In this case, a minimal symbol - matching the symbol's linkage name is first looked for in the - main objfile. If found, then that address is used; otherwise the - address in this symbol is used. */ - - unsigned maybe_copied : 1; - /* Non-zero if this symbol ever had its demangled name set (even if it was set to NULL). */ unsigned int name_set : 1; @@ -884,6 +876,15 @@ struct minimal_symbol : public general_symbol_info /* True if MSYMBOL is of some text type. */ bool text_p () const; + + /* For data symbols only, given an objfile, if 'maybe_copied' + evaluates to 'true' for that objfile, then the symbol might be + subject to copy relocation. In this case, a minimal symbol + matching the symbol's linkage name is first looked for in the + main objfile. If found, then that address is used; otherwise the + address in this symbol is used. */ + + bool maybe_copied (objfile *objfile) const; }; #include "minsyms.h" -- 2.41.0