[Lldb-commits] [PATCH] D114509: [lldb] Introduce PlatformQemuUser

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added a comment.

In D114509#3151461 <https://reviews.llvm.org/D114509#3151461>, @DavidSpickett wrote:

>> qemu also does not support macOS
>
> Do you mean qemu-user here?

Yes, definitely.

In D114509#3151581 <https://reviews.llvm.org/D114509#3151581>, @DavidSpickett wrote:

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



================
Comment at: lldb/packages/Python/lldbsuite/test/gdbclientutils.py:505
         except:
+            traceback.print_exc()
             return
----------------
DavidSpickett wrote:
> 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.
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.


================
Comment at: lldb/source/Plugins/Platform/Qemu/PlatformQemuProperties.td:11
+    DefaultStringValue<"">,
+    Desc<"Path to the emulator binary.">;
+}
----------------
DavidSpickett wrote:
> 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.
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.


================
Comment at: lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.cpp:85
+  return nullptr;
+}
+
----------------
DavidSpickett wrote:
> 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.
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.


================
Comment at: lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.h:46
+
+  void CalculateTrapHandlerSymbolNames() override {}
+
----------------
DavidSpickett wrote:
> 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)
Yeah, the idea is that this (and other functions) will eventually delegate to the host platform implementation.


================
Comment at: lldb/test/API/qemu/main.c:1
+int main() {}
----------------
DavidSpickett wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114509/new/

https://reviews.llvm.org/D114509