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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


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


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


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


================
Comment at: lldb/source/Plugins/Platform/Qemu/PlatformQemuUser.h:46
+
+  void CalculateTrapHandlerSymbolNames() override {}
+
----------------
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)


================
Comment at: lldb/test/API/qemu/TestQemuLaunch.py:32
+            e.write("runpy.run_path(%r, run_name='__main__')\n"%
+                    self.getSourcePath("qemu.py"))
+        self.runCmd(
----------------
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
```


================
Comment at: lldb/test/API/qemu/TestQemuLaunch.py:44
+        self.assertEqual(target.GetPlatform().GetName(), "qemu-user")
+        process = target.LaunchSimple(
+                [self.getBuildArtifact("state.log"), "arg2", "arg3"], None, None)
----------------
Comment here that this is launching the fake qemu not a program file?


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


================
Comment at: lldb/test/API/qemu/qemu.py:22
+            is not actually run. Instead a very basic mock process is presented
+            to lldb.  The emulated program must accept at least one argument.
+            This should be a path where the emulator will dump its state. This
----------------
Double space here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114509