[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D96458#2578760 <https://reviews.llvm.org/D96458#2578760>, @omjavaid wrote:

> In D96458#2566347 <https://reviews.llvm.org/D96458#2566347>, @labath wrote:
>
>> I'm sorry, my response times are pretty slow these days.
>>
>> I'm thinking about this `ConfigureRegisterInfos` business. I get that the vector length is variable, and so we need to refresh it after every stop. However, the set of enabled extensions seems more static.
>>
>> Is it possible for that to change during the lifetime of the process? I'd guess not, because otherwise we'd have to also resend the register lists after every stop. And, if that's the case, I'm wondering if there's a way to reflect this static-ness in the code. For example, by doing this setup in the constructor, instead of a late `ConfigureRegisterInfos` call. IIRC, the register contexts are created when the thread is stopped, so it should be possible to fetch the information about supported registers quite early.
>>
>> What do you think?
>
> Yes that is doable. I ll move ConfigureRegisterInfos part into constructor.

There's still one `ConfigureRegisterInfos` call in the `RegisterContextCorePOSIX_arm64::ConfigureRegisterContext` function. Is that an omission, or is it intentional (i.e., needed to support some arm feature)? Because, if that's intentional, then the moving the setup to the constructor does not achieve what I wanted, as the set of registers can be overridden afterwards. It also means we need to have a bigger discussion about how to communicate the changed registers between the client and server. OTOH, if it's not intentional, then I'd like to remove that call, to make it obvious that the set of registers cannot change (only their length, for sve registers and such).

So, let me try to ask a direct question: Is there any situation in which the set of registers accessible to the inferior (and lldb-server) can change during the lifetime of a single process. If so, what is it?



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:259
+void RegisterInfoPOSIX_arm64::ConfigureRegisterInfos(
+    lldb_private::Flags &opt_regsets) {
+  // Configures register sets supported by current target. If we have a dynamic
----------------
This should a const reference or just a plain value.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:266
+    m_reg_infos_is_dynamic = true;
+  }
+
----------------
We generally don't use braces around simple statements like this.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:28
+    eRegsetMaskSVE = (1 << 0),
+    eRegsetMaskDynamic = (~0u << 2),
+  };
----------------
mixing signed and unsigned constants looks weird. Maybe just use `~3`?


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

https://reviews.llvm.org/D96458