Ah, good catch!

Update to address Jonas' comments.

Yeah, you're right. And as I walk through in my head what I'm solving with this -- it's not the really expensive part of this whole operation, I don't think. I should check more carefully but it's the stat's over NFS partitions that are impacting performance, and I doubt the realpath() to expand the tilde's is a significant factor (the kernel should cache all of these inodes anyway if we're doing it in a hot fashion). I'll probably abandon this altogether unless I can prove to myself that it's important, and hoist it into the base class.

This is a nice piece of work, and a good cleanup, LGTM.

update to incorporate dave's suggestions.

In D125042#3495475, @JDevlieghere wrote:

Does that mean that the interactive crashlogs (crashlog -i) do this correctly?

Accidentally included an unrelated change in the last update.

Update to address Greg's feedback.

Ah, I can't share the binary/corefile that led me to this issue but I can show what it looks like. I have a function with a noreturn call, it's a crashing scenario where (as always) this is the most important bit to get right, to show the crash,

LGTM. I see how your workflow is set up. Yeah, you return an error if someone tries to run debugserver in translation (and the inferior isn't x86_64) - that's what we need to do, a translated debugserver won't be able to debug arm64 processes.

(tbh every case I've seen, it's someone who checked "Run in Rosetta" on the Xcode.app bundle with Finder, and are running their entire IDE as x86_64....)

Ah, I see what you're doing. You've got a shell running x86_64 or something (maybe started lldb as x86_64) so everything that is spawned from that is x86_64 -- debugserver and the inferior process -- and so you hit this. let me look more closely later tonight, but I didn't take that scenario into account. Normally when people are running debugserver as x86_64 it's because they unintentionally ran a parent as x86_64 and are paying an unintended perf hit across the debug session and part of this error reporting was to say "hey, you probably didn't mean to do this".

In D124597#3483197, @clayborg wrote:

I wonder if this will help optimized code debugging at all!? Good catch

cleanup patch to remove an unintended diff.

Thanks! Yeah, I only noticed because lldb-dotest threw an error when I was trying to run a test case earlier.

Hi, I noticed that we have two tests with the same name,

Hi all, getting back to this one after six months, hah. Reading through Jim's thoughts on this, I think we're OK to add this refinement. Tatyana also thought that removing this altogether would be an option, but this has been sitting unfixed for a while and I'd like to land the fix to it. Jim wishes for a world where HandleCommand requires an SBExecutionContext, or one in which we had an established way of deprecating old SB API. Given that we can't address that at the moment, I'm going to land this patch finally so we're not carrying the problem around for the foreseeable future.

I guess the other comment I would make is whether we should have a new ivar, instead of using the unused 0th bit of search_offset.

This is obviously building on Pavel's work of https://reviews.llvm.org/D66638 for Win32 systems. What problem with that existing patchset is this addressing? Pavel's original patch assumes that we can retrieve a function's expected stack frame size, and the size of the caller's arguments on the stack, plus a bit of searching around that location, as he describes. It seems like in ABIWindows_x86_64::CreateStackWalkingUnwindPlan you're adding a scheme that tries to look at the area directly around $sp for a return pc? This seems quite fragile by itself.

LGTM.

Sounds good. I think it's test-and-use for lldb, and debugserver ended up structure the same way.

fwiw,

commit 681f6c2f552fb9b57380f4bfd453e1fc9b6a099e
Author: Jason Molenda <jmolenda@apple.com>
Date:   Tue Dec 18 22:21:48 2018 +0000

I think we only support building debugserver on macOS systems that have libcompression. Was there a configuration where this resulted in an error? lldb needs to test for libcompression and use it if available, but with debugserver we assume it is available iirc. I'm fine with the patch, but I'm curious how it came up. It used to be tested before use etc but I removed that a couple of years ago when our minimum deployment target was high enough that it was always available.

LGTM but unobfuscated is a uint64_t, does this cast do anything now?

Update patch to remove unnecessary temporary std::string when dumping a string into its JSON representation.

Address Jonas' feedback by removing the parts of the patch that freed the ostringstream's after binary-escaped versions of them had been generated. Instead I'll add DumpEscaped methods to the JSONGenerator class.

I was chatting with Jonas about this a bit. The part of this patch which fixes all of the SendPacket(outbuf.c_str()) calls, to avoid a temporary copy of the std::string, is clearly necessary and correct. The changes to clear the ostringstreams eagerly after we've escaped the data is noisy, and I'll remove those and look at updating JSONGenerator to print escaped string representations directly, to eliminate the unescaped copy & the escaped copy duplication. I'll update the patch in a bit.

In D122848#3420581, @JDevlieghere wrote:

If debugserver linked against libSupport we could have saved the additional copy altogether by using llvm::raw_string_ostream:

std::string str;
llvm::raw_string_ostream stream(str);
stream.str() // Flushes and returns a reference to the stack allocated str

Barring that, I wonder if if a little wrapper around std::ostringstream could improve readability and avoid bugs where someone forgets to call stream.str("").

class AggressiveStream {
public:
  AggressiveStream() : m_stream(std::make_unique<std::ostringstream>()) {}

  std::ostringstream &operator*() {
    assert(m_stream && "cannot use stream after having called str()");
    return *m_stream;
  }

  std::string str() {
    std::string s = m_stream->str();
    m_stream.reset();
    return std::move(s);
  }

private:
  std::unique_ptr<std::ostringstream> m_stream;
};

WDYT?

Ah, good catch. Yes this looks correct to me.

LGTM.

Ach, that's unfortunate. So a caller can allocate space for the return object, pass that address to a callee in x8, but when callee returns, x8 has been overwritten. Agree that this is the only right way to behave.

Integrate Jonas' suggestion, update the disassemble shell test to match the new error messages.

In D120594#3346730, @jasonmolenda wrote:

In D120594#3346722, @JDevlieghere wrote:

Nit: can we use a colon instead of dash? We have a bunch of places already where we're "concatenating" errors with a colon so I'd like to keep that consistent.

Using a dash is definitely the kind of thing I would do, but I don't think I did here? I'm really not wedded to my specific wording in this patch.

In D120594#3346722, @JDevlieghere wrote:

Nit: can we use a colon instead of dash? We have a bunch of places already where we're "concatenating" errors with a colon so I'd like to keep that consistent.

In D120292#3338155, @kastiglione wrote:

@jasonmolenda one advantage this has over grepping is that the python specific extensions to SB are visible.

Nice. tbh when I'm SB API scripting, I am often sitting in include/lldb/API and grepping for keywords, because I can't remember exactly which class provides the API, or what arguments it takes or whatever.

Fix typeo in comment.

Update to address Jonas' feedback.

FWIW I started updating my implementation by reading through all the changes to the docs, and implementing them. Then I went back and looked at the patches and found some small changes that were made but without documentation being updated, or the documentation not being specific about expected details.

I see what you're doing, yeah, this makes sense. Sometimes with firmware debugging, we pick a random platform (basically) and this might change which platform gets picked, host versus remote-ios, but I don't think it will matter for them.

In D116211#3211319, @labath wrote:

What happened to the test case?