- User Since
- Sep 2 2014, 12:05 PM (379 w, 1 d)
Thu, Dec 2
Wed, Nov 10
Nov 8 2021
Hi Ted, sorry for not getting back to you earlier on this, yes this looks unintentional to me and I think this change is good. Nice test case. Thanks for the ping.
Nov 3 2021
Nice cleanup of this table that's been growing over the years.
Yep, looks good, we can't hardcode the OS in this triple, we might be getting iOS or macOS binaries and should accept any OS these days.
Oct 25 2021
Sorry for the delay in looking at this one. Yes, we usually put a new debugserver on devices every year, and we don't need to support devices this old any more, this is fine to remove. Thanks!
Oct 22 2021
Oct 14 2021
Oct 11 2021
I'm fine with the change, it's true it's inconsistent but I also hate how we don't prefix base16 numbers in gdb RSP - so many bugs and mistakes are introduced because of it. So I'm always trying to sneak in some 0x prefixes when no one is looking. A fun one I saw the other day - in qHostInfo the cputype field is base16, in qProcessInfo the cputype field is base10. Sweet.
Oct 6 2021
Another possible approach would be to allow an ExecutionContext with a nullptr Target to push on to the stack, but then in CommandInterpeter::GetExecutionContext skip over those until we find one with a Target, or fall back to the Debugger::GetSelectedExecutionContext like we do with an empty stack.
Sep 24 2021
Sep 21 2021
Sep 17 2021
Yeah, on macOS we'll always have a UUID so this is a good change. The only time an arch is really important is if we don't have a UUID and we have a universal binary (aka fat file) & need to select the correct architecture.
Sep 16 2021
Sep 15 2021
Sep 14 2021
This is much more comprehensive than what I was thinking of, nicely done. The mapping of the Exception Class in the esr reg to a name is AArch64 specific, not Apple specific, isn't it? I haven't looked in the ARM ARM, but I suspect it's just the specific names you're using here which might be what we use at apple, but the exception codes are the same for any AArch64 target.
Sep 10 2021
Sep 9 2021
Aug 31 2021
Update patch to incorporate Greg's suggestion.
Aug 30 2021
Aug 29 2021
I should note two more things. First, I make this code which was previously checking if the ObjectFile was a memory image unconditional if the dSYM's segment file address differs from the ObjectFile's segment file address. I can't see how this is possible in any case but a shared cache -- so I could check that the ObjectFile is in the shared cache -- but however we got to this point, the only correct thing to do is to use the dSYM (SymbolFile)'s file addresses, it's immaterial.
Aug 16 2021
Thanks!! I missed that.
Aug 12 2021
Address David's second review comments -- explained the unusual formatting of the main.c in the test case, change the m_adrp_insn to be an Optional instead of using 0 as a "not-set" value. I'll prob land this tomorrow unless someone has additional feedback, I think it's in reasonable shape.
Aug 11 2021
Aug 9 2021
Have debugserver report malloc memory regions too, aka heap. I don't know how to use this in lldb yet, but it was easy to add so I thought I'd add it. Maybe we'll come up with an idea for it later. On Darwin, there are multiple types of heap - malloc small, malloc large, malloc tiny, etc - so for these, debugserver will return "type:heap,malloc-small;" etc, so the generic "heap" word is there, but also the more darwin-specific type of heap is given.
Aug 6 2021
Handle a bit of state tracking in ObjectFileMachO a little bit more readably.
Aug 5 2021
I reverted this commit in 4d3d182c1dcb99ddcce7d077060d87111cb8dbfa to fix the build for now; I'm not sure where is_pad() is intended to come from.
Am I the only one seeing a compiler error with this patch?
Aug 4 2021
Ah, add one more comment that David suggested.
Update patch to address David's suggestion that the bit masking & testing needed some comments to make it possible to follow. In the process of doing that, I noticed that MachODump.cpp's SymbolizerSymbolLookUp, where I cribbed the bit stuff from, doesn't handle an ADRP referring to a 4k block before $pc; it doesn't sign extend the page imm value correctly from the ADRP. I updated the test case to have a symbol 4k earlier than $pc so lldb has to compute this correctly to symbolicate the address, and added both an arm64 and arm64_32 yaml encoded copy of the binary.
Ah sorry I missed this patch, LGTM.
LGTM, thanks for updating the docs too. I have a separate platform implementation ("lldb-platform") that I wrote but it's not in wide use outside the apple lldb team, I'm not going to worry about versioning the protocol or falling back to the old buggy behavior. I'll update my impl for https://reviews.llvm.org/D106985 and https://reviews.llvm.org/D106984 once these are landed.
Thanks so much for taking the time to read the patch over and make suggestions David. I cribbed the bit masking / manip from MachODump.cpp's SymbolizerSymbolLookUp but you're right about it being better with at least a little bit of comments about what is going on there.
Aug 3 2021
Jul 31 2021
An interesting side question as long as we're talking about this printer,
add x9, x0, #291, lsl #12 ; =1191936
once we have a shifted imm value, adding the value in base10 to the comment stream is maybe not the best choice. This could be printed as,
add x9, x0, #291, lsl #12 ; =0x123000
Sorry for not commenting on this on earlier, I wanted to think about it a bit.
Jul 30 2021
Jul 28 2021
Looks great! Thanks for doing this.
Thanks for fixing this, it's long overdue. For a long time I thought we'd need to operate in a compatibility mode to accept either set of constants, but I finally realized I was wrong and we should just align ourselves with the correct gdb behavior, but hadn't made the change myself yet.
Jul 27 2021
Looks good! Maybe we should add a setting like target.file-cache-memory-reads-verify (a boolean) to enable/disable this check instead of keying off NDEBUG. We can turn it on in the testsuite, and we can optionally have people with release installs of lldb enable it if we ever suspect that it might be an issue. The settings are added with an entry in source/Target/TargetProperties.td and a getter/setter in TargetProperties.
Jul 24 2021
Sorry for the delay in replying, I needed to think & look at this a bit.
Jul 22 2021
I do like the debug-lldb check that the file cache read matches the live memory part. :)
I should add — I also don’t think this will fix the bug you’re working on.
Hm there's no phab action which says "reject approach" :) the perf impact of forcing all reads to be out of memory would be dramatically bad for a remote system debug session, this can't be done. If there is a specific caller that must use live memory even for a section marked "read-only", it needs to use force_live_memory=true in its call to ReadMemory. This change would make app launch time for iOS apps etc probably take 30-60 seconds longer than they do right now.
One more bit I meant to do -- added output to "process status --verbose" to show the addressing masks in effect. There's no other way to check what lldb is currently using for ptrauth masks currently, to debug/confirm what lldb is using.
Update patch to address Jonas' suggestion of not duplicating the code to write the LC_NOTE load command & payloads (I was annoyed by this too but didn't deal with it in my first pass). Also add a test, although it only is really tested on an arm64e testsuite run where the compiler is doing PAC auth on return addresses.
Jul 20 2021
Jul 19 2021
Jun 20 2021
Update patch to:
May 20 2021
LGTM, thanks for doing this.
May 13 2021
May 12 2021
May 11 2021
May 3 2021
Yep looks good.
Apr 29 2021
Apr 26 2021
Nice addition, these are great guidelines. I'd do a s/just// pass over the text, there's some extraneous "just"s that could go. This is a very bad habit of my own lately, I re-read emails I write, and try to remove "justs" that snuck into the text I typed before I send. I think my keyboard driver might be responsible for inserting them into sentences.