[PATCH] [symtab/27831] Fix OBJF_MAINLINE assert

[PATCH] [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.  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

Re: [PATCH] [symtab/27831] Fix OBJF_MAINLINE assert

[Simon Marchi]

Thanks for the great explanation.  This is all new to me, but because
it's clearly expressed, I have some maybe meaningful feedback.

On 2021-05-08 12:46 a.m., Kevin Buettner via Gdb-patches wrote:
> 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 ()]))

So, if we take a step back: previously, symtabs weren't shared between
objfiles.  So if you loaded an ELF as mainline and the same ELF as
non-mainline, that would create two symtabs, two separate symbols for
you "foo".  One of the symbols would have "maybe_copied == true" and the
other "maybe_copied == false".

The flag is set here, solely based on the symbol itself and the objfile
in the context of which it is created:

  if ((objfile->flags & OBJF_MAINLINE) == 0
      && (ms_type == mst_data || ms_type == mst_bss))
    result->maybe_copied = 1;

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.

See the patch at the end of this message for what I mean.

I think your solution would fail if symbol is first created as part of a
mainline objfile and then used as part of a non-mainline objfile.  The
maybe_copied field would be set to false for that symbol, and it would
stay that way forever (although the second objfile would need it to be
true).

Since minimal symbols should be independent of the objfile, in the
context of which they are loaded, I think that record_minimal_symbol, in
elfread.c, should not take an objfile as a parameter.  Otherwise, that's
just a risk of doing things dependent on the objfile, like that.

Simon

From 20cfdf40f370183a1558ce5f407320f2caea5f29 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 13 May 2021 15:12:31 -0400
Subject: [PATCH] fix

Change-Id: I3af42c7754aa83449cdc49c765dc5300920ef3a5
---
 gdb/elfread.c |  8 +-------
 gdb/symtab.c  |  9 ++++++++-
 gdb/symtab.h  | 14 +++++---------
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 1dea226242db..80cca21bf32c 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -214,13 +214,7 @@ 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);
 
-  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 9555f94707de..23bf9db2fdcc 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -377,6 +377,13 @@ minimal_symbol::text_p () const
     || type == mst_file_text;
 }
 
+bool
+minimal_symbol::maybe_copied (objfile *objfile) const
+{
+  return ((objfile->flags & OBJF_MAINLINE) == 0
+	  && (this->type == mst_data || this->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
@@ -6413,7 +6420,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 efdbada97618..9ca58025076c 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -708,14 +708,6 @@ struct minimal_symbol : public general_symbol_info
      the object file format may not carry that piece of information.  */
   unsigned int 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;
@@ -737,6 +729,10 @@ struct minimal_symbol : public general_symbol_info
   /* True if MSYMBOL is of some text type.  */
 
   bool text_p () const;
+
+  // FIXME: copy and adapt the doc from the maybe_copied field.
+
+  bool maybe_copied (objfile *objfile) const;
 };
 
 /* Return the address of MINSYM, which comes from OBJF.  The
@@ -766,7 +762,7 @@ 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)) ? 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.  */
-- 
2.31.1