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

Tue May 19 03:45:14 PDT 2020

omjavaid added a comment.

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

> Sorry, about the delay. I had to find some time to dig into the code again, to understand all of the interactions.
>
> It seems that we (lldb) have painted ourselves in the corner somewhat. The thing I did not realize until now is that the "lldb" scheme is documented to be the same as the index into the register info array. (I believed it was used that way, but I didn't expect this level of intent in it.)  The relationship between `qRegisterInfo` and `p` register numbers is not mentioned explicitly anywhere, but reading the documentation of `qRegisterInfo`, I do get the impression that it was intended for them to match.
>
> If we take these two things as a given, we are left with a bunch of sub-optimal alternatives, most of which we have tried already. Basically, once we start having targets with multiple optional register sets, the only way to reconcile these two requirements is to introduce an extra renumbering _somewhere_. The only reason we got away with this so far is because register sets tend to be additive -- the presence of one register set usually implies the presence of all "older" sets as well. And the register context interfaces offers us a very crude tool (GetUserRegisterCount) to weed out any weird registers, by placing them last. This isn't going to be a maintainable long term state, as today's architectures are starting to have a lot of different "optional" registers. (Intel is not promising that all cpus will always have the new MPX registers, for example).
>
> I do believe something will need to be done about that. And I hope that something does not involve adding a new numbering scheme, because I believe we have too many of those already. I think the best solution would be to drop the "lldb regnum == reginfo index" requirement. It's redundant because the index can be computed by subtracting the RegisterInfo pointer from the beginning of the reginfo vector, and it enforces a linear order on the register descriptions (or forces the usage of complicated renumbering schemes). Without that restriction, it would be a simple thing for NativeRegisterContexts to only expose the registers they really want to expose, and there wouldn't be a need for renumberings at the gdb-remote level. I've done some experimenting with that, and while it required changes in a lot if places, the changes were not too complicated, and I did not run into any fundamental problems. Most of the time, the changes actually ended up simplifying the code.

So I agree with your thoughts on  dropping the "lldb regnum == reginfo index" and have already started moving in that direction wit D80105 <https://reviews.llvm.org/D80105>. I plan to come up with a feature based register infos and access to register sets will be available through RegisterInfoInterface. 
A register context can configure register infos to add features and those register sets will be exposed to the register context. The infos pointer will be pointer to a dynamically created list of register infos whcih are part on currently configured register context. This is partially being done for sve but a more complete solution is needed for future features.

> Now, as for your patch set, I don't think that it should be on you to implement all of this, though I would strongly encourage it, and promise to help along. I think this last version is something that we could go with, even though it's definitely still less than ideal. But before we go through with that, let me circle back to one of the previous ideas. A bunch of revisions ago, you mentioned the possibility of inserting the sve registers before the watch/breakpoint registers in the register info vector (and the lldb enumeration).

There is this issue of feature selection like SVE available for a target but pointer authentication registers are available or other way around then this problem of jumping over one of them will again present itself. So it wont be a good idea to go for that option.

> Do we know of any reason why that would not work? After playing around with this code (for far too long), I've come to believe that this should work right now. It won't solve any of the underlying problems, but it should unblock this patch set, and it would match what was e.g. done when we added the intel mpx registers (D24255 <https://reviews.llvm.org/D24255>). While it won't work for targets that wish to expose debug registers, I'm pretty sure we don't have any targets like that right now. Based on everything I've learned so far, I believe that change should only impact servers who actually depend on the the registers embedded in the source code (and not on lldb client communicating with them). Right now, only lldb-server depends on these, and it does not expose the debug registers...

After looking at RegisterContextWindows_arm64 and Darwin_arm64 it seems both use RegisterInfos_arm64.h directly and do not make use of RegisterInfosPOSIX_arm64 class. I am planning to extend register infos interface to have register set manipulation methods and then expose a dynamic register infos list with no dependence on LLDB register numbers. And meanwhile we can go with current solution of SVE registers support in its current form.

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

https://reviews.llvm.org/D77043