Page MenuHomePhabricator

[LLDB] Add support for Arm64/Linux dynamic register sets
ClosedPublic

Authored by omjavaid on Feb 10 2021, 3:51 PM.

Details

Summary

This is the first patch in series for supporting Arm64 dynamic features in LLDB. Arm64 has dynamic features like SVE, Pointer Authentication and MTE which means LLDB needs to decide at run time which registers it needs to pull in for the current executable based on underlying support for a certain feature.

This patch makes necessary adjustments to RegisterInfoPOSIX_arm64 to make way for dynamic register infos and dynamic register sets. This patch does not add any new feature per se but makes way for adding new dynamic register sets in following up patches.

Diff Detail

Event Timeline

omjavaid created this revision.Feb 10 2021, 3:51 PM
omjavaid requested review of this revision.Feb 10 2021, 3:51 PM
DavidSpickett added inline comments.
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
474–475

return GetRegisterInfo().IsSVEReg(reg) ?

718

I spent a good few minutes getting my head around this logic but tell me if I missed something. (a comment explaining it would be great)

  • If we don't have a cached sve header and we don't know that SVE is disabled...
  • Try to read the SVE header
  • if it succeeded and we're doing this read for the first time, setup register infos
  • otherwise it failed so SVE must be disabled
  • if it succeeded (first time or otherwise) query the vector length (this is the bit that tripped me up trying to think how you'd refactor it)
  • if we did have a cached sve header, we'd already know the config, no point reading it again
  • if it's disabled, then it's disabled, same thing.

I think that means you're missing the corner case where we have SVEState::Full say, but then the header fails to read. Is that possible on a real system though? I guess not.

And vector length can change at runtime, correct?

lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
42–43

return m_register_info_up->IsSVEReg(reg); ?

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
248

Should the last condition be <? Given:

m_per_regset_regnum_range[FPRegSet] = std::make_pair(fpu_v0, fpu_fpcr);

Where the last index is the first non FP register.

263

Comments here would really help.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
99

Just curious, is "mode" the term we use for the SVE length? I guess because it's going to be some multiples of 128 isn't it, not an arbitrary bit length.

thanks @DavidSpickett for the review. I have updated diff incorporating you suggestions.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
474–475

Ack.

718

I have updated the comment to reflect what this piece of code tries to do.

Register Infos are maintained by debuggers per process however for SVE we may have per thread register infos but what I understood from linux documentation is for a process to support SVE ptrace call to NT_ARM_SVE should succeed on first stop. This is what we are using to enabled or disabled SVE at startup.

if SVEState::Full is full and we fail to read NT_ARM_SVE at some stage for any reason. we can do try to read again on subsequent stop.

lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
42–43

Ack.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
248

m_per_regset_regnum_range includes start and end register number of a register set with start and end inlucded.

fpu_fpcr is included in FPU regset. On non SVE targets Vx, Dx. Sx register plus fpcr and fpsr are part of FPU regset.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
99

We had chosen mode when we started but then moves away from this terminology. I ve fixed it in next update.

omjavaid updated this revision to Diff 323928.Feb 16 2021, 2:22 AM

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?

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
18–19

let's not use macros here.

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.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
18–19

Ack.

omjavaid updated this revision to Diff 325433.Feb 22 2021, 6:28 AM

This moves ConfigureRegisterInfos into constructor.

rovka added a subscriber: rovka.Mar 5 2021, 4:25 AM

Hi Omair,

I had a look and I think the commit message is slightly misleading because it says "Arm64 has dynamic features like SVE, Pointer Authentication and MTE", but then you don't set m_reg_infos_is_dynamic to true for SVE. This got me a bit confused at first. Maybe it would help to elaborate a bit on this point in the commit message (and maybe also in the comments if you think it makes sense).

I also have a few nits inline.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
100

I was about to suggest using lldb_private::Flags for this, but after looking a bit more through the code it's not clear how this is intended to be used. The definition of eRegsetEnableSVE doesn't look flag-like at all (I would expect to see 1 << 1 rather than just plain 1, to make it clear what future values should look like), and also in ConfigureRegisterContext you check if (opt_regsets > eRegsetEnableSVE), which to me doesn't seem very idiomatic for working with flags.

What do you think about increasing the level of abstraction a bit and using a small wrapper class for the regset options, that can answer directly questions like IsRegsetDynamic()?

107

Nitpick: You should hoist the call out of the if, so it's easier to add other regsets in the future.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
133

Nitpick: Please use square brackets instead of parantheses, otherwise it's confusing (parentheses suggest an open interval).

omjavaid added inline comments.Mar 8 2021, 4:04 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
100

I have given this a thought and although Flags class doesnt completely fit in we still can adjust our logic to use it. I guess using Flags will look neat. I ll update. Thanks

107

This piece will be updated in a child patch of this series where we add logic for Pauth and MTE regsets.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
133

Ack.

omjavaid updated this revision to Diff 329232.Mar 9 2021, 12:23 AM

This update fixes review comments highlighted by @rovka

labath added a comment.Mar 9 2021, 1:06 AM

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?

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
263

This should a const reference or just a plain value.

270

We generally don't use braces around simple statements like this.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
29

mixing signed and unsigned constants looks weird. Maybe just use ~3?

This comment was removed by omjavaid.

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?

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.

labath added a comment.Mar 9 2021, 7:02 AM

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.

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 and also another function for doing the same for m_register_info_interface_up object in NativeRegisterContextRegisterInfo class. Rest will be same as you suggested above.

labath added a comment.Mar 9 2021, 7:57 AM

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.

omjavaid added a comment.EditedMar 9 2021, 8:13 AM

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.

We cannot calculated register set configuration untill we create register context. Both RegisterContextPOSIX_arm64 and NativeRegisterContextRegisterInfo are base class objects for their corresponding register contexts and its not trivial to make them immutable without serious refactoring. I am going to try reading SVE header using a static func as you suggested above but we ll also need one for NativeRegisterContextLinux_arm64 which requires reading SVE state for register config.

Would something like

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.

Would something like

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.

Yes this should work. Thanks for the prompt response. May I use this code in follow up?

Yes, definitely.

omjavaid updated this revision to Diff 330187.Mar 12 2021, 2:44 AM

This update incorporates changes written by @labath

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.

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
223–224

It would be better to initialize these in the else branch of the if (m_opt_regsets.AllSet(eRegsetMaskSVE)) statement

226

Can we make it so that m_register_info_count always contains a valid value?

235

ArrayRef<RegisterInfo> reginfo;

238–239

reginfo = makeArrayRef(g_register_infos_arm64_sve_le, sve_ffr);

244–245

ditto

248

I was also surprised at the <, as ranges in c++ are normally half-open. It may be better to stick with that, even if it means adding a +1 to the make_pair statement.

248–249

llvm::copy(reginfo, std::back_inserter);

250–251

std::copy_n(regset_start, m_register_set_count, ...)

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.

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.

@labath
I forgot to mention but recalled after looking at the code that the major problem/hurdle is g_contained_* and *_invalidates reg list. In case of SVE V, D and S registers are contained by Z register and therefore we will have to update their contained and value reg list accordingly. Only registers declaration which we can skip out of the SVE's register info array are the GPRs X and W. Larger the register set more complicated handling of its dynamic register set will be.
We have three options here:

  1. Keep current state and use separate register info array in case of SVE (Simple solution)
  2. We skip the GPRs out of SVE's register info array and declare all vector registers in SVE's register info array using appropriate contained and invalidate register lists.

Looking at this now I dont see this much of advantage just for the sake of pulling out GPR declaration from SVE register info array. What do you think?

Right, I see. The contained lists are going to make this trickier than I expected, and might even make the code more complicated. (Though this tradeoff will change if we ever end up with two optional regsets that need to mess with contained lists.)

Could we at least then structure the constructor code to avoid checking for SVE twice. Ideally, we would make this a two-step process:

  1. select the "base" register list (either gpr+fpr, or gpr+fpr+sve)
  2. add optional sets

Something like:

if (regsets & SVE) {
  m_register_set_p = reginfos_with_sve;
  // and similar for other variables
} else {
  m_register_set_p = reginfos_without_sve;
}
if (regsets & Dynamic) {
  m_dynamic_reg_infos.assign(m_register_set_p, m_register_set_count)
  if (regsets & pauth)
    m_dynamic_reg_infos.insert(end(), g_pauth_registers); // or whatever
  
  m_register_set_p = m_dynamic_reg_infos.data();
  // etc.
}
omjavaid updated this revision to Diff 331830.Mar 19 2021, 4:54 AM

This update address review comments from @labath

omjavaid updated this revision to Diff 331834.Mar 19 2021, 5:24 AM

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

labath accepted this revision.Mar 22 2021, 2:27 AM

I very happy with how this has turned out. Thanks for your patience.

This revision is now accepted and ready to land.Mar 22 2021, 2:27 AM
This revision was landed with ongoing or failed builds.Mar 30 2021, 4:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 4:39 PM
mgorny added a subscriber: mgorny.Apr 29 2021, 4:26 AM

This changed the API of RegisterInfoPOSIX_arm64 and broke the FreeBSD plugin. I'm going to try to fix it but next time it'd be nice if you checked that the class isn't used by something before making changes to its API.