[Lldb-commits] [PATCH] D128956: make debugserver able to inspect mach-o binaries present in memory, but not yet registered with dyld

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/test/API/macosx/unregistered-macho/TestUnregisteredMacho.py:46
+        self.expect (both_gdb_packet, substrs=['response: {"images":[{"load_address":%d,' % macho_addr])
+        self.expect (both_gdb_packet, substrs=['response: {"load_address":%d,' % invalid_macho_addr], matching=False)
+
----------------
DavidSpickett wrote:
> For these would it be more reliable to just look for the `load_address:` bits?
> 
> If the packet contains multiple `response`, then fine but if it's one response then the second one wouldn't ever match even if debugserver did somehow find the one at the invalid address. Also if it did would it put it in a `images` section and therefore not match because of that too?
Yeah, good suggestion.  I wrote the two tests in the order they're in, and realized I couldn't match the `images` key in the second one; didn't go back and make the first one consistent.


================
Comment at: lldb/test/API/macosx/unregistered-macho/main.c:48
+  memcpy(p, &uuid, sizeof(uuid));
+  p += sizeof(uuid);
+
----------------
DavidSpickett wrote:
> This is redundant (the test uses `macho_buf`).
I needed to increment `p` so it points to the end of the mach-o image if I want to write it to disk and try sending it through `otool` for testing.  You're right though, this has no effect to the program itself.


================
Comment at: lldb/test/API/macosx/unregistered-macho/main.c:33
+  seg.vmsize = 0x1000;
+  seg.fileoff = 0;
+  seg.filesize = 0;
----------------
DavidSpickett wrote:
> Does this need to be `sizeof (struct mach_header_64)`? Probably doesn't and/or it doesn't matter for this test.
I don't think it matters.  I'm saying that this segment doesn't exist in the file.  Quick check, I created a hello world program with a single BSS int, so it doesn't exist in the binary but is initialized to zero at runtime. 
```
Load command 2
      cmd LC_SEGMENT_64
  cmdsize 152
  segname __DATA
   vmaddr 0x0000000100004000
   vmsize 0x0000000000004000
  fileoff 0
 filesize 0
```


================
Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:768
+    return true;
+  };
+
----------------
DavidSpickett wrote:
> Personally I'd put this as a static function just before this one as it doesn't capture anything, but up to you.
Yeah good point.  It's operating on either a mach_header or mach_header_64 so there wasn't a single variable to capture and test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128956