Page MenuHomePhabricator

[lldb] Add GetCodeAddressMask and GetDataAddressMask to Process
ClosedPublic

Authored by JDevlieghere on Apr 14 2021, 4:47 PM.

Details

Summary

Implement Jason's suggestion from D99944 to have both a code and data mask, while still honoring the existing setting.

Diff Detail

Event Timeline

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.

Justin's case where ptrace(PTRACE_GETREGSET) gets us a code and data address mask.

Canonicalizing them as masks in Process, accepting either form, and providing the value as a mask, should make it easy to use in the ABI plugins where we have a FixCodeAddress and FixDataAddress.

On Darwin, we use the same number of bits for both code and data, but given the way ptrace() behaves on Linux, I'm guessing this may not be the case everywhere. Should we store both masks, and add FixCodeAddress + FixDataAddress methods in the ABI's, Justin? What do you think?

The Darwin support can be adapted to this centralized scheme that Jonas has written easily. Everyone else OK with this?

I don't feel strongly about the method name. Justin used Process::GetPointerAuthenticationAddressMask which makes it sound specific to ARM v8.3 ptrauth. The ARM v8.5 MTE tagging might be another use case (or TBI as Omair has noted).

DavidSpickett added a comment.EditedApr 15 2021, 1:39 AM

I don't feel strongly about the method name. Justin used Process::GetPointerAuthenticationAddressMask which makes it sound specific to ARM v8.3 ptrauth. The ARM v8.5 MTE tagging might be another use case (or TBI as Omair has noted).

I'd go with the generic code/address name for the full masks (which would remove MTE/PAC/TBI all at once) and if we find situations where you only need one, add those as separate methods.
(maybe we already have a few of those situations, just my guess that most callers want to remove all the non address bits)

For MTE removing only tags is currently in a plugin and we only really need to do it in certain commands. For everything else removing all the non address bits would be fine.

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.

Justin's case where ptrace(PTRACE_GETREGSET) gets us a code and data address mask.

Canonicalizing them as masks in Process, accepting either form, and providing the value as a mask, should make it easy to use in the ABI plugins where we have a FixCodeAddress and FixDataAddress.

On Darwin, we use the same number of bits for both code and data, but given the way ptrace() behaves on Linux, I'm guessing this may not be the case everywhere. Should we store both masks, and add FixCodeAddress + FixDataAddress methods in the ABI's, Justin? What do you think?

The Darwin support can be adapted to this centralized scheme that Jonas has written easily. Everyone else OK with this?

I don't feel strongly about the method name. Justin used Process::GetPointerAuthenticationAddressMask which makes it sound specific to ARM v8.3 ptrauth. The ARM v8.5 MTE tagging might be another use case (or TBI as Omair has noted).

pcc added a subscriber: pcc.Apr 15 2021, 10:08 AM
pcc added inline comments.
lldb/source/Target/Process.cpp
5569–5570

Is this part correct? (Same below.) In D100521 you have

if (pc & pac_sign_extension)
  return pc | mask;
return pc & (~mask);

So it looks like this would cause the pc to be set to 0 (or -1)?

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

Jonas was handling the Darwin case where debugserver returns the number of bits used in addressing in the qHostInfo packet here -- and we use the same value for both. On Linux, we would want lldb-server to fetch the masks and send them up as well. Given that they are fetched with ptrace(), it would make more sense to put these masks in the qProcessInfo response where we're attached to a process. I think we always send that packet after we've launched/attached to something.

You would get masks, so those would be set in Process directly.

IMO moving the mask calculation to ABI GetCodeAddress method may be a better option.

I believe Jonas still wants to have an ABI::FixCodeAddress and ABI::FixDataAddress (I'm still not SUPER thrilled with the name "Fix", but whatever, we can refine it easily if someone has a better suggestion that isn't super long). But the Process will maintain the dynamically-determined masks and the user-specified override setting. The ABI will get the current mask from Process when running these methods.

We're going to have the mask format be the canonical one in Process, so FixCodeAddress and FixDataAddress methods in ABI will get it in that form from Process.

justincohen accepted this revision.Apr 15 2021, 11:59 AM

On Darwin, we use the same number of bits for both code and data, but given the way ptrace() behaves on Linux, I'm guessing this may not be the case everywhere. Should we store both masks, and add FixCodeAddress + FixDataAddress methods in the ABI's, Justin? What do you think?

This all LGTM! I don't have a strong opinion on naming. My understanding is both code and data will be necessary in case TBID0 is set on Linux.

This revision is now accepted and ready to land.Apr 15 2021, 11:59 AM
jasonmolenda added inline comments.Apr 15 2021, 12:30 PM
lldb/source/Target/Process.cpp
5569–5570

I get confused so I like to do this by hand quickly to make sure I understand.

given mask of 1110000 and addr of xxx1011 where 'x' are PAC bits,

b55 == 1: m | a == 1111011
b55 == 0: ~m & a == 0001011

given mask of 1111111, low address 0001011 and high address 1111011,

b55 == 1: m & ha == ha
b55 == 0: ~m | la == la

am I not thinking of something that could unify these? I can confuse myself so easily with these things.

We could also detect a mask of -1 and just return the original address in FixCodeAddress/FixDataAddress, right. That would be very simple.

JDevlieghere added inline comments.Apr 15 2021, 1:59 PM
lldb/source/Target/Process.cpp
5569–5570

I've added checking for -1 in D100521

jasonmolenda accepted this revision.Apr 15 2021, 10:06 PM

I think we've heard from everyone involved with this & it's good to land. Thanks for bringing this home.

pcc added inline comments.Apr 16 2021, 12:07 AM
lldb/source/Target/Process.cpp
5569–5570

With a mask of 1111111 isn't it

b55 == 1: m | ha == 1111111
b55 == 0: ~m & la == 0000000

I think you can just remove lines 5569-5570 here as well as lines 819-820 from D100521.

jasonmolenda added inline comments.Apr 16 2021, 12:51 AM
lldb/source/Target/Process.cpp
5569–5570

urk. my caveat that I often confuse myself with these has been proven true. :)

So you're suggesting the default, uninitialized, mask is 0. If we have that, then

mask 0, low address 0001011, high address 1111011
b55 == 1: m | ha == 1111011
b55 == 0: ~m & la == 0001011

In the mask, any bit set to 0 is passed through as-is. Any bit set to 1 is going to be cleared or set in these FixAddress methods. I see.

pcc added inline comments.Apr 16 2021, 8:19 AM
lldb/source/Target/Process.cpp
5569–5570

Yes, exactly.

Out of curiosity: Typically should one be able to set target.process.virtual-addressable-bits after the target has been created? Or is it expected that users will need to run in the following order only:

settings set target.process.virtual-addressable-bits ...
target create -c ....

Setting virtual-addressable-bits won't do anythin after the target has been created (and perhaps that is working as intended?)

Out of curiosity: Typically should one be able to set target.process.virtual-addressable-bits after the target has been created? Or is it expected that users will need to run in the following order only:

settings set target.process.virtual-addressable-bits ...
target create -c ....

Setting virtual-addressable-bits won't do anythin after the target has been created (and perhaps that is working as intended?)

Yep, that was exactly why I'm always reading the setting if the mask isn't set yet (as opposed to setting it once in the constructor).

This revision was landed with ongoing or failed builds.Apr 16 2021, 12:31 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 12:31 PM

Out of curiosity: Typically should one be able to set target.process.virtual-addressable-bits after the target has been created? Or is it expected that users will need to run in the following order only:

settings set target.process.virtual-addressable-bits ...
target create -c ....

Setting virtual-addressable-bits won't do anythin after the target has been created (and perhaps that is working as intended?)

Yep, that was exactly why I'm always reading the setting if the mask isn't set yet (as opposed to setting it once in the constructor).

And it's an open question of what's the correct behavior if there's a dynamically-set value AND a user specified setting. The patch currently prefers the dynamically-set value and I think that's the right choice, but it'll be interesting to see if there's a platform/system where that doesn't work for some users.