JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
A few minor nits but the change looks sound to me. It's great to see the
existing code getting unified and nice job on that test. LGTM.
================
Comment at: lldb/source/Core/DynamicLoader.cpp:192-194
+ char namebuf[80];
+ snprintf(namebuf, sizeof(namebuf), "memory-image-0x%" PRIx64, value);
+ memory_module_sp = process->ReadModuleFromMemory(FileSpec(namebuf), value);
----------------
If it wasn't for the fixed-length buffer I probably would've ignored it, but
you have the same code on line 222. Might be worth extracting this into a
little static helper function/lambda. Something like:
```
BufferSP ReadModuleFromMemory(ProcessSP process, lldb::addr_t addr) {
char namebuf[80];
snprintf(namebuf, sizeof(namebuf), "memory-image-0x%" PRIx64, addr);
return process->ReadModuleFromMemory(FileSpec(namebuf), addr);
}
```
================
Comment at: lldb/source/Core/DynamicLoader.cpp:238-239
+ if (value != LLDB_INVALID_ADDRESS) {
+ if (log)
+ log->Printf("Loading binary UUID %s at %s 0x%" PRIx64,
+ uuid.GetAsString().c_str(),
----------------
Use `LLDB_LOGF` which includes the `if(log)` check. There's some more instances
of that below.
================
Comment at:
lldb/test/API/macosx/lc-note/multiple-binary-corefile/create-multibin-corefile.cpp:1
+#include <errno.h>
+#include <fcntl.h>
----------------
Are there other tests that can (or will) benefit from this utility? If so I
think it would make sense to make it a proper tool that we build with CMake
(similar to lldb-test and lldb-instr). That's more of a suggestion for the
future, as is I think it's totally fine for this to be part of the test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130813/new/
https://reviews.llvm.org/D130813
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
