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.

To me --jit sounds like an imperative (i.e. don't use the IR interpreter, jit and execute this expression), whereas --allow-jit seems closer to the behavior here.

Thanks Aleksandr, this all looks good. I'd like to see a definition of AFA (and we can say "CFA is DWARF's Canonical Frame Address" in the same spot) maybe in UnwindPlan.h, non-Windows people (ok, me) will not know what that is referring to. I was a little worried about and_rsp_pattern_p() but we're not recording the and'ed value. One long standing problem of the UnwindPlan intermediate representation is that it doesn't have any way to represent an alignment involved in the location computation (short of a DWARF expression). This is a problem on AArch32 / armv7 where the NEON registers are saved on higher alignment boundaries than the stack pointer on function call. (if the alignment is less-than or equal-to the stack frame, then there's no align instructions needed in the prologue.) But you're not doing anything like that, no worries.

Thanks for the ping aleksandr, I have somehow managed to set up my mail rules so I didn't see this. I'll try to look it over later today. Yes, the x86 instruction profiler has definitely overgrown over the years - the ARM64 instruction profiler is probably a better model, but this code is pretty stable so it hasn't been a priority to do anything to it.

Sorry for not replying to this. Yes the code is unused - it was the initial unwinder implementation when lldb is very young, before we had our current one. The main reason to keep it around is (as you say) an example of the simplest unwinder someone might use when porting lldb to a new platform. I don't feel strongly about it one way or the other, I'm fine with removing it if that's what you want to do.

(obviously this would only be the behavior if a filecheck path was not specified)

I've been meaning to look at this - for the command line style "cd test; ./dotest.py" approach, we could use similar logic to what packages/Python/lldbsuite/test/dotest.py does in getOutputPaths() where it finds an lldb binary in the "typical" location. for an Xcode style build, this would be in a directory like ../llvm-build/Release+Asserts/x86_64/bin/FileCheck where the build-style ("Release+Asserts") is going to vary but I'm guessing that the build configuration of the FileCheck binary picked is not important.

Haven't had time to read through the main part of the patch, but I think the changes to debugserver's JSON parser would be good additions even if you've switched away from using it. The debugserver JSON parser is a copy of lldb's with all the StringRef etc llvm classes removed quickly, so it's always been a bit of a mess, I should have forked it from before any of the llvm stuff started being integrated.

That's a good point Pavel. I tried to write one (below) but I never saw what the original failure mode was. Venkata, can you help to make a test case that fails before the patch and works after? Or explain what bug was being fixed exactly? I could see that the code was wrong from reading it, but I never understood how you got to this.

looks good.

I think the m_thread_pcs.clear() in UpdateThreadIDList() is to clear out any old thread pc values that we might have populated in the past. The only way I can see this happening is if the remote stub SOMETIMES returned the thread-pcs: list in the stop packet. UpdateThreadPCsFromStopReplyThreadsValue() which we call if we did get a thread-pcs: in this stop reply packet, clears m_thread_pcs so the only reason to have this also in UpdateThreadIDList() is if we had a stub that was inconsistent.

(just to be clear, the m_thread_pcs.clear() in ProcessGDBRemote::UpdateThreadIDList that I called a bug -- today we only have two ways of populating that, jThreadsInfo or the thread-pcs: value in the stop packet. So clearing it unconditionally here, and then populating it from thread_pcs: seems reasonable.)

I could imagine there being a bug in this ProcessGDBRemote::UpdateThreadIDList -> ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue sequence when talking to a gdb-remote stub that does not implement the lldb-enhancement jThreadsInfo. If jThreadsInfo isn't implemented, we fall back to looking for the thread-pcs: and threads: key-value pairs in the stop packet that we received. We clear m_thread_pcs, then look if thread-pcs: is present (that's one bug) to add all the thread pc values. After that, we look for the threads: list and if it is present, we call UpdateThreadIDsFromStopReplyThreadsValue which clears m_thread_pcs again (bug #2) and then adds any threads listed to the m_thread_ids.

Is this going to work with non-clang compilers? ('$CXX -x c++ -stdlib=libstdc++ ') I can imagine gcc supporting these same flags, but will this work on windows?

LGTM. If we added more knowledge specifically about kext bundle layouts, we could restrict which files we test to see if they are valid binaries - but we'd need to parse the Info.plist at the top (to get the CFBundleExecutable name, and look for variations on that prefix) and we'd need to handle shallow/deep kext bundles. Given how few files are in kext bundles besides the kexts themselves (a couple of plists), this is code is much simpler than encoding all that extra specifics about kexts.

This looks good. My original change in r311622 to add this "remove the last two file path components" should have explicitly said that this is only done for DBGVersion == 2 to make it clearer for people reading the code.

Thanks for untangling this Pavel, I hadn't noticed we weren't building this plugin for non-darwin systems. I'd agree with Adrian's comment that we should have a constants like LLDB_KERNEL_SUCCESS / LLDB_KERNEL_INVALID_ARGUMENT, I think the place where -1 was being returned currently would be considered incorrect (it's probably my fault for all I know ;)

Looks good.

Ah, no you couldn't set up a command alias like that. Still, if the full name was statistics, you could type 'stat' and it would match. 'stats' wouldn't, though.