Make file-based lookup more interruptable
Commit Message
Hi.
This patch makes, for example, "b 'foo.c':bar" more interruptable.
I put the QUIT calls at two logical levels for robustness and clarity:
iterate_over_symtabs and map_symtabs_matching_filename.
I plan to do the same for other QUIT calls I've already added
and plan to add.
2015-05-15 Doug Evans <dje@google.com>
* completer.c (location_completer): Protect file_to_match with cleanup.
* dwarf2read.c (dw2_map_symtabs_matching_filename): Add QUIT calls.
* psymtab.c (psym_map_symtabs_matching_filename): Add QUIT call.
* symtab.c (iterate_over_symtabs): Add QUIT calls.
Comments
Doug Evans <dje@google.com> writes:
> This patch makes, for example, "b 'foo.c':bar" more interruptable.
Does 'interruptable' mean user can type ctrl-c to cancel the command
"b 'foo.c':bar" which may take 10 minutes? If so, this change is
visible to users, and do we need to add a NEWS entry?
On Tue, May 19, 2015 at 4:55 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Doug Evans <dje@google.com> writes:
>
>> This patch makes, for example, "b 'foo.c':bar" more interruptable.
>
> Does 'interruptable' mean user can type ctrl-c to cancel the command
> "b 'foo.c':bar" which may take 10 minutes? If so, this change is
> visible to users, and do we need to add a NEWS entry?
I'd classify this as being no different than a performance improvement.
There already is a level of interruptability, the only change is in degree
of responsiveness.
We *can* start adding such things to NEWS, but we don't do so today.
Before we start imposing this on everyone, we'll need to establish
what does and does not qualify - I'm guessing we don't want to add
a NEWS entry for every little performance improvement.
On Fri, May 22, 2015 at 10:46 AM, Doug Evans <dje@google.com> wrote:
> On Tue, May 19, 2015 at 4:55 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> Doug Evans <dje@google.com> writes:
>>
>>> This patch makes, for example, "b 'foo.c':bar" more interruptable.
>>
>> Does 'interruptable' mean user can type ctrl-c to cancel the command
>> "b 'foo.c':bar" which may take 10 minutes? If so, this change is
>> visible to users, and do we need to add a NEWS entry?
>
> I'd classify this as being no different than a performance improvement.
> There already is a level of interruptability, the only change is in degree
> of responsiveness.
>
> We *can* start adding such things to NEWS, but we don't do so today.
> Before we start imposing this on everyone, we'll need to establish
> what does and does not qualify - I'm guessing we don't want to add
> a NEWS entry for every little performance improvement.
Hi.
"ping" :-)
Whaddayathink? Should we start adding perf improvements to NEWS
(and try to classify what is newsworthy)?
Or let it go, at least for now?
[Sorry for being late]
On 22/05/15 18:46, Doug Evans wrote:
> I'd classify this as being no different than a performance improvement.
I think they are different. IMO, performance improvement is the time
or memory reduction to some degree. This kind of change can be user
visible (10 min -> 10s) and can be user invisible (2s -> 1s).
> There already is a level of interruptability, the only change is in degree
> of responsiveness.
>
> We*can* start adding such things to NEWS, but we don't do so today.
> Before we start imposing this on everyone, we'll need to establish
> what does and does not qualify - I'm guessing we don't want to add
> a NEWS entry for every little performance improvement.
However, this change isn't performance improvement, because it doesn't
shorten the time of operation like "b 'foo.c':bar". This change only
makes GDB more interruptable during some operations, IMO this is a good
news to users and we should let them know.
Regarding to NEWS entry for performance improvement, I don't have an
opinion on it.
On Tue, Jun 2, 2015 at 12:59 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> This kind of change [a perf improvement can be user
> visible (10 min -> 10s) and can be user invisible (2s -> 1s).
An improvement of 10min -> 10s (or greater, I have made several)
would be "good news to users", no?
> However, this change isn't performance improvement, because it doesn't
> shorten the time of operation like "b 'foo.c':bar".
Oh?
"b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
How is this any different?
> This change only
> makes GDB more interruptable during some operations, IMO this is a good
> news to users and we should let them know.
On 02/06/15 13:34, Doug Evans wrote:
> An improvement of 10min -> 10s (or greater, I have made several)
> would be "good news to users", no?
They are good news to users, of course. However, whether letting users
know them is another thing we need to decide.
>
>> >However, this change isn't performance improvement, because it doesn't
>> >shorten the time of operation like "b 'foo.c':bar".
> Oh?
> "b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
> How is this any different?
>
Operation "b 'foo.c':bar" becomes more interruptable rather than more
efficient.
On Tue, Jun 2, 2015 at 6:08 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>>> >However, this change isn't performance improvement, because it doesn't
>>> >shorten the time of operation like "b 'foo.c':bar".
>>
>> Oh?
>> "b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
>> How is this any different?
>>
>
> Operation "b 'foo.c':bar" becomes more interruptable rather than more
> efficient.
Eh?
I'm sorry but the difference is just lost on me.
Ok, how about we add a rule saying that the addition of any calls to
QUIT shall require a NEWS entry.
[Since that is what you are requesting.]
On 02/06/15 14:20, Doug Evans wrote:
> Eh?
> I'm sorry but the difference is just lost on me.
>
> Ok, how about we add a rule saying that the addition of any calls to
> QUIT shall require a NEWS entry.
> [Since that is what you are requesting.]
Hi Doug,
I don't insist on adding a NEWS entry for this change. If it is not
necessary to do so for you, I am OK too.
The intention of my suggestion of adding NEWS entry for this change is
to show the good improvements to users as much as we can.
@@ -195,7 +195,6 @@ location_completer (struct cmd_list_element *ignore,
int quoted = *text == '\'' || *text == '"';
int quote_char = '\0';
const char *colon = NULL;
- char *file_to_match = NULL;
const char *symbol_start = text;
const char *orig_text = text;
size_t text_len;
@@ -241,12 +240,17 @@ location_completer (struct cmd_list_element *ignore,
text++;
text_len = strlen (text);
- /* Where is the file name? */
- if (colon)
+ /* Where is the file name?
+ If the text includes a colon, they want completion only on a
+ symbol name after the colon. Otherwise, we need to complete on
+ symbols as well as on files. */
+ if (colon != NULL)
{
- char *s;
+ char *s, *file_to_match;
+ struct cleanup *cleanup;
- file_to_match = (char *) xmalloc (colon - text + 1);
+ file_to_match = xmalloc (colon - text + 1);
+ cleanup = make_cleanup (xfree, file_to_match);
strncpy (file_to_match, text, colon - text + 1);
/* Remove trailing colons and quotes from the file name. */
for (s = file_to_match + (colon - text);
@@ -254,15 +258,9 @@ location_completer (struct cmd_list_element *ignore,
s--)
if (*s == ':' || *s == quote_char)
*s = '\0';
- }
- /* If the text includes a colon, they want completion only on a
- symbol name after the colon. Otherwise, we need to complete on
- symbols as well as on files. */
- if (colon)
- {
list = make_file_symbol_completion_list (symbol_start, word,
file_to_match);
- xfree (file_to_match);
+ do_cleanups (cleanup);
}
else
{
@@ -3448,6 +3448,8 @@ dw2_map_symtabs_matching_filename (struct objfile
*objfile, const char *name,
struct dwarf2_per_cu_data *per_cu = dw2_get_cu (i);
struct quick_file_names *file_data;
+ QUIT;
+
/* We only need to look at symtabs not already expanded. */
if (per_cu->v.quick->compunit_symtab)
continue;
@@ -3461,6 +3463,8 @@ dw2_map_symtabs_matching_filename (struct objfile
*objfile, const char *name,
const char *this_name = file_data->file_names[j];
const char *this_real_name;
+ QUIT;
+
if (compare_filenames_for_search (this_name, name))
{
if (dw2_map_expand_apply (objfile, per_cu, name, real_path,
@@ -169,6 +169,8 @@ psym_map_symtabs_matching_filename (struct objfile
*objfile,
ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
{
+ QUIT;
+
/* We can skip shared psymtabs here, because any file name will be
attached to the unshared psymtab. */
if (pst->user != NULL)
@@ -443,6 +443,8 @@ iterate_over_symtabs (const char *name,
ALL_OBJFILES (objfile)
{
+ QUIT;
+
if (iterate_over_some_symtabs (name, real_path, callback, data,
objfile->compunit_symtabs, NULL))
{
@@ -456,6 +458,8 @@ iterate_over_symtabs (const char *name,
ALL_OBJFILES (objfile)
{
+ QUIT;
+
if (objfile->sf
&& objfile->sf->qf->map_symtabs_matching_filename (objfile,
name,