This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCHv4] gdb: Add default frame methods to gdbarch
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