Ach, I should have noticed that during review.

In D100515#2695559, @JDevlieghere wrote:

In D100515#2695395, @justincohen wrote:

Out of curiosity: Typically should one be able to set target.process.virtual-addressable-bits after the target has been created? Or is it expected that users will need to run in the following order only:

settings set target.process.virtual-addressable-bits ...
target create -c ....

Setting virtual-addressable-bits won't do anythin after the target has been created (and perhaps that is working as intended?)

Yep, that was exactly why I'm always reading the setting if the mask isn't set yet (as opposed to setting it once in the constructor).

Looks good!

This looks good, thanks for doing this!

This looks good. Jonas, what do you think about having FixDataAddress() methods in the ABI's. We're going to be quickly sprinkling FixCodeAddress calls throughout lldb at places where Linux/Darwin ABI need them, and it'd be nice if we have the FixDataAddress method available (even if they're identical right now) so someone doesn't need to audit all the calls in the future and adjust them as appropriate.

I think we've heard from everyone involved with this & it's good to land. Thanks for bringing this home.

In D100515#2690972, @omjavaid wrote:

Hi Augusto, this is an interesting idea. This would simulate a slow serial connection well, but over a modern USB or Wifi or Bluetooth connection, we find the packet size is nearly irrelevant to performance -- but the number of packets is absolutely critical. If we need to read 100 kbytes, sending ten 10kb read packets is *much* slower than a single 100kb packet. For fun, I built a native debugserver that claims it supported a maximum 1k packet size, and then rebuilt it claiming it supports a maximum 512k packet size. It turns out lldb won't actually use more than a 128kbyte packet size (v. ProcessGDBRemote::GetMaxMemorySize) but anyway,

Thanks Jonas, I think this change to Process can cover

In D100338#2689846, @augusto2112 wrote:

@jasonmolenda I've updated all the function calls to prefer the file cache (except the one you pointed out), but I'm a little worried about this, since I changed a lot of function calls. Maybe I could update the function signature to force_live_memory (and default it to false), but keep all the current calls having the same behavior (by negating whatever was passed previously as prefer_file_cache). What do you think?

In D100338#2686410, @augusto2112 wrote:

Thanks for the input @jasonmolenda. FWIW I also think it's worth implementing this correctly.

Could you help me figure out which callers I should change to force_live_memory though?

I should say, my suggestion of changing Target::ReadMemory is just an opinion, please do feel free to disagree!

Trying to reword my thinking a little more clearly.

I think this will come down to, who wants to implement a patch that can set/get either form through Process API. The mask might be a more general format, so if Justin is willing to update his patch to that we can add Omair's test case to his patch?

Hm, a little more thinking. Target::ReadMemory prefers a file cache if it is available. Process::ReadMemory only uses live memory. But Process::ReadMemory doesn't work when we have run lldb on a file and not run yet. e.g.

% lldb
(lldb) tar cr -d /tmp/b.out
Current executable set to '/tmp/b.out' (x86_64).
(lldb) disass -n main
b.out`main:
b.out[0x100003f90] <+0>:  pushq  %rbp
b.out[0x100003f91] <+1>:  movq   %rsp, %rbp
b.out[0x100003f94] <+4>:  movq   0x65(%rip), %rax          ; (void *)0x0000000000000000
b.out[0x100003f9b] <+11>: movl   $0x0, -0x4(%rbp)

I'm not super convinced on the setting. It seems like something you'd add some people could work around a mistake we've made here.

In D100338#2684550, @aprantl wrote:

Another question for @jasonmolenda: Do you happen to know if the existing prefer_file_cache mechanism applies relocations?

Hi Omair, sorry for the delay in looking at this. It seems like we have two overlapping patches here, @justincohen patch in https://reviews.llvm.org/D98886 and this one.

Thanks for the feedback Shafik, updated the patch and landing in a sec.

In D98529#2651468, @justincohen wrote:

In D98529#2631946, @DavidSpickett wrote:

TCR_ELx begins at 1 (see D13.2.123 TCR_EL1, Translation Control Register (EL1) in the armarm) and covers EL0 and 1. Looking at the pseudocode access to this is undefined at EL0.

This fits with my reading of the DWARF5 doc; LGTM.

In D98529#2632424, @justincohen wrote:

uint32_t addressing_bits;
size_t len = sizeof (uint32_t);
ret = sysctlbyname("machdep.virtual_address_size", &addressing_bits, &len, NULL, 0);

returns ret == -1 on an iOS 14.4 device. I do see this work on an m1 mac.

In D98529#2631860, @justincohen wrote:

In the meantime, I'll look into adding something to the Crashpad minidump format to store an addrable bits mask, although I'm not clear how to grab this in userspace. Should sysctl machdep.virtual_address_size work on iOS? Can I grab TCR_ELx.T0SZ directly?

In D98529#2631876, @amccarth wrote:

Before reading Jason's response, I was independently wondering whether it makes sense to temporarily introduce a variable to let the user set the mask, just until the workaround is replaced with final code. Given that there's precedent, I would support that. (I've not implemented one of these LLDB settings before, but I imagine it's pretty straightforward.)

In D98529#2631946, @DavidSpickett wrote:

TCR_ELx begins at 1 (see D13.2.123 TCR_EL1, Translation Control Register (EL1) in the armarm) and covers EL0 and 1. Looking at the pseudocode access to this is undefined at EL0.

I added a Mach-O LC_NOTE which allows us to encode the number of bits used in addressing:

Sorry for not replying to this earlier. Yeah, correct number of bits to mask off must be determined dynamically at runtime, by looking at the CPU's TCR_EL0.T0SZ value (or TCR_EL1.T0SZ for EL1 code like a kernel), which is the inverse of the number of bits used for addressing (T0Sz value of 25, 39 bits used for addressing). When I added this support to the apple version of lldb, I hardcoded the number of bits used on our main processors and added a setting of target.process.virtual-addressable-bits for different cores -- because we didn't have any way of fetching these values at runtime. Since then we've added a k-v pair to the qHostInfo,

Rewrite patch to expose "get pc for symbolication" APIs instead of "behaves like zeroth frame" APIs so that we don't have different parts of lldb decrementing the pc for symbolication. Doing a final round of tests on different targets etc but I believe this is done.

Another alternative to RegisterContext::BehavesLikeZerothFrame that I've thought of is RegisterContext::GetPCForSymbolication, similar to GetPC() today. I think everyone who is decrementing $pc is doing it for symbolication, so having this hint and then leaving it to everyone to decrement-or-not may not be a great choice. It may be easier for higher-levels if RegisterContext can provide an Address suitable for symbolication directly.

Nice, LGTM.

In D96939#2570834, @teemperor wrote:

In D96939#2570831, @jasonmolenda wrote:

In D96939#2570830, @teemperor wrote:

What about:

error: Couldn't recognise core file format of <PATH>.
note: If the file is a core file in a compressed file archive, it first needs to be manually extracted before it can be loaded.

Looks good to me. Although I would write it with the freedom spelling of recognize. :)

We could compromise and spell it as recogniße (which combines s and z into one letter).

In D96939#2570830, @teemperor wrote:

What about:

error: Couldn't recognise core file format of <PATH>.
note: If the file is a core file in a compressed file archive, it first needs to be manually extracted before it can be loaded.

I mean, I'm also fine with suggesting that the file might be compressed, because this is a REAL common source of an unrecognized core file format. But also we can more clearly say "Doesn't look like a core file to me" and also noting that compression is something to look at is cool.

Thanks for taking time to deal with this annoying error message. Should we just remove the "Unable to find process plug-in for core file" text altogether? It means nothing unless you work on lldb and understand the plugin system. "Unrecognized core file format" is equivalent and more helpful to the user I think.

I marked Pavel as the reviewer for this because he's maybe the most likely to have an opinion about this change; I don't mean to sign you up for review work if you don't have time right now. I don't know if the specifics of the patch are especially interesting, but the testability problem and the decision to update all of the arch default unwind plans seem the most notable/discussable to me.

Yep, looks good.

This looks good to me. Are the bots all running macOS Big Sur (macOS 11)? The -target command line arguments in the Makefile might not work if the test is run on an older version of macOS, I think.

Looks good.

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.