[Lldb-commits] [PATCH] D77043: Fix process gdb-remote usage of value_regs/invalidate_regs

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 2 inline comments as done.
omjavaid added a comment.

In D77043#1971486 <https://reviews.llvm.org/D77043#1971486>, @labath wrote:

> Register infos in lldb are a mess. However lldb seems to be able to communicate (more or less successfully) with stub which know nothing about lldb or numbers it assigns to various registers. So it does not seem like there needs to be (or even _can_ be) one universal constant for a register in all situations.
>
> IIUC, the problem is that on the server side the "invalidate" numbers are given in the form of the "lldb" register numbers, but on we don't send that number over -- instead lldb client makes it up (from the index).
>
> Your patch fixes that by sending that number over explicitly. The thing I don't like about that is that it: a) increases the chance of things going wrong with non-lldb stubs; b) forks the code paths depending on whether one is talking to the old or new stub.
>
> It seems to me like this could be solved in another way -- changing the meaning of the "invalidate" numbers to mean register indexes -- basically blessing the thing that the client is doing now. Since for the current targets the two numbers are the same, that should not make any functional difference, but it would make things work for SVE without forking anything or introducing new concepts. The translation could be done at the protocol level, just before sending the data over.
>
> What do you think of that. Are there any downsides there?


For current implementation I dont think it will break any stubs because newly introduced regnum is totally optional. If regnum is not provided then mocked up register index is used and things will work as they were. It will will only trigger for lldbserver native register context which will have an implementation for   NativeRegisterContextRegisterInfo::GetNativeRegisterIndex

Default is just return the same index:

uint32_t NativeRegisterContextRegisterInfo::GetNativeRegisterIndex(

    uint32_t reg_index) const {
  return reg_index;

}

Downside to invalidate registers is 
It is not intended to store an id for the register it belongs to rather its store a register list for register numbers affected by the containing register.
I guess that is only used by x86 for now and I used it in the other patch AArch64 reginfo patch when I found some strange behavior or register not being updated after write with QEMU.

On a different note LLDB now supports sending over target xml packet and there it has to send a register number along with register name and everything else. Most of stubs like QEMU, OpenOCD, etc use target xml for register description exchange and we may consider giving up qRegisterInfo in favor of target xml in future.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:498
                 LLDB_INVALID_REGNUM, // generic reg num
                 reg_num,             // process plugin reg num
                 reg_num              // native register number
----------------
Here regnum is init to mocked up register index.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:557
+          } else if (name.equals("regnum")) {
+            if (value.getAsInteger(0,
+                                   reg_info.kinds[eRegisterKindProcessPlugin]))
----------------
Here we update register number if it is provided and that too in most cases will be equal to the mocked up register number as long as remote does not implement  NativeRegisterContextRegisterInfo::GetNativeRegisterIndex


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77043/new/

https://reviews.llvm.org/D77043