In D151765#4385711, @jingham wrote:

Why did you choose to have a separate FileSpecBuilder class, rather than making FileSpec smarter about the structure of the path (e.g. have an array of StringRef's into the paths for each component.) We could do the parse once the first time a path element was requested, and then operations like "RemovePathComponent" would be trivial. We could maybe even get rid of the distinction between "path" and "filename" since "filename" is really just "last path component".

In D151859#4385986, @jingham wrote:

Make sure we do something sensible with "target select <NAME>" if the user has given the same name to two targets (or disallow doing that, one or the other).

• Add more rigorous testing. Specifically, I'm testing every possible place where we we can bail early from DWARFAbbreviationDeclaration::extract.
• Replace the writeBadAbbreviationDeclarationData with more specific functions to handle both malformed input and input that is too large (both kinds of LEB128 extraction errors).

In D151597#4384734, @mib wrote:

TBH, it feels like a lot of - risky - changes just so IOHandlerGetControlSequence can return a llvm::StringRef ? Is that really necessary ?

Very straightforward. LGTM.

The change itself looks fine and is fairly mechanical. Easy to verify.

I've identified the root cause and I aim to address it in a separate change before this change goes in: https://reviews.llvm.org/D151755

Looks like my concerns were addressed. Thanks!

LGTM

LLDB_LOGF -> LLDB_LOG to avoid a temporary string construction.

Ok, the pre-submission checks failing are actually due to this change. Specifically, parsing the debug_abbrev section of the objects produced by these tests leads to an infinite loop. It goes as follows:

• We fail to get an ULEB128 value for the code because the contents of the debug_abbrev are invalid. The DataExtractor thus returns 0, signaling to the DWARFAbbreviationDeclaration parsing code that were done with this DWARFAbbreviationDeclaration.
• The DWARFAbbreviationDeclarationSet sees it's done and returns an empty Set to the DWARFDebugAbbrev parsing code. The offset is still at 0, so it's still technically a valid offset for the DataExtractor. So we continue on...

Remove use of std::call_once where not needed.

In D151353#4380119, @jhenderson wrote:

LGMT. There are a couple of pre-merge tests that failed that have something to do with DWARF. It's unlikely to be related to this patch, but it might be worth double-checking before landing this.

In D151615#4378441, @kastiglione wrote:

Do you have something in mind? I figured the test suite was enough. Do you mean stress for perf, or for ensuring this change has no behavior regressions?

After reading though the code and thinking about it, I believe this change is appropriate. It may or may not have a measurable impact on performance, but my bet is that it doesn't make it worse. I suppose you could stress test the C++ formatters if you really wanted to know though. Either way, thank you for doing this work!

Sounds good

What is the motivation behind this change?

In D151497#4374495, @JDevlieghere wrote:

In D151497#4374080, @bulbazord wrote:

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function.

If I were an end user who didn't know much about LLDB, I would think "I'm trying to print a variable, what's this about a function?" and "Why is memory allocation involved?". I would suggest changing the error message to something like: "Unable to evaluate expression while the process is $STATE: the process must be running and stopped to evaluate this expression".

What do you think?

I very much agree that error messages should first and foremost be helpful to our users. In this particular patch, we have two places where we emit this error. In UtilityFunction::MakeFunctionCaller I believe the current error is totally appropriate. That doesn't mean that I think it should be shown to the user as such. I didn't look at how this function is called, but if it trips, I would like to see an error along the lines of:

error: Couldn't run utility function. Can't make a function caller while the process is stopped: the process must be stopped to allocate memory.

On the other hand, in UserExpression::Evaluate I think it's totally appropriate to rephrase this, but at the same time I don't think we need to dumb this down.

error: Unable to evaluate expression while the process is $STATE: the process must be stopped because the expression might requires allocating memory.

I'll update the error messages accordingly.

I think it's good to improve the error messaging but I think we can probably do better. "Function caller" is specific to the internals of LLDB and isn't really meaningful for many users. It's also somewhat confusing from a user's perspective when the expression you're running isn't calling a function. For example:

Address feedback from @jhenderson

Convert return type to std::vector<llvm::StringRef>
Actually fix code so this thing compiles correctly

Thanks for doing this work!

I have no objections but you should probably wait for others to take another look.

EXPECT_TRUE -> ASSERT_TRUE when checking pointers

In D151001#4363438, @jhenderson wrote:

It would have been better to wait until I'd had a chance to review your latest updates, as there are a few (admittedly minor) points I'd have picked up on that now need you to go and update the in-tree code. I'm also adding a note that I haven't reviewed the coverage of these tests, just the general approach and style.

LGTM

Alright, looks good to me. Let's see what the bots think!

Ok, this is a pretty big patch so we probably don't want to leave it in review for a long time or it just gets harder to land correctly.

Looks like you're mostly moving things around which is fine. The bug that you're fixing is with thread handling in MultiplexerScriptedProcess right? That looks fine to me too.

• Yank common setup into its own function
• sizeof(void *) -> sizeof(uint64_t)
• Don't use auto where confusing

Ok, looks good to me now. Thanks!

You probably encountered this somewhere, is there a simple test we could add here? The change looks fine to me nonetheless.

Looks good! A few minor comments.

LGTM!

Bug fixes look good to me, the perf and code simplifications are also great to see. A few nits and I have one concern. Otherwise looks fine to me, I think.

Update SBModule::GetUUIDString -- I slightly changed behavior. Now the behavior matches the previous implementation exactly.