This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Initial version of FreeBSD remote process plugin
ClosedPublic

Authored by mgorny on Oct 4 2020, 3:23 AM.

Details

Summary

Add a new FreeBSD Process plugin using client/server model. This plugin
is based on the one used by NetBSD. It currently supports a subset
of functionality for amd64. It is automatically used when spawning
lldb-server. It can also be used by lldb client by setting
FREEBSD_REMOTE_PLUGIN environment variable (to any value).

The code is capable of debugging simple single-threaded programs. It
supports general purpose, debug and FPU registers (up to XMM) of amd64,
basic signalling, software breakpoints.

Adding the support for the plugin involves removing some dead code
from FreeBSDPlatform plugin (that was not ever used because
CanDebugProcess() returned false), and replacing it with appropriate
code from NetBSD platform support.

Diff Detail

Event Timeline

mgorny created this revision.Oct 4 2020, 3:23 AM
mgorny requested review of this revision.Oct 4 2020, 3:23 AM
krytarowski added inline comments.Oct 4 2020, 7:09 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
23

Why this line?

488

Maybe here and in other places: struct kinfo_vmentry *vm = kinfo_getvmmap(GetID(), &count);

mgorny updated this revision to Diff 296051.Oct 4 2020, 8:59 AM

Cleaned up unused includes + clang-format.

mgorny added inline comments.Oct 4 2020, 9:00 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
23

Because otherwise it fails to compile? ;-)

488

You've mentioned previously that you'd prefer for us not to use -lutil. Should I inline the sysctls instead?

krytarowski added inline comments.Oct 4 2020, 9:53 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
23

OK, you have removed the unused include.

488

Yes, please.

arichardson added inline comments.Oct 4 2020, 9:56 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
493

This code is C++ so using for-loop initializers is fine.

mgorny updated this revision to Diff 296059.Oct 4 2020, 10:55 AM

Inlined memory map getting via sysctl.

mgorny marked 5 inline comments as done.Oct 4 2020, 10:56 AM
mgorny added inline comments.
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
493

I'm sorry but the underlying code is gone already ;-).

labath added a comment.Oct 5 2020, 3:01 AM

Looks pretty straigh-forward. I'm getting a lot of deja-vu when looking at this code, though it's not clear to me whether its identical of just similar. Have you investigated the possibility of factoring some parts of this && the netbsd code (and maybe linux too) into some common place?

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
269–375

I think it's time for a switcheroo -- move this code into PlatformPOSIX::DebugProcess, and move that function into PlatformDarwin.

274

?

lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
82

I would expect that the check above is sufficient, is it not?

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
23

Bonus cookies for anyone who makes it possible to remove that line.

lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.cpp
9–19

This grouping is very random. Normal llvm style is to just put all includes in one block and let clang-format sort that out.

mgorny marked an inline comment as done.Oct 5 2020, 4:52 AM

Indeed I've been wondering how we could dedupe this. At least large part of hw watchpoint handling should be reusable.

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
274

Nów everyone knows I've been doing printf debugging!

lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
82

Could you be more specific? I'm pretty sure I had to do this or otherwise the old plugin ended up being used. This is also how windows does it.

mgorny added a comment.Oct 5 2020, 4:53 AM

Also a potentially interesting concept would be to unify reg constants between platforms.

labath added a comment.Oct 5 2020, 5:14 AM

BTW, are you running the lldb-server test suite on this? How's it shaping up?

Also a potentially interesting concept would be to unify reg constants between platforms.

Which constants do you have in mind?

lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
82

Hmm... interesting. But windows does not have the check in PlatformWindows. Does that mean that check is redundant? I find it strange that we need to check this at two places..

krytarowski added inline comments.Oct 5 2020, 7:47 AM
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
269–375

Please defer any refactoring to a separate, follow up commit. Here is a lot of room for code deduplication, at least for ELF platforms.

lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
491

Assuming that the process is always stopped, we don't need to increment len.

mgorny updated this revision to Diff 296238.Oct 5 2020, 11:11 AM
mgorny marked 2 inline comments as done.

For a start, removed leftover printf and len increment.

mgorny updated this revision to Diff 296439.Oct 6 2020, 6:41 AM

Removed now-redundant DebugProcess.

labath added inline comments.Oct 6 2020, 6:55 AM
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
269–375

I wasn't explicit in that, but I certainly did not mean for that change to be folded into this patch. That said, preparatory patches are generally better than follow-up changes for these kinds of things. Doing it later means that the commit will be larger than necessary (and creates the risk of the change not materializing).

krytarowski accepted this revision.Oct 6 2020, 1:07 PM
This revision is now accepted and ready to land.Oct 6 2020, 1:07 PM
mgorny added inline comments.Oct 8 2020, 7:03 AM
lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
82

That Platform check really means to preserve the old behavior. I don't really know whether it is really needed or not, I've presumed this is safer to do.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 7:03 AM