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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
