[PATCH v3 0/2] [symtab/27831] Fix OBJF_MAINLINE assert

[PATCH v3 0/2] [symtab/27831] Fix OBJF_MAINLINE assert

[Kevin Buettner]

This is a slightly modified version of a two-part series from over a
year ago.  As noted then, the fix is mostly Simon's work, though the
text of the commit log is still mine.  Also, it required a rebase, so
that work is mine as well.  I've also updated copyright dates as well
as the commit log for the fix to reflect the fact that macros are no
longers used in the affected code.

I've tested on Fedora 38 for both x86_64 and aarch64 - no regressions. 
Moreover, I've observed that the new test case is still relevant, i.e.
the bug hasn't been fixed by some other means in the interim.

Kevin Buettner (2):
  [symtab/27831] Fix OBJF_MAINLINE assert
  [symtab/27831] New test case: gdb.base/add-symbol-file-attach.exp

 gdb/elfread.c                                 |  8 +-
 gdb/symtab.c                                  | 13 ++-
 gdb/symtab.h                                  | 17 ++--
 .../gdb.base/add-symbol-file-attach.c         | 28 ++++++
 .../gdb.base/add-symbol-file-attach.exp       | 88 +++++++++++++++++++
 5 files changed, 137 insertions(+), 17 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/add-symbol-file-attach.c
 create mode 100644 gdb/testsuite/gdb.base/add-symbol-file-attach.exp

-- 
2.41.0

[PATCH v3 1/2] [symtab/27831] Fix OBJF_MAINLINE assert

[Kevin Buettner]

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 <simon.marchi@polymtl.ca>
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

[PATCH v3 2/2] [symtab/27831] New test case: gdb.base/add-symbol-file-attach.exp

[Kevin Buettner]

This commit adds a new test case for bug 27831.  See the contents
of the .exp file for a description of what it's about.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27831
---
 .../gdb.base/add-symbol-file-attach.c         | 28 ++++++
 .../gdb.base/add-symbol-file-attach.exp       | 88 +++++++++++++++++++
 2 files changed, 116 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/add-symbol-file-attach.c
 create mode 100644 gdb/testsuite/gdb.base/add-symbol-file-attach.exp

diff --git a/gdb/testsuite/gdb.base/add-symbol-file-attach.c b/gdb/testsuite/gdb.base/add-symbol-file-attach.c
new file mode 100644
index 00000000000..bd296efa1e1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/add-symbol-file-attach.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021-2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <stdio.h>
+
+volatile int foo = 42;
+
+int
+main (int argc, char **argv)
+{
+  pause ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/add-symbol-file-attach.exp b/gdb/testsuite/gdb.base/add-symbol-file-attach.exp
new file mode 100644
index 00000000000..654c05e1d7e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/add-symbol-file-attach.exp
@@ -0,0 +1,88 @@
+# Copyright (C) 2021-2023 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This is a test case for bug 27831:
+#
+# https://sourceware.org/bugzilla/show_bug.cgi?id=27831
+#
+# Prior to fixing this bug, GDB could be caused to assert by doing
+# the following:  A simple program with a global was started outside
+# of GDB.  Once GDB was started, the symbol file was loaded in an
+# atypical manner via the 'add-symbol-file' command.  Then, the
+# program was attached to via the 'attach' command at which time the
+# symbol file was loaded again.  Once attached, printing the global
+# variable in the program would trigger the assertion error.
+#
+# This is what the assert looked like:
+#
+# gdb/symtab.c:6599: internal-error: get_msymbol_address:
+#   Assertion `(objf->flags & OBJF_MAINLINE) == 0' failed.
+
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+standard_testfile
+
+if [get_compiler_info] {
+    return -1
+}
+
+if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
+    untested "failed to compile"
+    return -1
+}
+
+# Use 'spawn_wait_for_attach' to start the test program running.  It'll
+# also sleep for a short time in order to make sure that it's running
+# so that GDB may attach to it.
+
+set test_spawn_id [spawn_wait_for_attach $binfile]
+set testpid [spawn_id_get_pid $test_spawn_id]
+
+gdb_start
+
+# Load the symbol file in an atypical manner by using the add-symbol-file
+# command.
+
+set test "add-symbol-file before attach"
+gdb_test_multiple "add-symbol-file $binfile" $test {
+    -re "add symbol table from file.*y or n. $" {
+	send_gdb "y\n"
+	exp_continue
+    }
+    -re "Reading symbols from.*" {
+	pass $test
+    }
+}
+
+# Attach to the process started above.  GDB will want to load a "new"
+# symbol table, so handle that case.
+
+set test "attach"
+gdb_test_multiple "attach $testpid" $test {
+    -re "Attaching to process.*Load new symbol table.*y or n. $" {
+	send_gdb "y\n"
+	exp_continue
+    }
+    -re ".*in \[_A-Za-z0-9\]*pause.*$gdb_prompt $" {
+	pass $test
+    }
+}
+
+# Print out the value of the global.  Prior to fixing bug 27831, GDB
+# would assert while executing this command.
+
+gdb_test "print foo" "42"
-- 
2.41.0

Re: [PATCH v3 1/2] [symtab/27831] Fix OBJF_MAINLINE assert

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner via Gdb-patches <gdb-patches@sourceware.org> writes:

Kevin> This commit fixes a bug mentioned by Florian Weimer during the
Kevin> libpthread/ld.so load order discussion from 2021.  Florian provided
Kevin> instructions for reproducing the bug here:

Thanks for the patch.

I think this looks good but I had one possible concern.

Kevin> diff --git a/gdb/elfread.c b/gdb/elfread.c
Kevin> index 349a8b75bcc..dbe5951c802 100644
...
Kevin> -    result->maybe_copied = 1;

Kevin> +bool
Kevin> +minimal_symbol::maybe_copied (objfile *objfile) const
Kevin> +{
Kevin> +  return ((objfile->flags & OBJF_MAINLINE) == 0
Kevin> +	  && (m_type == mst_data || m_type == mst_bss));

The old approach was specific to ELF, while the new approach will be
used by any object format.

I am not sure if other object formats even have copy relocs.  Do they?
If so then this seems fine.  If not, then it seems like it could result
in an incorrect evaluation in some scenario.

Tom