[PATCH] [symtab/27831] Fix OBJF_MAINLINE assert

[Kevin Buettner <kevinb@redhat.com>]

This commit fixes a bug mentioned by Florian Weimer during the
libpthread/ld.so load order discussion.  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 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, the MSYMBOL_VALUE_ADDRESS macro looks like this:

 #define MSYMBOL_VALUE_ADDRESS(objfile, symbol)				\
  (((symbol)->maybe_copied) ? get_msymbol_address (objfile, symbol)	\
   : ((symbol)->value.address						\
      + (objfile)->section_offsets[(symbol)->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.

I've adjusted the MSYMBOL_VALUE_ADDRESS macro to include a test
of the OBFILE_MAINLINE flag.  So, in order for it to call
get_msymbol_address(), the symbol's "maybe_copied" flag must
be set AND the objfile's OBJF_MAINLINE flag must not be set...

 #define MSYMBOL_VALUE_ADDRESS(objfile, symbol)				\
  (((symbol)->maybe_copied && (((objfile)->flags & OBJF_MAINLINE) == 0)) \
   ? get_msymbol_address (objfile, symbol)	\
   : ((symbol)->value.address						\
      + (objfile)->section_offsets[(symbol)->section_index ()]))

I tried looking for a solution which would cause maybe_copied to be
set correctly, but I don't see a way to do it due to the fact that
a symtab can be shared amongst objfiles.

Also, it should be noted 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.

gdb/ChangeLog:

	PR symtab/27831
	* symtab.h (MSYMBOL_VALUE_ADDRESS): Don't call
	get_msymbol_address() when objfile flag OBJF_MAINLINE is
	set.

gdb/testsuite/ChangeLog:

	PR symtab/27831
	* gdb.base/add-symbol-file-attach.c: New file.
	* gdb.base/add-symbol-file-attach.exp: New file.
---
 gdb/symtab.h                                  |  3 +-
 .../gdb.base/add-symbol-file-attach.c         | 28 +++++++++
 .../gdb.base/add-symbol-file-attach.exp       | 63 +++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 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/symtab.h b/gdb/symtab.h
index efdbada9761..8ccb7952ffc 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -766,7 +766,8 @@ extern CORE_ADDR get_msymbol_address (struct objfile *objf,
 /* The relocated address of the minimal symbol, using the section
    offsets from OBJFILE.  */
 #define MSYMBOL_VALUE_ADDRESS(objfile, symbol)				\
-  (((symbol)->maybe_copied) ? get_msymbol_address (objfile, symbol)	\
+  (((symbol)->maybe_copied && (((objfile)->flags & OBJF_MAINLINE) == 0)) \
+   ? get_msymbol_address (objfile, symbol)	\
    : ((symbol)->value.address						\
       + (objfile)->section_offsets[(symbol)->section_index ()]))
 /* For a bound minsym, we can easily compute the address directly.  */
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..ac25df65db8
--- /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 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..59198dd171b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/add-symbol-file-attach.exp
@@ -0,0 +1,63 @@
+# Copyright (C) 2021 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/>.
+
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+standard_testfile
+
+# Create and source the file that provides information about the compiler
+# used to compile the test case.
+if [get_compiler_info] {
+    return -1
+}
+
+if {[build_executable $testfile.exp $testfile $srcfile debug] == -1} {
+    untested "failed to compile"
+    return -1
+}
+
+# Start the program running and then wait for a bit, to be sure
+# that it can be attached to.
+
+set test_spawn_id [spawn_wait_for_attach $binfile]
+set testpid [spawn_id_get_pid $test_spawn_id]
+
+gdb_start
+
+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
+    }
+}
+
+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
+    }
+}
+
+gdb_test "print foo" "42"
-- 
2.30.2