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

Mon May 18 09:07:37 PDT 2020

labath added a comment.

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.

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

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

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

https://reviews.llvm.org/D77043