[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[António Afonso via Phabricator via lldb-commits]

aadsm marked 4 inline comments as done.
aadsm added a comment.

@labath

> Regarding the test, I am wondering whether requiring the /proc/%d/maps and the linker rendezvous structure to match isn't too strict of a requirement. Dynamic linkers (and particularly android dynamic linkers) do a lot of crazy stuff, and trying to assert this invariant exposes us to all kinds of "optimizations" that the linkers do or may do in the future. I'm wondering if it wouldn't be better to just build an executable with one or two dependent libraries, and then just assert that their entries are present in the library list. However, I'm not completely sure on this, so I'd like to hear what you think about that..

Yeah, that was actually my first idea for the test but then realized we already had that main.c all set up and ready to use so I decide to use proc maps instead. I was not aware that things like that could happen (although I had a suspicion this might be brittle) so let me revisit this.



================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:37-39
+  lldb::addr_t link_map;
+  lldb::addr_t base_addr;
+  lldb::addr_t ld_addr;
----------------
clayborg wrote:
> These seem very linux specific. Why do we need 3 addresses here? Seems like we should just need one. Or is this the structure of the "xfer:libraries-svr4:read" that is required to be returned? If so, maybe we rename "SharedLibraryInfo" to "SVR4ModuleInfo"?
Yes, that is a good point and yes this is the data the SVR4 returns. Given this structure it makes more sense to move this into the NativeProcessLinux instead and name it more specifically.


================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:40
+  lldb::addr_t ld_addr;
+  bool main;
+  lldb::addr_t next;
----------------
clayborg wrote:
> Why is "main" important? Hopefully the dynamic linker can figure out what is what without needing to know this? Seems verify linux specific. Or is this "xfer:libraries-svr4:read" specific?
It is packet specific yes. And I just realized I'm not putting that info in the XML I'm returning, will update.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2191
+  char name_buffer[PATH_MAX];
+  error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
+                     bytes_read);
----------------
clayborg wrote:
> Use ReadCStringFromMemory?
ReadCStringFromMemory doesn't exist here yet. That's on the next diff of this stack of diffs.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2247
+    library_list.push_back(info);
+    link_map = info.next;
+  }
----------------
clayborg wrote:
> Seems this we are using "info.next" just because it is a linked list on linux. Can we remove "next" and just add an extra "next" reference parameter to ReadSharedLibraryInfo<T>(link_map, info, next);?
> 
Yes, this is the only reason, sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502