[Patch] More responsive QUITs

[Patch] More responsive QUITs

[Sterling Augustine]

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

Hello,

Enclosed is a patch that adds calls to QUIT in various places which
makes GDB more responsive to CTRL-C. In particular, reading a symbol
file at start up can now be interrupted (although perhaps still not as
responsive as one might like), as well as various other long running
operations. I know at least two GDB developers have one or another of
these on their TODO lists.

All three of these calls to QUIT have relatively close cleanup handlers:

The new call in dwarf2_build_psymtabs is cleaned up by the handlers in
the same function.
Likewise for the new call in process_type_comp_unit, which is just a
level or two down.

The new call in map_symbol_filenames_psymtab occurs in a location
where I don't believe there could be any inconsistent state.

Comments?

Sterling

=-=-=-=-=-
2011-07-12  Sterling Augustine  <saugustine@google.com>

	* dwarf2read.c (process_type_comp_unit): Call QUIT.
	(dwarf2_build_psymtabs_hard): Likewise.
	* psymtab.c (map_symbol_filenames_psymtab): Likewise.

[-- Attachment #2: quit.patch --]
[-- Type: text/x-patch, Size: 1127 bytes --]

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.550
diff -d -u -r1.550 dwarf2read.c
--- dwarf2read.c	12 Jul 2011 20:59:03 -0000	1.550
+++ dwarf2read.c	12 Jul 2011 22:18:49 -0000
@@ -3417,6 +3417,7 @@
   struct objfile *objfile = (struct objfile *) info;
   struct dwarf2_per_cu_data *this_cu;
 
+  QUIT;
   this_cu = &entry->per_cu;
 
   gdb_assert (dwarf2_per_objfile->types.readin);
@@ -3500,6 +3501,7 @@
     {
       struct dwarf2_per_cu_data *this_cu;
 
+      QUIT;
       this_cu = dwarf2_find_comp_unit (info_ptr
 				       - dwarf2_per_objfile->info.buffer,
 				       objfile);
Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.30
diff -d -u -r1.30 psymtab.c
--- psymtab.c	10 Jun 2011 21:48:04 -0000	1.30
+++ psymtab.c	12 Jul 2011 22:18:49 -0000
@@ -1093,6 +1093,7 @@
       if (ps->readin)
 	continue;
 
+      QUIT;
       fullname = psymtab_to_fullname (ps);
       (*fun) (ps->filename, fullname, data);
     }

Re: [Patch] More responsive QUITs

[Tom Tromey]

>>>>> "Sterling" == Sterling Augustine <saugustine@google.com> writes:

Sterling> Enclosed is a patch that adds calls to QUIT in various places which
Sterling> makes GDB more responsive to CTRL-C. In particular, reading a symbol
Sterling> file at start up can now be interrupted (although perhaps still not as
Sterling> responsive as one might like), as well as various other long running
Sterling> operations. I know at least two GDB developers have one or another of
Sterling> these on their TODO lists.

Yeah, I'm one of those.

Sterling> All three of these calls to QUIT have relatively close cleanup
Sterling> handlers:
[...]

I agree these are safe as to their immediate surrounds; but the problem
is what happens later on.  That is, I think a quit when building
psymtabs will just leave the remaining psymtabs unread.  This will
greatly interfere with debugging.

We have lazy psymtab reading now.  So, it is tempting to try to record
where the processing was stopped and then restart it there.  However,
this may be difficult to do without leaking memory, due to the use of
obstacks.  Perhaps some leakage would be ok, though; or maybe by making
the granularity a single psymtab it would be possible not to leak at all
(not sure).

Tom

Re: [Patch] More responsive QUITs

[Sterling Augustine]

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

On Fri, Jul 15, 2011 at 1:46 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Sterling" == Sterling Augustine <saugustine@google.com> writes:
>
> Sterling> Enclosed is a patch that adds calls to QUIT in various places which
> Sterling> makes GDB more responsive to CTRL-C. In particular, reading a symbol
> Sterling> file at start up can now be interrupted (although perhaps still not as
> Sterling> responsive as one might like), as well as various other long running
> Sterling> operations. I know at least two GDB developers have one or another of
> Sterling> these on their TODO lists.
>
> Yeah, I'm one of those.
>
> Sterling> All three of these calls to QUIT have relatively close cleanup
> Sterling> handlers:
> [...]
>
> I agree these are safe as to their immediate surrounds; but the problem
> is what happens later on.  That is, I think a quit when building
> psymtabs will just leave the remaining psymtabs unread.  This will
> greatly interfere with debugging.

In my tests with the original patch, interrupting the startup
symbol-file read leaves it unavailable, and therefore needing to be
manually read or some such. That seemed reasonable to me. (But this
feature leaks memory, and so I'm withdrawing that part. See below.)

But interrupting any other time in the new locations doesn't leave
things undebuggable in any way that I can see. Every operation that
wasn't completed is retried on demand, and seems to work as far as I
can tell. I'm happy to try tests other than poking around in some of
my large files (where this is a real issue), if you have any
suggestions.

> We have lazy psymtab reading now.  So, it is tempting to try to record
> where the processing was stopped and then restart it there.  However,
> this may be difficult to do without leaking memory, due to the use of
> obstacks.  Perhaps some leakage would be ok, though; or maybe by making
> the granularity a single psymtab it would be possible not to leak at all
> (not sure).

I think that is already done. The new QUIT is guarded by "ps->readin",
so you effectively restart where you left off.

In any event, if I interrupt a long-process at one of the new quit
points inside dwarf2read.c, then restart it and let it run to
completion, GDB consumes slightly more memory than if I just start it
and let it run to completion the first time.

So the original patch leaks memory. However, without the QUITs inside
dwarf2read.c, but leave the new one in map_symbol_filenames_psymtab
then the leaks go away. This still solves my original tab-completion
quitting problem, and I can still debug.

This does remove the ability to break out of the initial symbol-file
read, which would have been a nice feature, but sounds like one to
defer to another day.

Updated patch enclosed.

Sterling

2011-07-20  Sterling Augustine  <saugustine@google.com>

       * psymtab.c (map_symbol_filenames_psymtab): Call QUIT.

[-- Attachment #2: quit.diff --]
[-- Type: text/x-patch, Size: 433 bytes --]

Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.30
diff -d -u -r1.30 psymtab.c
--- psymtab.c	10 Jun 2011 21:48:04 -0000	1.30
+++ psymtab.c	20 Jul 2011 18:18:10 -0000
@@ -1093,6 +1093,7 @@
       if (ps->readin)
 	continue;
 
+      QUIT;
       fullname = psymtab_to_fullname (ps);
       (*fun) (ps->filename, fullname, data);
     }

Re: [Patch] More responsive QUITs

[Tom Tromey]

>>>>> "Sterling" == Sterling Augustine <saugustine@google.com> writes:

Sterling> 2011-07-20  Sterling Augustine  <saugustine@google.com>
Sterling>        * psymtab.c (map_symbol_filenames_psymtab): Call QUIT.

I think this exposes gdb to a memory leak in
make_source_files_completion_list.

I think the bug is a QUIT can unwind:

   map_symbol_filenames_psymtab
-> map_partial_symbol_filenames
-> make_source_files_completion_list

... which does not use cleanups to protect the allocations it does.

Tom

Re: [Patch] More responsive QUITs

[Sterling Augustine]

On Mon, Jul 25, 2011 at 1:41 PM, Tom Tromey <tromey@redhat.com> wrote:
> Sterling> New revision of the patch which addresses this issue attached.
>
> You forgot to CC the list :)
>
> Sterling> +  struct cleanup *back_to;
>
> Sterling>    list[0] = NULL;
>
> Sterling>    if (!have_full_symbols () && !have_partial_symbols ())
> Sterling>      return list;
>
> Sterling> +  back_to = make_cleanup (xfree, list);
>
> This captures 'list' at this point in time, but list can be reallocated.
> See add_filename_to_list.

Will work on a fix for this.

> Also, each element of list is itself mallocd and so must be freed.

The calling function (completer.c:308 and below) doesn't free the
individual elements either, so there is a more general gdb leak. I'll
fix both.

Sterling

Re: [Patch] More responsive QUITs

[Tom Tromey]

>>>>> "Sterling" == Sterling Augustine <saugustine@google.com> writes:

Sterling> The calling function (completer.c:308 and below) doesn't free the
Sterling> individual elements either, so there is a more general gdb leak. I'll
Sterling> fix both.

I think location_completer is moving the items from one list to another,
then relying on the caller to eventually free them.

This makes me wonder whether the callers of
make_file_symbol_completion_list also need cleanups though.

Tom