Re: [PATCH] value_optimized_out and value_fetch_lazy

Pedro Alves
2013-06-19T15:50:42Z

On 06/10/2013 11:24 AM, Andrew Burgess wrote:
> Hi,
>
> I ran into an issue with gdb that appears to be caused by an incorrect
> use of value_optimized_out.
>
> Currently, if value_optimized_out returns true, this means we know the
> value is optimized out, if value_optimized_out returns false this
> means the value might, or might not, be optimized out; if the value
> is currently lazy then we don't know it has been optimized out, and
> so the answer from value_optimized_out is false.

Looks like the main culprit would be frame_register_unwind.

>
> I believe that currently, not every caller of value_optimized_out
> follows these rules, and some cases if value_optimized_out returns
> false, they treat the value as though it is NOT optimized out.
>
> The patch below changes value_optimized_out so that if the value
> is lazy it will be loaded, this should ensure that by the time
> value_optimized_out returns, the value will be correct. I've fixed
> the one place I could see where we were obviously taking care to
> resolve lazy values and I've also made a few tweaks to some of the
> other value code in order to prevent recursion while loading lazy
> values, and to try and keep values lazy as much as possible.
>
> Feedback welcome, or ok to commit?

I agree with this. It might be possible to factor out the bits
of value_fetch_lazy that handle optimized out values, and use it
from value_optimized_out, avoiding fetching the value's contents,
but given the case I believe where it could most matter, backtracing,
in frame_register_unwind, already does:

*optimizedp = value_optimized_out (value);
*unavailablep = !value_entirely_available (value);

and value_entirely_available does already:

int
value_entirely_available (struct value *value)
{
/* We can only tell whether the whole value is available when we try
to read it. */
if (value->lazy)
value_fetch_lazy (value);

... it's super fine with me to make the code correct first,
and consider optimizing value_optimized_out's internals at some
other point.

(while at it, it seems value_entirely_available doesn't really
need to call value_fetch_lazy for optimized out values.)

I'm finding the patch a bit hard to read though. Could you
split it up? At least the move of value_fetch_lazy to value.c
would be better done in its own. Pretty hard to reason
about or even tell which were the value_fetch_lazy changes
from the diff.

Can you explain a little better the value_primitive_field
changes? Is that code motion mainly an optimization that could
be done separately, or is it necessary for correctness? IOW, is
that something that could be split out too?

Please add copyright headers to the tests. I also noticed
the patch adds spurious trailing whitespace:

$ git am /tmp/mbox
Applying: value_optimized_out and value_fetch_lazy
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:609: trailing whitespace.
.uleb128 0x7 /* ULEB128 register */
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1032: trailing whitespace.
int
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1036: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1104: new blank line at EOF.
+
warning: 4 lines add whitespace errors.

--
Pedro Alves