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

Mon Apr 27 08:01:50 PDT 2020

labath added a comment.

Thanks for elaborating. Responses inline.

In D77043#1998621 <https://reviews.llvm.org/D77043#1998621>, @omjavaid wrote:

> 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.

I'm not convinced that this extra flexibility is needed for qRegisterInfo. I mean, there's already a register number  present in the `qRegisterInfo` query. If the stub can look up the appropriate register info when responding to `qRegisterInfoXY`, it should also be able to find the register when it receives a `p` packet. If the stub wants/needs to internally use different register numbers for any reason, it can remap the numbers internally any way it wants.

The situations gets trickier when it comes to `target.xml`. While we could assign register numbers sequentially (and this is what happens if `regnum` is not present), that could quickly get confusing, particularly if the xml description is spread over multiple files. That's why I think it makes sense to have such field in the target.xml. Another reason is that this field (unlike the `invalidate_regnums` or `ehframe_regnum` fields) is actually documented in the gdb spec for this packet.

>> 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.

The question of instruction emulation is more interesting (and I'd really like to hear @jasonmolenda's take on all of this). I presume you mean the use of intstruction emulation on client side, right? (we shouldn't need instruction emulation in lldb-server on aarch64, and in any case, there it can use any register number it wishes).

I believe the only use of instruction emulation in the client right now is to compute the unwind plans. For that, you're right that the numbers of SVE registers don't matter much, as for this use case we are mostly interested in PC, SP, FP, etc., and those won't change. However, the dependence of that component on the "lldb" register numbers does make this approach smell. The question is what to do about that. One possibility is to make lldb-server (and all stubs which want to interact with lldb) send over the "lldb" register numbers. Another would be to make instruction emulation (and lldb in general) less dependant on the information received from the stub. I believe the current opinion (see e.g. http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20190218/048008.html) is that we should do the latter. There hasn't been much work on that front since then as this is never a priority, but I have started some refactors which I hope will be a step towards that (and you might be able to help by reviewing D75607 <https://reviews.llvm.org/D75607>).

Since there are no immediate downsides to instruction emulation from not sending the lldb register numbers I'd say we should go with the flow of requiring less information from the stub rather than against it.

> 
> 
>> (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.

Right but you could in theory get a fairly good approximation debug register value (the only thing missing would be some super-user bits masked by the kernel) through the `NT_ARM_HW_WATCH/BREAK` interface. I think I might find that useful if I was starting to doubt what my debugger is doing and wanted to verify that. However, I don't think we should do that just to "fix" this problem...

And FWIW, I don't have a problem with moving the debug registers down in the list....

> 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.

Well, I hope that if we come up with a reasonable way to remap register numbers, then there should be no problem regardless of how many optional register sets we have.

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

https://reviews.llvm.org/D77043