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.

Yeah, I'm ok with this, thanks.

Looks good to me.

LGTM fwiw I think the --min-gdbserver-port and --max-gdbserver-port options were primarily/only used for iOS etc testsuite runs, where a fixed set of ports is relayed between the devices. I was having some problems with lldb-server in platform mode on these devices and thought it would be nice to have a standalone implementation of the platform protocol packets, so I wrote one a couple months ago and that's what we're using now for this environment. I should probably upstream it, I think it would work on linux too, it doesn't use any of lldb/llvm so it has several areas that are primitive because I was going for "good enough" and moving on, e.g. it has a dumb logging class and the networking could use some work, etc. I want to keep this completely free of lldb/llvm dependencies so it's a little bit of an oddball.

Looks good, that was my only concern.

Will this hide a thread that jumps through a null function pointer? That's the only user process case where a pc of 0 needs to be reported to the developer.

Thanks for fixing this and adding a testcase. I don't know why 0xc9 leave was here; it's handled over in x86AssemblyInspectionEngine::leave_pattern_p. Do you have commit access?

The change looks good to me, thanks for fixing this. I'm not sure you're testing what you meant to test in TestPopRBPWithREX because you're pushing/popping r13. You could get the unwind state at byte offset 2 (after the push r13 has executed) and see that you can GetRegisterInfo() r13, and the unwind state at byte offset 4 and verify that you can't GetRegisterInfo() r13. That's a good test to make sure we handle the B bit correctly.

Sorry for the delay in checking this out. It works fine. You need to add a #include "lldb/Host/Config.h" to GDBRemoteCommunication.h.

I'll test this later today. The cleanup looks good; the goal of the most recent changes was to make HAVE_LIBCOMPESSION always enabled on Darwin systems - the setting would occasionally get lost in the Xcode project settings and we'd have slow channels using uncompressed communication until we looked into the perf regressions. After this sequence happened twice, I wanted to make it always-on (it also needed to be conditional for a while because these API were only added to Darwin ~4 years ago)

Yep, looks good. It would be nice if we found a dSYM with a Spotlight search (mdfind) if we could look NEXT to the dSYM bundle and see if there is a real binary, and load that. But this is a good first step, and it gets us the source-level information for symbolicating the crash. More advanced crashlog users may want to look at the assembly instructions around the crash site, and for that we need the actual binary.

I don't feel strongly about it, your choice. But if I misunderstood it this time, I'll likely misunderstand it again in the future. :)

LGTM.

LGTM, my guess is that this was Greg's code originally. He may have made a singleton object out of caution.