This is an archive of the discontinued LLVM Phabricator instance.

NativeProcessLinux: Remove some register context boilerplate
ClosedPublic

Authored by labath on Aug 26 2019, 7:35 AM.

Details

Summary

This patch follows the spirit of D63594, and removes some null checks
for things which should be operating invariants. Specifically
{Read,Write}[GF]PR now no longer check whether the supplied buffers are
null, because they never are. After this, the Do*** versions of these
function no longer serve any purpose and are inlined into their callers.

Other cleanups are possible here too, but I am taking this one step at a
time because this involves a lot of architecture-specific code, which I
don't have the hardware to test on (I did do a build-test though).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Aug 26 2019, 7:35 AM

{Read,Write}[GF]PR now no longer check whether the supplied buffers are null, because they never are

But then there is NativeRegisterContextLinux_x86_64::GetFPRBuffer(). Which is never used so maybe together with NativeRegisterContextLinux_x86_64::GetFPRSize() they could be just assert(0);. It can be also considered as a different cleanup patch.

I did not test these patches but at least I am finally setting up now a (silent) buildbot for s390x+ppc64le (I haven't found a suitable ARM offer yet).

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
918 ↗(On Diff #217153)

This keeps the existing bug in place as the size should be rather sizeof(ioVec). But then sizeof(ioVec) is also not much useful as it is used only for PtraceDisplayBytes. It is probably considered as a different cleanup patch so this patch does not touch it.

jankratochvil accepted this revision.Aug 27 2019, 1:53 AM
This revision is now accepted and ready to land.Aug 27 2019, 1:53 AM
labath marked an inline comment as done.Aug 27 2019, 2:24 AM

{Read,Write}[GF]PR now no longer check whether the supplied buffers are null, because they never are

But then there is NativeRegisterContextLinux_x86_64::GetFPRBuffer(). Which is never used so maybe together with NativeRegisterContextLinux_x86_64::GetFPRSize() they could be just assert(0);. It can be also considered as a different cleanup patch.

Hm.. I didn't notice that. I'll put that in separately. Ideally, I'd say these functions should be used, but that may require more cleanups in the x86 register context, such as detecting the register type earlier on instead of just when the operations fail. Overall, there's a lot of room for improvement in this area...

I did not test these patches but at least I am finally setting up now a (silent) buildbot for s390x+ppc64le (I haven't found a suitable ARM offer yet).

That would be awesome. Btw, we already have some arm (32&64) lldb bots set up by linaro, though they don't seem to be in a particularly good shape right now...

source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp
918 ↗(On Diff #217153)

Yeah, I noticed that. I am keeping that for some other patch. Ideally, I'd like to refactor this even more so that one does not have to construct the iovec manually, and fix PtraceDisplayBytes to show the contents of the iovec, but I don't have a clear idea on the best way to do that.

But then there is NativeRegisterContextLinux_x86_64::GetFPRBuffer(). Which is never used so maybe together with NativeRegisterContextLinux_x86_64::GetFPRSize() they could be just assert(0);. It can be also considered as a different cleanup patch.

Hm.. I didn't notice that. I'll put that in separately. Ideally, I'd say these functions should be used, but that may require more cleanups in the x86 register context, such as detecting the register type earlier on instead of just when the operations fail.

If it is left as runtime detected then the current default implementation of NativeRegisterContextLinux::ReadFPR() and NativeRegisterContextLinux::WriteFPR() could be also another class layer with that virtual void *GetFPRBuffer() = 0; virtual size_t GetFPRSize() = 0;.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 5:49 AM