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

jankratochvil added a comment. Even after @labath's rL363772 the testcase `TestGdbRemoteLibrariesSvr4Support` is still FAILing on Fedora 30 x86_64, I will check it more later. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502

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

labath added a comment. As I kind of expected, one of these tests was flaky because the names for the vdso pseudo-module were not adding up. When reading from /proc/%pid/maps, we got "[vdso]" (as that's how the kernel likes to call this page), but while reading the linker rendezvous structure,

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL363707: Implement xfer:libraries-svr4:read packet (authored by aadsm, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm

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

aadsm updated this revision to Diff 204850. aadsm added a comment. Only extend support to NetBSD Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lldb/Host/common/NativeProcessProtocol.

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

krytarowski added a comment. I would leave the NetBSD version as it is in this patch and let us to fix/test it later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 _

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

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. LGTM, with the following comment taken into account. The ProcessFreeBSD parts are definitely wrong, as ProcessFreeBSD does not use lldb-server yet (it uses local only path). Just revert that p

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

aadsm updated this revision to Diff 204423. aadsm added a comment. Herald added a subscriber: emaste. - Move SVR4 reading to NativeProcessELF - Made NetBSD and FreeBSD process implementations extend NativeProcessELF I'm not 100% confident on this last one because I don't have a net and free bsd

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

aadsm added a comment. Yeah, that makes sense. I was thinking about this yesterday after checking the freebsd code. I was concerned if there's something different that I'm not aware of but we do have tests and I guess we can also override these functions if needed in the future. Repository:

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

labath added a comment. Ok, so given that both NetBSD and FreeBSD implement this feature (and the current DynamicLoaderPOSIXDYLD plugin reads it), I think we should move this bit of code into NativeProcessELF -- after all, that's what we've created it for. Repository: rG LLVM Github Monorep

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

aadsm updated this revision to Diff 203947. aadsm added a comment. - Update test to just wait for the process to stop - Change ReadSVR4LibraryInfo signature to return an Expected to the list of libraries and stop taking params by ref. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

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

aadsm marked an inline comment as done. aadsm added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2181 +template +Status NativeProcessLinux::ReadSVR4LibraryInfo(lldb::addr_t link_map_addr, + SV

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

aadsm marked an inline comment as done. aadsm added a comment. It's the same for freebsd https://github.com/freebsd/freebsd/blob/master/sys/kern/link_elf.c#L291 although behind a GDB flag (which NetBSD doesn't seem to be: https://nxr.netbsd.org/xref/src/libexec/ld.elf_so/rtld.c#1040). ==

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

krytarowski added a comment. https://nxr.netbsd.org/xref/src/include/link_elf.h#9 In general this code should be close to functional on NetBSD (if not already compatible). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.l

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

labath added a subscriber: mgorny. labath added a comment. This looks mostly good. The thing I'm wondering about is whether this part of the code is really linux specific (in which case we should rename ELFLinkMap into LinuxLinkMap), or if it can be useful on other platforms (in which case this

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

aadsm added a comment. Sounds good, I put it there initially because the XML.cpp set of classes abstract the libxml2 specific away but I guess it is odd to have XMLDocument::XMLEnabled() returning false and still be able to call that new function. Repository: rG LLVM Github Monorepo CHANGE

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

aadsm updated this revision to Diff 203656. aadsm added a comment. Move XMLEncodeAttributeValue to GDBRemoteCommunicationServerLLGS. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lld

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

labath added a comment. In D62502#1533490 , @aadsm wrote: > @labath that's a really good point. I've talked with @clayborg and @xiaobai > about this and we decided it would be fine to do it manually as well so I > added a `XMLEncodeAttributeValue` funct

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

aadsm added a comment. @labath that's a really good point. I've talked with @clayborg and @xiaobai about this and we decided it would be fine to do it manually as well so I added a `XMLEncodeAttributeValue` function to make it happen. I'm not 100% sure about its implementation.. Should I use a

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

aadsm updated this revision to Diff 203477. aadsm added a comment. No longer depend on libxml2 to quote the attribute value Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lldb/Host/XM

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

labath added a comment. Using the libxml function for the escaping is fine, but there are a couple of caveats. I believe people expect to be able to build lldb without libxml. At least that what all the existing xml-related code assumes (you'll see it has some #ifdefs and stuff). If we don't in

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

aadsm updated this revision to Diff 203083. aadsm added a comment. - Improved the tests by using a binary that is specificly linked to other shared libs. - USe libxml2 to generate the name attribute (I've talked with Greg about this and he recommended to create a new class for this) - Address ot

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

aadsm marked an inline comment as done. aadsm added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2778 +for (auto const &library : library_list) { + response.Printf(" labath wrote: > > You're completely ignoring

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

aadsm marked 3 inline comments as done. aadsm added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteLibrariesSvr4Support.py:51-52 + +OFFSET = 0 +LENGTH = 0x + labath wrote: > Why are these all c

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

labath added a comment. In D62502#1528382 , @aadsm wrote: > @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

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

aadsm updated this revision to Diff 202855. aadsm added a comment. Address comments except the one about a different test plan. Also removed the main property, after searching on the internet I'm not confident with the way I was using to detect the main executable. Since the "lm-main" is an opti

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

aadsm marked an inline comment as done. aadsm 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; aadsm wrote: > clayborg wrote: > > These see

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

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

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

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 s

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

labath added a comment. 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 invarian

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

aadsm updated this revision to Diff 202652. aadsm added a comment. Address comments and add tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lldb/Host/common/NativeProcessProtocol

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

aadsm updated this revision to Diff 202328. aadsm added a comment. Update with the correct commit... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lldb/Host/common/NativeProcessProto

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

aadsm updated this revision to Diff 202326. aadsm added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62502/new/ https://reviews.llvm.org/D62502 Files: lldb/include/lldb/Host/common/NativeProcessProtocol.h lldb/source/Plugins

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

aadsm created this revision. aadsm added reviewers: clayborg, xiaobai. Herald added subscribers: lldb-commits, krytarowski, srhines. Herald added a project: LLDB. aadsm added a parent revision: D62501: Implement GetSharedLibraryInfoAddress. aadsm added a child revision: D62503: Add ReadCStringFromM