User Details
- User Since
- Sep 2 2014, 12:05 PM (332 w, 6 d)
Fri, Jan 15
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.
Wed, Jan 13
Mon, Jan 11
Sat, Jan 9
Fri, Jan 8
Thanks Jonas, LGTM.
Dec 9 2020
Dec 4 2020
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.
Dec 3 2020
Nice cleanup.
Bien.
Dec 1 2020
Looks good overall, nice test.
Nov 12 2020
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.
Nov 11 2020
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.
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.
Oct 29 2020
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.
Oct 21 2020
(*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?
Oct 20 2020
Oct 16 2020
Oct 15 2020
Oct 14 2020
Oct 13 2020
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.
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.
Oct 8 2020
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.
Oct 7 2020
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.
Oct 6 2020
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.
Oct 2 2020
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.
Oct 1 2020
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.
Sep 30 2020
Sep 28 2020
Thanks for the suggestions Greg, solid ideas. Let me rework this a bit.
Sep 27 2020
Sep 25 2020
Thanks for the feedback!
Sep 2 2020
Ah sorry lost track of this one; that looks correct, thanks for the fix.
Aug 29 2020
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.
Aug 28 2020
Hi, could you try
Hi, thanks so much for showing all the unwind plans, sorry for dropping off the thread for a day.
Aug 25 2020
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
Aug 24 2020
Looks good!
LGTM, that call to dispatch_get_global_queue QOS_CLASS_UNSPECIFIED does not look valid.
Aug 19 2020
LGTM
This looks good to me, getting any logic out of the Makefile templates that we can, that's a good improvement for maintainability.
Aug 15 2020
Aug 14 2020
Aug 10 2020
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".
Aug 6 2020
LGTM, it's hard to keep all the supported behaviors in my head but I think this is right.
Aug 5 2020
Update to std::unique_ptr -- thanks for the reminder!
Made the change in Xcode and the indentation was off.
Jul 30 2020
Jul 21 2020
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?
Jul 15 2020
The rewrite of the ObjectFileMachO parts is very nice. LGTM.