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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
