- User Since
- Sep 2 2014, 12:05 PM (346 w, 4 h)
Ach, I should have noticed that during review.
Fri, Apr 16
This looks good, thanks for doing this!
Thu, Apr 15
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.
Wed, Apr 14
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
Tue, Apr 13
I should say, my suggestion of changing Target::ReadMemory is just an opinion, please do feel free to disagree!
Mon, Apr 12
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.
Fri, Apr 9
Thu, Apr 8
Sun, Apr 4
Fri, Apr 2
Thu, Apr 1
Thanks for the feedback Shafik, updated the patch and landing in a sec.
Tue, Mar 30
Thu, Mar 25
Wed, Mar 24
Tue, Mar 23
Mar 19 2021
This fits with my reading of the DWARF5 doc; LGTM.
Mar 17 2021
Mar 16 2021
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,
Mar 12 2021
Mar 9 2021
Mar 3 2021
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.
Feb 28 2021
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.
Feb 24 2021
Feb 18 2021
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.
Feb 17 2021
Feb 16 2021
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.
Feb 3 2021
Yep, looks good.
Feb 2 2021
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.
Jan 22 2021
Jan 20 2021
Jan 15 2021
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.
Jan 13 2021
Jan 11 2021
Jan 9 2021
Jan 8 2021
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
Dec 1 2020
Looks good overall, nice test.