Re: [PATCH v2][PR gdb/19893] Fix handling of synthetic C++ references

[Martin Galvan <martin.galvan@tallertechnologies.com>]

I've just realized that my comment isn't entirely clear. The
"@address" string is shown only for structure types, not for regular
variables, arrays or such. It makes sense because only those are
affected by set print object, but the comment doesn't explicitly say
so.

I don't know whether it'd be preferable to have <synthetic pointer>
everywhere as opposed to @address. I guess @address is more consistent
with non-synthetic references, and it also hides the fact they're
synthetic from the user, but there are cases (such as
DW_AT_const_value) where @address wouldn't work/make sense.

So I think before proceeding we should decide which output is better.
Perhaps we could show @address whenever possible, and <synthetic
pointer> for the corner cases?

On Tue, May 24, 2016 at 11:51 AM, Pedro Alves <palves@redhat.com> wrote:
> But normal pointers don't print @address either, only references do.

Yeah, I was referring to references :)

> Not printing "@address" with "set print object off" seems like
> hiding information from the user, information that we could show.
> We always print it for non-synthetic references, AFAICS.

Yes, we do.

> Your comment in the patch, in generic_val_print_ref, reads:
>
> +        if options->objectprint is true, c_value_print will call value_addr
> +        on the reference, which coerces synthetic references and returns a
> +        'not_lval'.  */
>
> So if that works, I don't understand -- wouldn't calling value_addr
> or coerce_ref in generic_val_print_ref if you have a synthetic
> reference, or any reference even, be what you'd want?

If I'm not mistaken, doing that would cause us to always print
"@address". Which again, may be in fact the right thing to do.

>> I *could*, however, manually call
>> value->location.computed.funcs->check_synthetic_pointer in
>> generic_val_print_ref instead of using value_bits_synthetic_pointer,
>> thus avoiding the check for lval_computed. But that's a bit ugly IMHO.
>
> I don't understand this one.  Only lval_computed values have a
> "location.computed.funcs" to call.

Yeah, you're right. For a moment I thought all values had an
lval_funcs member, but only computed ones do.

> So is the problem that this bit:
>
>    if (options->addressprint)
>      {
>       CORE_ADDR addr
>         = extract_typed_address (valaddr + embedded_offset, type);
>
> doesn't work / doesn't make sense with synthetic pointers?

Exactly. IIRC extract_typed_address would return zero (or maybe just
garbage?), because valaddr would actually be a pointer to the
synthetic pointer's lval_funcs instead of a target address.

On a related note, it'd be great (for debugging at least!) if unions
such as this had at least a discriminant of sorts.

> Should we be calling value_addr instead?

Perhaps. I stuck to calling extract_typed_address because I tried to
change as little code as possible. I'm seeing that
generic_val_print_ref has a path which coerces the reference anyway,
so we could always coerce it, get its address, and only print the
referenced value when required. I'd have to test it thoroughly to be
sure, though.

> Or are we perhaps missing a lval_funcs method?  (Ideally, all
> value properties/methods would go through a vtable like
> lval_funcs; think "making struct value a proper C++ class" going
> forward.)

Right now I don't think so. The existing methods should be enough to
handle these cases. Speaking of which, are there any plans to rewrite
this sort of object-oriented code in C++? I'd love to take a shot at
this in the future.

> I don't know where, but I think this should indeed be covered by
> tests somewhere.

Ok.