Be lazy about refreshing the windows in tui_show_frame_info (PR tui/13378)
Commit Message
This revised patch makes sure that tui_set_locator_info returns 1 when the
locator is first constructed, just in case none of the later checks trigger
for some reason.
gdb/ChangeLog:
* tui/tui-stack.c (tui_set_locator_info): Change prototype to
return an int instead of void. Return whether the locator
window has changed.
(tui_show_frame_info): If the locator info has not changed, then
bail out early to avoid refreshing the windows.
---
gdb/tui/tui-stack.c | 67 ++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 16 deletions(-)
Comments
On 06/30/2015 03:32 AM, Patrick Palka wrote:
> This revised patch makes sure that tui_set_locator_info returns 1 when the
> locator is first constructed, just in case none of the later checks trigger
> for some reason.
I have a couple questions below, but I'm fine with this approach.
> @@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
> {
> struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
> struct tui_locator_element *element;
> + int locator_changed_p = 0;
>
> /* Allocate the locator content if necessary. */
> if (locator->content_size <= 0)
> {
> locator->content = tui_alloc_content (1, LOCATOR_WIN);
> locator->content_size = 1;
> + locator_changed_p = 1;
> }
>
> element = &locator->content[0]->which_element.locator;
> +
> + if (procname != NULL)
> + locator_changed_p |= strncmp (element->proc_name, procname,
> + MAX_LOCATOR_ELEMENT_LEN) != 0;
Can't element->proc_name be NULL here?
For the string fields, do we also need to compare
whether we go from NULL <-> non-NULL ?
locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
etc.?
Thanks,
Pedro Alves
On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:32 AM, Patrick Palka wrote:
>> This revised patch makes sure that tui_set_locator_info returns 1 when the
>> locator is first constructed, just in case none of the later checks trigger
>> for some reason.
>
> I have a couple questions below, but I'm fine with this approach.
>
>> @@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
>> {
>> struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
>> struct tui_locator_element *element;
>> + int locator_changed_p = 0;
>>
>> /* Allocate the locator content if necessary. */
>> if (locator->content_size <= 0)
>> {
>> locator->content = tui_alloc_content (1, LOCATOR_WIN);
>> locator->content_size = 1;
>> + locator_changed_p = 1;
>> }
>>
>> element = &locator->content[0]->which_element.locator;
>> +
>> + if (procname != NULL)
>> + locator_changed_p |= strncmp (element->proc_name, procname,
>> + MAX_LOCATOR_ELEMENT_LEN) != 0;
>
> Can't element->proc_name be NULL here?
Don't think so, since it is an inline array. It's defined as:
struct tui_locator_element
{
...
char full_name[MAX_LOCATOR_ELEMENT_LEN];
char proc_name[MAX_LOCATOR_ELEMENT_LEN];
}
(and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').
>
> For the string fields, do we also need to compare
> whether we go from NULL <-> non-NULL ?
>
> locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>
> etc.?
Yeah, that would be more correct I think. But I think the logic would
have to look like "if (procname == NULL) locator_changed_p |= strlen
(element->proc_name) != 0;" because proc_name cannot be NULL. When
procname is NULL, proc_name[0] gets set to 0.
>
> Thanks,
> Pedro Alves
>
On 06/30/2015 03:44 PM, Patrick Palka wrote:
> On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
>> Can't element->proc_name be NULL here?
>
> Don't think so, since it is an inline array. It's defined as:
>
> struct tui_locator_element
> {
> ...
> char full_name[MAX_LOCATOR_ELEMENT_LEN];
> char proc_name[MAX_LOCATOR_ELEMENT_LEN];
> }
>
> (and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').
Ah.
>
>>
>> For the string fields, do we also need to compare
>> whether we go from NULL <-> non-NULL ?
>>
>> locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>>
>> etc.?
>
> Yeah, that would be more correct I think. But I think the logic would
> have to look like "if (procname == NULL) locator_changed_p |= strlen
> (element->proc_name) != 0;" because proc_name cannot be NULL. When
> procname is NULL, proc_name[0] gets set to 0.
>
Or alternatively:
if (fullname == NULL)
fullname = "";
locator_changed_p |= strncmp (element->proc_name, procname,
MAX_LOCATOR_ELEMENT_LEN) != 0;
...
Thanks,
Pedro Alves
On Tue, Jun 30, 2015 at 11:11 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:44 PM, Patrick Palka wrote:
>> On Tue, Jun 30, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
>
>>> Can't element->proc_name be NULL here?
>>
>> Don't think so, since it is an inline array. It's defined as:
>>
>> struct tui_locator_element
>> {
>> ...
>> char full_name[MAX_LOCATOR_ELEMENT_LEN];
>> char proc_name[MAX_LOCATOR_ELEMENT_LEN];
>> }
>>
>> (and tui_alloc_content makes sure to set full_name[0] = proc_name[0] = '\0').
>
> Ah.
>
>>
>>>
>>> For the string fields, do we also need to compare
>>> whether we go from NULL <-> non-NULL ?
>>>
>>> locator_changed_p |= ((fullname == NULL) != (element->full_name == NULL));
>>>
>>> etc.?
>>
>> Yeah, that would be more correct I think. But I think the logic would
>> have to look like "if (procname == NULL) locator_changed_p |= strlen
>> (element->proc_name) != 0;" because proc_name cannot be NULL. When
>> procname is NULL, proc_name[0] gets set to 0.
>>
>
> Or alternatively:
>
> if (fullname == NULL)
> fullname = "";
> locator_changed_p |= strncmp (element->proc_name, procname,
> MAX_LOCATOR_ELEMENT_LEN) != 0;
I'll do that.
> ...
>
> Thanks,
> Pedro Alves
>
@@ -48,10 +48,10 @@ static char *tui_get_function_from_frame (struct frame_info *fi);
static void tui_set_locator_fullname (const char *fullname);
/* Update the locator, with the provided arguments. */
-static void tui_set_locator_info (struct gdbarch *gdbarch,
- const char *fullname,
- const char *procname,
- int lineno, CORE_ADDR addr);
+static int tui_set_locator_info (struct gdbarch *gdbarch,
+ const char *fullname,
+ const char *procname,
+ int lineno, CORE_ADDR addr);
static void tui_update_command (char *, int);
@@ -292,8 +292,12 @@ tui_set_locator_fullname (const char *fullname)
strcat_to_buf (element->full_name, MAX_LOCATOR_ELEMENT_LEN, fullname);
}
-/* Update the locator, with the provided arguments. */
-static void
+/* Update the locator, with the provided arguments.
+
+ Returns 1 if any of the locator's fields were actually changed,
+ and 0 otherwise. */
+
+static int
tui_set_locator_info (struct gdbarch *gdbarch,
const char *fullname,
const char *procname,
@@ -302,21 +306,36 @@ tui_set_locator_info (struct gdbarch *gdbarch,
{
struct tui_gen_win_info *locator = tui_locator_win_info_ptr ();
struct tui_locator_element *element;
+ int locator_changed_p = 0;
/* Allocate the locator content if necessary. */
if (locator->content_size <= 0)
{
locator->content = tui_alloc_content (1, LOCATOR_WIN);
locator->content_size = 1;
+ locator_changed_p = 1;
}
element = &locator->content[0]->which_element.locator;
+
+ if (procname != NULL)
+ locator_changed_p |= strncmp (element->proc_name, procname,
+ MAX_LOCATOR_ELEMENT_LEN) != 0;
+ locator_changed_p |= lineno != element->line_no;
+ locator_changed_p |= addr != element->addr;
+ locator_changed_p |= gdbarch != element->gdbarch;
+ if (fullname != NULL)
+ locator_changed_p |= strncmp (element->full_name, fullname,
+ MAX_LOCATOR_ELEMENT_LEN) != 0;
+
element->proc_name[0] = (char) 0;
strcat_to_buf (element->proc_name, MAX_LOCATOR_ELEMENT_LEN, procname);
element->line_no = lineno;
element->addr = addr;
element->gdbarch = gdbarch;
tui_set_locator_fullname (fullname);
+
+ return locator_changed_p;
}
/* Update only the full_name portion of the locator. */
@@ -327,11 +346,14 @@ tui_update_locator_fullname (const char *fullname)
tui_show_locator_content ();
}
-/* Function to print the frame information for the TUI. */
+/* Function to print the frame information for the TUI. The windows are
+ refreshed only if frame information has changed since the last refresh. */
+
void
tui_show_frame_info (struct frame_info *fi)
{
struct tui_win_info *win_info;
+ int locator_changed_p;
int i;
if (fi)
@@ -349,15 +371,23 @@ tui_show_frame_info (struct frame_info *fi)
&& tui_source_is_displayed (symtab_to_fullname (sal.symtab));
if (get_frame_pc_if_available (fi, &pc))
- tui_set_locator_info (get_frame_arch (fi),
- (sal.symtab == 0
- ? "??" : symtab_to_fullname (sal.symtab)),
- tui_get_function_from_frame (fi),
- sal.line,
- pc);
+ locator_changed_p
+ = tui_set_locator_info (get_frame_arch (fi),
+ (sal.symtab == 0
+ ? "??" : symtab_to_fullname (sal.symtab)),
+ tui_get_function_from_frame (fi),
+ sal.line,
+ pc);
else
- tui_set_locator_info (get_frame_arch (fi),
- "??", _("<unavailable>"), sal.line, 0);
+ locator_changed_p
+ = tui_set_locator_info (get_frame_arch (fi),
+ "??", _("<unavailable>"), sal.line, 0);
+
+ /* If the locator information has not changed, then frame information has
+ not changed. If frame information has not changed, then the windows'
+ contents will not change. So don't bother refreshing the windows. */
+ if (!locator_changed_p)
+ return;
tui_show_locator_content ();
start_line = 0;
@@ -431,7 +461,12 @@ tui_show_frame_info (struct frame_info *fi)
}
else
{
- tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+ locator_changed_p
+ = tui_set_locator_info (NULL, NULL, NULL, 0, (CORE_ADDR) 0);
+
+ if (!locator_changed_p)
+ return;
+
tui_show_locator_content ();
for (i = 0; i < (tui_source_windows ())->count; i++)
{