[Lldb-commits] [lldb] [lldb] New expedited register specfication for unavailable regs (PR #193894)

[Jason Molenda via lldb-commits]

================
@@ -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