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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
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;
----------------
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"?


================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:40
+  lldb::addr_t ld_addr;
+  bool main;
+  lldb::addr_t next;
----------------
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?


================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:41
+  bool main;
+  lldb::addr_t next;
+};
----------------
Why do we need this "next" member here? We get a list of these from:
```
virtual Status GetLoadedSharedLibraries(std::vector<SharedLibraryInfo> &library_list);
```
Seems like we shouldn't need this. Darwin based systems doesn't store the shared library list as a linked list. Hopefully not "xfer:libraries-svr4:read" specific?


================
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);
----------------
Use ReadCStringFromMemory?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2247
+    library_list.push_back(info);
+    link_map = info.next;
+  }
----------------
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);?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502