Re: [PATCH v2 0/3] Read pseudo registers from frame instead of regcache

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Simon Marchi wrote:

> This is a follow-up of
> 
>   https://sourceware.org/ml/gdb-patches/2018-05/msg00705.html
> 
> The main changes are:
> 
> - Re-use the regcache interface to read registers from frames
> - Deal with debug info that contains unwind info for pseudo registers

Sorry for not looking at this earlier ...

I'm not sure I fully agree with this approach, it seems to make the
frame vs. regcache distinction even more confusing (at least to me)
than it already was :-)

We used to have pseudo-registers as a property of the regcache, and
at the frame level there was really no such distinction.  Now, there
may be good reasons to have pseudo-registers instead a property of
the frame, so that they can be independently constructed at each
frame level.  But what seems a bit confusing is to have them as
properties of *both* layers ...

>From that perspective, I actually liked your original approach
better, where you modified the pseudo register read/write
routines to operate on a frame instead of a regcache.  Why did
you decide to move away from that?  Is it just to avoid having
to change all back ends?

In case, maybe we can have a gradual transition here by allowing
targets to introduce new per-frame pseudo-register accessors,
and fall back to the per-regcache accessors if we're at the
sentinel frame and the gdbarch has no per-frame accessors.

(As an aside, the new interfaces should all be value-based, so
we'd merge that transition into the gdbarch_pseudo_register_read
vs. gdbarch_pesudo_register_read_value transition.)

This would also make the new register_readers interfaces
unnecessary.  While I'm not really the C++ expert, I'm not
sure I like the use of multiple (in particular virtual)
inheritance here; I thought this was typically one of the
features that people are trying to avoid ...  (B.t.w. do
we have a formal coding style documenting which C++ features
we want to use vs. avoid in GDB code?)

As a minor thing that occurred to me in the patch itself:

+	  value
+	    = gdbarch_pseudo_register_read_value (gdbarch, &reg_read, regnum);
+	  VALUE_LVAL (value) = not_lval;

We really want to get away from "resetting" VALUE_LVAL to
something else.  Instead, values should be created correctly
once in the first place.  This is another of those decades-long
transitions we have, but we should probably avoid making it worse.

Note that the implementation in amd64_pseudo_register_read_value
already does:
  value *result_value = allocate_value (register_type (gdbarch, regnum));
  VALUE_LVAL (result_value) = lval_register;
  VALUE_REGNUM (result_value) = regnum;
  gdb_byte *buf = value_contents_raw (result_value);

which also violates the rule, and is actually wrong since lval_register
values really should always be based relative to a *frame*.  This is
another argument b.t.w. for making the _pseudo_register_read_value
routines be frame-based ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com