[PATCH 2/2] gdb: move call to clear_symtab_users out from finish_new_objfile

[Andrew Burgess <aburgess@redhat.com>]

Move the call to clear_symtab_users from within finish_new_objfile to
instead reside within symbol_file_add_with_addrs.  This resolves an
issue where clear_symtab_users can be called multiple times during the
loading of the main executable, which causes important information to
be discarded.

To understand the problem we must understand two things.  First, how
does clear_symtab_users discard critical information?  This is the
easy part of the problem, clear_symtab_users notifies the
all_objfiles_removed observer, in auto-load.c there is a listener for
this observer, clear_section_scripts, which discards information about
any auto-loaded scripts.  If we call clear_symtab_users after
auto-loading a script within a program space, then information about
that script having been loaded will be discarded.

The second thing we must consider is the order in which functions are
called when loading a main executable with separate debug
information.  I'm only listing some of the most important functions in
the process here, the ones relevant to the issue being fixed.  In the
following text I'll use "main objfile" to refer to the actual
executable, and "debug info objfile" to refer to the split debug info
corresponding to the "main objfile".

  1. Call symbol_file_add_with_addrs for the main objfile.

  2. Call syms_from_objfile passing in the main objfile.

  3. Call objfile::find_and_add_separate_symbol_file on the main
     objfile.

  4. Call symbol_file_add_separate for the debug info objfile.

  5. Call symbol_file_add_with_addrs for the debug info objfile.

  6. Call syms_from_objfile for the debug info objfile.  The debug
     symbols are contained in this objfile so they are read and added
     to GDB.

  7. Call finish_new_objfile for the debug info objfile, followed by
     triggering the new objfile observer for the debug info objfile.

  8. We are now done in symbol_file_add_with_addrs for the debug info
     objfile.  We now unwind the stack back to (1).

  9. Back in symbol_file_add_with_addrs for the main objfile, the
     debug symbols have now been added (from the separate debug info
     objfile), so we can now call finish_new_objfile for the main
     objfile, followed by triggering the new objfile observer for the
     main objfile.

 10. Success!  The main objfile, and the associated debug info objfile
     have now been added to GDB.

Notice that we end up with a recursive call back into
symbol_file_add_with_addrs, which results in two calls to
finish_new_objfile, the debug info objfile is processed first, and the
main objfile is processed second.

The main objfile will have SYMFILE_MAINLINE in its symfile_add_flags,
and this will end up being passed to symbol_file_add_separate.

This means that in finish_new_objfile we currently call
clear_symtab_users after finishing both the debug info objfile, and
after the main objfile, in that order.

Auto-loaded scripts are loaded by load_auto_scripts_for_objfile (in
auto-load.c) which is called by the new_objfile observer.  If the
debug info objfile contains any scripts within the .debug_gdb_scripts
section, then these will be loaded (7) in the above list, after the
debug info objfile has been added, but before the main objfile is
added.

Now, this shouldn't be a problem, except that currently, when
finishing the main objfile, we call clear_symtab_users, which triggers
the all_objfiles_removed observer, which calls
clear_section_scripts (in auto-load.c), which discards all records of
auto-loaded scripts.

This issue is exposed by the test extension added in this commit.  An
executable is compiled with debug information, the executable includes
a .debug_gdb_scripts section.  We then use objcopy to split the debug
information into a separate objfile.  This takes the
.debug_gdb_scripts section with it.  We then ask GDB to load the
executable, which triggers loading of the separate debug information
file.  Loading the separate debug information file loads the
.debug_gdb_scripts section, then GDB finishes loading the main
executable and discards the record of loading the .debug_gdb_scripts.
This is exposed by using 'info auto-load'.

The solution I propose is to move the call to clear_symtab_users
earlier within symbol_file_add_with_addrs, and to guard the call so
that it is only called for the main objfile, and not for the debug
info objfile.  The new location for the clear_symtab_users call is
before the syms_from_objfile call.  This means that it will be called
for the main objfile before GDB starts loading the debug info objfile.

There are two additional, related changes in this commit.  The new
location for the clear_symtab_users call is before
program_space::symfile_object_file is removed by unlinking the
referenced objfile, so we don't want the breakpoint_re_set call in
clear_symtab_users to resolve breakpoints against the old objfiles.
To avoid this, at the new call to clear_symtab_users, we force the
SYMFILE_DEFER_BP_RESET.  Then we restructure finish_new_objfile so
that breakpoint_re_set can always be called if appropriate.  This
replaces the old breakpoint_re_set call which was reached via the old
call to clear_symtab_users.

Additionally, in finish_new_objfile, we only set
program_space::symfile_object_file for the main objfile.  Previously
we would set this field first to the debug info objfile, and then
reset this field to the main objfile.  This set then overwrite
sequence was harmless, but also pointless.  I think by restricting
this code so we only set it to the main objfile better reflects the
state we want GDB to be in.
---
 gdb/symfile.c                                 | 33 ++++++++-------
 .../gdb.python/py-section-script.exp          | 41 +++++++++++++++++++
 2 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index c2844eb88a5..e3aa81fbc0c 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -969,20 +969,15 @@ syms_from_objfile (struct objfile *objfile,
 static void
 finish_new_objfile (struct objfile *objfile, symfile_add_flags add_flags)
 {
-  /* If this is the main symbol file we have to clean up all users of the
-     old main symbol file.  Otherwise it is sufficient to fixup all the
-     breakpoints that may have been redefined by this symbol file.  */
-  if (add_flags & SYMFILE_MAINLINE)
-    {
-      /* OK, make it the "real" symbol file.  */
-      current_program_space->symfile_object_file = objfile;
+  /* If this is the main symbol file then record it as such in the program
+     space.  Don't record any separate debug files loaded as a consequence
+     of loading the main symbol file though.  */
+  if (add_flags & SYMFILE_MAINLINE
+      && objfile->separate_debug_objfile_backlink == nullptr)
+    current_program_space->symfile_object_file = objfile;
 
-      clear_symtab_users (add_flags);
-    }
-  else if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0)
-    {
-      breakpoint_re_set ();
-    }
+  if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0)
+    breakpoint_re_set ();
 
   /* We're done reading the symbol file; finish off complaints.  */
   clear_complaints ();
@@ -1047,7 +1042,17 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
     error (_("Not confirmed."));
 
   if (mainline)
-    flags |= OBJF_MAINLINE;
+    {
+      flags |= OBJF_MAINLINE;
+
+      /* Before we create the new objfile, or load any debug symbols,
+	 discard any existing cached symbols.  Don't reset breakpoints at
+	 this point as we haven't discarded the previous main objfile yet,
+	 and we'd only end up resolving the breakpoints against the old
+	 symbols.  The breakpoints will be reset in finish_new_objfile.  */
+      if (parent == nullptr)
+	clear_symtab_users (add_flags | SYMFILE_DEFER_BP_RESET);
+    }
 
   objfile *objfile
     = objfile::make (abfd, current_program_space, name, flags, parent);
diff --git a/gdb/testsuite/gdb.python/py-section-script.exp b/gdb/testsuite/gdb.python/py-section-script.exp
index a3b9631ade7..d1a5a901570 100644
--- a/gdb/testsuite/gdb.python/py-section-script.exp
+++ b/gdb/testsuite/gdb.python/py-section-script.exp
@@ -183,4 +183,45 @@ foreach_with_prefix variant {plain compressed} {
 	gdb_test "info auto-load python-scripts no-script-matches-this" \
 	    "No auto-load scripts matching no-script-matches-this."
     }
+
+    # Unlike eu-strip, objcopy moves the .debug_gdb_scripts into the .debug
+    # file, removing it from the executable.  GDB should still load the
+    # script though, just when it loads the symbol file, not the main
+    # executable.
+    with_test_prefix "sepdebug from objcopy" {
+	clean_restart
+
+	set objcopy_testfile ${the_testfile}-objcopy
+	set objcopy_binfile [standard_output_file $objcopy_testfile]
+	file copy -force $the_binfile $objcopy_binfile
+
+	if { [gdb_gnu_strip_debug $objcopy_binfile] != 0 } {
+	    fail "strip $objcopy_testfile debuginfo"
+	    return
+	}
+
+	set_auto_load_safe_path $remote_python_file \
+	    [file dirname ${objcopy_binfile}]
+	gdb_load $objcopy_binfile
+
+	# Verify gdb loaded each script and they appear once in the list.
+	gdb_test_multiple "info auto-load python-scripts" "verify scripts loaded" {
+	    -re -wrap "Yes.*${testfile}.py.*Yes.*inlined-script.*" {
+		pass $gdb_test_name
+	    }
+	    -re -wrap "${testfile}.py.*${testfile}.py.*" {
+		fail $gdb_test_name
+	    }
+	    -re -wrap "inlined-script.*inlined-script.*" {
+		fail $gdb_test_name
+	    }
+	}
+
+	# Again, with a regexp this time.
+	gdb_test "info auto-load python-scripts ${testfile}" "Yes.*${testfile}.py.*"
+
+	# Again, with a regexp that matches no scripts.
+	gdb_test "info auto-load python-scripts no-script-matches-this" \
+	    "No auto-load scripts matching no-script-matches-this."
+    }
 }
-- 
2.25.4