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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
