Ah, Pavel's change was correct to run the first test.

Ah, part of my confusion is mine - the alignas() requires c++11 and I wasn't building with that mode when I did it by hand.

Thanks Jonas, LGTM.

Ah very cool, I didn't realize we had a new posix_spawn attr setter that could set the cpu subtype.

Ah, I forgot to add. I added a test by having an existing corefile test (which loads an x86_64 binary) have a plist that claims it is i386. It's close to the same thing, and results in the same kind of erroring if lldb trusts the arch from the plist.

Nice cleanup.

Bien.

Looks good overall, nice test.

Minor update to the patch to move the test case under API/macosx for now, because it depends on debugserver behavior. Add comments to note that we're all in agreement this should really be handled up in lldb - but that's more a TODO for a future day, for now.

I guess the register context part of the crashlog.json would be the most architecture-dependent, but I bet crashlog.json doesn't use any of that when printing the backtrace -- it only uses the stack trace's pc values. So you could have a "x86_64" crashlog.json, build an arm64e userland program, substitute in the slide-adjusted values of a few functions and the UUID and it would load & symbolicate in lldb.

In D91130#2389867, @clayborg wrote:

Yeah, I don't know where it's best to centralize the logic of (1) recognize a builtin_debugtrap instruction, and (2) set the pc so we step past it, know how to step past it when the user asks to resume control. debugserver (and lldb-server I'm sure) are already lying a *little* with a normal x86 user breakpoint - when you execute 0xcc at addr 0x100, the stop pc is 0x101 and someone is rolling that back to 0x100 to report the breakpoint hit (debugserver does this, I'm sure lldb-server does the same). So we're just adding more lies into the remote stub.

You could also make a fakey dsymForUUID script like we do in some other corefile tests. Compile a C file with foo(), bar(), baz(), substitute the addresses of those symbols from the binary into the crashlog.json, substitute the UUID of the compiled program in the crashlog.json, then the fakey-dsymForUUID finds the binary, lldb loads it at the binary at the correct offset. It's a lot of futzing around, but it would be possible.

I discussed this with KevinE in the past and he was adamant that he didn't want LC_IDENT being used for this. I don't have strong feelings. I think it was an old load command used for something else altogether decades ago and it started getting used in kernel corefiles and he wasn't thrilled with that. There is a Mach-O LC_NOTE load command that allows for the kernel version string to be stored in the kernel file ("kern ver str") but it hasn't been adopted by darwin kernel dumpers yet.

Update to address previous review comments. (1) Update SBMemoryRegionInfo API to provide access to the dirty page information. (2) Update the 'memory region' command output to print the dirty page list. (3) Update the 'process save-core' command to take a --style option with accepts an enum for the type of corefile. (4) Update 'process save-core' text to explain the limitations of a modified-memory-only corefile. (5) Add a API/functionalities/gdb_remote_client test with a stub that returns the region infos with these new fields, and tests the SBMemoryRegionInfo object filled in on the debugger side. I think that's everything.

(*cough* "requesting 256 instructions", wrote too quickly, or rather I am speaking in gdb RSP and don't always prefix my base16 numbers).

So in this patch you're setting the limit to be the size of the range (for someone who did 'disass -s 0x100 -e 0x200' they would be requesting 100 instructions) but the disassembly returned would be limited by the byte range requested, so this works OK. The asm(nop) is to guarantee that sum() has at least one instruction? There's no documentation for the Disassembler ctors where we might mention that the use case is either (1) an AddressRange, or (2) a start address and instruction count, but maybe there should be?

Just to confirm one quick detail (I couldn't remember for sure and had to check) - if we strtoul will stop parsing when it finds a character outside the range of the specified base. e.g. calling strtoul("0x100", nullptr, 10) will yield 0; strtoul("10f", nullptr, 10) will yield 10.

In D89227#2327443, @labath wrote:

In D89227#2327068, @DavidSpickett wrote:

And I'm saying lldb-server and not that and debug-server because if I'm honest, I don't really know what the latter is. I assume this applies to both though.

debugserver is an apple-only implementation of the remote stub. However, I think that may be a red herring, because in lldb land the vFile packets are handled by lldb-server platform, which is used even on apple platforms (IIUC).

Thanks for adding the clarifying text, I wrote the doc based on the existing lldb-server implementation of these packets while I was writing a separate implementation of the platform packets, so I hadn't seen the gdb docs.

In D82863#2319568, @labath wrote:

In D82863#2313676, @jasonmolenda wrote:

(about g packets...) they cause so many problems if there is a mis-coordination between the remote stub and lldb, I never want to stop the remote stub from providing those offsets.

I am not sure how to interpret this in the context of SVE registers. The stub cannot send the offsets of those, as their size/offset might change depending on their configuration.

Thanks for the feedback Pavel. Good point on piping the dirty pages list through the memory region command and adding tests for that. And good thoughts on different memory types that could be included in the corefile. I think we may revisit how the 'process save-core' command arguments work as we add more formats. We could do some pretty cool stuff here.

Yeah, I'm not opposed to mutually exclusive options to process save-core to request a specific style of coredump to be created, whether it's a --core-style enum or a series of flags isn't important to me. Right now I only intend to add the two that are possible - a full coredump or a modified-memory (dirty memory) coredump, on macOS. Whether it's passed as an enum or bitflag I don't have a preference, although a bitflag makes it sound like there could be multiple of them selected, when that's not what I'd be envisioning here. (if we were doing a series of flags for the lldb API, it would make more sense if we had different types of memory that could be coredumped and you could set the flags for what you want included. stack, heap, shared memory, binary images, etc.)

I was thinking about the UI of this some more today.

Thinking about process save-core and how it will default to a skinny corefile on macOS but a full corefile elsewhere, I'd like to surface some notification to the user about the type of corefile that was created to the user. Maybe add an PluginManager::SavedCoreType {eSavedCoreFull, eSavedCoreDirtyOnly, eSavedCoreDirtyPlusExecutingIMages}, have PluginManager::SaveCore return that enum value so that the "process save-core" user command can print text about the type of corefile that was saved. For a dirty corefile on macos, it could say "Modified-memory-only corefile has been written. This corefile will not be portable between systems, or usable as software updates are applied; --full-corefile can create a corefile appropriate for sharing/archiving." or something.

Hey all, got distracted while reading the patch and jotted a few notes, but I know that's not as much what Pavel wanted to discuss. We already have some target specific goop in these ProcessGDBRemote type classes for registers and such, this doesn't cause my great pain to see added.

This patch update incorporates most of Greg's suggestions. Most importantly, I isolated all of the loading of the binaries specified in the corefile down into ObjectFileMachO so I didn't have to add so much special case API to the ObjectFile base API.

Incorporate Jonas' suggestions, including changing the NULLs to nullptr's in the section of code I was changing right now. Also found an unintentional shadowing of module_sp in GetSharedModuleKernel (that was present before my patchset) and fixed that; it works either way, but it's not correct.

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.