================
@@ -274,23 +295,31 @@ bool GDBRemoteRegisterContext::ReadRegisterBytes(const
RegisterInfo *reg_info) {
success = false;
else {
// Read the containing register if it hasn't already been read
- if (!GetRegisterIsValid(prim_reg))
+ if (GetRegisterIsUnfetched(prim_reg))
success = GetPrimordialRegister(prim_reg_info, gdb_comm);
----------------
jasonmolenda wrote:
Just working through this in my head a little bit. We have two places where we
decide what to do with a successfully read register (all bytes provided), a
failed read (no bytes) or a partial read (fewer bytes than needed) --
`PrivateSetRegisterValue` for an actual register ("primordial") and then in
`ReadRegisterBytes` for a "slice" register (slice "w0" in the primordial x0 on
AArch64, slice "eax" in the primordial rax on x86_64) where we need to read one
or more primordial registers, and behave the same as `PrivateSetRegisterValue`
based on the results of all primordials.
FTR when a register is a slice register ("w0"), it has one or more
`RegisterInfo::value_regs` which are the primordial registers ("x0") that have
the actual bytes. I don't think there's any target which has a slice register
that spans multiple real (primordial) registers, but shrug the capability
exists.
`PrivateSetRegisterValue` returns `true` if the full number of bytes needed for
a register are provided. If an insufficient number of bytes is provided, the
register is marked as `Unfetched` so we will try to read it again. `false` is
returned if 0 bytes, or an insufficient number of bytes, were provided.
(I'm not convinced about marking a fewer-than-needed bytes as `Unfetched` -
we'll surely get the same result again if the user tries to read the register a
second time. But from a perf point of view, I don't care if we try to read it
again, it's one packet. And this preserves the current lldb behavior, before
my PR)
In `ReadRegisterBytes` we try to read all primordial registers (`value_regs`)
that provide the bytes for the register. We return `success=true` if we have a
RegisterInfo for all value_regs listed (primordial registers), we have the data
or we were able to read the data. If there was a value_reg we don't have a
RegisterInfo for, or we can't get the lldb reg number for it, we return
`success=false`. If `GetRegisterIsUnavailable(prim_reg)` -- that is, we tried
to read the register at this stop previously, and could not, then we return
`success=false`.
I think this last behavior - if a register is marked as `Unavailable`, then we
tried to read it earlier, and it was expedited as not available - is a
reasonable one.
For each primordial register that is Unfetched, and not marked as Unavailable,
we do
```
// Read the containing register if it hasn't already been read
if (GetRegisterIsUnfetched(prim_reg))
success = GetPrimordialRegister(prim_reg_info, gdb_comm);
```
and if `success==false`, that exits our loop and we do
```
if (success) {
// If we reach this point, all primordial register requests have
// succeeded. Validate this composite register.
SetRegisterIsValid(reg_info);
} else {
SetRegisterIsUnavailable(reg_info);
}
```
and here we see my preference expressed in the code - if we failed to read all
the bytes for one of the registers, we mark this slice register as Unavailable.
You are right that the primordial register `PrivateSetRegisterValue` will set
the primordial register to `Unfetched` if it got too few bytes, but I don't
think that's helping anything, it's assuming that re-reading a register will
get different results the second time, if the user tries to do it. It doesn't
seem that bad to mark the slice register Unavailable here, to me.
I'll think about this more, but I think if anything, I would more aggressively
mark a register as Unavailable (instead of `Unfetched`, meaning we can try
again) if we get a short data reply or such, in all the different places we do
this.
https://github.com/llvm/llvm-project/pull/193894
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
