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

Thu Apr 23 01:34:22 PDT 2020

omjavaid added a comment.

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

> Where does the option which I proposed fit in? It sounds like it would be something like 1a), where we don't actually modify the "invalidate" numbers stored within lldb-server, but we just do the conversion at the protocol level. That way the "invalidate" numbers keep their meaning as "lldb" register numbers in lldb-server, just like they used to. They also keep referring to "lldb" register numbers on the client side, because the client puts the register "index" into the "lldb" field. The only inconsistency is that the "lldb" register numbers assigned to a specific register will be different on the client and server side. However:
>
> - this is not different from what happens in your patch, because you put the "server lldb" number into the "process plugin" field on the client
> - it's not something we should actually rely on if we want to be able to communicate with non-matching server stubs

As far as non-matching stubs are concerned lldb qRegisterInfo handler on host only looks for eRegisterKindEHFrame, eRegisterKindGeneric, and eRegisterKindDWARF. The remaining two fields of register kinds list i-e eRegisterKindProcessPlugin and eRegisterKindLLDB are assigned same value of index by host. It is interesting to note here that when LLDB goes on to access any register on remote side by sending a 'g/G' or 'p/P' packets it makes use of eRegisterKindProcessPlugin although both eRegisterKindLLDB and eRegisterKindProcessPlugin are assigned the same value. So this patch actually gives a remote stub or process plugin an option to send its own regnum if it has to otherwise index will be used as the default behavior. I think eRegisterKindProcessPlugin is there for this purpose in case a stub wants to provide its own register number it can do so but we are actually not allowing any provision of doing so in qRegisterInfo packet while this provision is available in target xml packet and we use it while parsing target xml.

> It seems to me like this solution has the same downsides as the current patch, while having the upside of not requiring protocol changes. That means it can be revisited if we find it to be a problem, while a protocol change is more permanent. That's why I'd like to get a good understanding of why it won't work (or be ugly) if we're not going to do it, and so far I haven't gotten that.

options 1) is similar to what you have proposed whereby we update value_regs and invalidate_regs nos to mean indices before first qRegisterinfo. What this actually means is that register number of all concerned registers in register info list will be updated (All SVE register will have an updated register no). These registers numbers are also used by instruction emulation by accessing the register infos array which for now does pose any problem for now.

> (BTW, if other aarch64 targets (which ones?) are reporting debug registers in their register lists, I don't think it would be unreasonable to do the same on linux)

Linux does not expose AArch64 debug register via ptrace interface and they are mostly needed for manipulation of debug features like hw watchpoints/breakpoints. On Linux PTrace provides interface for installing hw watch/breakpoints but no access for debug regs. The reason i proposed moving debug register down is that they have the least chance of ever being used elsewhere in LLDB code.

Also apart from SVE registers in another patch we ll also be introducing AArch64 Pointer Authentication registers access support. Those register will also have to be moved up in register infos array because  and they may or may not be available for given target and we ll have to choose that by querying target after inferior is spawned. Future features and the features being optional with their registers being available or not is the reason I choose to write this patch rather than taking the options under discussion.

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

https://reviews.llvm.org/D77043