Re: [patch] Do not close BFDs, breaking deleted inferior shlibs

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

On Tue, 17 Feb 2015 08:44:58 +0100, Doug Evans wrote:
> Hi.  The comments in the code for the call to bfd_cache_close_all
> explain the windows situation well enough, but IIUC it is necessary
> to *not* call bfd_cache_close_all here on linux, but there are no
> comments in the code explaining this.
> Can you add something?  Thanks.

Added.


> Also, IIUC there's a fragileness here that we still need to address.
> AFAICT there's nothing in the bfd cache API that prevents bfd
> from randomly closing the file in the future, and yet from reading
> the bug report it is necessary that we do not close the file.

Primarily it is always racy.  At _r_debug_state (or probe) time the file may
be already mapped in inferior and already unlinked from disk.
See /proc/PID/mem discussion below.

And yes, personally I do not think there should ever be any "BFD cache" and
any closing of fds.  Defaults:
ulimit -a -S
open files                      (-n) 1024
ulimit -a -H
open files                      (-n) 4096

GDB could even increase the rlimit to 4096 by default and that should be
enough for everybody incl. Google (==2000 shared libraries with separate
.debug file).
I haven't checked if -gsplit-dwarf could need even more open fds?

But for some reason BFD contains this FDs closing complexity, by default the
limit is 1024/8==128 files==64 shared libraries which is less than minimal
Linux GUI apps.


> E.g., does gdb need to keep open, for example, every shared library
> used by the inferior?

Do you mean that GDB may no longer need to access BFD for a file after it is
initially mapped?  I would not depend on that...


> Perhaps a better way to go would be to teach gdb to handle
> the file going way.

Do you mean like elfutils to read for unlinked files /proc/PID/mem instead?
One cannot access unmapped areas of the file then.


Jan

gdb/ChangeLog
2015-02-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* corefile.c (reopen_exec_file): Wrap bfd_cache_close_all call into
	MS-Windows conditional.
	* exec.c (exec_file_attach): Likewise.
	* symfile.c (symbol_file_add_with_addrs): Likewise.

gdb/testsuite/ChangeLog
2015-02-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/close-deleted-bfd-lib.c: New file.
	* gdb.base/close-deleted-bfd-main.c: New file.
	* gdb.base/close-deleted-bfd.exp: New file.

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a042e6d..bee2b0f 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -148,10 +148,18 @@ reopen_exec_file (void)
   if (exec_bfd_mtime && exec_bfd_mtime != st.st_mtime)
     exec_file_attach (filename, 0);
   else
-    /* If we accessed the file since last opening it, close it now;
-       this stops GDB from holding the executable open after it
-       exits.  */
-    bfd_cache_close_all ();
+    {
+      /* MS-Windows cannot replace file with a new build if the file is
+	 still open.  Therefore close the file now, BFD will reopen it
+	 when needed.
+	 On other systems it is better to keep the file open, both for
+	 performance reasons and also if the file gets unlinked on disk
+	 it still executes fine and GDB should still be able to continue
+	 debugging it.  */
+#if defined _WIN32 || defined __CYGWIN__
+      bfd_cache_close_all ();
+#endif
+    }
 
   do_cleanups (cleanups);
 }
diff --git a/gdb/exec.c b/gdb/exec.c
index 124074f..b1f6157 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -258,7 +258,11 @@ exec_file_attach (const char *filename, int from_tty)
 
   do_cleanups (cleanups);
 
+  // See reopen_exec_file for the reasons.
+#if defined _WIN32 || defined __CYGWIN__
   bfd_cache_close_all ();
+#endif
+
   observer_notify_executable_changed ();
 }
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index c2a71ec..45277b3 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1238,7 +1238,11 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name, int add_flags,
 
   observer_notify_new_objfile (objfile);
 
+  // See reopen_exec_file for the reasons.
+#if defined _WIN32 || defined __CYGWIN__
   bfd_cache_close_all ();
+#endif
+
   return (objfile);
 }
 
diff --git a/gdb/testsuite/gdb.base/close-deleted-bfd-lib.c b/gdb/testsuite/gdb.base/close-deleted-bfd-lib.c
new file mode 100644
index 0000000..782302c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/close-deleted-bfd-lib.c
@@ -0,0 +1,22 @@
+/* Copyright 2015 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>
+
+void
+foo (void)
+{
+  raise (SIGUSR1);
+}
diff --git a/gdb/testsuite/gdb.base/close-deleted-bfd-main.c b/gdb/testsuite/gdb.base/close-deleted-bfd-main.c
new file mode 100644
index 0000000..669a275
--- /dev/null
+++ b/gdb/testsuite/gdb.base/close-deleted-bfd-main.c
@@ -0,0 +1,34 @@
+/* Copyright 2015 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 <dlfcn.h>
+#include <assert.h>
+#include <stddef.h>
+
+int
+main (void)
+{
+  void *handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+  void (*fp) (void);
+
+  assert (handle != NULL);
+
+  fp = (void (*) (void)) dlsym (handle, "foo");
+  assert (fp != NULL); /* break-here */
+
+  fp ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/close-deleted-bfd.exp b/gdb/testsuite/gdb.base/close-deleted-bfd.exp
new file mode 100644
index 0000000..cdbbd33
--- /dev/null
+++ b/gdb/testsuite/gdb.base/close-deleted-bfd.exp
@@ -0,0 +1,47 @@
+# Copyright 2015 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 { [skip_shlib_tests] || [is_remote target] } {
+    return 0
+}
+
+standard_testfile close-deleted-bfd-main.c close-deleted-bfd-lib.c
+
+set libname $testfile-solib
+set binfile_lib [standard_output_file $libname.so]
+
+if { [gdb_compile_shlib $srcdir/$subdir/$srcfile2 $binfile_lib debug] != "" } {
+    untested "Could not compile $binfile_lib."
+    return -1
+}
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile \
+      [list debug shlib_load additional_flags=-DSHLIB_NAME=\"$binfile_lib\"]] } {
+    return -1
+}
+
+if ![runto_main] then {
+    return 0
+}
+
+gdb_breakpoint [gdb_get_line_number "break-here"]
+gdb_continue_to_breakpoint "break-here" ".* break-here .*"
+
+file rename -force -- $binfile_lib $binfile_lib-moved
+
+gdb_test "continue" "\r\nProgram received signal SIGUSR1, .*"
+
+# FAIL was: BFD: reopening $$binfile_lib: No such file or directory
+gdb_test "bt" " in foo \[^\r\n\]*\r\n\[^\r\n\]* in main \[^\r\n\]*"