[Lldb-commits] [PATCH] D116094: Add support for a version 2 of "main bin spec" Mach-O LC_NOTE in corefiles, handle specifications with a slide value

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

A few small nits but the change itself looks sound to me. LGTM.



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:7052
+      if (image.load_address != LLDB_INVALID_ADDRESS)
+        sprintf(namebuf, "mem-image-0x%" PRIx64, image.load_address);
+      else
----------------
No need to change this, but if you ever have situation where the buffer is variable length, LLVM's `formatv` makes this very easy:

```
std::string s = llvm::formatv("mem-image+{0:x}", image.load_address)
```


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:224
+        if (value != LLDB_INVALID_ADDRESS) {
+          bool changed = false;
+          module_sp->SetLoadAddress(target, value, value_is_offset, changed);
----------------
You can hoist this out of the if-check and reuse it in all the blocks instead of having to duplicate the variable each time.


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:230
+          bool changed = false;
+          module_sp->SetLoadAddress(target, 0, value_is_slide, changed);
+        }
----------------
When I was reading this code, I found this a little confusing, given that `value_is_offset` is an input parameter and this is meant as a constant. Not sure how to make this better other than possibly using `/*value_is_offset=*/ true`. Anyway small nit, don't feel like you need to change anything here.


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:41
     @skipIf(debug_info=no_match(["dsym"]), bugnumber="This test is looking explicitly for a dSYM")
-    @skipIf(archs=no_match(['x86_64']))
+    @skipIf(archs=no_match(['x86_64', 'arm64', 'arm64e', 'aarch64']))
+    @skipIfRemote
----------------
Nice


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:72-88
+        target = self.dbg.CreateTarget('')
         err = lldb.SBError()
         if self.TraceOn():
             self.runCmd("script print('loading corefile %s')" % self.verstr_corefile_addr)
-        self.process = self.target.LoadCore(self.verstr_corefile_addr)
+        self.process = target.LoadCore(self.verstr_corefile_addr)
         self.assertEqual(self.process.IsValid(), True)
         if self.TraceOn():
----------------
Given that you already build the corefiles only once, would it make sense to make these separate tests so that *if* they fail, you can immediately tell from the logs? Or maybe these tests are sharing something that I missed?


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:177
+        dsym_python_dir = '%s.dSYM/Contents/Resources/Python' % (aout_exe)
+        os.makedirs(dsym_python_dir)
+        python_os_plugin_path = os.path.join(self.getSourceDir(),
----------------
I believe you need to pass `exist_ok` to avoid this failing when the dir already exists.


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:184
+                ]
+        with open(dsym_python_dir + "/a_out.py", "w") as writer:
+            for l in python_init:
----------------
`os.path.join`


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:191
+        dwarfdump_cmd_output = subprocess.check_output(
+                ('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True).decode("utf-8")
+        aout_uuid = None
----------------
We shouldn't be hardcoding this, we already have `llvm-dwarfdump` as a test dependency. That said, unlike `dsymutil` there's no good way to get the `dwarfdump` binary. There's other tests doing the same, so we (I?) should address this in another patch.


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:200-227
+        shell_cmds = [
+                '#! /bin/sh',
+                '# the last argument is the uuid',
+                'while [ $# -gt 1 ]',
+                'do',
+                '  shift',
+                'done',
----------------
I feel like we have a handful of tests creating this dsymforuuid wrapper. At some point we should consider moving this in the `lldbutil` so we don't have to copy-paste this everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116094