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

[Andrew Burgess <aburgess@redhat.com>]

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