tui: replace deprecated_register_changed_hook with observer
Commit Message
This is a straightforward replacement of the TUI's use of the
aforementioned hook with the register_changed observer. Since this was
the only user of the hook, this patch also removes the hook.
[ I am not sure if the changes to the function tui_register_changed are
correct. In particular, the inputted frame argument is now passed down
to tui_check_data_values instead of the frame returned by
get_selected_frame. The frame argument passed to each register_changed
observer corresponds to the VALUE_FRAME_ID of the register being
modified within a register assignment, e.g. the $rax in "print $rax =
FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
register not be the currently selected frame? ]
gdb/ChangeLog:
* defs.h (deprecated_register_changed_hook): Remove prototype.
* interps.c (clear_iterpreter_hooks): Remove reference to
deprecated_register_changed_hook.
* top.c (deprecated_register_changed_hook): Remove prototype.
* valops.c (value_assign): Remove reference to
deprecated_register_changed_hook.
* tui/tui-hooks.c (tui_register_changed): Add parameter "frame".
Use it instead of calling get_selected_frame.
(tui_register_changed_observer): Define.
(tui_install_hooks): Remove reference to
deprecated_register_changed_hook. Set
tui_register_changed_observer.
(tui_remove_hooks): Remove reference to
deprecated_register_changed_hook. Unset
tui_register_changed_observer.
---
gdb/defs.h | 1 -
gdb/interps.c | 1 -
gdb/top.c | 5 -----
gdb/tui/tui-hooks.c | 16 +++++++---------
gdb/valops.c | 2 --
5 files changed, 7 insertions(+), 18 deletions(-)
Comments
On 07/06/2015 02:17 AM, Patrick Palka wrote:
> This is a straightforward replacement of the TUI's use of the
> aforementioned hook with the register_changed observer. Since this was
> the only user of the hook, this patch also removes the hook.
>
> [ I am not sure if the changes to the function tui_register_changed are
> correct. In particular, the inputted frame argument is now passed down
> to tui_check_data_values instead of the frame returned by
> get_selected_frame. The frame argument passed to each register_changed
> observer corresponds to the VALUE_FRAME_ID of the register being
> modified within a register assignment, e.g. the $rax in "print $rax =
> FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
> register not be the currently selected frame? ]
>
Grepping for value_assign callers finds e.g., varobjs:
varobj.c: val = value_assign (var->value, value);
Adding an assertion like this:
@@ -1169,6 +1169,7 @@ value_assign (struct value *toval, struct value *fromval)
}
}
+ gdb_assert (frame == get_selected_frame (NULL));
observer_notify_register_changed (frame, value_reg);
if (deprecated_register_changed_hook)
deprecated_register_changed_hook (-1);
and playing with varobjs shows the assertion failing:
(gdb) interpreter-exec mi "-var-create - * $rax"
^done,name="var1",numchild="0",value="6295640",type="int64_t",has_more="0"
(gdb) up
#1 0x000000000040082a in thread_function0 (arg=0x0) at threads.c:69
69 usleep (1); /* Loop increment. */
(gdb) up
#2 0x0000003616a07ee5 in start_thread (arg=0x7ffff7fc1700) at pthread_create.c:309
309 THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
(gdb) interpreter-exec mi "-var-assign var1 1"
~"/home/pedro/gdb/mygit/build/../src/gdb/valops.c:1172: internal-error: value_assign: Assertion `frame == get_selected_frame (NULL)' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
The TUI doesn't use MI, but there are probably other similar cases
in the tree. E.g., I'd assume you can create a register Value with Python,
and then assign to it when the selected frame is not
the register's frame.
Thanks,
Pedro Alves
On Wed, Jul 8, 2015 at 7:41 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/06/2015 02:17 AM, Patrick Palka wrote:
>> This is a straightforward replacement of the TUI's use of the
>> aforementioned hook with the register_changed observer. Since this was
>> the only user of the hook, this patch also removes the hook.
>>
>> [ I am not sure if the changes to the function tui_register_changed are
>> correct. In particular, the inputted frame argument is now passed down
>> to tui_check_data_values instead of the frame returned by
>> get_selected_frame. The frame argument passed to each register_changed
>> observer corresponds to the VALUE_FRAME_ID of the register being
>> modified within a register assignment, e.g. the $rax in "print $rax =
>> FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
>> register not be the currently selected frame? ]
>>
>
> Grepping for value_assign callers finds e.g., varobjs:
>
> varobj.c: val = value_assign (var->value, value);
>
> Adding an assertion like this:
>
> @@ -1169,6 +1169,7 @@ value_assign (struct value *toval, struct value *fromval)
> }
> }
>
> + gdb_assert (frame == get_selected_frame (NULL));
> observer_notify_register_changed (frame, value_reg);
> if (deprecated_register_changed_hook)
> deprecated_register_changed_hook (-1);
>
> and playing with varobjs shows the assertion failing:
>
> (gdb) interpreter-exec mi "-var-create - * $rax"
> ^done,name="var1",numchild="0",value="6295640",type="int64_t",has_more="0"
> (gdb) up
> #1 0x000000000040082a in thread_function0 (arg=0x0) at threads.c:69
> 69 usleep (1); /* Loop increment. */
> (gdb) up
> #2 0x0000003616a07ee5 in start_thread (arg=0x7ffff7fc1700) at pthread_create.c:309
> 309 THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
> (gdb) interpreter-exec mi "-var-assign var1 1"
> ~"/home/pedro/gdb/mygit/build/../src/gdb/valops.c:1172: internal-error: value_assign: Assertion `frame == get_selected_frame (NULL)' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
>
> The TUI doesn't use MI, but there are probably other similar cases
> in the tree. E.g., I'd assume you can create a register Value with Python,
> and then assign to it when the selected frame is not
> the register's frame.
Ah okay.. So it seems to me that if the frame argument !=
get_selected_frame, then we should not update the register window at
all since the register window is supposed to show the register values
of the currently selected frame.
Or instead, just ignore the frame argument and always pass
get_selected_frame to tui_check_data_values, even if frame !=
get_selected_frame. Seems to me that this is the safest option.
On 07/08/2015 01:30 PM, Patrick Palka wrote:
> On Wed, Jul 8, 2015 at 7:41 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 07/06/2015 02:17 AM, Patrick Palka wrote:
>>> This is a straightforward replacement of the TUI's use of the
>>> aforementioned hook with the register_changed observer. Since this was
>>> the only user of the hook, this patch also removes the hook.
>>>
>>> [ I am not sure if the changes to the function tui_register_changed are
>>> correct. In particular, the inputted frame argument is now passed down
>>> to tui_check_data_values instead of the frame returned by
>>> get_selected_frame. The frame argument passed to each register_changed
>>> observer corresponds to the VALUE_FRAME_ID of the register being
>>> modified within a register assignment, e.g. the $rax in "print $rax =
>>> FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
>>> register not be the currently selected frame? ]
>>>
>>
>> Grepping for value_assign callers finds e.g., varobjs:
>>
>> varobj.c: val = value_assign (var->value, value);
>>
>> Adding an assertion like this:
>>
>> @@ -1169,6 +1169,7 @@ value_assign (struct value *toval, struct value *fromval)
>> }
>> }
>>
>> + gdb_assert (frame == get_selected_frame (NULL));
>> observer_notify_register_changed (frame, value_reg);
>> if (deprecated_register_changed_hook)
>> deprecated_register_changed_hook (-1);
>>
>> and playing with varobjs shows the assertion failing:
>>
>> (gdb) interpreter-exec mi "-var-create - * $rax"
>> ^done,name="var1",numchild="0",value="6295640",type="int64_t",has_more="0"
>> (gdb) up
>> #1 0x000000000040082a in thread_function0 (arg=0x0) at threads.c:69
>> 69 usleep (1); /* Loop increment. */
>> (gdb) up
>> #2 0x0000003616a07ee5 in start_thread (arg=0x7ffff7fc1700) at pthread_create.c:309
>> 309 THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
>> (gdb) interpreter-exec mi "-var-assign var1 1"
>> ~"/home/pedro/gdb/mygit/build/../src/gdb/valops.c:1172: internal-error: value_assign: Assertion `frame == get_selected_frame (NULL)' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
>>
>> The TUI doesn't use MI, but there are probably other similar cases
>> in the tree. E.g., I'd assume you can create a register Value with Python,
>> and then assign to it when the selected frame is not
>> the register's frame.
>
> Ah okay.. So it seems to me that if the frame argument !=
> get_selected_frame, then we should not update the register window at
> all since the register window is supposed to show the register values
> of the currently selected frame.
Yes, I think so.
> Or instead, just ignore the frame argument and always pass
> get_selected_frame to tui_check_data_values, even if frame !=
> get_selected_frame. Seems to me that this is the safest option.
That'd be a 1-1 with the current code. Though, I believe
that results in spuriously clearing the highlight of
previously changed registers (of the selected frame), because
nothing will have changed. So seems like the other option
actually fixes a bug.
Thanks,
Pedro Alves
On Wed, Jul 8, 2015 at 8:48 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/08/2015 01:30 PM, Patrick Palka wrote:
>> On Wed, Jul 8, 2015 at 7:41 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 07/06/2015 02:17 AM, Patrick Palka wrote:
>>>> This is a straightforward replacement of the TUI's use of the
>>>> aforementioned hook with the register_changed observer. Since this was
>>>> the only user of the hook, this patch also removes the hook.
>>>>
>>>> [ I am not sure if the changes to the function tui_register_changed are
>>>> correct. In particular, the inputted frame argument is now passed down
>>>> to tui_check_data_values instead of the frame returned by
>>>> get_selected_frame. The frame argument passed to each register_changed
>>>> observer corresponds to the VALUE_FRAME_ID of the register being
>>>> modified within a register assignment, e.g. the $rax in "print $rax =
>>>> FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
>>>> register not be the currently selected frame? ]
>>>>
>>>
>>> Grepping for value_assign callers finds e.g., varobjs:
>>>
>>> varobj.c: val = value_assign (var->value, value);
>>>
>>> Adding an assertion like this:
>>>
>>> @@ -1169,6 +1169,7 @@ value_assign (struct value *toval, struct value *fromval)
>>> }
>>> }
>>>
>>> + gdb_assert (frame == get_selected_frame (NULL));
>>> observer_notify_register_changed (frame, value_reg);
>>> if (deprecated_register_changed_hook)
>>> deprecated_register_changed_hook (-1);
>>>
>>> and playing with varobjs shows the assertion failing:
>>>
>>> (gdb) interpreter-exec mi "-var-create - * $rax"
>>> ^done,name="var1",numchild="0",value="6295640",type="int64_t",has_more="0"
>>> (gdb) up
>>> #1 0x000000000040082a in thread_function0 (arg=0x0) at threads.c:69
>>> 69 usleep (1); /* Loop increment. */
>>> (gdb) up
>>> #2 0x0000003616a07ee5 in start_thread (arg=0x7ffff7fc1700) at pthread_create.c:309
>>> 309 THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
>>> (gdb) interpreter-exec mi "-var-assign var1 1"
>>> ~"/home/pedro/gdb/mygit/build/../src/gdb/valops.c:1172: internal-error: value_assign: Assertion `frame == get_selected_frame (NULL)' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
>>>
>>> The TUI doesn't use MI, but there are probably other similar cases
>>> in the tree. E.g., I'd assume you can create a register Value with Python,
>>> and then assign to it when the selected frame is not
>>> the register's frame.
>>
>> Ah okay.. So it seems to me that if the frame argument !=
>> get_selected_frame, then we should not update the register window at
>> all since the register window is supposed to show the register values
>> of the currently selected frame.
>
> Yes, I think so.
>
>> Or instead, just ignore the frame argument and always pass
>> get_selected_frame to tui_check_data_values, even if frame !=
>> get_selected_frame. Seems to me that this is the safest option.
>
> That'd be a 1-1 with the current code. Though, I believe
> that results in spuriously clearing the highlight of
> previously changed registers (of the selected frame), because
> nothing will have changed. So seems like the other option
> actually fixes a bug.
Is it actually the case that a register change made on one frame can
not show up on some other frame?
If I debug gdb with gdb, doing "start" followed by "step" a couple
dozen times, do "layout regs", then select the outermost frame and do
"print $rbx = 50", the regs window shows that $rbx has not changed on
the (selected) outermost frame but if i select the innermost frame,
$rbx has changed to 50. And the frame_id of the register $rbx was
indeed the (selected) outermost frame, yet the registers of the
selected frame did not change after the value assignment and the
registers of some other frame did. I don't know why this particular
example behaves this way, but it seems to illustrates that it's
possible that a register change made in one frame can affect the
register values of another frame. So I don't know if the "frame !=
get_selected_frame ()" check is 100% correct.
On 07/08/2015 02:37 PM, Patrick Palka wrote:
> On Wed, Jul 8, 2015 at 8:48 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 07/08/2015 01:30 PM, Patrick Palka wrote:
>>> On Wed, Jul 8, 2015 at 7:41 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 07/06/2015 02:17 AM, Patrick Palka wrote:
>>>>> This is a straightforward replacement of the TUI's use of the
>>>>> aforementioned hook with the register_changed observer. Since this was
>>>>> the only user of the hook, this patch also removes the hook.
>>>>>
>>>>> [ I am not sure if the changes to the function tui_register_changed are
>>>>> correct. In particular, the inputted frame argument is now passed down
>>>>> to tui_check_data_values instead of the frame returned by
>>>>> get_selected_frame. The frame argument passed to each register_changed
>>>>> observer corresponds to the VALUE_FRAME_ID of the register being
>>>>> modified within a register assignment, e.g. the $rax in "print $rax =
>>>>> FOO". When would the frame corresponding to the VALUE_FRAME_ID of a
>>>>> register not be the currently selected frame? ]
>>>>>
>>>>
>>>> Grepping for value_assign callers finds e.g., varobjs:
>>>>
>>>> varobj.c: val = value_assign (var->value, value);
>>>>
>>>> Adding an assertion like this:
>>>>
>>>> @@ -1169,6 +1169,7 @@ value_assign (struct value *toval, struct value *fromval)
>>>> }
>>>> }
>>>>
>>>> + gdb_assert (frame == get_selected_frame (NULL));
>>>> observer_notify_register_changed (frame, value_reg);
>>>> if (deprecated_register_changed_hook)
>>>> deprecated_register_changed_hook (-1);
>>>>
>>>> and playing with varobjs shows the assertion failing:
>>>>
>>>> (gdb) interpreter-exec mi "-var-create - * $rax"
>>>> ^done,name="var1",numchild="0",value="6295640",type="int64_t",has_more="0"
>>>> (gdb) up
>>>> #1 0x000000000040082a in thread_function0 (arg=0x0) at threads.c:69
>>>> 69 usleep (1); /* Loop increment. */
>>>> (gdb) up
>>>> #2 0x0000003616a07ee5 in start_thread (arg=0x7ffff7fc1700) at pthread_create.c:309
>>>> 309 THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
>>>> (gdb) interpreter-exec mi "-var-assign var1 1"
>>>> ~"/home/pedro/gdb/mygit/build/../src/gdb/valops.c:1172: internal-error: value_assign: Assertion `frame == get_selected_frame (NULL)' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
>>>>
>>>> The TUI doesn't use MI, but there are probably other similar cases
>>>> in the tree. E.g., I'd assume you can create a register Value with Python,
>>>> and then assign to it when the selected frame is not
>>>> the register's frame.
>>>
>>> Ah okay.. So it seems to me that if the frame argument !=
>>> get_selected_frame, then we should not update the register window at
>>> all since the register window is supposed to show the register values
>>> of the currently selected frame.
>>
>> Yes, I think so.
>>
>>> Or instead, just ignore the frame argument and always pass
>>> get_selected_frame to tui_check_data_values, even if frame !=
>>> get_selected_frame. Seems to me that this is the safest option.
>>
>> That'd be a 1-1 with the current code. Though, I believe
>> that results in spuriously clearing the highlight of
>> previously changed registers (of the selected frame), because
>> nothing will have changed. So seems like the other option
>> actually fixes a bug.
>
> Is it actually the case that a register change made on one frame can
> not show up on some other frame?
Oh, you're right, good point. Registers can well be at the
same physical location across frames.
>
> If I debug gdb with gdb, doing "start" followed by "step" a couple
> dozen times, do "layout regs", then select the outermost frame and do
> "print $rbx = 50", the regs window shows that $rbx has not changed on
> the (selected) outermost frame but if i select the innermost frame,
> $rbx has changed to 50. And the frame_id of the register $rbx was
> indeed the (selected) outermost frame, yet the registers of the
> selected frame did not change after the value assignment and the
> registers of some other frame did. I don't know why this particular
> example behaves this way, but it seems to illustrates that it's
> possible that a register change made in one frame can affect the
> register values of another frame. So I don't know if the "frame !=
> get_selected_frame ()" check is 100% correct.
>
Yeah. Sorry, somehow forgot this...
Thanks,
Pedro Alves
@@ -649,7 +649,6 @@ extern void (*deprecated_readline_begin_hook) (char *, ...)
ATTRIBUTE_FPTR_PRINTF_1;
extern char *(*deprecated_readline_hook) (const char *);
extern void (*deprecated_readline_end_hook) (void);
-extern void (*deprecated_register_changed_hook) (int regno);
extern void (*deprecated_context_hook) (int);
extern ptid_t (*deprecated_target_wait_hook) (ptid_t ptid,
struct target_waitstatus *status,
@@ -370,7 +370,6 @@ clear_interpreter_hooks (void)
deprecated_readline_begin_hook = 0;
deprecated_readline_hook = 0;
deprecated_readline_end_hook = 0;
- deprecated_register_changed_hook = 0;
deprecated_context_hook = 0;
deprecated_target_wait_hook = 0;
deprecated_call_command_hook = 0;
@@ -221,11 +221,6 @@ void (*deprecated_detach_hook) (void);
void (*deprecated_interactive_hook) (void);
-/* Tell the GUI someone changed the register REGNO. -1 means
- that the caller does not know which register changed or
- that several registers have changed (see value_assign). */
-void (*deprecated_register_changed_hook) (int regno);
-
/* Called when going to wait for the target. Usually allows the GUI
to run while waiting for target events. */
@@ -67,15 +67,12 @@ tui_new_objfile_hook (struct objfile* objfile)
static int tui_refreshing_registers = 0;
static void
-tui_register_changed_hook (int regno)
+tui_register_changed (struct frame_info *frame, int regno)
{
- struct frame_info *fi;
-
- fi = get_selected_frame (NULL);
if (tui_refreshing_registers == 0)
{
tui_refreshing_registers = 1;
- tui_check_data_values (fi);
+ tui_check_data_values (frame);
tui_refreshing_registers = 0;
}
}
@@ -226,6 +223,7 @@ static struct observer *tui_inferior_exit_observer;
static struct observer *tui_about_to_proceed_observer;
static struct observer *tui_before_prompt_observer;
static struct observer *tui_normal_stop_observer;
+static struct observer *tui_register_changed_observer;
/* Install the TUI specific hooks. */
void
@@ -253,8 +251,8 @@ tui_install_hooks (void)
= observer_attach_before_prompt (tui_before_prompt);
tui_normal_stop_observer
= observer_attach_normal_stop (tui_normal_stop);
-
- deprecated_register_changed_hook = tui_register_changed_hook;
+ tui_register_changed_observer
+ = observer_attach_register_changed (tui_register_changed);
}
/* Remove the TUI specific hooks. */
@@ -263,8 +261,6 @@ tui_remove_hooks (void)
{
deprecated_print_frame_info_listing_hook = 0;
deprecated_query_hook = 0;
- deprecated_register_changed_hook = 0;
-
/* Remove our observers. */
observer_detach_breakpoint_created (tui_bp_created_observer);
tui_bp_created_observer = NULL;
@@ -280,6 +276,8 @@ tui_remove_hooks (void)
tui_before_prompt_observer = NULL;
observer_detach_normal_stop (tui_normal_stop_observer);
tui_normal_stop_observer = NULL;
+ observer_detach_register_changed (tui_register_changed_observer);
+ tui_register_changed_observer = NULL;
}
void _initialize_tui_hooks (void);
@@ -1170,8 +1170,6 @@ value_assign (struct value *toval, struct value *fromval)
}
observer_notify_register_changed (frame, value_reg);
- if (deprecated_register_changed_hook)
- deprecated_register_changed_hook (-1);
break;
}