Re: [PATCHv4] gdb: Add default frame methods to gdbarch

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-09-09 00:19, Andrew Burgess wrote:
>> > +
>> > +  release_value (value);
>> 
>> I'm not too familiar with the lifetime of struct value objects, but 
>> why is this
>> needed?
> 
> For why, I'm not really sure.
> 
> If we follow the code through then any call to frame_unwind_register,
> frame_register_unwind, or frame_unwind_register_unsigned will do the
> same release_value.  This patch never set out to change the behaviour
> of existing code, just to remove duplication, so on those grounds
> alone, in this patch at least I'd want the call to stay.
> 
> There is one clue in frame_register_unwind, where we have this code:
> 
>     /* Dispose of the new value.  This prevents watchpoints from
>        trying to watch the saved frame pointer.  */
>     release_value (value);

Ok, that's interesting.  My hypothesis is that when GDB needs to watch 
an expression (a + b + c), it probably looks at the all_values chain to 
know all the intermediary values (that represent a memory location or a 
register) that were read from the target to evaluate that expression.  
It then puts some hardware or software watches on these memory locations 
or registers.  So if the values chain contains a value representing 
something we don't really watch to watch (such as fp or sp, according to 
the comment), GDB would watch it, even though it doesn't really affect 
the result of the expression.  That's just my guess.

> Pretty cryptic, I'm not really sure how we could end up watching that
> value, so I don't really understand that.  (This was added in commit
> 669fac235d5e about 10 years ago).
> 
> My understanding of value lifetime is that generally values live in a
> single all_values list at various points we'll call value_set_mark,
> run some code that might generate some values, and then call
> value_delete_to_mark to delete all values as we know they are no
> longer needed.
> 
> Values can be moved off the global all_values list, for example into
> the value history, which means they will not be deleted by the call to
> value_delete_to_mark.

That's what I understand too.

> Other than, calling this release_value here will help keep the
> all_values list shorter (and the cryptic comment above) I don't see
> why we couldn't just leave the $pc value on the all_values list, and
> clean it up at the next call to value_delete_to_mark....
> 
> ... however, like I said, I'd prefer this patch be just a refactor,
> unless the code is obviously wrong I'd rather keep the call in.

I am fine with that (unless someone who really knows this comes up with 
a better explanation).  Since it just causes the value we don't need 
anymore to be deleted immediatly, it should be harmless.

> All your other comments are addressed in the version below.

This version LGTM.  Can you wait 2-3 days before pushing to see if 
somebody has something to add?

Simon