Re: [External Mail]Re: [PATCH] [gdb/python]: add the orginal_value field in RegisterChangedEvent

[Andrew Burgess <aburgess@redhat.com>]

郭胜元 <guoshengyuan1@xiaomi.com> writes:

> 在 2025/9/28 23:59, Andrew Burgess 写道:
>> guoshengyuan1 <guoshengyuan1@xiaomi.com> writes:
>>
>>> When the register_changed event is triggered, the user should be told
>>> what the previous value was so they can make a decision, rather
>>> than just being told what the modified value is.
>> Thanks for working on this.
>>
>>> Signed-off-by: guoshengyuan1 <guoshengyuan1@xiaomi.com>
>> We don't currently use Signed-off-by headers in GDB.
>>
>> Do you currently have a copyright assignment in place?  If not then take
>> a look at:
>> https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment
>> and feel free to ask if you have any questions.
>>
>>> ---
>>>   gdb/doc/python.texi                   |  3 +++
>>>   gdb/observable.h                      |  4 +++-
>>>   gdb/python/py-event.h                 |  2 +-
>>>   gdb/python/py-inferior.c              |  6 ++++--
>>>   gdb/python/py-infevents.c             | 19 ++++++++++++++++---
>>>   gdb/testsuite/gdb.python/py-events.py |  1 +
>>>   gdb/tui/tui-hooks.c                   |  4 +++-
>>>   gdb/valops.c                          |  4 +++-
>>>   8 files changed, 34 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
>>> index 3763eee9d63..1d74910f400 100644
>>> --- a/gdb/doc/python.texi
>>> +++ b/gdb/doc/python.texi
>>> @@ -3979,6 +3979,9 @@ A gdb.Frame object representing the frame in which the register was modified.
>>>   @defvar RegisterChangedEvent.regnum
>>>   Denotes which register was modified.
>>>   @end defvar
>>> +@defvar RegisterChangedEvent.original_value
>>> +Denotes the value of the register before it is modified.
>> This should describe the type of original_value.  More on this below...
>>
>>> +@end defvar
>>>
>>>   @item events.breakpoint_created
>>>   This is emitted when a new breakpoint has been created.  The argument
>>> diff --git a/gdb/observable.h b/gdb/observable.h
>>> index 4d913010c56..f65d42a2b00 100644
>>> --- a/gdb/observable.h
>>> +++ b/gdb/observable.h
>>> @@ -20,6 +20,7 @@
>>>   #ifndef GDB_OBSERVABLE_H
>>>   #define GDB_OBSERVABLE_H
>>>
>>> +#include "gdbsupport/common-types.h"
>>>   #include "gdbsupport/observable.h"
>>>   #include "target/waitstatus.h"
>>>
>>> @@ -222,7 +223,8 @@ extern observable<ptid_t /* thread */, CORE_ADDR /* address */>
>>>       inferior_call_post;
>>>
>>>   /* A register in the inferior has been modified by the gdb user.  */
>>> -extern observable<const frame_info_ptr &/* frame */, int /* regnum */>
>>> +extern observable<const frame_info_ptr &/* frame */, int /* regnum */,
>>> +                 LONGEST /* original_value */>
>> Not every register value can fit into a LONGEST.  For example many
>> architectures these days have vector registers which are longer than a
>> LONGEST.
>>
>> You would be better passing a 'value *' around.
>>
>>>       register_changed;
>>>
>>>   /* The user-selected inferior, thread and/or frame has changed.  The
>>> diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h
>>> index 3938368c9ca..2b696e23ff8 100644
>>> --- a/gdb/python/py-event.h
>>> +++ b/gdb/python/py-event.h
>>> @@ -56,7 +56,7 @@ enum inferior_call_kind
>>>   extern int emit_inferior_call_event (inferior_call_kind kind,
>>>                                       ptid_t thread, CORE_ADDR addr);
>>>   extern int emit_register_changed_event (const frame_info_ptr &frame,
>>> -                                       int regnum);
>>> +                                       int regnum, LONGEST original_value);
>>>   extern int emit_memory_changed_event (CORE_ADDR addr, ssize_t len);
>>>   extern int evpy_emit_event (PyObject *event,
>>>                              eventregistry_object *registry);
>>> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
>>> index 2aa11d3160d..91f4bd862e5 100644
>>> --- a/gdb/python/py-inferior.c
>>> +++ b/gdb/python/py-inferior.c
>>> @@ -19,6 +19,7 @@
>>>
>>>   #include "auto-load.h"
>>>   #include "gdbcore.h"
>>> +#include "gdbsupport/common-types.h"
>>>   #include "gdbthread.h"
>>>   #include "inferior.h"
>>>   #include "objfiles.h"
>>> @@ -158,11 +159,12 @@ python_on_memory_change (struct inferior *inferior, CORE_ADDR addr, ssize_t len,
>>>      command). */
>>>
>>>   static void
>>> -python_on_register_change (const frame_info_ptr &frame, int regnum)
>>> +python_on_register_change (const frame_info_ptr &frame, int regnum,
>>> +                          LONGEST orginal_value)
>>>   {
>>>     gdbpy_enter enter_py (current_inferior ()->arch ());
>>>
>>> -  if (emit_register_changed_event (frame, regnum) < 0)
>>> +  if (emit_register_changed_event (frame, regnum, orginal_value) < 0)
>>>       gdbpy_print_stack ();
>>>   }
>>>
>>> diff --git a/gdb/python/py-infevents.c b/gdb/python/py-infevents.c
>>> index f74fb017edc..424abffcd1c 100644
>>> --- a/gdb/python/py-infevents.c
>>> +++ b/gdb/python/py-infevents.c
>>> @@ -17,6 +17,7 @@
>>>      You should have received a copy of the GNU General Public License
>>>      along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>>
>>> +#include "gdbsupport/common-types.h"
>>>   #include "py-event.h"
>>>
>>>   /* Construct either a gdb.InferiorCallPreEvent or a
>>> @@ -62,7 +63,8 @@ create_inferior_call_event_object (inferior_call_kind flag, ptid_t ptid,
>>>
>>>   static gdbpy_ref<>
>>>   create_register_changed_event_object (const frame_info_ptr &frame,
>>> -                                     int regnum)
>>> +                                     int regnum,
>>> +                                     LONGEST original_value)
>>>   {
>>>     gdbpy_ref<> event = create_event_object (&register_changed_event_object_type);
>>>     if (event == NULL)
>>> @@ -82,6 +84,15 @@ create_register_changed_event_object (const frame_info_ptr &frame,
>>>     if (evpy_add_attribute (event.get (), "regnum", regnum_obj.get ()) < 0)
>>>       return NULL;
>>>
>>> +  gdbpy_ref<> original_value_obj = gdb_py_object_from_longest (original_value);
>> If the original_value argument becomes 'value *' then you should switch
>> to value_to_value_object here which will give you a gdb.Value object,
>> which I think would be the right thing to pass to the user.
>>
>>> +  if (original_value_obj == NULL)
>>> +    return NULL;
>>> +
>>> +  auto original_value_status = evpy_add_attribute
>>> +    (event.get (), "original_value", original_value_obj.get ());
>>> +  if (original_value_status < 0)
>>> +    return NULL;
>>> +
>>>     return event;
>>>   }
>>>
>>> @@ -150,12 +161,14 @@ emit_memory_changed_event (CORE_ADDR addr, ssize_t len)
>>>      will create a new Python register changed event object. */
>>>
>>>   int
>>> -emit_register_changed_event (const frame_info_ptr &frame, int regnum)
>>> +emit_register_changed_event (const frame_info_ptr &frame, int regnum,
>>> +                            LONGEST original_value)
>>>   {
>>>     if (evregpy_no_listeners_p (gdb_py_events.register_changed))
>>>       return 0;
>>>
>>> -  gdbpy_ref<> event = create_register_changed_event_object (frame, regnum);
>>> +  gdbpy_ref<> event = create_register_changed_event_object
>>> +    (frame, regnum, original_value);
>>>     if (event != NULL)
>>>       return evpy_emit_event (event.get (), gdb_py_events.register_changed);
>>>     return -1;
>>> diff --git a/gdb/testsuite/gdb.python/py-events.py b/gdb/testsuite/gdb.python/py-events.py
>>> index 0573b16659b..663b9fb2119 100644
>>> --- a/gdb/testsuite/gdb.python/py-events.py
>>> +++ b/gdb/testsuite/gdb.python/py-events.py
>>> @@ -90,6 +90,7 @@ def register_changed_handler(event):
>>>       assert isinstance(event.frame, gdb.Frame)
>>>       print("frame: %s" % (event.frame))
>>>       print("num: %s" % (event.regnum))
>>> +    print("num: %s" % (event.original_value))
>> I don't think using 'num:' here is correct.
>>
>> It's a shame that the event testing isn't very extensive in
>> py-events.exp, but I think at a minimum you should extend these blocks:
>>
>>    gdb_test_sequence {set $sp = 0} "" {
>>        "event type: register-changed"
>>        "frame: "
>>        "num: "
>>    }
>>
>> to look for whatever new tag you add.
>>
>> Ideally we'd be checking for the actual previous value, but given how
>> limited the existing tests are this might be an unfair request.  Still,
>> if you were feeling adventurous...
>>
>> Thanks,
>> Andrew
>>
>
> Thank you for your review. I changed the value of original_value to
> struct value and modified the testsuite part. The following is my new patch

Hi!

Do you have a copyright assignment in place?  If not then there's the
web resource here:

  https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

Or you can email "assign@gnu.org" and say that you have some gdb
patches; they will get you started.

Thanks,
Andrew