[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

This revision was automatically updated to reflect the committed changes. Closed by commit rL363458: Implement GetSharedLibraryInfoAddress (authored by aadsm, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm updated this revision to Diff 204422. aadsm added a comment. Improve tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.org/D62501 Files: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp lldb/sour

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm added a comment. Initially one at a time but then thought it might be better to do it as a batch because I was afraid I was missing some dependency and would brake something unexpectedly. But I guess that since I've already landed D62168 it's probably fin

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath accepted this revision. labath added a comment. LGTM. BTW, I see you still haven't submitted the previous two changes in the stack. Are you planning to submit all of them together or what? It's usually better to avoid batch submitting a bunch of dependent changes, as that way, if any pr

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm updated this revision to Diff 203654. aadsm added a comment. Removed unwanted change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.org/D62501 Files: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm updated this revision to Diff 203653. aadsm marked 2 inline comments as done. aadsm added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.org/D62501 Files: lldb/source/Plugins/Proce

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added a comment. I think this is pretty good now. The only concern I have is about the placement NativeProcessTestUtils. The way you're including it now looks weird. I think we should put that in a separate place, as per the inline coment. In D62501#1533483

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm updated this revision to Diff 203476. aadsm added a comment. Herald added a subscriber: emaste. This creates a NativeProcessELF to deal with ELF specific things. The NativeProcessLinux now extends this one. Added a test case for the GetAuxValue and GetELFImageInfoAddress. I refactored Nati

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added a comment. Ok, makes sense, I'm happy that you've considered the dynamic loader sharing tradeoffs already. Let's go with the NativeProcessELF thingy then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.o

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm added a comment. Interesting, I actually don't think there's much to gain from turning this into an utility. Creating a NativeProcessELF makes sense to me (I already did it and have all the tests up and running as well) and putting it into POSIX also makes sense to me based on what you sa

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added a comment. In D62501#1531959 , @aadsm wrote: > @labath while working on this I had to also move the GetAuxValue function > into the NativeProcessELF, which I think it's fine. However, this means this > class now depends on the AuxValue class

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm added a comment. @labath while working on this I had to also move the GetAuxValue function into the NativeProcessELF, which I think it's fine. However, this means this class now depends on the AuxValue class defined in the PluginUtility. Initially I was going to put the NativeProcessELF i

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm added a comment. > I haven't given this much thought, but it may be possible to reuse the stuff > in MockProcess by making it a template (so you'd have a > MockProcess, and a MockProcess) Ah interesting, will explore that instead. I was actually thinking if it would be possible to extrac

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added a comment. In D62501#1531202 , @aadsm wrote: > > Another advantage of having this in an abstract class is that you could > > test this in isolation, as NativeProcessProtocol is already setup to mock > > memory accesses: > > https://github.c

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm added a comment. > Another advantage of having this in an abstract class is that you could test > this in isolation, as NativeProcessProtocol is already setup to mock memory > accesses: > https://github.com/llvm-mirror/lldb/blob/master/unittests/Host/NativeProcessProtocolTest.cpp. I migh

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2122 + size_t phdr_num_entries = *maybe_phdr_num_entries; + lldb::addr_t load_base = phdr_addr - sizeof(ELF_EHDR); + aadsm wrote: > labath wrote: > > This innocen

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm marked an inline comment as done. aadsm added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2122 + size_t phdr_num_entries = *maybe_phdr_num_entries; + lldb::addr_t load_base = phdr_addr - sizeof(ELF_EHDR); + labath

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm marked 2 inline comments as done. aadsm added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2136 +sizeof(phdr_entry), bytes_read); +if (!error.Success()) + return LLDB_INVALID_ADDRESS;

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added subscribers: mgorny, krytarowski. labath added a comment. +@mgorny, @krytarowski in case they know anything interesting about dynamic linkers. NativeProcessProtocol is certainly too generic for this sort of thing, but perhaps it would make sense to put this in a slightly more gener

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm updated this revision to Diff 202854. aadsm added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://reviews.llvm.org/D62501 Files: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp lldb/s

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm marked an inline comment as done. aadsm added inline comments. Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:430 + lldb::addr_t GetELFImageInfoAddress(); + clayborg wrote: > This doesn't seem like it belongs in the base class. Maybe j

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:430 + lldb::addr_t GetELFImageInfoAddress(); + This doesn't seem like

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

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

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

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

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

labath added a comment. Looks relatively straight-forward, but I'll wait with the full review of this and subsequent patches until we sort out the first two patches in this series. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62501/new/ https://r

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

aadsm created this revision. aadsm added reviewers: clayborg, xiaobai. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. aadsm added a parent revision: D62500: Add support to read aux vector values. This is the third patch to improve module loading in a series that started her