In D100515#2690225, @jasonmolenda wrote:

Thanks Jonas, I think this change to Process can cover

Omair's use case, where the number of bits are used hardcoded.

The Darwin use case were ProcessGDBRemote, DynamicLoaderDarwinKernel, and ProcessMachCore can all set the number of bits used in addressing programmatically, detected at runtime.

The firmware use case where people (often in a python script in a dSYM) can set the number of bits manually so it is correct in their environment, when the dynamic detection schemes are not available.

This looks very good and I have couple of highlights to make from Linux/AArch64 perspective:
qHostInfo is not sufficient for mask calculation on Linux because both data/code ptr auth masks are read by thread register context (both masks are currently same and they same for all the threads of a process).
IMO moving the mask calculation to ABI GetCodeAddress method may be a better option. We can pull in information of addressable bits from process and also read register context for code/data masks by passing threadsp to ABI::GetCodeAddress. But in case we dont get agreement on moving mask calculation to ABI we can calculate mask somewhere inside ProcessGDBRemote and set it from there.... similar to what I have done in D99947.

In D99944#2684280, @jasonmolenda wrote:

Hi Omair, sorry for the delay in looking at this. It seems like we have two overlapping patches here, @justincohen patch in https://reviews.llvm.org/D98886 and this one.

I'm not convinced that we'll see a target where we have noncontiguous bits used for addressing (the best reason for using a mask), but Justin points out that Linux ARM's NT_ARM_PAC_MASK (gettable by ptrace PTRACE_GETREGSET) is expressed as a mask (and googling around, it looks like you can have a different mask for code pointers and data pointers, so Process would really need to store both, but it may be that for crashpad only the code mask needs to be tracked in Process).

lldb needs to allow for users to set the number of bits used for addressing in terms of a number -- 52 in your example -- but given that Linux wants to give us a mask via the ptrace call, I think we should preserve and use that mask. Darwin's API also provide the value in terms of the number of bits used for addressing, and as a user setting I think that's the most natural way of handling it and we should allow it. But internal to Process, we should convert this to a CodeAddress mask and down in the ABI's where we support AArch v8.3 ptrauth, we use bit 55 to clear/set the non-addressing high bits.

Omair, Justin, what do you think here? I don't think it's especially hard to accept this in terms of # of bits OR a mask, and we should use the more general internal rep in lldb. Another alternative would be "the mask should be converted to the # of bits in addressing and stored in Process in those terms".

(Honestly the idea of an address mask makes me nervous all over - if the rule is "take bit 55 and set all non-addressing bits to 0 or 1 depending on that", then we're implicitly saying "the mask only masks off high bits". If it ever masked off bit 0, for instance, on a code address, then we're going to set that to 0 or 1 when we set/clear non-addressing bits? Obviously not. So it's not REALLY a mask, is it, it's just the # of bits used for addressing in another more flexible format that you can't actually flex or it all breaks. :)

I am going ahead and commit this change till we find a better way to fix the sync issue. sleep time set to 2s now.

Pulling GetAuxValue from NativeProcessELF. @labath what do you think.

In D98529#2643850, @jasonmolenda wrote:

In D98529#2631946, @DavidSpickett wrote:

TCR_ELx begins at 1 (see D13.2.123 TCR_EL1, Translation Control Register (EL1) in the armarm) and covers EL0 and 1. Looking at the pseudocode access to this is undefined at EL0.

I updated my local documentation & impl to only record the shared EL0/EL1 T0SZ value, thanks.

So the OS would have to provide you some other way to read that, I know this is missing on Linux at the moment. A user setting might be good for us supporting Linux too. (though I realise your immediate concern is MacOS)

OK we may need to retain the manual setting when I upstream this, instead of going with the pure Process-maintained value determined dynamically by gdb packet or corefile metadata. If this is something you need for your own FixCodeAddress prelim patch, I can upstream the target.process.virtual-addressable-bits setting (I think the name is fine, even once Process can determine this dynamically). We'll need to decide at some point what the correct behavior is when they conflict, but if only one is set the choice is straightforward.

In D98619#2641136, @labath wrote:

In D98619#2641104, @omjavaid wrote:

In D98619#2640734, @labath wrote:

REQUIRES: x86 disables the test in those builds, which don't have the X86 target enabled (in the build config). That is exactly what we want here -- the test is not actually running this code, so it should succeed regardless of the host architecture. It's working fine on aarch64, for instance (http://lab.llvm.org:8011/#/builders/96/builds/5891). I'm not sure what's the deal with the arm bot (cc @omjavaid). It could be some 32-bit thing. I'm going to try to get a better error message first, and then we'll see what to do next...

This is crashing LLDB on arm linux I have reported a bug and marked it xfail. I ll look for a fix later.

Interesting. Crashing actually explains the test output. Thanks for disabling that test. The issue seems to be arm specific as I couldn't reproduce this on i386 lldb (and that makes it pretty weird).

Do you have a backtrace handy by any chance?

In D98619#2640734, @labath wrote:

REQUIRES: x86 disables the test in those builds, which don't have the X86 target enabled (in the build config). That is exactly what we want here -- the test is not actually running this code, so it should succeed regardless of the host architecture. It's working fine on aarch64, for instance (http://lab.llvm.org:8011/#/builders/96/builds/5891). I'm not sure what's the deal with the arm bot (cc @omjavaid). It could be some 32-bit thing. I'm going to try to get a better error message first, and then we'll see what to do next...

Rebased after changes to parent D96458

This update further cleans up and removes static functions that calculated m_register_info_p GetRegisterInfoPtr and m_register_info_count GetRegisterInfoCount

This update address review comments from @labath

In D96458#2625554, @omjavaid wrote:

In D96458#2624962, @labath wrote:

Thanks. This looks much better, but there is still one thing which bugs me about the register info constructor. Currently, there are three paths through that constructor:

1. when we don't have any fancy registers (this is the original one)
2. when we have just SVE (added with the sve support)
3. when we have pauth et al. (added now)

Do we need all three of them? Is there anything which makes SVE special that it deserves its own register info array? Could it be just another "dynamic" register set like the other features? (If that's true, I might consider also removing the first code path, making it just a special case of an empty set of dynamic features.)

I also have some inline comments, but there are just simple stylistic issues, I hope.

I agree with your about SVE being dynamic I just didn't go for the change because I was being a little conservative about changing existing functionality. But I think If we look at the changes now making SVE dynamic seems right thing to do. Let me make the change and update this rev.

In D96458#2624962, @labath wrote:

Thanks. This looks much better, but there is still one thing which bugs me about the register info constructor. Currently, there are three paths through that constructor:

1. when we don't have any fancy registers (this is the original one)
2. when we have just SVE (added with the sve support)
3. when we have pauth et al. (added now)

Do we need all three of them? Is there anything which makes SVE special that it deserves its own register info array? Could it be just another "dynamic" register set like the other features? (If that's true, I might consider also removing the first code path, making it just a special case of an empty set of dynamic features.)

I also have some inline comments, but there are just simple stylistic issues, I hope.

Rebased.

Updated after changes in parent. Also removed D96459

This update incorporates changes written by @labath

In D96458#2618846, @labath wrote:

Would something like

reginfo.diff9 KBDownload

do the trick? It needs a bit of cleanup (I haven't actually removed ConfigureRegisterInfos function yet -- I just moved it right next to the reginfo creation), but I think should make it clear where I'm going with this.

In D96458#2614101, @labath wrote:

In D96458#2614076, @omjavaid wrote:

In D96458#2613948, @labath wrote:

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

In D96458#2613948, @labath wrote:

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

In D96458#2613244, @labath wrote:

In D96458#2578760, @omjavaid wrote:

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

In D96463#2613498, @DavidSpickett wrote:

Did you forget to add the changes? I don't see any diff for the last update.

FIxes stale diff uploaded by mistake.

fixes review comments.

Minor fix to write unique value to all z/p registers when writing them using register write command.

Rebased after changes to parent.

Rebased after changes in parent.

This update fixes review comments highlighted by @rovka

This looks good apart from minor nits inline

I guess we also need LLDB API interface for reading/writing memory tags. So that we can do something like process.ReadMemoryTags() from python. Do you plan on adding it in a later patch?

This looks good overall except for one comment inline.

This is accepted with a caveat above.

Ran clang format and fixed typos.

Review comments resolved.

Rebased after updates to parent.

Rebased after updates to parent.

This moves ConfigureRegisterInfos into constructor.