In D88387#2299413, @clayborg wrote:

The other thing to think about is using minidump files instead of mach-o core files. We have them in very good shape and they already have a built in notion of "all image infos". Many more things in there too. Let me know if you want to know more, but I'm not holding my breath! hhaha

Thanks for the suggestions Greg, solid ideas. Let me rework this a bit.

Thanks for the feedback!

Ah sorry lost track of this one; that looks correct, thanks for the fix.

Hahaha I remember looking at this constructor and expecting to find an uninitialized ivar, but it was initialized correctly and I went to look for another place where the bug might be. I think I see why there's some confusion -- this change landed Thrusday:

Ah, so we've still got a bug in x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite somehow for abort(). This method takes an UnwindPlan from eh_frame instructions (which says it is not a trap handler - correct), and adds some epilogue unwind rows at the end if needed and sets a couple of flags. I'm not sure how the trap handler lazybool is getting set to True in the process of this conversion. :/ I'll read through this method more closely Monday and try to spot how that is happening.

Hi, could you try

In D86667#2243878, @labath wrote:

In D86667#2243781, @mib wrote:

In D86667#2242566, @jingham wrote:

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

I think platform shell --shell sounds/looks repetitive so I opted for -i|--interpreter instead, as in Command-line Interpreter.

Yes, it is repetitive. OTOH:

• the unix commands that take a shell as an argument (e.g., su, sudo), do so via a --shell/-s argument
• we have a memory load --load command

Overall, I can understand where you're coming from, but I think I'd go with --shell nonetheless...

Hi, thanks so much for showing all the unwind plans, sorry for dropping off the thread for a day.

Minor followup on the 'image show-unwind' output -- I just landed a patch to print when a function or unwindplan are marked as being a trap handler.

separate from the fact that UnwindPlan::Dump does not print m_plan_is_for_signal_trap for an UnwindPlan, what we need to see from a real trap handler -- in lldb's terminology -- is that we have restore rules for all of the registers.

If I understand correctly, in

Hi, I'll review this problem / suggested patch in a bit but I think it might be helpful to outline what the intention of all this is. Let's say we have a stack of

Looks good!

LGTM, that call to dispatch_get_global_queue QOS_CLASS_UNSPECIFIED does not look valid.

LGTM

This looks good to me, getting any logic out of the Makefile templates that we can, that's a good improvement for maintainability.

In D85284#2219805, @MrHate wrote:

In D85284#2219639, @jasonmolenda wrote:

In D85284#2219603, @MrHate wrote:

1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
2. Updated the packet doc;
3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

What if the response is "matches:<ASCIIHEX>[,<ASCIIHEX>]" and if there are no matches, just "matches:"? We don't want to send an empty string because that means "packet not known" in the remote serial protocol.

"matches:" takes up 8 chars, maybe we can just use a 'M' as vFile:size uses 'F', so it may look like: "M<ASCIIHEX>[,<ASCIIHEX>]". What do you think?

In D85284#2219603, @MrHate wrote:

1. Removed the unnecessary print line in GDBRemoteCommunicationServerPlatform.cpp;
2. Updated the packet doc;
3. Refactored the for-loop at line 359-364 in GDBRemoteCommunicationServerPlatform.cpp;
4. Refactored the server-side test with str.encode().hex().

If we need to response for qPathComplete with empty match result. I think we could provide a char like R or what as the response. The match result is encoded into hex-bytes, so it might be easy to resolve the error symbol from the result IMHO.

Nice set of changes, and you're right it should be a lower-case "q" packet, not an upper-case "Q" packet like I suggested, it's just querying information from the remote side.

This is mostly fine. You need to escape the completed filenames you send back. You should have a test case where multiple matches are returned, like "a" matches "abc1" and "abc2".

LGTM, it's hard to keep all the supported behaviors in my head but I think this is right.

Update to std::unique_ptr -- thanks for the reminder!

Made the change in Xcode and the indentation was off.

With this patch, debugging a translated app will only work when we invoke debugserver with --fd for its communication back to lldb. e.g. running 'debugserver localhost:2000 --attach=translated-app' will pass -fd=-1 as a command line argument to /Library/Apple/usr/libexec/oah/debugserver. Doing this properly would involve passing the same hostspec:portnum that debugserver received in this case -- but could we at least error out in DNBProcessAttach if communcation_fd==-1 so it's clear that this is not yet a supported workflow?

The rewrite of the ObjectFileMachO parts is very nice. LGTM.

Sorry for the delay, this looks good to me.

Sorry yes this patch is fine. To be honest, on x86_64 at least, as you've pointed out, maybe we should just live on eh_frame completely without going through any of these augmentation checks at all. AFAIK gdb is living off of eh_frame exclusively, it may be that all the compilers have caught on to that and are emitting asynchronous unwind information in eh_frame.

Hi Pavel, sorry I've been doing a bunch of random things today and haven't really had a chance to look at this yet. eh_frame is so problematic for lldb, we never know what we're getting. I keep thinking we should take over a few augmentation string characters so that the producer can definitively indicate one of:

Yep.

LGTM. With p_flag, we only need to evaluate (processInfo.kp_proc.p_flag & P_TRANSLATED) as a boolean, but that's a style pref as much as anything.

Hi Jan, I noticed our sanitizer bot started getting failures in

Nice cleanup of a long standing undefined behavior warning.

This looks good to me, thanks Raphael.

I skimmed it, I guess the one question that comes to mind is how this will behave if we an operating system thread provider plugin active, where it may not actually list all of the threads that exist -- where we try to be tolerant of threads disappearing and then re-appearing later -- and the thread running an inferior function call got scheduled out while the function call was still happening. Pretty unlikely scenario, but I thought I'd throw it out there.

Thanks for the feedback.

In D79614#2028295, @clayborg wrote:

In D79614#2027869, @jasonmolenda wrote:

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

lldb wouldn't know whether to decode the returned error string or not -- short of using a debugserver version #. We'll need to interoperate with debugservers that send the unescaped qLaunchSuccess error messages for years to come.

It should always decode a raw string so I don't see what you mean here. Any packet that returns a normal string, not hex ascii encoded, must escape the string. The only bug here is that we are not escaping the string that debugserver sends back. No need for the hex ASCII stuff if we escape any special characters right?

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

Update to address mistake Greg identified; also remove two unused variables that were in this method before my changes.

In D78077#1980116, @clayborg wrote:

If you test worked, then there is something wrong with this test? See inline comment for copy and paste error

I was a little worried about the case where we have an 8-byte region being watched that is 4-byte aligned (so it's spanning two 8-byte watchpoints), but a quick test of that with this patch shows it works correctly. This is fine with me.

Updated the patch to incorporate Jonas and Greg's suggestions.

Yea, this looks good, I don't think any of those other search methods were valid any longer.

In D76758#1944612, @clayborg wrote:

So I know the mach-o symbol table parsing code is a mess already, but it seems like this patch can be simpler if we make a std::set<lldb:addr_t> at the top of ObjectFileMachO::ParseSymtab() and every time we add a symbol that has a valid address value, add the file addr to this set. This will avoid the need to do any sorting. This std::set can be used to not add LC_FUNCTION_START entries that already have a symbol at the address, and it can be used to only add TrieEntry values that have symbols. Thoughts?

Thanks Adrian, great comments, incorporated most of them. The use of the block to search the trie entries when eliminating duplication between nlist/tries was a good idea, much cleaner now.

Update patch with Adrian's suggestions.

(and if you're still seeing mystery reads, put a breakpoint on ProcessGDBRemote::DoReadMemory and see who is doing it)

In D74398#1935431, @jasonmolenda wrote:

In D74398#1935043, @jarin wrote:

Regarding the packet savings - there are still things that worry me.

First of all, when lldb CLI stops on a breakpoint, it will first unwind top of the stack of each thread as part of ThreadList::ShouldStop. This sends lots of "x" packets to lldb-server and only then issues jThreadInfo, which then replies with the stack memory uselessly (with my patch). I am yet to investigate why the CLI does that.

I haven't looked into this, but this sound like the difference between a "private stop" and a "public stop". When you do a source-level "next" command, lldb will put breakpoints on branches/function calls, run to those, then instruction step those instructions, etc. Each time it stops at these breakpoints or instruction-steps. Each of these stops are called private stops - the UI layer (lldb command line driver or the SB API driver) never hear about these. When we've completed executing the instructions for that source line, then we declare this to be a "public stop" - this is when lldb driver or SB API driver, and the user, are notified that execution has stopped.

In D74398#1935043, @jarin wrote:

Regarding the packet savings - there are still things that worry me.

First of all, when lldb CLI stops on a breakpoint, it will first unwind top of the stack of each thread as part of ThreadList::ShouldStop. This sends lots of "x" packets to lldb-server and only then issues jThreadInfo, which then replies with the stack memory uselessly (with my patch). I am yet to investigate why the CLI does that.

nice catch, lgtm.

Nice patch, thanks for doing this. I wouldn't have added the fallback code in DNBArchMachARM64::NumSupportedHardwareBreakpoints to handle the case where the sysctl fails - it doesn't fail - but there's no harm in it.

LGTM.

Nice fix, this looks good to me but I haven't been following this area of code too closely before now.