LGTM, if you want to remove the last example, I say go ahead. If you want to dig in more, then we should go through another review for the useful example.

LGTM. The KVO decorated class name is almost never useful, and certainly not here...

LGTM

In D86652#2244043, @labath wrote:

This is going to derail this patch somewhat (sorry), but since this is an important long-standing open issue, I think we should discuss it in more detail.

The part that's bothering me about this for quite some time is... Why do we even have this DoOnRemoval stuff in the first place?

I mean instead of doing this work in DoOnRemoval, why don't we do this work /before/ enqueueing the relevant event. Then the event would be of purely informative character and it would not matter if its received by one, two, ten, or zero listeners. Of course, if a user wanted to program lldb in a correct and race-free manner, it would /have to/ listen to these events (as otherwise it would not know when is it safe to run some actions), but that would be something that's entirely up to him.

Back when I was debugging some races in tests with asynchronous listeners, I remember it was pretty hard to reason about things and prove they are correct, since the dequeueing of the event (and the resulting flurry of actions within lldb) could come at any moment. If those actions were done internally to lldb, they would be more sequenced more deterministically and be easier to reason about than if they are executed whenever the user chooses to dequeue an event. (I remember things got particularly messy when a test finished before dequeuing an event, at which point the processing of the event was executing concurrently with the process.Kill() statement we execute at the end of each test.)

This still seems to me a pretty fragile way to program lldb. For instance, if the controlling thread gets the stop event first, and decides it wants to continue, it will set the process running again and by the time the memory reading thread gets the event, the process will have already proceeded and the memory read will fail. But it it happens the other way around, it will succeed. That will make for subtle bugs in your implementation.

I want to separate out two things here. One is whether lldb should internally ask questions of a thread once we've invalidated the thread list before running, the other is how we present threads to the user while the process is running.

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

We should be able to calculate ShouldReportRun before we actually set the run going. That's better than just querying potentially stale threads. It would also be good to find a way to prevent ourselves from consulting the thread list after we've decided to invalidate it for run, but that's a second order consideration.

This change is fine for what it does, but I don't think the model that it allows is really supportable. If you have multiple listeners on the process events, and Listener A wants to do a "step" when notified that the process has stopped, but Listener B wants to do a "Continue", then there's no way to coordinate these and we're just allowing race conditions in controlling the process. I think you really need to have one agent that controls the process, and then other threads that are passive listeners.

What's calling into the thread plans when WillResume has already been called? That seems wrong, since the thread list is in an uncertain state, having been cleared out for resume and not reset after the stop. It seems to me it would be a better fix to ensure that we aren't doing that.

I'm confused as to how this patch actually fixes the problem. When the thread gets removed from the thread list, it should get Destroy called on it - which should set m_destroy_called, causing IsValid to return false.. So I am not clear under what circumstances FindThreadByID will fail, but the cached thread shared pointer's IsValid is still true? If IsValid is holding true over the thread's removal from the thread list, then I'm worried that this change will keep us using the old ThreadSP that was reported the next time we stopped and this thread ID was represented by a different ThreadSP.

In D83468#2226461, @aprantl wrote:

In D83468#2224774, @dblaikie wrote:

In D83468#2224754, @aprantl wrote:

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

How does LLDB handle this? Might also be consumer-fixable.

My understanding is that like with a regular breakpoint it would also slide the prologue-end breakpoint forward to the first non-zero line number. I'm not sure what happens if it's line 0 all the way to the end of the basic block — I think it would break on line 0 in that case. @jingham can correct me.

reduce comment to the part appropriate to this change.

Clarify comment

Remove a file that got inadvertently included from another commit.

clang-format

The implementation looks fine.

LGTM

In D85106#2194840, @clayborg wrote:

lldb/test/API is usually for testing the lldb::SB* interfaces IIRC. So not sure this move makes sense?

Remember to do the action thingie...

This LGTM with a couple of quibbles but I haven't been working on the platform support recently so we should wait till people who have done so weight in.

I'm a little curious how you would specify the path to your binary on a remote platform using the command line. But this way certainly wasn't it...

This is doing the right thing. Ismail and I talked a little off-line and I allayed his concerns.

This overall change makes sense to me.

LGTM

The current lldb behavior is that when you start up lldb, it reads the editline command-history file (so the history of the previous session), and loads it into it's internal command history, which you can get to by up and down arrows or ^R searches. But it doesn't prime the list that the "command history" command has access to. So command history starts out empty. But IMO, that's just bug. If a command shows up in scrolling through with up-arrow, it should also be listed in the history command wherever it lives. This really reduces the utility of the "history" command... I think we do want to show this. We might want mark in the "command history" output where the imported commands end and the new commands start, but we really should make them available.

I think you missed two more places to pass a target, then this looks good to me. How tedious, thanks for doing it...

I pointed out a couple of places where you have either at hand or near at hand a useful exe_ctx that you could pass instead of nullptr.

The !text test is correct, since you intend to pass *text in as a ValueObject &. But I wouldn't add the GetValueAsUnsigned check, that seems confusing. The NSStringSummaryProvider is returning a bool to tell you whether it succeeded or not, so it seems odd to pre-judge one of it's error states.

The patch lost seems to have lost everything but the PPC, SystemZ and MIPS code...

Yes, the only way you can get your hands on an SBThreadPlan is in the process of queueing one, so the ThreadPlan stack will always be the one to manage the lifecycle of a plan. And if an SBThreadPlan represents a ThreadPlan that has been discarded, it should convert back to an empty SBThreadPlan rather than keeping the underlying plan alive. So this is correct.

LGTM

LGTM.

Address review comments

Address review comments.

BTW, note that target delete has a -c option which does:

In D83933#2156772, @clayborg wrote:

Who is calling delete target anyway? Can't be Xcode, because we have huge performance issues right now with Xcode 11.4.1 and later due to the "parse all line tables (and link them all if DWARF in .o files is being used) everywhere on the first source file and line breakpoint being set" and we don't see slowdowns in subsequent runs with Xcode (lldb-rpc-server).

In D83933#2156695, @clayborg wrote:

In D83933#2156468, @jingham wrote:

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

You are correct. That isn't what this patch is trying to do. The global file cache might already be doing what I suggested (ejecting older versions from the global cache), so there may be nothing to do.

But I do think deleting a target is a nice time to do module some cleanup if possible, though it might be better controlled with a bool argument to delete target instead of always being done.

If you are shared library linking against LLDB.framework and you rarely use LLDB, but you use it for symbolication every once and a while, it might be nice to be able to regain memory by deleting the target with an argument instead of now having to call SBDebugger::MemoryPressureDetected() and hope it cleans things up.

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

Note, you want to be careful how you reap executables whose binary has changed. If you are debugging with the debug info in .o files, and you change one .cpp file & rebuild, you can throw away the Module for the executable, but you don't want to throw away the modules that back the .o files that haven't changed.

We certainly don't want to clear the shared module cache when the Debugger or the SBDebugger is destroyed. Most IDE's that use LLDB as a library use a debugger per debugging session, and destroy the debugger when the debugging session ends. As it stands now, when the first debugging session ends we don't remove any of the already parsed modules, so that the second debugging session is much faster. That would not be true if you did RemoveOrphanedSharedModules in Debugger::Destroy. 99% of all the libraries that get loaded by an app don't change over an Xcode run, and are in fact shared among most of the targets you might debug on a given platform. So clearing the cache after every run would result in a lot of pointless reparsing of system libraries.

This looks good. In the breakpoint case, I added a --dummy option so that you could set breakpoints directly on the dummy target. You would do that if you were going to do something that would make new targets, and want to ensure that the breakpoints get set in all the new targets. That's probably a very little used feature, so while it would be nice to maintain consistency amongst commands that add to the dummy target, that's definitely extra-credit.

The change looks pretty straightforward, and having the recognizers be per target seems like a better model to me. The one difficulty with adding a target to the recognizers is that you can't add a recognizer in your .lldbinit file, since you don't have a target yet. If anybody has gotten around to doing that yet (the recognizers aren't brand new at this point) will be broken by this change.

LGTM

Couple of minor comments.

In D83450#2143631, @labath wrote:

Right. It does bug me a little that this has no test, but I don't think it's the worst that could happen. And you're the one who's going to have to debug this all over again if the next value object change breaks swift again.

In D83446#2143464, @labath wrote:

In D83446#2142564, @JDevlieghere wrote:

Seems like my thoughts got lost a bit with all the inline replies: we can solve this particular issue by making process connect block in synchronous mode. The fact that that's not happening today is a bug beyond the reproducers. I don't think we should change the current behavior in asynchronous mode. I think it's probably worthwhile to do that even if we decide to add extra synchronization for the reproducers in which case this would no longer be an example of a problem solved by the current patch.

I'm starting to understand why you say "process connect" is not blocking/synchronous. Although it is waiting for the "stopped" event to be sent to the public queue (here), it is not waiting for it to be processed by the public queue. The "process continue" command OTOH, is kind of waiting for the public queue processing to finish. I say only "kind of" because it does not actually process it on the _real_ public queue -- instead it "hijacks" the events and processes then on its own little event loop. The part that I got wrong was that I was assuming that the hijacked event loop would rebroadcast the stop event to the real public queue. In fact, it doesn't, which means that the stop event processing is completely finished by the time that the "process continue" command returns.

It seems reasonable that "process connect" (and it's SB equivalent) should behave the same way. This would also explain why so many of the "gdb-client" tests need to use lldbutil.expect_state_changes when connecting to the mock server, even though they are running in synchronous mode (the only other tests using this function explicitly choose to use the async mode).

For async mode, we can say that the user needs to ensure that the event is handled before he can issue other commands -- this is the same situation as with "process continue". Of course, in the asynch case, the reproducers are "the user" and they will need to do something about this. But maybe we don't need to cross that bridge right now?

IIRC from when Shafik and I were looking at this the ObjC runtime data lists the same offset for all the members of the bitfield: the offset of the first member of the bitfield. Apparently the ObjC runtime doesn't need to know where the individual members are dynamically - the offsets are baked into the code or something? So to find where the member actually is you have to take the offset into the field - which is given in the DWARF, and add it to the dynamic offset you get from the runtime for the first member.

I want to be careful not to conflate lldb's general event behavior with the way the command interpreter happens to run things.

In D83446#2142350, @JDevlieghere wrote:

In D83446#2142301, @jingham wrote:

I think the proper way to gate this sort of "race" is to use the process state. If the current process state is "stopped" then that means we're done with the stop processing and ready to allow commands that require the process to be stopped.

When I originally did this, the plan was that the the Public state would not be set to stopped until the stop event was consumed by the process listener. So if you tried to "continue" the process before the work to handle the stop event was done, you would see the state as "running" and the continue would not succeed.

However, that ran up against the problem that part of the "asynchronous" work that you might do on stop can involve running commands and SB API. Those need to see a public stop state or they won't work correctly. So the public state gets set to stopped too early, and then it's possible for this to be racy. IMO the solution to this problem should be for the Event Handler to be able to run its commands "as if" the public state were stopped, but everybody else who is trying to run commands will still see it as running. Then when the event handler is done with it's job, the process Public state is switched to stopped, and now commands coming from other sources will run in the stopped state.

How would that work with the reproducer issue? How would you prevent the command interpreter from processing the next command before the event handler thread issues the process info packet?

I think the proper way to gate this sort of "race" is to use the process state. If the current process state is "stopped" then that means we're done with the stop processing and ready to allow commands that require the process to be stopped.

I am not selling this as the correct long-term structure for ValueObjects. So far as I can see, in all cases it should be possible to tell a ValueObject where it's going to find both its own store and the storage for pointer children when the ValueObject is created. It's not like that's a feature we discover as we go through the process of creating the ValueObject. That isn't how it is currently done. Now we almost always start off an incorrect setting, and often goes through several rounds of resetting before it settles on the final value. That's what I meant by saying it is "frustratingly self-healing".

Other than a quibble about using _sp suffix, this is fine.

In D83327#2136814, @davide wrote:

Aside from cosmetics, I'm not entirely sure this is the correct fix. Why are we calling this code _at all_ if the type is incomplete?

This is fine. If there were a universal convention (e.g. : separated lists) for environment variables, it would be reasonable for "append" behavior to extend rather than replace extant variables, but in the absence of the universality of such a convention, the best we can do is overwrite.

In D81810#2095902, @sirmc wrote:

Yes, I wasn't sure what the exact semantics were for step-inst for LLDB. I think the issue is not mainly the mimicking of GDB behavior, but rather that it is inconvenient for some use-cases.

To give some context on why I propose this change: the current behavior is pretty cumbersome when doing automated testing with LLDB of unknown/ generated programs. For example making use of the Python API, AFAIK, there is no interface where you can run a bounded number of instructions if lldb.SBThread.StepInstruction is not guaranteed to return (the current hack around it would be run this in a separate thread with a timeout).

Just to be clear, the lldb -> gdb command map doesn't prescribe behavior for lldb commands. It just suggests the analogous command in gdb. We are still free to implement lldb behavior however seems best to us.

LGTM

In D74136#2087863, @jingham wrote:

I need to read this in more detail at this point but I'm caught up in something else.

One of the not so great design decisions I made was to try to have "break set" be a single command governed by flags. The way the command turned out, there are some flags that tell you what kind of breakpoint you are setting, and then other flags that qualify that kind of search. But that means --file can mean "I'm specifying the file to use to set a file & line breakpoint" or it can be a narrowing specifier for a source regex or function name based breakpoint. Doing it this way makes it hard to have consistent meanings for the flags in all contexts.

If I had it to do over, I would make:

(lldb) break set source
(lldb) break set symbol
(lldb) break set source-pattern
(lldb) break set address