[patch] release handle on object files after program exits

[patch] release handle on object files after program exits

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hello,

One of our Windows users reported that he was not able to delete
the exe file even after the program was run to completion. As it
turns out, we did fix something similar a while ago but only when
the program was kill-ed.

The attached patch fixes the problem in the case when the program
is let to run to completion.

2009-04-03  Joel Brobecker  <brobecker@adacore.com>

        * target.c (target_mourn_inferior): Call bfd_cache_close_all.

Tested on x86_64-linux. Also tested on x86-windows using AdaCore's
testsuite.

I will write a testcase for this one. I have to run now, but I wanted
to put this patch out, in case someone has some comments about it.
Will commit in a few days if no objection.

One thing that crossed my mind while working on this is wondering
whether it would make sense for target_kill to call target_mourn_inferior
at the end. Right now, it looks like a lot (most? all?) implementations
of the target_kill method call target_mourn_inferior...

-- 
Joel

[-- Attachment #2: mourn.diff --]
[-- Type: text/x-diff, Size: 527 bytes --]

diff --git a/gdb/target.c b/gdb/target.c
index 86cdb71..3c37ee4 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1934,6 +1934,12 @@ void
 target_mourn_inferior (void)
 {
   struct target_ops *t;
+
+  /* We no longer need to keep handles on any of the object files.
+     Make sure to release them to avoid unnecessarily locking any
+     of them while we're not actually debugging.  */
+  bfd_cache_close_all ();
+
   for (t = current_target.beneath; t != NULL; t = t->beneath)
     {
       if (t->to_mourn_inferior != NULL)	

Re: [patch] release handle on object files after program exits

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> 2009-04-03  Joel Brobecker  <brobecker@adacore.com>
Joel>         * target.c (target_mourn_inferior): Call bfd_cache_close_all.

I wonder how this will interact with the multi-process support.
It seems like too big a hammer.

Please don't consider this an objection.

Tom

Re: [patch] release handle on object files after program exits

[Joel Brobecker]

Hi Tom,

> Joel> 2009-04-03  Joel Brobecker  <brobecker@adacore.com>
> Joel>         * target.c (target_mourn_inferior): Call bfd_cache_close_all.
> 
> I wonder how this will interact with the multi-process support.
> It seems like too big a hammer.

I finally had time to look deeper into this - I came up with the patch
while looking at what we did for the "kill" command, so I didn't have
to think too much about this.

As far as I can tell, things should still be fine, because the file
should automatically be re-opened if GDB tries to read from the bfd
again. Perhaps we could possibly improve the situation, but I propose
we do that if the current approach becomes an actual problem? (in which
case we can look at fixing both cases - kill and exit)

-- 
Joel

Re: [patch] release handle on object files after program exits

[Pedro Alves]

A Wednesday 08 April 2009 17:48:39, Joel Brobecker wrote:
> Hi Tom,
> 
> > Joel> 2009-04-03  Joel Brobecker  <brobecker@adacore.com>
> > Joel>         * target.c (target_mourn_inferior): Call bfd_cache_close_all.
> > 
> > I wonder how this will interact with the multi-process support.
> > It seems like too big a hammer.
> 
> I finally had time to look deeper into this - I came up with the patch
> while looking at what we did for the "kill" command, so I didn't have
> to think too much about this.
> 
> As far as I can tell, things should still be fine, because the file
> should automatically be re-opened if GDB tries to read from the bfd
> again. Perhaps we could possibly improve the situation, but I propose
> we do that if the current approach becomes an actual problem? (in which
> case we can look at fixing both cases - kill and exit)
> 

I'm left wondering if generic_mourn_inferior wouldn't be a better place
for this.  That is, at the tail end of mourning, instead of before mourning,
which e.g., has a better change of not triggering a file reopen.
generic_mourn_inferior already calls reopen_exec_file, which
conditionaly calls bfd_cache_close_all.

-- 
Pedro Alves

Re: [patch] release handle on object files after program exits

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

Hi Pedro,

> I'm left wondering if generic_mourn_inferior wouldn't be a better place
> for this.  That is, at the tail end of mourning, instead of before mourning,
> which e.g., has a better change of not triggering a file reopen.
> generic_mourn_inferior already calls reopen_exec_file, which
> conditionaly calls bfd_cache_close_all.

Looks like generic_mourn_inferior is not necessarily called at
the end of the execution of the program. Lots of targets actually
call it at the end of their own mourn_inferior, but it is conceivable
that some may not.

But you do have a point.  How about the following patch instead?
Instead of closing all descriptors before calling the mourn_inferior
routine, it does it after.

2009-04-08  Joel Brobecker  <brobecker@adacore.com>

        * target.c (target_mourn_inferior): Call bfd_cache_close_all
        after having executed the target mourn_inferior routine.

Tested on x86-windows.

-- 
Joel

[-- Attachment #2: target.c.diff --]
[-- Type: text/x-diff, Size: 590 bytes --]

Index: target.c
===================================================================
--- target.c	(revision 147356)
+++ target.c	(working copy)
@@ -1947,6 +1942,12 @@ target_mourn_inferior (void)
 	  t->to_mourn_inferior (t);
 	  if (targetdebug)
 	    fprintf_unfiltered (gdb_stdlog, "target_mourn_inferior ()\n");
+
+          /* We no longer need to keep handles on any of the object files.
+             Make sure to release them to avoid unnecessarily locking any
+             of them while we're not actually debugging.  */
+          bfd_cache_close_all ();
+
 	  return;
 	}
     }

Re: [patch] release handle on object files after program exits

[Pedro Alves]

On Wednesday 08 April 2009 23:38:29, Joel Brobecker wrote:
> Looks like generic_mourn_inferior is not necessarily called at
> the end of the execution of the program. Lots of targets actually
> call it at the end of their own mourn_inferior, but it is conceivable
> that some may not.

Hmmmm, all process_stratum targets do, but yes, I can imagine
a target not calling it.  We might have one internally --- remote
targets where a "process exit" is just an event and still leaves
execution in the target.  Oh, and when debugging multi-forks in linux,
and one of the forks exits, we skip the normal mourning tail end.  (Gosh,
how the checkpoints support is in the way for multiprocess...)  Not
that that matters much for Windows though.

> But you do have a point.  How about the following patch instead?
> Instead of closing all descriptors before calling the mourn_inferior
> routine, it does it after.

Looks fine to me.  Thanks!

-- 
Pedro Alves

Re: [patch] release handle on object files after program exits

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

> 2009-04-08  Joel Brobecker  <brobecker@adacore.com>
> 
>         * target.c (target_mourn_inferior): Call bfd_cache_close_all
>         after having executed the target mourn_inferior routine.

I just checked this patch in.
I also wrote the following testcase, which I checked in as well.

2009-04-14  Joel Brobecker  <brobecker@adacore.com>

        * gdb.base/exe-lock.exp: New testcase.

Tested on x86_64-linux.
-- 
Joel

[-- Attachment #2: exe-lock.exp --]
[-- Type: text/plain, Size: 2002 bytes --]

# Copyright 2009 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 $tracelevel {
    strace $tracelevel
}

# The intent of this testcase is to verify that GDB does not keep
# a filesystem lock on the executable file once the executable
# is no longer running.

set testfile "arrayidx"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
    untested "Couldn't compile ${srcfile}"
    return -1
}

gdb_exit
gdb_start
gdb_reinitialize_dir $srcdir/$subdir
gdb_load ${binfile}

# Sanity-check: Verify that the executable exists. This is just to
# make sure that, when we verify later that the file does not exist,
# it really has been deleted.
if { ! [file exists $binfile] } {
    fail "executable does not exist (${binfile})"
    return -1
    }

if ![runto_main] then {
    perror "couldn't run to breakpoint"
    continue
}

gdb_test "continue" \
         ".*Program exited normally\\." \
         "continue until program exits"

# Try deleting the executable file, now that the program has exited,
# and make sure that the deletion worked by verifying that the exe
# is no longer there (the "file delete" tcl command does not return
# any status, apparently).
file delete $binfile
if { [file exists $binfile] } {
    fail "executable still exists (${binfile})"
    }