[Lldb-commits] [PATCH] D130813: Add ability for lldb to load binaries in a process/corefile given only a load address & no dynamic loader

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