From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25720 invoked by alias); 2 Nov 2012 09:56:58 -0000 Received: (qmail 25706 invoked by uid 22791); 2 Nov 2012 09:56:56 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_BJ,TW_JV X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 02 Nov 2012 09:56:49 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1TUDzb-00052p-1u from Yao_Qi@mentor.com ; Fri, 02 Nov 2012 02:56:47 -0700 Received: from SVR-ORW-FEM-04.mgc.mentorg.com ([147.34.97.41]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 2 Nov 2012 02:56:46 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.1.289.1; Fri, 2 Nov 2012 02:56:45 -0700 Message-ID: <509398D8.7060104@codesourcery.com> Date: Fri, 02 Nov 2012 09:56:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: Tom Tromey CC: Subject: Re: [RFC] MI notification on register changes References: <1350977008-28632-1-git-send-email-yao@codesourcery.com> <87lielqcba.fsf@fleche.redhat.com> In-Reply-To: <87lielqcba.fsf@fleche.redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-11/txt/msg00034.txt.bz2 Tom, thanks for the review. On 11/02/2012 04:48 AM, Tom Tromey wrote: > Yao> This patch also extends the usage of field 'regnum' of 'struct value'. > Yao> Originally, this field is used to store the register number if content > Yao> is from register. With this patch, this field also store the register > Yao> number if it "presents" a register. For example, on non-innermost > Yao> frame, the value "reg" may be from memory instead of register. > Yao> set $reg = foo > > The register may be stored in memory, but it this case really ever > represented as lval_memory in gdb? I thought the reason for Yes. > value::frame_id was to represent the notion of "register in a specific > frame", and that all registers are represented as lval_register. > That is a good point. I didn't realize that value::frame_id is for register. Then, the patch to frame used in 'register_change' observer should be got in a different way. New version reflects this. > Yao> +proc test_register_change { } { with_test_prefix "simple" { > > For new tests I would prefer correct indentation. That is, > with_test_prefix on its own line. I think the existing tests are > written in this funny way to avoid a massive reindentation, but that > isn't a factor here. > Fixed. > Yao> +++ b/gdb/valops.c > Yao> @@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval) > > Yao> + /* Figure out which frame this is in currently. */ > Yao> + frame = frame_find_by_id (VALUE_FRAME_ID (toval)); > Yao> + > > I find it pretty hard to convince myself that moving this out of the > switch can't cause some obscure problem. > > How about hoisting the declaration of frame, initializing it to NULL, > and then changing this: > I hoist the declaration of frame, as you suggested, but set it to the return value of deprecated_safe_get_selected_frame. It will be updated later if 'VALUE_LVAL (toval)' is lval_register, otherwise, we will use it to pass to observer. > Yao> + if (VALUE_REGNUM (toval) >= 0) > Yao> + observer_notify_register_changed (VALUE_REGNUM (toval), frame, > Yao> + inferior_ptid, > Yao> + value_contents (val)); > > ... to read 'if (frame != NULL)'? Done. Besides these changes, the test case mi-reg-changed.exp is updated a little bit to choose a call-saved register for the testing. -- Yao gdb/doc: 2012-11-02 Yao Qi * gdb.texinfo (GDB/MI Async Records): Add '=register-changed'. * observer.texi (GDB Observers): Add observer 'register_changed'. gdb: 2012-11-02 Yao Qi * mi/mi-cmd-var.c (mi_cmd_var_assign): Set mi_suppress_notification.register_changed to 1 and restore it on exit. * mi/mi-interp.c: Include "regcache.h". Declare mi_register_changed. (mi_interpreter_init): Install mi_register_changed to observer 'register_changed'. (mi_register_changed): New. * mi/mi-main.h (struct mi_suppress_notification) : New. * value.c (struct value) : Update comments. * valops.c (value_assign): Get frame info of 'toval' earlier. (value_assign): Call observer_notify_register_changed if 'toval' is related to a register. * NEWS: Mention it. gdb/testsuite/ 2012-11-02 Yao Qi * gdb.mi/mi-reg-changed.c: New. * gdb.mi/mi-reg-changed.exp: New. * gdb.mi/mi-cli.exp: Update tests for new MI notification. --- gdb/NEWS | 2 + gdb/doc/gdb.texinfo | 5 + gdb/doc/observer.texi | 5 + gdb/eval.c | 12 ++- gdb/mi/mi-cmd-var.c | 12 ++- gdb/mi/mi-interp.c | 49 ++++++++++ gdb/mi/mi-main.h | 2 + gdb/testsuite/gdb.mi/mi-cli.exp | 6 +- gdb/testsuite/gdb.mi/mi-reg-changed.c | 30 ++++++ gdb/testsuite/gdb.mi/mi-reg-changed.exp | 159 +++++++++++++++++++++++++++++++ gdb/valops.c | 10 ++- gdb/value.c | 4 +- 12 files changed, 283 insertions(+), 13 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.c create mode 100644 gdb/testsuite/gdb.mi/mi-reg-changed.exp diff --git a/gdb/NEWS b/gdb/NEWS index 069b84f..908f97d 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -64,6 +64,8 @@ py [command] async record "=record-started" and "=record-stopped". ** Memory changes are now notified using new async record "=memory-changed". + ** Register changes are now notified using new async record + "=register-changed". *** Changes in GDB 7.5 diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 21db475..f0a5e5d 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -27704,6 +27704,11 @@ written in an inferior. The @var{id} is the identifier of the thread group corresponding to the affected inferior. The optional @code{type="code"} part is reported if the memory written to holds executable code. + +@item =register-changed,name=@var{name},value=@var{value},group-id=@var{id},frame=@var{frame} +Reports that register @var{name} on frame @var{frame} was changed to +@var{value} in an inferior. The @var{id} is the identifier of the +thread group corresponding to the affected inferior. @end table @node GDB/MI Frame Information diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi index 106475b..54c566e 100644 --- a/gdb/doc/observer.texi +++ b/gdb/doc/observer.texi @@ -98,6 +98,11 @@ inferior has stopped. The target's register contents have changed. @end deftypefun +@deftypefun void register_changed (int @var{reg_num}, struct frame_info *@var{frame}, ptid_t @var{inferior_ptid}, const gdb_byte *@var{buf}) +The content of register @var{reg_num} in frame @var{frame} inferior +@var{inferior_ptid} has changed to the new content pointed by @var{buf}. +@end deftypefun + @deftypefun void executable_changed (void) The executable being debugged by GDB has changed: The user decided to debug a different program, or the program he was debugging has diff --git a/gdb/eval.c b/gdb/eval.c index 26e0cc8..b099499 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -939,7 +939,17 @@ evaluate_subexp_standard (struct type *expect_type, if (val == NULL) error (_("Value of register %s not available."), name); else - return val; + { + /* Store the register number if it is not a user register. + We don't have an easy way to get the number of cooked/frame + register for a given user register, because on some arch, + a 64-bit user register is composed by two 32-bit frame + registers. */ + if (regno < (gdbarch_num_regs (exp->gdbarch) + + gdbarch_num_pseudo_regs (exp->gdbarch))) + VALUE_REGNUM (val) = regno; + return val; + } } case OP_BOOL: (*pos) += 2; diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c index dc47bc1..41cd889 100644 --- a/gdb/mi/mi-cmd-var.c +++ b/gdb/mi/mi-cmd-var.c @@ -618,6 +618,7 @@ mi_cmd_var_assign (char *command, char **argv, int argc) struct varobj *var; char *expression, *val; struct cleanup *cleanup; + int *p; if (argc != 2) error (_("-var-assign: Usage: NAME EXPRESSION.")); @@ -630,11 +631,14 @@ mi_cmd_var_assign (char *command, char **argv, int argc) expression = xstrdup (argv[1]); - /* MI command '-var-assign' may write memory, so suppress memory - changed notification if it does. */ - cleanup - = make_cleanup_restore_integer (&mi_suppress_notification.memory); + /* MI command '-var-assign' may write memory or register, so suppress + memory or register changed notification if it does. */ + p = &mi_suppress_notification.memory; + make_cleanup_restore_integer (p); mi_suppress_notification.memory = 1; + p = &mi_suppress_notification.register_changed; + cleanup = make_cleanup_restore_integer (p); + mi_suppress_notification.register_changed = 1; if (!varobj_set_value (var, expression)) error (_("-var-assign: Could not assign " diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index d3c3d81..5c78b9a 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -36,6 +36,7 @@ #include "solist.h" #include "gdb.h" #include "objfiles.h" +#include "regcache.h" /* These are the interpreter setup, etc. functions for the MI interpreter. */ @@ -79,6 +80,8 @@ static void mi_breakpoint_modified (struct breakpoint *b); static void mi_command_param_changed (const char *param, const char *value); static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr, ssize_t len, const bfd_byte *myaddr); +static void mi_register_changed (int regnum, struct frame_info *frame, + ptid_t ptid, const gdb_byte *buf); static int report_initial_inferior (struct inferior *inf, void *closure); @@ -142,6 +145,7 @@ mi_interpreter_init (struct interp *interp, int top_level) observer_attach_breakpoint_modified (mi_breakpoint_modified); observer_attach_command_param_changed (mi_command_param_changed); observer_attach_memory_changed (mi_memory_changed); + observer_attach_register_changed (mi_register_changed); /* The initial inferior is created before this function is called, so we need to report it explicitly. Use iteration in @@ -885,6 +889,51 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr, gdb_flush (mi->event_channel); } +/* Emit notification about the register change. */ + +static void +mi_register_changed (int regnum, struct frame_info *frame, ptid_t ptid, + const gdb_byte *buf) +{ + if (!mi_suppress_notification.register_changed) + { + struct mi_interp *mi = top_level_interpreter_data (); + struct ui_out *mi_uiout + = interp_ui_out (top_level_interpreter ()); + struct gdbarch *gdbarch = get_frame_arch (frame); + int regsize = register_size (gdbarch, regnum); + ULONGEST val = extract_unsigned_integer (buf, regsize, + gdbarch_byte_order (gdbarch)); + struct thread_info *ti = find_thread_ptid (ptid); + struct inferior *inf = find_inferior_pid (ptid_get_pid (ptid)); + struct ui_out *saved_uiout; + + gdb_assert (ti); + gdb_assert (inf); + + target_terminal_ours (); + + fprintf_unfiltered (mi->event_channel, "register-changed"); + + ui_out_redirect (mi_uiout, mi->event_channel); + + ui_out_field_string (mi_uiout, "name", + gdbarch_register_name (gdbarch, regnum)); + ui_out_field_fmt (mi_uiout, "value", "0x%s", phex_nz (val, regsize)); + ui_out_field_int (mi_uiout, "group-id", inf->num); + ui_out_field_int (mi_uiout, "thread-id", ti->num); + + saved_uiout = current_uiout; + current_uiout = mi_uiout; + print_stack_frame (frame, 0, SRC_AND_LOC); + current_uiout = saved_uiout; + + ui_out_redirect (mi_uiout, NULL); + + gdb_flush (mi->event_channel); + } +} + static int report_initial_inferior (struct inferior *inf, void *closure) { diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h index a815fbe..0f4b064 100644 --- a/gdb/mi/mi-main.h +++ b/gdb/mi/mi-main.h @@ -43,6 +43,8 @@ struct mi_suppress_notification int traceframe; /* Memory changed notification suppressed? */ int memory; + /* Register changed notification suppressed? */ + int register_changed; }; extern struct mi_suppress_notification mi_suppress_notification; diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp index bab2373..ad0cdb8 100644 --- a/gdb/testsuite/gdb.mi/mi-cli.exp +++ b/gdb/testsuite/gdb.mi/mi-cli.exp @@ -188,12 +188,8 @@ mi_gdb_test "-interpreter-exec console \"help set args\"" \ {\~"Set argument list to give program being debugged when it is started\.\\nFollow this command with any number of args, to be passed to the program\.".*\^done} \ "-interpreter-exec console \"help set args\"" -# NOTE: cagney/2003-02-03: Not yet. -# mi_gdb_test "-interpreter-exec console \"set \$pc=0x0\"" \ -# {.*=target-changed.*\^done} \ -# "-interpreter-exec console \"set \$pc=0x0\"" mi_gdb_test "888-interpreter-exec console \"set \$pc=0x0\"" \ - {888\^done} \ + {.*=register-changed,name=\".*\",value=\"0x0\",group-id=\".*\",frame=.*\^done} \ "-interpreter-exec console \"set \$pc=0x0\"" #mi_gdb_test "-interpreter-exec console \"\"" \ diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.c b/gdb/testsuite/gdb.mi/mi-reg-changed.c new file mode 100644 index 0000000..3d6950b --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-reg-changed.c @@ -0,0 +1,30 @@ +/* This test is part of GDB, the GNU debugger. + + Copyright 2012 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +static int +middle (int x) +{ + x = 2 * x; + x++; + return x; +} + +int +main (int argc, char **argv) +{ + return middle (argc); +} diff --git a/gdb/testsuite/gdb.mi/mi-reg-changed.exp b/gdb/testsuite/gdb.mi/mi-reg-changed.exp new file mode 100644 index 0000000..9934b4d --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-reg-changed.exp @@ -0,0 +1,159 @@ +# Copyright 2012 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +load_lib mi-support.exp +set MIFLAGS "-i=mi" + +standard_testfile .c + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested mi-reg-changed.exp + return -1 +} + +proc test_register_change { } { + with_test_prefix "simple" { + if [mi_gdb_start] { + return + } + mi_run_to_main + + global hex + global decimal + global expect_out + + set reg "" + # Extract the name of a register. + mi_gdb_test "-data-list-register-names" \ + "\\\^done,register-names=\\\[\"(\[a-zA-Z0-9\]+)\".*\\\].*" \ + "-data-list-register-names" + set reg $expect_out(3,string) + + if { $reg == "" } { + fail "get the register name" + return + } + + set reg_val "" + if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] { + set reg_val $expect_out(3,string) + } + if { $reg_val == "" } { + fail "get the content of register ${reg}" + return + } + + mi_gdb_test "set \$${reg} = ${reg_val}" \ + ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}.*\\^done" \ + "change reg ${reg}" + + mi_gdb_test "-var-create fp_var * \$${reg}" \ + "\\^done,name=\"fp_var\",numchild=\"0\",value=\".*\",type=\".*\",has_more=\"0\"" \ + "create objvar for reg ${reg}" + # GDB should not emit MI notification for register change requested + # from MI frontend. + mi_gdb_test "-var-assign fp_var ${reg_val}" \ + "\\^done,value=\".*\"" "change reg ${reg} thru. varobj" + + mi_gdb_exit + } +} + +proc test_register_on_frame { } { + with_test_prefix "frame" { + if [mi_gdb_start] { + return + } + mi_run_to_main + + global decimal + global hex + global expect_out + global mi_gdb_prompt + + # A (register) value in memory in previous frame. E.g., + # call-saved register. + set reg "" + if [is_amd64_regs_target] { + set reg "rbp" + } elseif [is_x86_like_target] { + set reg "ebp" + } else { + return + } + + mi_gdb_test "-break-insert -t middle" \ + "\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"del\".*" \ + "-break-insert -t middle" + mi_execute_to "exec-continue" "breakpoint-hit" "middle" ".*" ".*" \ + ".*" { "" "disp=\"del\"" } "-exec-continue to middle" + + set reg_val "" + + if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] { + set reg_val $expect_out(3,string) + } + + mi_gdb_test "set \$${reg} = ${reg_val}" \ + ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"middle\",.*}.*\\^done" \ + "change reg ${reg}" + + mi_gdb_test "up" ".*" "up" + + set reg_val "" + if ![mi_gdb_test "p/x \$${reg}" ".*($hex).*\\^done" "print register ${reg}"] { + set reg_val $expect_out(3,string) + } + + set test "change reg ${reg} in upper frame" + set see_memory_change 0 + set see_register_change 0 + send_gdb "set \$${reg} = ${reg_val}\n" + # Changing register saved on frame will trigger two notifications, + # register change notification and memory change notification. Check + # them. + gdb_expect { + -re ".*=memory-changed,thread-group=\"i1\",addr=\"${hex}\",len=\"${hex}\"" { + set see_memory_change 1 + exp_continue + } + -re ".*=register-changed,name=\"${reg}\",value=\"${reg_val}\",group-id=\"${decimal}\",thread-id=\"${decimal}\",frame={addr=.*,func=\"main\",.*}" { + set see_register_change 1 + exp_continue + } + -re ".*${mi_gdb_prompt}$" { + } + } + + if $see_register_change { + pass "${test}: register change" + } else { + fail "${test}: register change" + } + if $see_memory_change { + pass "${test}: memory change" + } else { + fail "${test}: memory change" + } + + mi_gdb_exit + } +} + +test_register_change + +test_register_on_frame + +return 0 diff --git a/gdb/valops.c b/gdb/valops.c index 502fb0d..dfea1cb 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1217,6 +1217,7 @@ value_assign (struct value *toval, struct value *fromval) struct type *type; struct value *val; struct frame_id old_frame; + struct frame_info *frame; if (!deprecated_value_modifiable (toval)) error (_("Left operand of assignment is not a modifiable lvalue.")); @@ -1239,7 +1240,8 @@ value_assign (struct value *toval, struct value *fromval) /* Since modifying a register can trash the frame chain, and modifying memory can trash the frame cache, we save the old frame and then restore the new frame afterwards. */ - old_frame = get_frame_id (deprecated_safe_get_selected_frame ()); + frame = deprecated_safe_get_selected_frame (); + old_frame = get_frame_id (frame); switch (VALUE_LVAL (toval)) { @@ -1305,7 +1307,6 @@ value_assign (struct value *toval, struct value *fromval) case lval_register: { - struct frame_info *frame; struct gdbarch *gdbarch; int value_reg; @@ -1462,6 +1463,11 @@ value_assign (struct value *toval, struct value *fromval) set_value_pointed_to_offset (val, value_pointed_to_offset (fromval)); } + if (VALUE_REGNUM (toval) >= 0 && frame != NULL) + observer_notify_register_changed (VALUE_REGNUM (toval), frame, + inferior_ptid, + value_contents (val)); + return val; } diff --git a/gdb/value.c b/gdb/value.c index 3feb1ca..7720cd1 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -318,7 +318,9 @@ struct value taken off this list. */ struct value *next; - /* Register number if the value is from a register. */ + /* Register number if the value presents a register. The contents may + be from a register or memory, which is determined by field + 'LVAL'. */ short regnum; /* Actual contents of the value. Target byte-order. NULL or not -- 1.7.7.6