This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCHv4 3/5] gdb: PR mi/20395: Fix -var-update for registers in frames 1 and up


Hi Andrew,

On 2017-10-19 09:27 AM, Andrew Burgess wrote:
> This patch fixes a problem with using the MI -var-update command
> to access the values of registers in frames other than the current
> frame.  The patch includes a test that demonstrates the problem:
> 
> * run so there are several frames on the stack
> * create a fixed varobj for $pc in each frame, #'s 1 and above
> * step one instruction, to modify the value of $pc
> * call -var-update for each of the previously created varobjs
>   to verify that they are not reported as having changed.
> 
> Without the patch, the -var-update command reported that $pc for all
> frames 1 and above had changed to the value of $pc in frame 0.
> 
> A varobj is created as either fixed, the expression is evaluated within
> the context of a specific frame, or floating, the expression is
> evaluated within the current frame, whatever that may be.
> 
> When a varobj is created by -var-create we set two fields of the varobj
> to track the context in which the varobj was created, these two fields
> are varobj->root->frame and var->root->valid_block.
> 
> If a varobj is of type fixed, then, when we subsequently try to
> reevaluate the expression associated with the varobj we must determine
> if the original frame (and block) is still available, if it is not then
> the varobj can no longer be evaluated.
> 
> The problem is that for register expressions varobj->root->valid_block
> is not set correctly.  This block tracking is done using the global
> 'innermost_block' which is set in the various parser files (for example
> c-exp.y).  However, this is not set for register expressions.
> 
> The fix then seems like it should be to just update the innermost block
> when parsing register expressions, however, that solution causes several
> test regressions.
> 
> The problem is that in some cases we rely on the expression parsing code
> not updating the innermost block got registers.

The "got" seems misplaced here, what did you mean to say?

> 
> One example is when we parse the expression for a 'display' command.
> The display commands treats registers like floating varobjs, but symbols
> are treated like fixed varobjs.  So 'display $reg_name' will always show
> the value of '$reg_name' even as the user moves from frame to frame,
> while 'display my_variable' will only show 'my_variable' while it is in
> the current frame and/or block, when the user moves to a new frame
> and/or block (even one with a different 'my_variable' in) then the
> display of 'my_variable' stops.  For the case of 'display', without the
> option to force fixed or floating expressions, the current behaviour is
> probably the best choice.  For the varobj system though, we can choose
> between floating and fixed, and we should try to make this work for
> registers.
> 
> There's only one existing test case that needs to be updated, in that
> test a fixed varobj is created using a register, the MI output now
> include the thread-id in which the varobj should be evaluated, which I
> believe is correct behaviour.  I also added a new floating test case
> into the same test script, however, right now this also includes the
> thread-id in the expected output, which I believe is an existing gdb
> bug, which I plan to fix next.
> 
> Tested on x86_64 Linux native and native-gdbserver, no regressions.

I read the (quite long) history of this patch series to catch up, and
the approach seems good to me.  I think you have addressed Pedro's
concerns (from 2016-08), but I'll let him take a look, if he has time.

I noted a few minor comments, mostly in the new test.

> gdb/ChangeLog:
> 
>         PR mi/20395

Make sure to use a tab here instead of spaces.

> 	* ada-exp.y (write_var_from_sym): Pass extra parameter when
> 	updating innermost block.
> 	* parse.c (innermost_block_tracker::update): Take extra type
> 	parameter, and check types match before updating innermost block.
> 	(write_dollar_variable): Update innermost block for registers.
> 	* parser-defs.h (enum innermost_block_tracker::track_type): New.
> 	(innermost_block_tracker::reset): Take type parameter.
> 	(innermost_block_tracker::update): Take type parameter, and pass
> 	type through as needed.
> 	(innermost_block_tracker::type): New member.
> 	(operator|): New function for innermost_block_tracker::track_type.
> 	* varobj.c (varobj_create): Pass type when reseting innermost
> 	block.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.mi/basics.c: Add new global.
> 	* gdb.mi/mi-frame-regs.exp: New file.
> 	* gdb.mi/mi-var-create-rtti.exp: Update expected results, add new
> 	case.
> ---
>  gdb/ChangeLog                               |  17 +++
>  gdb/ada-exp.y                               |   2 +-
>  gdb/parse.c                                 |   9 +-
>  gdb/parser-defs.h                           |  24 +++-
>  gdb/testsuite/ChangeLog                     |   8 ++
>  gdb/testsuite/gdb.mi/basics.c               |   2 +
>  gdb/testsuite/gdb.mi/mi-frame-regs.exp      | 187 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.mi/mi-var-create-rtti.exp |   5 +-
>  gdb/varobj.c                                |   3 +-
>  9 files changed, 249 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-frame-regs.exp
> 
> diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
> index ff5650672c..6a5548093b 100644
> --- a/gdb/ada-exp.y
> +++ b/gdb/ada-exp.y
> @@ -761,7 +761,7 @@ write_var_from_sym (struct parser_state *par_state,
>  		    struct symbol *sym)
>  {
>    if (orig_left_context == NULL && symbol_read_needs_frame (sym))
> -    innermost_block.update (block);
> +    innermost_block.update (block, innermost_block_tracker::for_symbols);
>  
>    write_exp_elt_opcode (par_state, OP_VAR_VALUE);
>    write_exp_elt_block (par_state, block);
> diff --git a/gdb/parse.c b/gdb/parse.c
> index 945ef295e6..0333623ee8 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -126,9 +126,12 @@ static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
>     the currently stored block, or if no block is stored yet.  */
>  
>  void
> -innermost_block_tracker::update (const struct block *b)
> +innermost_block_tracker::update (const struct block *b,
> +				 enum innermost_block_tracker::track_type t)
>  {
> -  if (innermost_block == NULL || contained_in (b, innermost_block))
> +  if ((type & t) != 0
> +      && (innermost_block == NULL
> +	  || contained_in (b, innermost_block)))
>      innermost_block = b;
>  }
>  
> @@ -707,6 +710,8 @@ handle_register:
>    str.ptr++;
>    write_exp_string (ps, str);
>    write_exp_elt_opcode (ps, OP_REGISTER);
> +  innermost_block.update (expression_context_block,
> +			  innermost_block_tracker::for_registers);
>    return;
>  }
>  
> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index d9de10dda8..aaf9ce27bd 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -69,20 +69,27 @@ extern CORE_ADDR expression_context_pc;
>  class innermost_block_tracker
>  {
>  public:
> +  enum track_type
> +    {
> +     for_symbols = 0x1,
> +     for_registers = 0x2
> +    };
> +
>    innermost_block_tracker ()
>      : innermost_block (NULL)
>    { /* Nothing.  */ }
>  
> -  void reset ()
> +  void reset (enum track_type t = for_symbols)
>    {
> +    type = t;
>      innermost_block = NULL;
>    }
>  
> -  void update (const struct block *b);
> +  void update (const struct block *b, track_type t);
>  
>    void update (const struct block_symbol &bs)
>    {
> -    update (bs.block);
> +    update (bs.block, for_symbols);
>    }
>  
>    const struct block *block () const
> @@ -91,9 +98,20 @@ public:
>    }
>  
>  private:
> +  enum track_type type;
>    const struct block *innermost_block;
>  };
>  
> +/* Allow the enum for block type to be bitwise-or together.  */
> +
> +inline enum innermost_block_tracker::track_type operator|
> +		(enum innermost_block_tracker::track_type a,
> +		 enum innermost_block_tracker::track_type b)
> +{
> +  return static_cast<enum innermost_block_tracker::track_type>
> +    (static_cast<int> (a) | static_cast<int> (b));
> +}

track_type would ideally be an enum_flags type.  Unfortunately, it doesn't
seem to work to put DEF_ENUM_FLAGS_TYPE in a class, so I assume this is why
you've done it this way.  I'm fine with this, we can look into improving
enum flags later (if it's even possible).  Otherwise, I'd be fine with putting
the type outside the class.

>  /* The innermost context required by the stack and register variables
>     we've encountered so far.  This should be cleared before parsing an
>     expression, and queried once the parse is complete.  */
> diff --git a/gdb/testsuite/gdb.mi/basics.c b/gdb/testsuite/gdb.mi/basics.c
> index 9105e90d1b..d7f024766e 100644
> --- a/gdb/testsuite/gdb.mi/basics.c
> +++ b/gdb/testsuite/gdb.mi/basics.c
> @@ -20,6 +20,8 @@
>   *      on function calls.  Useful to test printing frames, stepping, etc.
>   */
>  
> +unsigned long long global_zero = 0;
> +
>  int callee4 (void)
>  {
>    int A=1;
> diff --git a/gdb/testsuite/gdb.mi/mi-frame-regs.exp b/gdb/testsuite/gdb.mi/mi-frame-regs.exp
> new file mode 100644
> index 0000000000..7a0fff26ac
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-frame-regs.exp
> @@ -0,0 +1,187 @@
> +# Copyright 1999-2016 Free Software Foundation, Inc.

This should probably say -2017.

> +
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# Test essential Machine interface (MI) operations
> +#
> +# Verify that -var-update will provide the correct values for floating
> +# and fixed varobjs that represent the pc register.
> +#
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +standard_testfile basics.c
> +
> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +		 executable {debug}] != "" } then {
> +     untested mi-frame-regs.exp
> +     return -1
> +}
> +
> +# Return the address of the specified breakpoint.
> +
> +proc breakpoint_address {bpnum} {
> +    global hex
> +    global expect_out
> +    global mi_gdb_prompt
> +
> +    send_gdb "info breakpoint $bpnum\n"
> +    gdb_expect {
> +	-re ".*($hex).*$mi_gdb_prompt$" {
> +	    return $expect_out(1,string)
> +	}
> +	-re ".*$mi_gdb_prompt$" {
> +	    return ""

Should we put a FAIL here?  If this happens, it means the test isn't going as planned.

> +	}
> +	timeout {
> +	    return ""

Same here.

I just saw in the caller you check if the return value is "".  I think it would still
be good to put a FAIL (or UNRESOLVED) here, in case somebody uses this function without
checking the return value.  And if they do:

  set addr [breakpoint_address 1]  # this returns ""
  gdb_test "some_command" ".*$addr.*"

the failure would go unnoticced.

> +	}
> +    }
> +}
> +
> +# Test that a floating varobj representing $pc will provide the
> +# correct value via -var-update as the program stops at
> +# breakpoints in different functions.
> +
> +proc do_floating_varobj_test {} {
> +    global srcfile
> +    global hex
> +    global expect_out
> +
> +    gdb_exit
> +    if {[mi_gdb_start]} then {
> +	continue

This "continue" is not in a loop, so it wouldn't work.  I suggest

  fail "couldn't start gdb"
  return

> +    }
> +
> +    with_test_prefix "floating" {

Instead of with_test_prefix, I would suggest declaring the proc with "proc_with_prefix".
The name of the proc is expressive enough to be used as the "scope".

> +	mi_run_to_main
> +
> +	# Create a floating varobj for $pc.
> +	mi_gdb_test "-var-create --thread 1 --frame 0 - @ \$pc" \
> +		    "\\^done,.*value=\"$hex.*" \
> +	            "create varobj for pc in frame 0"
> +
> +	set nframes 4
> +	for {set i 1} {$i < $nframes} {incr i} {
> +
> +	    # Run to a breakpoint in each callee function in succession.
> +	    # Note that we can't use mi_runto because we need the
> +	    # breakpoint to be persistent, so we can use its address.
> +	    set bpnum [expr $i + 1]
> +	    mi_create_breakpoint \
> +	        "basics.c:callee$i" \
> +		"insert breakpoint at basics.c:callee$i" \
> +		-number $bpnum -func callee$i -file ".*basics.c"
> +
> +	    mi_execute_to "exec-continue" "breakpoint-hit" \
> +			  "callee$i" ".*" ".*${srcfile}" ".*" \
> +			  { "" "disp=\"keep\"" } "breakpoint hit"
> +
> +	    # Get the value of $pc from the floating varobj.
> +	    mi_gdb_test "-var-update 1 var1" \
> +			"\\^done,.*value=\"($hex) .*" \
> +			"-var-update for frame $i"
> +	    set pcval $expect_out(3,string)
> +
> +	    # Get the address of the current breakpoint.
> +	    set bpaddr [breakpoint_address $bpnum]
> +	    if {$bpaddr == ""} then {
> +		unresolved "get address of breakpoint $bpnum"
> +		return
> +	    }
> +
> +	    # Check that the addresses are the same.
> +	    if {[expr $bpaddr != $pcval]} then {
> +	        fail "\$pc does not equal address of breakpoint"
> +	    } else {
> +	        pass "\$pc equals address of breakpoint"
> +	    }

I think it would be good to use the same test name if possible.  It's not a huge deal, but
in the buildbot for example, if this test starts failing, it would appear as a new test
that fails, rather than a PASS -> FAIL.

For that purpose, you can use gdb_assert:

  gdb_assert "$bpaddr == $pcval" "\$pc equals address of breakpoint"

> +	}
> +    }
> +}
> +
> +# Create a varobj for the pc register in each of the frames other
> +# than frame 0.
> +
> +proc var_create_regs {nframes} {
> +    global hex
> +
> +    for {set i 1} {$i < $nframes} {incr i} {
> +	mi_gdb_test "-var-create --thread 1 --frame $i - \* \$pc" \
> +		    "\\^done,.*value=\"$hex.*" \
> +	            "create varobj for pc in frame $i"
> +
> +	mi_gdb_test "-var-create --thread 1 --frame $i - \* \"global_zero + \$pc\"" \
> +		    "\\^done,.*value=\"$hex.*" \
> +	            "create varobj for 'global_zero + pc' in frame $i"
> +    }
> +}
> +
> +# Check that -var-update reports that the value of the pc register
> +# for each of the frames 1 and above is reported as unchanged.
> +
> +proc var_update_regs {nframes} {
> +

While at it, would it be good to create a varobj frame 0's $pc and confirm
that it did change?

> +    for {set i 1} {$i < $nframes} {incr i} {
> +	mi_gdb_test "-var-update 1 var$i" \
> +	            "\\^done,(changelist=\\\[\\\])" \
> +	            "pc unchanged in -var-update for frame $i"
> +    }
> +}
> +
> +# Test that fixed varobjs representing $pc in different stack frames
> +# will provide the correct value via -var-update after the program
> +# counter changes (without substantially changing the stack).
> +
> +proc do_fixed_varobj_test {} {
> +    global srcfile
> +
> +    gdb_exit
> +    if {[mi_gdb_start]} then {
> +	continue
> +    }
> +
> +    with_test_prefix "fixed" {
> +	mi_run_to_main
> +
> +	# Run to the function 'callee3' so we have several frames.
> +	mi_create_breakpoint "basics.c:callee3" \
> +			     "insert breakpoint at basics.c:callee3" \
> +			     -number 2 -func callee3 -file ".*basics.c"
> +
> +	mi_execute_to "exec-continue" "breakpoint-hit" \
> +	              "callee3" ".*" ".*${srcfile}" ".*" \
> +		      { "" "disp=\"keep\"" } "breakpoint hit"
> +
> +	# At the breakpoint in callee3 there are 4 frames.  Create a
> +	# varobj for the pc in each of frames 1 and above.
> +	set nframes 4
> +	var_create_regs $nframes
> +
> +	# Step one instruction to change the program counter.
> +	mi_execute_to "exec-next-instruction" "end-stepping-range" \
> +		      "callee3" ".*" ".*${srcfile}" ".*" "" \
> +		      "next instruction in callee3"
> +
> +	# Check that -var-update reports that the values are unchanged.
> +	var_update_regs $nframes
> +    }
> +}
> +
> +do_fixed_varobj_test
> +do_floating_varobj_test
> +
> +mi_gdb_exit
> +return 0

I don't think these last two lines are needed.


> diff --git a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> index ffd448656f..afc9c248ae 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-create-rtti.exp
> @@ -49,6 +49,9 @@ mi_gdb_test "-gdb-set print object on" ".*"
>  # We use a explicit cast to (void *) as that is the
>  # type that caused the bug this testcase is testing for.
>  mi_gdb_test "-var-create sp1 * ((void*)\$sp)" \
> -	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",has_more=\"0\"" \
> +	    "\\^done,name=\"sp1\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",thread-id=\"$decimal\",has_more=\"0\"" \
>  	    "-var-create sp1 * \$sp"
> +mi_gdb_test "-var-create sp2 @ ((void*)\$sp)" \
> +	    "\\^done,name=\"sp2\",numchild=\"0\",value=\"$hex\",type=\"void \\*\",thread-id=\"$decimal\",has_more=\"0\"" \
> +	    "-var-create sp2 @ \$sp"
>  gdb_exit
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 5b5ffd2baf..56affc73f0 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -324,7 +324,8 @@ varobj_create (const char *objname,
>  	}
>  
>        p = expression;
> -      innermost_block.reset ();
> +      innermost_block.reset (innermost_block_tracker::for_symbols
> +			     | innermost_block_tracker::for_registers);
>        /* Wrap the call to parse expression, so we can 
>           return a sensible error.  */
>        TRY
> 

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]