Page MenuHomePhabricator

[lldb] Introduce PlatformQemuUser
ClosedPublic

Authored by labath on Nov 24 2021, 12:14 AM.

Details

Summary

This adds a new platform class, whose job is to enable running
(debugging) executables under qemu.

(For general information about qemu, I recommend reading the RFC thread
on lldb-dev
https://lists.llvm.org/pipermail/lldb-dev/2021-October/017106.html.)

This initial patch implements the necessary boilerplate as well as the
minimal amount of functionality needed to actually be able to do
something useful (which, in this case means debugging a fully statically
linked executable).

The knobs necessary to emulate dynamically linked programs, as well as
to control other aspects of qemu operation (the emulated cpu, for
instance) will be added in subsequent patches. Same goes for the ability
to automatically bind to the executables of the emulated architecture.

Currently only two settings are available:

  • architecture: the architecture that we should emulate
  • emulator-path: the path to the emulator

Even though this patch is relatively small, it doesn't lack subtleties
that are worth calling out explicitly:

  • named sockets: qemu supports tcp and unix socket connections, both of them in the "forward connect" mode (qemu listening, lldb connecting). Forward TCP connections are impossible to realise in a race-free way. This is the reason why I chose unix sockets as they have larger, more structured names, which can guarantee that there are no collisions between concurrent connection attempts.
  • the above means that this code will not work on windows. I don't think that's an issue since user mode qemu does not support windows anyway.
  • Right now, I am leaving the code enabled for windows, but maybe it would be better to disable it (otoh, disabling it means windows developers can't check they don't break it)
  • qemu also does not support macOS, so one could contemplate disabling it there too. However, macOS does support named sockets, so one can even run the (mock) qemu tests there, and I think it'd be a shame to lose that.
  • some things in this patch are called "Qemu" (the plugin folder, for instance), while others (e.g., the class) have "QemuUser" in their name. I did this intentionally to make room for a hypothetical system-mode qemu plugin. However, that may have been premature...

Diff Detail

Event Timeline

labath requested review of this revision.Nov 24 2021, 12:14 AM
labath created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2021, 12:14 AM
mgorny added inline comments.Nov 24 2021, 2:20 AM
lldb/packages/Python/lldbsuite/test/gdbclientutils.py
403

It's customary (read: PEP8) to use two empty lines between global-scope stuff like classes (and then one line between functions inside the class).

lldb/test/API/qemu/TestQemuLaunch.py
32

%r will use the repr() form, i.e. suitable for direct reuse in Python with the right escaping and so on.

43–44

Wouldn't it make sense to reverse these two? i.e. if it fails, then platform is unlikely to be correct, is it?

lldb/test/API/qemu/qemu.py
39

Oh no, trailing empty line!

labath marked 4 inline comments as done.Nov 24 2021, 2:53 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/gdbclientutils.py
403

Ok, sounds good.

lldb/test/API/qemu/TestQemuLaunch.py
32

good point. thanks.

43–44

Yeah, I suppose it does.

lldb/test/API/qemu/qemu.py
39

:P

labath updated this revision to Diff 389443.Nov 24 2021, 2:54 AM
labath marked 4 inline comments as done.

Address Michał's feedback.

mgorny accepted this revision.Nov 24 2021, 3:01 AM

That said, I wonder if we should use some formatter (e.g. black) for Python files. While I can't really say I like black's coding style, it's quite popular and using some formatter would be consistent with using clang-format for C++ code.

This revision is now accepted and ready to land.Nov 24 2021, 3:01 AM

qemu also does not support macOS

Do you mean qemu-user here?

I agree being able to run the tests there is nice.

DavidSpickett added inline comments.Nov 24 2021, 7:37 AM
lldb/packages/Python/lldbsuite/test/gdbclientutils.py
505

Is this something that happens every so often during testing and can be mostly ignored?

Just wondering if it's fatal to the test in which case, just remove the try/except and let this raise.

lldb/source/Plugins/Platform/Qemu/PlatformQemuProperties.td
11 ↗(On Diff #389443)

Do we handle relative paths for this? My assumption would be yes and if the binary is on PATH we'd also find it.

If not, worth noting the caveats.

lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.cpp
85 ↗(On Diff #389443)

Is this where the sort of priority behaviour we talked about happens? Meaning qemu-user not claiming a program file unless specifically asked for. (I think that's what force=true would mean)

Not particularly bothered if it isn't, just trying to understand what the arguments mean.

lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.h
46 ↗(On Diff #389443)

Is this just boilerplate for now? Seems like it should somehow delegate/copy what PlatformLinux does.
(but I get that you can't make a new platform class without some implementation)

lldb/test/API/qemu/TestQemuLaunch.py
33

You can make this a bit neater by doing:

e.write("\n".join([
  "#! %s\n"%sys.executable,
  "import runpy",
  "import sys",
  "sys.path = %r\n"%sys.path,
  "runpy.run_path(%r, run_name='__main__')\n" % self.getSourcePath("qemu.py")])

Or if you want to write one string:

from textwrap import dedent
<...>
e.write(dedent("""\
<...>""".format(<args>))

Dedent will remove the common indentation from the lines. But you end up with all the formatting args at the end so it's not as straightforward to read.

Regardless of all that, adding spaces around the % would help the readability:

"sys.path = %r\n" % sys.path
45

Comment here that this is launching the fake qemu not a program file?

lldb/test/API/qemu/main.c
2

Comment here that this is never actually run? (I was confused why this would return 0 but you check for 0x47 above)

I assume something in the test framework expects there to be a sourcefile even if we don't need one.

lldb/test/API/qemu/qemu.py
23

Double space here.

some things in this patch are called "Qemu" (the plugin folder, for instance)

I wouldn't pay much attention to the qemu-system idea, it was just a hypothetical from me. Unless it's something that you would call a public API. Like the platform's name for example, but you've made that specific so that's fine.

Anything else can be shuffled around if qemu-system ever happens.

labath marked 4 inline comments as done.Nov 25 2021, 3:30 AM

qemu also does not support macOS

Do you mean qemu-user here?

Yes, definitely.

some things in this patch are called "Qemu" (the plugin folder, for instance)

I wouldn't pay much attention to the qemu-system idea, it was just a hypothetical from me. Unless it's something that you would call a public API. Like the platform's name for example, but you've made that specific so that's fine.

Anything else can be shuffled around if qemu-system ever happens.

OK, I've renamed everything into qemu-user. The platform name is pretty much the only user-visible name here, but I would say that is set in stone either.

lldb/packages/Python/lldbsuite/test/gdbclientutils.py
505

The trick here is that in the "normal" (gdb-client tests), this code gets executed on a separate thread. Raising an exception will not terminate the test. Or at least, not in a clean way, but instead if will confuse the hell out of everything. So instead, this just prints the exception (as does the try block below) and lets the main thread abort the test when it fails to connect.

lldb/source/Plugins/Platform/Qemu/PlatformQemuProperties.td
11 ↗(On Diff #389443)

Relative paths work, but I'm not sure how much I want to advertise them. Searching the PATH does not currently work, but I like the idea. I can do it in a follow-up patch, where I wanted to automatically try to guess the emulator binary based on the selected architecture.

lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.cpp
85 ↗(On Diff #389443)

Pretty much yes. force=true gets passed when the user explicitly requests a platform (platform select, target create --platform, etc.). But there is still the negotiation that happens through the GetSupportedArchitectures function (so a bare "target create" would only select qemu-user if it is the currently selected platform and it matches the architecture of the file). I'm leaving the fancier selection logic to later patches.

lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.h
46 ↗(On Diff #389443)

Yeah, the idea is that this (and other functions) will eventually delegate to the host platform implementation.

lldb/test/API/qemu/main.c
2

I don't really need a source file, but I do need a realistic looking-executable so I can "run" it. A real qemu, will actually emulate the file, and respond accordingly, but that's not interesting for the tests here, as that is just standard gdb-remote stuff. The main thing that needs to be checked here is that qemu is being invoked with the right arguments.

I contemplated just yaml2obj-ing a small binary here, but the thing which made that difficult was that this should be a valid binary for the host OS (a linux binary on linux, a freebsd one on freebsd, etc). The easiest way to produce that is to use the compiler. The downside to that is that it makes it hard to produce foreign-architecture binaries, and that's a bridge I'll have to cross when I get there. I'm hoping I won't need many tests like that.

labath updated this revision to Diff 389717.Nov 25 2021, 3:31 AM
labath marked an inline comment as done.

feedback David

That said, I wonder if we should use some formatter (e.g. black) for Python files. While I can't really say I like black's coding style, it's quite popular and using some formatter would be consistent with using clang-format for C++ code.

lldb python files were formatted with autopep8 during the Great Reformat. I don't know what the output of black looks like, but autopep8 is not that good.

Overall I would be supportive of using (any) automated python formatter. I think the main thing missing is for some one to set up the integration.

Thanks for the review. I'm going to wait until the US folks get back before submitting this.

This revision was automatically updated to reflect the committed changes.