LGTM

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.

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.

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!

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.

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.

In D110013#3013733, @aprantl wrote:

Without having dug into this deeper, do you think not specifying the arch could cause any problems with universal binaries? I guess not, because we still have the UUID, and the UUID is per arch?

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.

very nice.

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.

Update patch to incorporate Greg's suggestion.

In D108889#2973300, @clayborg wrote:

So do the shared library files on disk that are exploded from a shared cache of an iOS device have the file addresses set to where they are in the shared cache? I am guessing this must the issue that you were running into where the module was not in memory by still it needed to have its file addresses adjusted.

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.

Thanks!! I missed that.

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.

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.

In D107182#2930666, @OmarEmaraDev wrote:

@jasonmolenda Okay, I will just reimplement the patch without is_pad somehow.

Handle a bit of state tracking in ObjectFileMachO a little bit more readably.

In D107182#2930541, @OmarEmaraDev wrote:

is_pad was added in 2009 in ncurses 5.7 - patch 20090906. So it is sufficiently old to be used, is it not?

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?

if (is_pad(m_window))

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.

In D107196#2921607, @jhenderson wrote:

In D107196#2921591, @jasonmolenda wrote:

In D107196#2921590, @t.p.northover wrote:

In D107196#2917773, @MaskRay wrote:

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

I figured I would do commits locally and push them at the same time. I'm doing a final testsuite run right now and was hoping to land it if you think this might be a poor idea.

I wouldn't land them as two separate commits - even though the build bots might be fine, it'll mean that for at least one commit, LLVM as a whole is in a broken state, which can cause various issues e.g. when bisecting, or if a system does a commit-by-commit build and test cycle.

In D107196#2921590, @t.p.northover wrote:

In D107196#2917773, @MaskRay wrote:

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

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.

Nice fix.

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.

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.

In D106584#2902200, @jasonmolenda wrote:

Sorry for the delay in replying, I needed to think & look at this a bit.

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.

Update patch to:

LGTM, thanks for doing this.

LGTM!

Yep looks good.

LGTM.

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.