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.

In D63667#1558027, @JosephTremoulet wrote:

I've updated this with code to recognize the 'S' in the eh_frame augmentation and record it in a new bool member of UnwindPlan, m_plan_is_for_signal_trap. I haven't hooked up any consumers of the new bit; as you say, with the current code flow we don't parse the eh_frame info until after we've decided whether the current frame is a trap handler frame or not.

I took a look at behaves_like_zeroth_frame. In my case, we're not getting to the point of consulting it, because before checking it, if we are processing a trap handler frame or if AlwaysRelyOnEHInfo returns true (both of which are true in my case), we use the EH Frame unwind plan if it is valid at the current address, and that valid at current address check is already passing. I'm somewhat reluctant to fiddle with the behaves_like_zeroth_frame bit speculatively. Note that with this change, it will get set for the next frame above __kernel_rt_sigreturn (because the current test checks if the next-lower frame is an eTrapHandler frame), which I think is where we want the async processing.

Thanks Jonas, yes we need to come up with a perf testing infrastructure in lldb but there isn't a lot of value in this original attempt from c. 2013.

This looks good. Over in RegisterContextLLDB::GetFullUnwindPlanForFrame there's a behaves_like_zeroth_frame bool with similar setup -- in that case, it's trying to decide what type of UnwindPlan to pick for the frame, either one that is synchronous ("valid only at call sites / throwable locations") or asynchronous ("valid at every instruction"). For a behaves_like_zeroth_frame it needs to pick an asynchronous unwind plan. You may want to look at that too.

Nice.

Does your feature print *all* the registers including floating point/vector registers? Or just the main general purpose registers? In debugserver, we expedite the register values for the stopping thread:

My one concern is how this will work in a swift-lldb where we have a swift ValueObject, the print description fails, and we fall back to the ObjC print description. That's a change in behavior, right? I imagine worst case is that the objc print description will fail to produce anything and we'll have the same behavior as we do today.

Looks good to me.

I had some more revisions on this patch, but I finally understood that I was solving the wrong problem with this patchset. lldb should not fetch class names up front when scanning the objective c class tables, they should only be read lazily as we evaluate an expression. The fact that we're reading names during the table scan is the problem that needs to be fixed.

LGTM.

Fix one small bug in the jitted expressions. Increase the estimated size of class names that lldb uses to pre-allocate the string pool buffer. Added a method to read the string pool out of the inferior, to reduce code duplication, and have it read the last name to get the full size of the string pool correct.

Sorry for the delay in looking at this. It looks like a good change, an improvement on the original for sure. I've never seen multiple eh_frame FDEs for a single function, so that's fine. The .ARM.exidx format and Apple's compact unwind format can theoretically have multiple different unwind plans for a single function - but I've never seen it done in practice. More commonly, a single unwind plan will be used for multiple functions that have the same unwind state. (these two formats don't describe prologue/epilogues - they only describe the unwind state in the middle of the function where exceptions can be thrown)

very nice.

Thanks for digging in to this Pavel. I haven't had time to look over the patch today, but I'd like to; I'll make time for it in the next couple days if that's OK.

Agreed, this change is correct. I double checked this in the http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038b/IHI0038B_ehabi.pdf doc (mod 24 Nov 2015) and the only difference in Section 9.3 "Frame unwinding instructions" Table 4 and this code is in the comment - they now refer to this as 'Pop VFP double-precision registers D[8]-D[8+nnn] saved (as if) by VPUSH (see remark d)'. But there's no benefit in updating this one comment while the others are using the older comments (this code was originally written against the 30 Nov 2012 version of the doc).

Added a testcase, TestModuleLoadedNotifys.

Thanks Pavel, I'll convert this to use Expected<> while I'm working on it. I'm writing a test case right now, and looking at how DynamicPluginDarwin works more closely for adding & removing binaries, e.g. there's also no way to batch remove libraries and have a group broadcast notification be sent. There was also a bug in my original patch for calling LoadScriptingResourceForModule() to find a python resource file in a dSYM bundle - it is currently called from Target::ModuleUpdated when a binary is added to the target's ModuleList, but I'm suppressing that call with this change, so I need to move that over to ModulesDidLoad.

Remove const bool notify's. Rename method to Target::GetOrCreateModule. Refine the method headerdoc a bit.

OK looked things over a bit.

Thanks for looking the patch over.

Looks good. Removing that printf is good. Could you also remove the printf("mach_header:...") in the same function?

In D55718#1443487, @clayborg wrote:

As long as the numbers _can_ still come from the GDB server, I am all for that. If they are not supplied, then we use arch defaults. That allows new system/arch bringups where the info might be changing often, and also allows the info to be locked down. The architecture plug-ins are a great place for this. We should also move the breakpoint opcode logic into the architecture plug-ins.

In D55718#1442965, @clayborg wrote:

It would be really nice to get the GDB remote server for ARC so that it vends the correct regs to begin with, but if that isn't possible I guess we need to do what we are doing here. I would really like to not see more architectures have to add code to ProcessGDBRemote.cpp at all. If that isn't possible this solution will need to be used.

Sorry for not weighing in earlier, yes this is good.