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

Wed May 13 01:02:40 PDT 2020

omjavaid marked 2 inline comments as done.
omjavaid added a comment.

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

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

I have tried other solution like overriding GetRegisterInfoAtIndex for NativeRegisterContextLinux_arm64 and doing all this register number magic their but then this culminated into lots of changes related to register sets and the currently presented solution looked cleaner than others.
In case of all registers other than SVE registers UserRegIndexToRegInfosIndex is going to do nothing but return the input as ouput. So using this approach doesnt really impact anything else other than AArch64+SVE.

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

The problem we have is that qRegisterInfo packets uses its own register numbering scheme and we dont have mechanism to push those register numbers through to registerinfos list. lldb register numbers are actually indices and not register number so to speak.

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

The only problem/hurdle we have here is the way we construct our register sets from the indices of registerinfos array which is static. This will require some reshuffling of register sets a revamp of how sets are created in the register context. Any solution that requires changes to register sets will be more extensive than this.

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

This solution requires quite a lot of register numbering magic in RegisterInfos_arm64 and I was reluctant to do that as they are used all over the code with a consideration of a static array with lldb_reg_num as indices.

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

================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:467
+    reg_index = reg_context.RegInfosIndexToUserRegIndex(*reg_num);
     if (i > 0)
       response.PutChar(',');
----------------
Just to explain this call to RegInfosIndexToUserRegIndex: It is more relevant for host's needs to have correct register number living in value_regs list in order to access the correct parent for all the pseudo registers.

As far as invalidate_regs are concerned they actually do not matter much as far as process gdb remote + aarch64 is concerned. Mainly because if we write a pseudo reg we ll actually be writing its parent from value_regs list. This parent will automatically be invalidated by the call to writeregister. When any pseudo register having the same value_regs register is read it will force an invalidation.

So as far as i can see aarch64/linux dont really need invalidate regs list and at one point i thought about removing them but then kept them as it is in case some other transport other than gdb-remote might wanna use it.

================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1809
   const char *const register_set_name =
-      reg_context.GetRegisterSetNameForRegisterAtIndex(reg_index);
+      reg_context.GetRegisterSetNameForRegisterAtIndex(reg_infos_index);
   if (register_set_name)
----------------
GetRegisterSetNameForRegisterAtIndex also needs modifications if we go for the solution you are talking about

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

https://reviews.llvm.org/D77043