This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Process/NetBSD] Remove unnecessary register buffer abstraction
ClosedPublic

Authored by mgorny on Jun 20 2019, 4:18 AM.

Details

Summary

Remove most of the abstraction over ptrace() register operations,
as it has little value and introduces more code than it saves.
Instead, leave a single ptrace() wrapper method and call it directly
from ReadRegisterSet() and WriteRegisterSet() with correct PT_* request
and buffer.

Remove the remaining direct ReadGPR() and WriteGPR() invocations
with ReadRegisterSet() and WriteRegisterSet().

Cleanup suggested by Pavel Labath in D63545.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Jun 20 2019, 4:18 AM

How about we go even a bit further? The non-Do functions are now so trivial that the Do functions could be inlined into them, producing something like return NativeProcessNetBSD::PtraceWrapper(PT_GETFPREGS, GetProcessPid(), GetFPRBuffer(),m_thread.GetID());. If you wanted to be really fancy, you could define a function like ReadRegisterSet(T regset, void *buffer), and call make that be ReadRegisterSet(PT_GETFPREGS, GetFPRBuffer());

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

I think the idea is that they will be reused when we introduce additional architectures.

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

I think the idea is that they will be reused when we introduce additional architectures.

How exactly would that work? If the other architectures follow the pattern of x86_64, then they will also have a ReadRegisterSet function, which can be used for reading everything. And doing things this way doesn't force every architecture to fit into predefined set of register sets.

This is your code, so that's ultimately up to you, but I'd try to aim for simplicity first, and if it later turns out to be useful to reintroduce these functions, then so be it...

I'll try to simplify it in a few steps and see how that goes. All I'm saying is that ultimately having some DoRegisterSet() wrapper that calls ptrace() and avoids repeating getting PID/TID may help. Though admittely it saves very little actual code.

mgorny updated this revision to Diff 205784.Jun 20 2019, 5:11 AM
mgorny retitled this revision from [lldb] [Process/NetBSD] Simplify register buffer usage to [lldb] [Process/NetBSD] Remove unnecessary register buffer abstraction.
mgorny edited the summary of this revision. (Show Details)

How about this? I've removed almost all abstraction, leaving only simple ptrace() wrapper.

labath accepted this revision.Jun 20 2019, 5:14 AM

Yes, that's exactly what I had in mind. Let's ship it.

This revision is now accepted and ready to land.Jun 20 2019, 5:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 5:42 AM