- User Since
- Apr 27 2019, 10:34 PM (11 w, 2 d)
Wed, Jul 10
@labath can I ask for a re-review on this one please? :)
@jankratochvil I'm planning to follow Pavel's plan now that the new setting has landed and finally got some time for this.
Wed, Jul 3
Tue, Jul 2
Mon, Jul 1
Address comments, add GetAction that returns an enum with what to do.
Replaced by D62503
Yap, that makes a lot of sense to me. I looked into this over the weekend and figured out the reason why it was broken (tl;dr: we can't use LoadModules) and created a tidier version of D63868 that, like D62504 avoids fetching the svr4 packet twice.
Thu, Jun 27
I looked into this a bit today and at one point in time (when the list of modules is being processed) it seems that some previously loaded modules are somehow removed (at least I stop seeing them in the image list) including the one where the rendezvous breakpoint is so that breakpoint becomes "unresolved" and that's why it doesn't stop anymore. I'll need to check why this is the case tomorrow.
The library is part of the GDB protocol itself: https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets and it's to be used with conjunction of qXfer:libraries:read (or libraries-svr4 like we do here).
@jankratochvil I imagine the first test to be failing because this patch doesn't handle correctly the unload (or rather as soon as it unloads one shared object it stops working as expected).
I'm going to look at this today (right now it's 9am for me).
I'm not sure why DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit happens only once, I thought I had fixed that here: D62168.
I don't think we can stop loading/unloading the libraries in ProcessGDBRemote::LoadModules. The windows dynamic loader relies on this and the library entry on the stop packet does as well: https://github.com/llvm-mirror/lldb/blob/9689521b477c75b52257fba9655cabefc3db676c/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2374-L2375
Tue, Jun 25
I can't repro it right now so I prefer to revert it (I'm curious how this new function could influence those tests though). I actually thought this was sorted, sorry about that.
Wed, Jun 19
dang, I missed filtering that test as well, thank you.
Tue, Jun 18
Mon, Jun 17
- Add a test to cover cross page reads
- Make cache_line_size a static const variable inside the function.
- Fix how we're passing name_buffer to ReadCStringFromMemory.
Sun, Jun 16
- Address missing comments
- Make ReadCStringFromMemory return llvm::Expected<llvm::StringRef>>
- Add documentation
- Address other comments
- Created static const size_t g_cache_line_size to avoid calling llvm::sys::Process::getPageSizeEstimate() all the time, that thing is expensive.
Jun 14 2019
Only extend support to NetBSD
Jun 13 2019
I see what you mean, this is a good point. Yes, like you say I'm assuming the string I want to read is in a page that I can read using the fast method. And in this situation I prefer to minimize the number of calls to ReadMemory than the amount of data I'll read (given that I'll be reading from within the same page that is already cached, and also assuming the destination is also cached (probably true if in the stack)). I also wanted to side step the question of what is a good string average size because I don't have good data to back it up.
So yeah, optimizing for the case I know it exists, and we can tweak this better once we have other cases to analyse.
Jun 12 2019
- Move SVR4 reading to NativeProcessELF
- Made NetBSD and FreeBSD process implementations extend NativeProcessELF
Yeah, that's a good question and I did think about it at the time I wrote D62715. Here's my rationale:
Regarding the chunk reading in the ReadMemory I was not sure because I don't know how common that is and it is faster to read the entire size without chunking (the code is also less complex this way). I'd imagine that in the common case we usually know what we're reading? (e.g.: reading the entire function assembly code)
That's why I thought about the string "extraction" as a special case for memory reading. However, even if we had a ReadMemory that did chunking I'd still want to do chunking in the ReadCStringFromMemory. Otherwise we'd be reading much more than needed for a string because there is no way for the ReadMemory to know it could stop (after finding a '\0') so it might end up reading say 2KB word by word with ptrace. (I did think for 5s to pass an optional stop character to ReadMemory but that would be shoehorning an edge case into a generic and simple function, it would be better to put the chunk reading logic into a separate thing that both ReadMemory and ReadCStringFromMemory could reuse).
Jun 11 2019
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.
Jun 10 2019
- 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.
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 fine to land one at a time.
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).
Jun 9 2019
Do not try to use LoadModules again if they failed in the past
Jun 7 2019
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.
Fix a little bug and add tests
Move XMLEncodeAttributeValue to GDBRemoteCommunicationServerLLGS.
Removed unwanted change
Jun 6 2019
@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 StreamString instead?
No longer depend on libxml2 to quote the attribute value
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 NativeProcessTestUtils to reuse that code.
I also addressed other comments on the review.
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 said.
Jun 5 2019
@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 in the lldbHost but it feels weird that this will now depend on a plugin, even if it's the utility one. Should I create a new ELF plugin just for this?
Address final 2 comments
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<NativeProcessProtocol>, and a MockProcess<NativeProcessELF>)
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.
Jun 4 2019
Make sure we always return a null terminated string
- 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 other comments
Jun 3 2019
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 optional attribute I'm not going to implement it now.
However, I'm excluding from the list the libraries without name or that are at base addr == 0. They shouldn't be there by the spec.
Move test to a better place, address other comments related to tests.
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..
Jun 2 2019
Address comments and add tests
Fix patch snafu
Address comments and add tests