[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#2614076 <https://reviews.llvm.org/D96458#2614076>, @omjavaid wrote:

> In D96458#2613948 <https://reviews.llvm.org/D96458#2613948>, @labath wrote:
>
>> In D96458#2613894 <https://reviews.llvm.org/D96458#2613894>, @omjavaid wrote:
>>
>>> ConfigureRegisterContext is called only once in the lifetime of a core from RegisterContextCorePOSIX_arm64 constructor. A process registers cannot change during execution which is what made basis of this change. for NativeRegisterContextLinux_arm64 I moved the ConfigureRegisterInfos logic out of configure register context because it will be called again and again during execution. But for RegisterContextCorePOSIX_arm64 we call it once in the constructor so its safe to keep the function.
>>
>> Ok, I understand what's happening now -- thanks for re-iterating. I have managed to miss the fact that this patch deals with both core file and live register contexts.
>>
>> In that case, I'd like to go one step further, and remove the "configuration" operation from the `RegisterInfoPOSIX_arm64` class as well (by making it a part of the construction). It should be possible to do that by fetching the information needed to determine the registers before the class is instantiated. For the live context, that could be done inside the `CreateHostNativeRegisterContextLinux` function. For the core file context, we could create a similar factory function as well. I.e. move the construction of `RegisterInfoPOSIX_arm64` from ThreadElfCore into some static function inside `RegisterContextCorePOSIX_arm64` -- this function could examine the core data to determine the registers and construct the appropriate info class.
>
> hmm .. in that case we will have to defer creation of RegisterInfoPOSIX_arm64 as you said above. I will create a function that will allow us to update unique_ptr m_register_info_up in RegisterContextPOSIX_arm64

That would make the registers of a RegisterInfoPOSIX_arm64 instance immutable (which is good), but that function would allow one to mutate the RegisterContextPOSIX_arm64 class in the middle of its lifetime by swapping the register info backing it (which is exactly what I'm trying to avoid). I'm trying to create an apis that is impossible/hard to use incorrectly. I'd like to avoid this kind of "configuration" functions unless they are really necessary. I could be wrong, but right now I don't see a reason why they'd be necessary.


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

https://reviews.llvm.org/D96458