Looks like a good change.

Looks good.

Sorry for the delay, looks good to me.

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.

Yep, looks good.

Taking a different approach to fixing this.

In D72813#1825006, @clayborg wrote:

ok if this works for now that is fine. Just close the socket_fd when we fail to write the pid to the socket and this is good to go.

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.

Thanks for the ping, yes this looks good to me.

In D63540#1766693, @omjavaid wrote:

Hi Jan,

I have done further investigation on this with new ampere arm server. This problem manifests itself on all kind of Debian flavors and is not related to underlying libraries. I have tested LLDB on Arm 32bit host with Ubuntu Xenial, Bionic and Debian Buster all show same problem.

Moreover this problem seems to be emerging from clang expression parser generating symbols while evaluating certain types of expressions. These symbols are rejected by expression interpreter. LLDB symtab classes are utilized for expressions as well so it is a highly likely problem so I would suggest you to investigate further on these lines

For example:
./bin/lldb-dotest -p TestVirtual.py -v -t

test_virtual_madness_dwarf (TestVirtual.CppVirtualMadness) fails with following error

runCmd: expression a_as_A->a()
runCmd failed!
error: Can't run the expression locally: Interpreter doesn't handle one of the expression's opcodes

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.

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.

Thanks for looking this over Pavel.

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.

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.

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.

In D62931#1724433, @guiandrade wrote:

Yeah, more tests would be useful. They made me notice an issue btw. I was simply sending a 'g' packet and checking if the server replied back nicely; however, IIUC with 'G' packets it isn't so simple because we also need to send the registers data in the same packet. I'm not sure how I could do that, and I'm also afraid that check could get too expensive. Do you have any idea what could be a nice way to handle that?

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. ;)

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.

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.

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.

Awesome.

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.

In D67892#1680317, @labath wrote:

In either case, it's sad that there are no tests breaking due to this. Ideally we'd have a small arm64 macho core file with well known register values that we could just assert are read correctly. You probably don't have the means to create such a core file, so it's not fair to ask you to do that, but it would be great if one of the apple folks added a test like that.

LGTM.

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.

LGTM, nice job with the test case, I see myself copying that in the future. :)

We don't need to keep this command around. It was originated by Tamas in 2015,
https://reviews.llvm.org/D10868
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.

LGTM.

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.

In D66250#1633455, @clayborg wrote:

Why are we not just using ObjectFileJIT? I am guessing these breakpoint expressions create one of these by compiling the breakpoint expression and JIT'ing it just like any other expression. If this is the case, then why do we need to create a ObjectFileTrampoline? Seems like we could add .cfi directives to the assembly we use for the breakpoint condition function so that we can unwind out of it?

In D66250#1631200, @labath wrote:

I have one high-level question. Who is generating the code inside the "trampoline" object file? Couldn't we have him generate proper unwind information so that the regular unwind machinery "just works"? I imagine all it would take is a couple of well-placed .cfi assembler directives...

Overall this changeset looks good to me. A few small comments.

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.

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.

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.

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.

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.

In D65109#1597492, @sgraenitz wrote:

Seriously, discussion for changes like this should be open for more than 1h15min! I am in favor of the change in principle, but there's a number of things that have been rushed over here,

LGTM. scripts/sort-pbxproj.rb can go too.

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.

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.

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.