- User Since
- Sep 2 2014, 12:05 PM (282 w, 4 h)
Fri, Jan 24
Looks like a good change.
Sorry for the delay, looks good to me.
Wed, Jan 22
Tue, Jan 21
Ah, sorry yes, the initial debugserver implementation was written back when macOS ppc was still a thing (although on its way out) - but the time the switch happened to lldb, we'd dropped support entirely. I doubt any of this code was ever actually used.
Fri, Jan 17
Yep, looks good.
Taking a different approach to fixing this.
Thu, Jan 16
Wed, Jan 15
Thanks for the review Greg. To really do this reliably, we probably either need to use a different kernel API, or push this down into debugserver where we can get the process' task port and inspect what its status is. This patch is more a preliminary one to get it working much of the time -- getting it to always work is going to require a slightly different approach.
Thu, Jan 9
Wed, Jan 8
Tue, Jan 7
Mon, Jan 6
Dec 18 2019
Dec 11 2019
Dec 10 2019
Dec 9 2019
Dec 4 2019
Thanks for the ping, yes this looks good to me.
Dec 3 2019
Dec 2 2019
I don't want to add noise to this, but there's a related change that I'd like to upstream soon -- ARMv8.3 pointer authentication bits in addresses. With the ARMv8.3 ISA, higher bits that aren't used for addressing can be used to sign & authenticate pointers that are saved in memory, where they may be overwritten via buffer over-runs et al, to verify that they have not been modified before they are used. I hope to put together a patchset for upstreaming the changes I made to lldb to support this soon.
Good change, lgtm.
Nov 22 2019
Nov 21 2019
Hi, sorry I haven't had time to get back to this one. I wanted to look into the m_avoid_g_packets ivar in GDBRemoteCommunicationClient and GDBRemoteCommunicationClient::AvoidGPackets. It looks like it was written to specifically avoid the g packet with arm64 devices, and I think it's trying to avoid the g packet for older debugservers.
Nov 20 2019
Thanks for looking this over Pavel.
Nov 19 2019
Updated GDBRemoteRegisterContext::ReadRegisterBytes to only mark registers as valid if the full contents for the register are included in the g packet. Updated gdbclientutils.py to strip off the thread specifier from the G packet, if it's present. Change the test file to check that the G packet either includes the full register context or just the general purpose registers.
Nov 18 2019
It might be worth adding an extra conditional to check for a buffer of 0 bytes returned. I haven't checked if ReadAllRegisters() would return success if a 0-length register context was received.
Nov 8 2019
Nov 7 2019
Nov 5 2019
I'll be honest I'm having trouble thinking of all the possible problems these regexes might have just by looking at them, but overall this looks good to me.
Oct 31 2019
Oct 28 2019
Oct 16 2019
committed in r375032 where I accidentally copied in the wrong phabracator URL, so lol.
Thanks for looking it over, Pavel. It touches lots of different files, but the changes are so tiny in most cases, I didn't know who to set as a reviewer. ;)
Oct 15 2019
Oct 10 2019
Thanks for all of the work on this patch Aleksandr, I really appreciate the work and this will be a nice addition to lldb. Apologies again for not looking over the revised patch earlier -- this looks really good to me, please do commit it when you have a chance.
Oct 9 2019
Hi Jaroslav, I apologize for taking so long to look at this; I've been heads-down on a project the past few weeks an my email inbox is a disaster right now.
Oct 8 2019
Nice fix. The real cost of computing the aranges manually is expanding all the line tables and calling GetContiguousFileAddressRanges on them. I wonder if avoiding the full linetable expansion here is shifting that to a different place for this operation.
Oct 4 2019
Sep 27 2019
Hm, if the goal is to get -version (an unrecognized cmd line option) to explicitly print to stderr, don't we want to change the lines in show_usage_and_exit? I think this change would result in us not getting the version string in console logs for normal debug sessions, where we want to keep it.
Sep 24 2019
Sep 12 2019
Hi Aleksandr, nice job getting this working. I read over the patch and had a couple of initial questions.
Sorry for taking so long to look at this one -- it looks like a good solution to this.
Sep 10 2019
Sep 9 2019
LGTM, nice job with the test case, I see myself copying that in the future. :)
Sep 6 2019
Sep 5 2019
We don't need to keep this command around. It was originated by Tamas in 2015,
but never got used for anything - Tamas was working on unwind things back then iirc, and may it looks like he thought it would be useful for that. On Darwin systems we do ship with a python command for diagnosing unwind problems ("script import lldb.diagnose"; "diagnose-unwind") which can be used when the current backtrace looks like it may be missing stack frames, or truncate early. The unwinder hasn't seen a lot of active development for years, so we don't have cause to use this very often.
Sep 3 2019
Aug 29 2019
LGTM. In debugserver we have the definition for the V registers invaliding the D and S registers it contains. If the user modifies v10, we want any cached s10 and d10 values to be marked as invalid / refresh them. The same thing with the X and W registers. But the definitions you're rewriting already had this as incorrect, you've just replicated it more compactly, so I wouldn't call that a reason to hold off. Let's give Pavel a chance to look at this too.
Aug 27 2019
Aug 26 2019
Aug 22 2019
Aug 21 2019
Aug 20 2019
Aug 15 2019
Aug 14 2019
Overall this changeset looks good to me. A few small comments.
Aug 6 2019
Aug 5 2019
Yeah, it's not currently hooked up to anything; I'm returning to remote-platform testing soon, if we've lost some necessary functionality I can re-add it, but this option right now isn't doing anything.
Aug 1 2019
This looks good, this is in line with what we discussed, thanks for taking it on! Sorry for the delay at looking this over, it has been a little crazy this week.
fwiw this behavior is documented here,
I'm sure this is fine, I read the code through and I've realized I haven't had to do anything with bitfields in lldb up to now.
Jul 29 2019
I'm not sure it's really relevant when lit is the driver, but I would often use --no-multiprocess with dotest to see the full trace output of a single test while it was running. In multiprocess mode that wasn't displayed iirc. That's my only use of it. I do use --threads 1 when doing a testsuite run against a device because there's some buggy behavior in the lldb testsuite when running multiple tests simultaneously on a remote device (at least a remote darwin device). But I'm sure you're not talking about removing that control.
Jul 24 2019
LGTM. I don't think the use of the posix_spawn API are important for the test case to exercise. If posix_spawn had a problem on darwin systems, lldb/debugserver's process launching would turn it up pretty quick.
Jul 23 2019
LGTM. There was another problem I was discussing with someone the other week (I can't remember exactly the details) where we'd need to retrieve the Xcode install path so this would have been needed there too.
Jul 22 2019
LGTM. scripts/sort-pbxproj.rb can go too.
Jul 18 2019
Jul 17 2019
Hm, that could be really helpful! I think I'd like to do that as a separate piece of work a little bit later, but I'll look at that.
I know there are a lot of specific notes floating around right now, but two quick suggestions.
Jul 16 2019
Sorry yes, I should have marked this as approved. I don't have a linux machine handy so I haven't looked into the problem Jan reports with the change -- fwiw if you turn on unwind logging ('log enable lldb unwind' before you backtrace at a stop), it will tell you which unwind rule it is using to find the caller's return pc value as it goes up the stack. It's a little tricky to read at first, but once you get the hang of it you can usually spot the problem quickly and understand why lldb is behaving the way it is. But just looking at the source changes you're making, I'm fine with this.
Jul 15 2019
Ah, regarding the S bit, looks good to me. I think I misunderstood the original problem you were working on. On Darwin systems, we have our friend sigtramp. When an async interrupt is received, the kernel saves the register context to stack and then calls sigtramp which does some instructions and then calls the trap handler function. Backing up the pc by 1 for __sigtramp is fine, because it did a normal CALL instruction to the trap handler.