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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The invalidate_regs part looks as I would expect. I think it ought to be implemented a bit differently, but that can wait until the bigger issue is resolved.

The bigger issue being the fact that this patch now renumbers all numbers going in/out of the stub (qRegisterInfo, p,P, etc.). It seems I got more than what I bargained for there, though in retrospect, that does not seem totally unexpected, as you were mentioning the problem of the extra dbg registers in the middle of the register list. The part which I didn't get is that this does not only apply to the "lldb" register list, but that these registers also make their way to `NativeRegisterContextLinux_arm64::GetRegisterInfoAtIndex`. Currently that class was culling them (in the same way as other register contexts do for "optional" registers) by returning a smaller value via GetUserRegisterCount. That is, of course, not possible if you have other registers in the list that you want to make available.

So, IIUC, what the current patch does is expose the registers through GetRegisterInfoAtIndex, but then ensures that the `UserRegIndexToRegInfosIndex` conversion skips over them. That wouldn't be too bad were it not for the fact that accessing the register "the right way" is very hard now.

So here's my current doubt. Basically, the main thing this patch does is "hide" the uninteresting (debug) registers from the client. However, those registers still remain exposed through the NativeRegisterContext<->GDBRemoteCommunicationServerLLGS boundary via the `GetRegisterInfoAtIndex` function. What if we took this one step further, and made is so that the debug registers are not exposed from NativeRegisterContextLinux_arm64 at all?

One way to achieve that would be to handle the renumbering inside `GetRegisterInfoAtIndex`. Another would be to ensure that these registers don't even appear in the RegisterInfo array that backs this function (then the current implementation of `GetRegisterInfoAtIndex` would "just work"). All other things being equal, I would prefer the second approach. It seems like that could be easily achieved as you're defining a custom RegisterInfo array for SVE anyway (`g_register_infos_arm64_sve_le` in D77047 <https://reviews.llvm.org/D77047>). We could just exclude dbg registers from that list.

That would still leave us with the question of what does `invalidate_regs` refer to. If it still makes sense for it to refer to "lldb" register numbers, then we can remap it as we discussed previously. But it may turn out that after no remapping is needed -- in which case, even better.


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

https://reviews.llvm.org/D77043