Re: [non-commit patch] Fix gdbserver qXfer:libraries-svr4 regression in special cases

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 05 Mar 2012 16:53:02 +0100, Pedro Alves wrote:
> GDBserver can ask GDB the address of _r_debug, using the qSymbols mechanism,
> so it doesn't look that much trouble.  WDYT of the alternative patch below?
> It fixes the testcase too.

I worked on it and it works great; just the testcase was bogus.

The testcase did:
	gdbserver ld.so prog &
	gdb:
	file ld.so
	target remote ...
	file prog
But in reality people do:
	gdbserver ld.so prog &
	gdb:
	file prog
	target remote ...

In this case qSymbol for _r_debug cannot work as GDB will never find any new
library, therefore GDB will never find ld.so to find _r_debug there.
A chicken-and-egg problem.

There was another issue being dealt with in the patch below that originally
GDB send qXfer:libraries-svr4:read before qSymbol and therefor GDB did the
expensive (and unsuccessful) non-accelerated reading from gdbserver for the
first time.

> @@ -4522,6 +4523,8 @@ linux_look_up_symbols (void)
>  #ifdef USE_THREAD_DB
>    struct process_info *proc = current_process ();
>  
> +  svr4_look_up_symbols ();
> +
>    if (proc->private->thread_db != NULL)
>      return;
>  
 - Also here it is put into USE_THREAD_DB conditional, I think it should not
   be related to USE_THREAD_DB.

If you move 'gdb_test "file ${binfile}"'... down before the "continue" command
it works great; but it is not useful in practice.

So I plan to use the former patch of mine to just always fall-back to the
non-accelerated linux-nat reading of memory.


Thanks,
Jan


diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 043451d..56d0d26 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -205,6 +205,7 @@ static int finish_step_over (struct lwp_info *lwp);
 static CORE_ADDR get_stop_pc (struct lwp_info *lwp);
 static int kill_lwp (unsigned long lwpid, int signo);
 static void linux_enable_event_reporting (int pid);
+static void svr4_look_up_symbols (void);
 
 /* True if the low target can hardware single-step.  Such targets
    don't need a BREAKPOINT_REINSERT_ADDR callback.  */
@@ -4656,6 +4657,8 @@ linux_look_up_symbols (void)
 #ifdef USE_THREAD_DB
   struct process_info *proc = current_process ();
 
+  svr4_look_up_symbols ();
+
   if (proc->private->thread_db != NULL)
     return;
 
@@ -5278,6 +5281,23 @@ linux_get_min_fast_tracepoint_insn_len (void)
   return (*the_low_target.get_min_fast_tracepoint_insn_len) ();
 }
 
+/* Cache all future symbols that svr4 handling might request.  We can
+   not request symbols at arbitrary states in the remote protocol,
+   only when the client tells us that new symbols are available.  */
+
+static void
+svr4_look_up_symbols (void)
+{
+  struct process_info *proc = current_process ();
+  struct process_info_private *priv = proc->private;
+
+  if (priv->r_debug != 0 && priv->r_debug != (CORE_ADDR) -1)
+    return;
+
+  clear_symbol_cache (&current_process ()->symbol_cache);
+  look_up_one_symbol ("_r_debug", &priv->r_debug_from_syms, 1);
+}
+
 /* Extract &phdr and num_phdr in the inferior.  Return 0 on success.  */
 
 static int
@@ -5418,11 +5438,12 @@ get_dynamic (const int pid, const int is_elf64)
   return 0;
 }
 
-/* Return &_r_debug in the inferior, or -1 if not present.  Return value
-   can be 0 if the inferior does not yet have the library list initialized.  */
+/* Return &_r_debug in the inferior (via PT_DYNAMIC), or -1 if not
+   present.  Return value can be 0 if the inferior does not yet have
+   the library list initialized.  */
 
 static CORE_ADDR
-get_r_debug (const int pid, const int is_elf64)
+get_r_debug_dynamic (const int pid, const int is_elf64)
 {
   CORE_ADDR dynamic_memaddr;
   const int dyn_size = is_elf64 ? sizeof (Elf64_Dyn) : sizeof (Elf32_Dyn);
@@ -5528,6 +5549,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   struct process_info_private *const priv = current_process ()->private;
   char filename[PATH_MAX];
   int pid, is_elf64;
+  CORE_ADDR r_debug;
 
   static const struct link_map_offsets lmo_32bit_offsets =
     {
@@ -5562,10 +5584,28 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
   is_elf64 = elf_64_file_p (filename);
   lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
 
+  /* We'll only try this once per inferior.  */
   if (priv->r_debug == 0)
-    priv->r_debug = get_r_debug (pid, is_elf64);
+    {
+      priv->r_debug = get_r_debug_dynamic (pid, is_elf64);
 
-  if (priv->r_debug == (CORE_ADDR) -1 || priv->r_debug == 0)
+      /* If there's no DT_DEBUG, r_debug will now be -1.  */
+    }
+
+  /* This may be a static executable.  Look for the symbol
+     conventionally named _r_debug, as a last resort.  */
+  if (priv->r_debug == (CORE_ADDR) -1 && priv->r_debug_from_syms != 0)
+    r_debug = priv->r_debug_from_syms;
+  else
+    r_debug = priv->r_debug;
+
+  /* We failed to find DT_DEBUG.  Such situation will not change for this
+     inferior - do not retry it.  Report it to GDB as E01, see for the reasons
+     at the GDB solib-svr4.c side.  */
+  if (r_debug == (CORE_ADDR) -1)
+    return -1;
+
+  if (r_debug == 0)
     {
       document = xstrdup ("<library-list-svr4 version=\"1.0\"/>\n");
     }
@@ -5582,7 +5622,7 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
       p = document + strlen (document);
 
       r_version = 0;
-      if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
+      if (linux_read_memory (r_debug + lmo->r_version_offset,
 			     (unsigned char *) &r_version,
 			     sizeof (r_version)) != 0
 	  || r_version != 1)
@@ -5591,11 +5631,11 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	  goto done;
 	}
 
-      if (read_one_ptr (priv->r_debug + lmo->r_map_offset,
+      if (read_one_ptr (r_debug + lmo->r_map_offset,
 			&lm_addr, ptr_size) != 0)
 	{
 	  warning ("unable to read r_map from 0x%lx",
-		   (long) priv->r_debug + lmo->r_map_offset);
+		   (long) r_debug + lmo->r_map_offset);
 	  goto done;
 	}
 
@@ -5661,6 +5701,11 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	      p = p + strlen (p);
 	    }
 
+	  /* We have found at least one valid entry.  No longer try to look up
+	     R_DEBUG_FROM_SYMS which is expensive.  */
+	  if (priv->r_debug == (CORE_ADDR) -1 && priv->r_debug_from_syms != 0)
+	    priv->r_debug = priv->r_debug_from_syms;
+
 	  if (l_next == 0)
 	    break;
 
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 07eda12..e25458b 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -56,8 +56,14 @@ struct process_info_private
      thread_db, and it is active.  */
   struct thread_db *thread_db;
 
-  /* &_r_debug.  0 if not yet determined.  -1 if no PT_DYNAMIC in Phdrs.  */
+  /* &_r_debug.  0 if not yet determined.  -1 if no PT_DYNAMIC in
+     Phdrs, and not found by querying GDB's symbols for _r_debug's
+     address either.  */
   CORE_ADDR r_debug;
+
+  /* Fallback &_r_debug, from qSymbols, for static executables.  0 if
+     not asked/found yet.  */
+  CORE_ADDR r_debug_from_syms;
 };
 
 struct lwp_info;
diff --git a/gdb/remote.c b/gdb/remote.c
index caf6116..2cf5657 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3406,6 +3406,9 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 
       get_offsets ();		/* Get text, data & bss offsets.  */
 
+      if (exec_bfd)		/* No use without an exec file.  */
+	remote_check_symbols (symfile_objfile);
+
       /* If we could not find a description using qXfer, and we know
 	 how to do it some other way, try again.  This is not
 	 supported for non-stop; it could be, but it is tricky if
@@ -3492,13 +3495,13 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 
       /* Report all signals during attach/startup.  */
       remote_pass_signals (0, NULL);
-    }
 
-  /* If we connected to a live target, do some additional setup.  */
-  if (target_has_execution)
-    {
-      if (exec_bfd) 	/* No use without an exec file.  */
-	remote_check_symbols (symfile_objfile);
+      /* If we connected to a live target, do some additional setup.  */
+      if (target_has_execution)
+	{
+	  if (exec_bfd) 	/* No use without an exec file.  */
+	    remote_check_symbols (symfile_objfile);
+	}
     }
 
   /* Possibly the target has been engaged in a trace run started
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 69d3cb5..9049443 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1260,6 +1260,14 @@ svr4_current_sos (void)
   int ignore_first;
   struct svr4_library_list library_list;
 
+  /* Fall back to manual examination of the target if the packet is not
+     supported or gdbserver failed to find DT_DEBUG.  gdb.server/solib-list.exp
+     tests a case where gdbserver cannot find the shared libraries list while
+     GDB itself is able to find it via SYMFILE_OBJFILE.
+
+     Unfortunately statically linked inferiors will also fall back through this
+     suboptimal code path.  */
+
   if (svr4_current_sos_via_xfer_libraries (&library_list))
     {
       if (library_list.main_lm)
diff --git a/gdb/testsuite/gdb.server/solib-list-lib.c b/gdb/testsuite/gdb.server/solib-list-lib.c
new file mode 100644
index 0000000..504a34e
--- /dev/null
+++ b/gdb/testsuite/gdb.server/solib-list-lib.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 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 <signal.h>
+
+static int libvar = 23;
+
+int
+libfunc (void)
+{
+  raise (SIGUSR1);
+
+  return libvar;
+}
diff --git a/gdb/testsuite/gdb.server/solib-list-main.c b/gdb/testsuite/gdb.server/solib-list-main.c
new file mode 100644
index 0000000..8abee3f
--- /dev/null
+++ b/gdb/testsuite/gdb.server/solib-list-main.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 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/>.  */
+
+extern int libfunc (void);
+
+int
+main (void)
+{
+  libfunc ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/solib-list.exp b/gdb/testsuite/gdb.server/solib-list.exp
new file mode 100644
index 0000000..6d98549
--- /dev/null
+++ b/gdb/testsuite/gdb.server/solib-list.exp
@@ -0,0 +1,104 @@
+# Copyright 2010-2012 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/>.  */
+
+# In some cases gdbserver cannot find the DT_DEBUG library list on its own and
+# it needs to fall back to GDB solib-svr4.c look up via symbols from the main
+# executable.
+# gdbserver :1234 ld-linux-x86-64.so.2 /bin/prog
+# gdb /bin/prog
+# (gdb) target remote ...
+
+load_lib gdbserver-support.exp
+load_lib prelink-support.exp
+
+if {[skip_gdbserver_tests] || [skip_shlib_tests]} {
+    return
+}
+
+set testfile "solib-list"
+set srclibfile ${testfile}-lib.c
+set srcfile ${testfile}-main.c
+set binlibfile ${objdir}/${subdir}/${testfile}.so
+set executable ${testfile}
+set objfile ${objdir}/${subdir}/${executable}.o
+set binfile ${objdir}/${subdir}/${executable}
+
+if { [get_compiler_info unused]
+     || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfile}" "${binlibfile}" [list debug ldflags=-Wl,-soname,${binlibfile}]] != ""
+     || [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objfile}" object {debug}] != ""
+     || [gdb_compile "${objfile} ${binlibfile}" "${binfile}" executable {}] != "" } {
+    untested "could not compile sources"
+    return -1
+}
+
+set interp_system [section_get ${binfile} .interp]
+verbose -log "system interpreter is: $interp_system"
+
+foreach nonstop { 0 1 } { with_test_prefix "non-stop $nonstop" {
+
+    # clean_restart assumes ${objdir}/${subdir}/ which is not applicable
+    # for ${interp_system}.
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load ${interp_system}
+    gdb_load_shlibs ${binfile}
+    gdb_load_shlibs ${binlibfile}
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    gdb_test "set non-stop $nonstop"
+    gdb_test "set target-async $nonstop"
+    #gdb_test "set debug remote 1"
+
+    # It is required for the non-stop mode, GDB would try to step over
+    # _dl_debug_state breakpoint will still only ld.so loaded in gdbserver.
+    # But GDB having symbols from the main executable it would try to use
+    # displaced-stepping buffer at unmapped that time address _start.
+    gdb_test "set displaced-stepping off"
+	
+    set res [gdbserver_spawn ${binfile}]
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+
+    # Load BINFILE after gdbserver_spawn (so that gdbserver executes ld.so)
+    # but before "target remote" below so that qSymbol data get already
+    # initialized from BINFILE (and not from ld.so first needing a change to
+    # BINFILE later).
+    gdb_test "file ${binfile}" {Reading symbols from .*\.\.\.done\..*} "file binfile" \
+	     {(Are you sure you want to change the file|Load new symbol table from ".*")\? \(y or n\) } "y"
+
+    set test "target $gdbserver_protocol"
+    gdb_test_multiple "target $gdbserver_protocol $gdbserver_gdbport" $test {
+	-re "Remote debugging using .*\r\n$gdb_prompt " {
+	    # Do not anchor end, there may be more output in non-stop mode.
+	    pass $test
+	}
+    }
+
+    if $nonstop {
+	set test "non-stop interior stop"
+	gdb_test_multiple "" $test {
+	    -re " #1 stopped\\.\r\n" {
+		pass $test
+	    }
+	}
+    }
+
+    gdb_test "continue" "Program received signal SIGUSR1, .*"
+    gdb_test "p libvar" " = 23"
+}}