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

[Pedro Alves <palves at redhat dot com>]

Hi Andrew,

This version of the series all looks good to me.  I only have
comments in this patch.  Fix them, and you're good to go.
Please push.

On 01/20/2018 09:34 PM, Andrew Burgess wrote:

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 13733ea00e8..74789a1abae 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,20 @@
> +2017-10-19  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	PR mi/20395
> +	* 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.
> +

ChangeLog needs updating.  (Here and in commit log.)

> diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
> index 01ac0cd363a..11865b42ba9 100644
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -75,6 +75,24 @@ extern const struct block *expression_context_block;
>     then look up the macro definitions active at that point.  */
>  extern CORE_ADDR expression_context_pc;
>  
> +/* While parsing expressions we need to track the innermost lexical block
> +   that we encounter.  In some situations we need to track the innermost
> +   block just for symbols, and in other situations we want to track the
> +   innermost block for symbols and registers.  These flags are used by the
> +   innermost block track to control which blocks we consider for the
> +   innermost block, these flags can be combined together as needed.  */

The last sentence reads a bit weird to me.  I think you meant:

  "innermost block track" -> "innermost block tracker"

Maybe a period instead of a comma before "these flags"?

>  
>    /* Update the stored innermost block if the new block B is more inner
> -     than the currently stored block, or if no block is stored yet.  */
> -  void update (const struct block *b);
> +     than the currently stored block, or if no block is stored yet.  The
> +     type T tells us whether the block B was for a symbol or for a
> +     register, the stored innermost block is only updated if the type T is
> +     a type we are interested in, which was set during reset.  */

Period instead of comma before "the stored" ?

> +  void update (const struct block *b, innermost_block_tracker_types t);

Thanks,
Pedro Alves