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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to