Sorry, I got distracted, but after the fact I also agree this is reasonable.

This seems good to me. Tests that aren't using stdio for a purpose shouldn't have to worry about random output.

I implemented Alex's suggestion for a GetListeners filter, but doing that triggered some bugs due to the fact that we weren't really pruning the m_listeners correctly as listeners come and go. I fixed some bugs there along with implementing this suggestion.

Respond to review comments.

LGTM

Not sure if codesign exists on iOS devices, but at least we could do this on macOS.

In this function we have the path to the binary. We could spawn codesign -d -entitlements - and then we would know whether it had that entitlement.

It's fairly common for AppKit developers to use access to static methods under NSApplication to see the state of the application. Since you aren't actually accessing the frame variables for this purpose, it's also pretty common for those folks to do:

Remove redundant code

Use llvm::popcount instead of using the builtin directly.

LGTM. Thanks for adding this.

Excellent!

LGTM, with a couple suggestions.

This looks like it might be the place where we also say "set s could either be set or show". But that's actually done elsewhere so this change is okay. We should maybe make sure we have a test for setting s<TAB> -> {set or show} options.

Add some std::move's, Jonas agrees all the cool kids are doing that these days.

Yes, that makes sense. lldb always updates its shared library state in reaction to a stop, so it's much safer to have the scripted process emulate this behavior. Plus, refreshing the state for a given stop makes more sense in the place where you generate the stop, than it does in the place where you tell yourself you've resumed.

Protect InterruptRequested from null function & format strings.

Address review comments:

The fact that you had to duplicate code from Target::Launch means the various paths to Launching a process aren't properly factored out. However, that seems like a lot just to get Dave's fix for "platform process launch" working, so this LGTM as a short-term patch.

In D153657#4445292, @bulbazord wrote:

I think this patch is probably okay to do, but it does break a supported use case that I'm aware of: One way you can do "object oriented programming" in C is to do exactly what you're trying to prevent. Using the test, you could say that "MyStruct" is the base class and "MyBiggerStruct" would be a derived class. You might have a pointer to a "MyStruct" object, even though you called malloc(sizeof(MyBiggerStruct)), and maybe you can perform that cast if you're sure that you actually have a MyBiggerStruct object.

I know there's not necessarily a good way to support that without also supporting the bug you're trying to fix though. :(

There's similar code for the runtime versions 12-15 which actually looks correct. It seems to claim that the duplicate classes are all stuffed after the capacity in the classOffsets array and that the one that won will indeed be in that list but will not be marked as a duplicate. Did it not work to fix what look like transcription errors going from the 12-15 version of this code to the 16 version?

LGTM. Let's see if we can get this to actually trap the concurrent test failures.

LGTM

It's OK to retain this as the default, and as you say, taking it out would be a trivial patch after this work. The control does allow you to do "Have I already made this child" before setting about to make it, though I can't currently see anywhere where we could use this information.

I can dream up a few speculative uses of can_create => false but they are all a little far-fetched. Certainly passing true is the dominant mode, so making it a default seems fine to me and reduces a bunch of noise.

That does look like a thinko. Nothing about a file's section addresses changing will change the name to symbol map as that isn't dependent on load addresses.

I don't think we need to support the case where the "end line token" doesn't have to stand on its own line. It's reasonable for lldb to impose this policy, and if we are going to do that then requiring the "\n" after the end line token also seems odd. The way this is done seems okay to me.

I wonder about this one. In every instance where the API is used, its result is turned into a ConstString first. That's because this variable name lives in the same slot as normal variable names, which come from the debug information and so tend to be in the ConstString pool for better reasons. Do you project being able to get rid of that latter requirement? If not, it seems a bit odd to go to the trouble to avoid this value starting life as a ConstString when the first thing everybody does with it is to turn it into a ConstString.

LGTM, if no one has found a use for this way of filtering by this point, there probably isn't a good one.

I'd also maybe call this the Target "Label" not the Name. We have a fairly strong use of Name for breakpoint names, and this doesn't have that character at all. Also, if they are Labels, I think it's legit for us to keep them unique, which I think is more sane than trying to handle two targets with the same label...

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).

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".

The help string for the setting seems clear. There's also some logic to handle the setting vrs. the values we find from the stub which you describe in the comment to the review, but it would be nice to see that in a comment in the code somewhere to help out future generations.

If a Scripted process is able to provide private events and enough info to lldb's lower level calls (read memory, registers) to work the thread-plan machinery then it won't need to generate the inferred stop reasons like PlanComplete and Instrumentation. But the scripted threads might just be producing public stops w/o the underlying model that would allow lldb to generate them. In that case, the scripted thread will also need to be able to produce the inferred stop reasons.

This seems fine in general, with one quibble:

LGTM, Adrian's comment is still outstanding however.

In D149379#4331059, @wallace wrote:

The alias still works because it still holds a reference to it. I could add that as a test

Your test measured setting a found simple breakpoint. That should measure filling all the names caches - we do that the first time you try to set a breakpoint of any sort. But doesn't measure the effects on lookup. I am guessing you will find the same "not much difference" here as well, but it would be good to test that. So it would be good to also ensure you aren't slowing down looking for a selector by name, and looking for a selector you aren't going to find by name, and looking by full ObjC name. But if that's also true, then I'm fine with this.

I added the enum, that's a good idea.

Address Review Comments

Address review comments

LGTM for support of something that really should hurt a little more than you are making it...

In D140630#4276855, @cimacmillan wrote:

Ping.

Most of this is fine. I wonder about avoiding caching the full name and name w/o category & selector name. One of the main uses of this class is to take incoming ObjC names from the ConstString pool, chop them up into full name, name w/o category, and selectorName, which we then insert into the lookup names tables for the modules they are found in. All those lookup table names will also exist in the ConstString pool.