Fix handling of hijacked events in synchronous mode

[ki.stfu]

Summary

This patch includes the following changes:

• Fix Target::Launch to handle hijacked event in synchronous mode
• Improve MiStartupOptionsTestCase tests to expect *stopped (MI)
• Add SBProcess::GetStopEventForStopID
• Add ProcessModID::SetStopEventForLastNaturalStopID/GetStopEventForStopID
• Add const qualifier to ProcessModID::GetLastNaturalStopID
• Add SBProcess::GetStopEventForStopID
• Don't broadcast hijacked event in Target::Launch
• Add CMICmnLLDBDebugger::CheckIfNeedToRebroadcastStopEvent/RebroadcastStopEvent

[ki.stfu]

Friendly ping.

[clayborg]

I am ok as long as Jim is OK.

[jingham]

I am a little confused by the change to ResumeSynchronous. The change there means that ResumeSynchronous is now broadcasting the stop event. But it doesn't look like most of the callers (e.g. "process continue" i.e. CommandObjectProcessContinue, or SBProcess::Continue are fetching that event. Doesn't that mean there's going to be an extraneous "stop" event now sitting in the event queue that nobody looks at?

source/Target/Process.cpp

1741–1750

This ends up calling RestoreProcessEvents twice. Probably better not to do this.

[ki.stfu]

In D9371#166030, @jingham wrote:

I am a little confused by the change to ResumeSynchronous. The change there means that ResumeSynchronous is now broadcasting the stop event. But it doesn't look like most of the callers (e.g. "process continue" i.e. CommandObjectProcessContinue, or SBProcess::Continue are fetching that event. Doesn't that mean there's going to be an extraneous "stop" event now sitting in the event queue that nobody looks at?

You are right. At the moment the lldb core doesn't require this event and it works well without it. I added it for public API in case of someone will wait it. Currently it has helped me to fix few bugs: the first in test/functionalities/signal/TestSendSignal.py (line #94) and the second in lldb-mi (I'll commit a test case for it a bit later, see D9278).

[ki.stfu]

source/Target/Process.cpp

1741–1750

afaik the second call will be ignored and actually it will not do anything.

UPDATE:
I can add "else" on line #1748 and move RestoreProcessEvents from line #1744 to line #1738. Is it ok for you?

[ki.stfu]

Rebase against ToT

[ki.stfu]

Improve MiStartupOptionsTestCase tests to expect *stopped (MI)

[ki.stfu]

@jingham, friendly ping

[ki.stfu]

@jingham, friendly ping

[jingham]

The more serious question I had with this patch was that it looks like this change means you are now generating a stop event when you do a synchronous continue. I don't think anybody is expecting synchronous continues to generate events. Why is this not a problem?

[ki.stfu]

In D9371#169036, @jingham wrote:

The more serious question I had with this patch was that it looks like this change means you are now generating a stop event when you do a synchronous continue. I don't think anybody is expecting synchronous continues to generate events. Why is this not a problem?

These events are needed by lldb-mi to print *stopped notification. It always should print them in both async and sync mode, like it GDB does:

$ gdb --interpreter=mi hello
[...]
-gdb-set target-async off
^done
(gdb)
-gdb-show target-async
^done,value="off"
(gdb)
-break-insert -f main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00401442",func="main(int, char**)",file="hello.cpp",fullname="c:\\p\\tests\\hello.cpp",line="46",thread-groups
=["i1"],times="0",original-location="main"}
(gdb)
-exec-run
[...]
*stopped,reason="breakpoint-hit",disp="keep",bkptno="1",frame={addr="0x00401442",func="main",args=[{name="argc",value="1"},{name="argv",value="0x32520"}],file="hello.cpp",fullname="c:
\\p\\tests\\hello.cpp",line="46"},thread-id="1",stopped-threads="all"
(gdb)
-break-insert hello.cpp:48
^done,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x00401476",func="main(int, char**)",file="hello.cpp",fullname="c:\\p\\tests\\hello.cpp",line="48",thread-groups
=["i1"],times="0",original-location="hello.cpp:48"}
(gdb)
-exec-continue
[...]
*stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x00401476",func="main",args=[{name="argc",value="1"},{name="argv",value="0x32520"}],file="hello.cpp",fullname="c:
\\p\\tests\\hello.cpp",line="48"},thread-id="1",stopped-threads="all"
(gdb)

[jingham]

I don't understand this. The Command line worked just fine and informed the user about stops without this extra event being generated. Why does the MI need what the command line doesn't?

Jim

[ki.stfu]

@jingham wrote:

I don't understand this. The Command line worked just fine and informed the user about stops without this extra event being generated. Why does the MI need what the command line doesn't?

The CL notifies user by printing the (lldb) prompt when something happened (exception or BP hit etc.) and execution was stopped. If user don't want to wait he can press ^C and execution will be interrupted and the (lldb) prompt will be printed.

Usually IDE uses MI in async mode and therefore it can't determine whether execution was stopped or not. In that case MI should notify IDE using the *stopped notification and IDE always expects to see it when execution was stopped. This patch fixes the *stopped notification in sync mode (which is used for handling of --source option) because IDE expects to see *stopped in any case in despite of current mode of debugger

[jingham]

That seems like something you should fix on the MI side. The point of synchronous execution is that you don't have to worry about the events. Now you are introducing an extra event that everybody is going to have to deal with. That doesn't seem right to me.

Jim

[ki.stfu]

In D9371#169068, @jingham wrote:

That seems like something you should fix on the MI side. The point of synchronous execution is that you don't have to worry about the events. Now you are introducing an extra event that everybody is going to have to deal with. That doesn't seem right to me.

The lldb-mi uses public API entirely (not lldb_private) and I don't think it's possible to print *stopped if that event wasn't generated in the lldb core.

[jingham]

Tell me again what problem you are trying to solve? In synchronous mode, you always know that when a command that runs the target returns either you stopped, or the program exited. I don't see why you need an event for that.

Jim

[ki.stfu]

In D9371#169086, @jingham wrote:

Tell me again what problem you are trying to solve? In synchronous mode, you always know that when a command that runs the target returns either you stopped, or the program exited. I don't see why you need an event for that.

Eclipse works in sync mode and always waits for *stopped notification. I tried to find rules about when *stopped should be printed but I haven't found anything about printing *stopped in sync mode. It just says that it is async record that used when target was stopped. Nevertheless, GDB/MI shows it every time the target was stopped and therefore Eclipse waits it too.

Adding @abidh because he is interested in this patch too.

[jingham]

*stopped has nothing to do with lldb events. It just means that you issues a command to the MI that ran the target and stopped. If you are running in async mode, you start the target going, then wait for a stopped event, then print *stopped and the stop reason. If you are running in sync mode, you issue the SB API call that continues the target, and that call won't return till the process has either stopped or exited. At that point you can generate the *stopped reply to the original MI request. You don't need to change how LLDB's sync mode works to achieve this so far as I can see.

Jim

[ki.stfu]

In D9371#169145, @jingham wrote:

*stopped has nothing to do with lldb events. It just means that you issues a command to the MI that ran the target and stopped. If you are running in async mode, you start the target going, then wait for a stopped event, then print *stopped and the stop reason. If you are running in sync mode, you issue the SB API call that continues the target, and that call won't return till the process has either stopped or exited. At that point you can generate the *stopped reply to the original MI request. You don't need to change how LLDB's sync mode works to achieve this so far as I can see.

The problem is lldb-mi actually doesn't know what the commands do. There are specific set of commands that can cause that event, but I don't want do something like this:

if (IsSync() && (
    command == "-exec-run" ||
    command == "-exec-step" ||
    command == "-exec-next" ||
    command == "-exec-..."
   )) {
      <print a fake *stopped notification>
}

The lldb-mi has event subsystem and in such case I should generate a fake *stopped message and put it into listener's queue instead of printing it after execution of command. In addition lldb-mi can execute CL commands using the -interpreter-exec and therefore I'll need to looking for r/ru/run/process la/proce launch and so on to generate my fake *stopped. It's too big set of commands (including their reduction) and I sure that it's wrong solution.

[jingham]

Why do you ever need to run the MI in sync mode? If you want to get events when the process changes state, use async mode, that's what it is for. Changing the nature of "sync" so it starts producing events is also the wrong solution.

Jim

[abidh]

Do you have any use case where you need to put lldb-mi into sync mode because I use it with eclipse in async mode and it works without a problem.

[jingham]

BTW, if you are in async mode and you want to know if some command caused the process to run, you can just check the Process StopID (SBProcess::GetStopID().) You want the default value for include_expression_stops, since you don't want to say "stopped" if somebody ran a function call. Anyway, if the StopID has gone up, the target was run, and (in sync mode) has stopped again...

Jim

[jingham]

Of course, that first sentence should have said: "if you are in SYNC mode"...

Jim

[ki.stfu]

In D9371#169165, @jingham wrote:

Why do you ever need to run the MI in sync mode? If you want to get events when the process changes state, use async mode, that's what it is for. Changing the nature of "sync" so it starts producing events is also the wrong solution.

1. I use sync mode while handling the --source option, but to analyze program's behavior I want to see these notices.
2. Also I thought that it's needed for Eclipse because it works in sync mode but @abidh said it's not. Perhaps I understood something wrong.

In D9371#170020, @abidh wrote:

Do you have any use case where you need to put lldb-mi into sync mode because I use it with eclipse in async mode and it works without a problem.

Some time ago you said me that after my implementation of "-gdb-set target-async" the Eclipse ceased to get the running/stopped notifications. You noticed that the Eclipse gives that command to lldb-mi:

14-gdb-set target-async off

and you suggested that we can use the hard-coded target-async=on value for some time until we be able to correctly support the sync mode in lldb. As I understand this isn't a problem any more, right?

In D9371#170273, @jingham wrote:

BTW, if you are in async mode and you want to know if some command caused the process to run, you can just check the Process StopID (SBProcess::GetStopID().) You want the default value for include_expression_stops, since you don't want to say "stopped" if somebody ran a function call. Anyway, if the StopID has gone up, the target was run, and (in sync mode) has stopped again...

Ok. I'll take a look if it will help me.

[abidh]

1. I use sync mode while handling the --source option, but to analyze program's behavior I want to see these notices.

Some time ago you said me that after my implementation of "-gdb-set target-async" the Eclipse ceased to get the running/stopped notifications. You noticed that the Eclipse gives that command to lldb-mi:

14-gdb-set target-async off

and you suggested that we can use the hard-coded target-async=on value for some time until we be able to correctly support the sync mode in lldb. As I understand this isn't a problem any more, right?

Right. What I meant to say was that this started happening once we implemented -gdb-set target-async. Otherwise it was working ok. Anyway, I understand Jim's point that it is not a bug in lldb proper and we need to handle it in lldb-mi (if we decide to fix it).

[abidh]

on a separate note, the name of target-async option has been changes in latest gdb.

https://sourceware.org/gdb/onlinedocs/gdb/Asynchronous-and-non_002dstop-modes.html

[ki.stfu]

In D9371#170935, @abidh wrote:

on a separate note, the name of target-async option has been changes in latest gdb.

https://sourceware.org/gdb/onlinedocs/gdb/Asynchronous-and-non_002dstop-modes.html

Yep, I saw. Perhaps we also should do that.

[ki.stfu]

Rebase against ToT

[ki.stfu]

Generate fake *stopped event if StopID was changed

Also I did the following:

1. Added SBProcess::CreateProcessEventStateChanged to create that event
2. Remove generation of *stopped event in sync mode in Target::Launch
3. Added CMICmnLLDBDebugger::PrepareToGenerateStoppedEvent and CMICmnLLDBDebugger::GenerateStoppedEventIfNeeded

All tests pass, but I think it's not ready to commit "as is".

[ki.stfu]

@clayborg, @jingham, @abidh,

Take a look again please.

[ki.stfu]

Remove unused event_sp in Target::Launch

[clayborg]

I defer to Jim on this since you have been working with him on this.

[jingham]

I don't like the idea of SBProcess::CreateProcessEventStateChanged. That makes it sound like it is okay to fabricate StateChanged events of any sort and send them to the world, as though that wouldn't in the general case cause all sorts of havoc.

Do you need a real process event in order to handle this? I don't know how the MI is hooked up, but it seems GenerateStopEventIfNeeded could just tell the MI that the command you just interpreted resulted in a stop and that would trigger emitting a *stopped event.

If that's not possible, something more narrow like RebroadcastStopEvent, that would check to make sure we really are stopped, and then generate the stop event otherwise it would return an empty event, just so that it's clear that this has to be tied to the real state of the process would be a little better.

[jingham]

In D9371#172280, @jingham wrote:

I don't like the idea of SBProcess::CreateProcessEventStateChanged. That makes it sound like it is okay to fabricate StateChanged events of any sort and send them to the world, as though that wouldn't in the general case cause all sorts of havoc.

Do you need a real process event in order to handle this? I don't know how the MI is hooked up, but it seems GenerateStopEventIfNeeded could just tell the MI that the command you just interpreted resulted in a stop and that would trigger emitting a *stopped event.

Bad language, should say "a *stopped RESULT" since the point is that it should be possible to decouple *stopped command results from stop events in Sync mode.

If that's not possible, something more narrow like RebroadcastStopEvent, that would check to make sure we really are stopped, and then generate the stop event otherwise it would return an empty event, just so that it's clear that this has to be tied to the real state of the process would be a little better.

[ki.stfu]

In D9371#172280, @jingham wrote:

I don't like the idea of SBProcess::CreateProcessEventStateChanged. That makes it sound like it is okay to fabricate StateChanged events of any sort and send them to the world, as though that wouldn't in the general case cause all sorts of havoc.

I dislike it too.

Do you need a real process event in order to handle this? I don't know how the MI is hooked up, but it seems GenerateStopEventIfNeeded could just tell the MI that the command you just interpreted resulted in a stop and that would trigger emitting a *stopped event.

Unfortunately not. The *stopped notification is printed by event-thread (that handles events :)), and it sleeps until a new event come. After that, it calls HandleEvent(event) then HandleEventSBProcess(event) -> HandleProcessEventBroadcastBitStateChanged(event) -> HandleProcessEventStateStopped(event).
I can't just call HandleProcessEventStateStopped without the instance of SBEvent.
Another reason is that the ProcessEventData contains the m_interrupted flag which is set when target was interrupted, and I can't predict it.

If that's not possible, something more narrow like RebroadcastStopEvent, that would check to make sure we really are stopped, and then generate the stop event otherwise it would return an empty event, just so that it's clear that this has to be tied to the real state of the process would be a little better.

Do you mean to add SBProcess::RebroadcastStopEvent()? I think it's also a wrong way. Public API shouldn't contain something like that "hack".

Can you explain me, why you don't want to broadcast a stop event in sync mode? I agree that it's not what the sync mode means, but if nobody listening broadcaster, then this event will be dropped without any effects, or it will make sense if someone want to receive these events (like lldb-mi).

[ki.stfu]

@jingham wrote:

This is not going to be handled by the your "CreateProcessEventStateChanged" since that has no way of knowing whether the event you are emulating was interrupted. You also need to know whether the event was a stop & restart, which you don't know.

Agreed. I missed that.

I like it more than "generate random stop event" since it says exactly what you are doing, you are choosing to rebroadcast the last stop.

Yes, it's better, but I'm not sure it's a good decision. In addition, where you want to store the last stop event?

An even more explicit way of doing this (though it would require a little side-bookkeeping that we don't currently do) would be SBProcess::GetStopEventForStopID(uint32_t stop_id). That might actually be more generally useful, though of course if it wasn't the current stop id, you wouldn't currently be able to reconstruct thread states and the like. But one of our long-term goals for lldb is to be able to store and replay "historical" stop events, with some API to say what you wanted to gather at these historical points. So it would fit in with that direction.

Sounds good for me. I'll try to do it.

What if there is a Listener fetching events, but then for some reason somebody beneath that Listener switches to sync mode, does some stuff that stops & starts the target, and then switches to async mode on the way out. The agent which is fetching events is going to see some stopped event with no corresponding running event. Depending on what the code using async does it could even see a bunch of these, some while the sync code is still working. How is it going to understand that?

Why it doesn't get corresponding running event?

Depending on what the code using async does it could even see a bunch of these, some while the sync code is still working.

How? The stop event can be sent only when command was executed. It will not be sent while we are waiting when the command will be finished.

[ki.stfu]

Use SBProcess::GetStopEventForStopID to get the stop event instead of SBProcess::CreateProcessEventStateChanged

[ki.stfu]

@jingham, take a look again please.

[ki.stfu]

Refactor CMICmnLLDBDebugger

[jingham]

Given we don't have any use for it right not it's fine that GetStopEventForStopID only gets a real event for the last user stop ID, it's fine not to implement this in full generality. But can you add a comment particularly to the SB version of the API noting that this is currently the case? Other that that it looks okay.

[ki.stfu]

@abidh, is it ok for you too?

[ki.stfu]

Add a comment to SBProcess::GetStopEventForStopID

[ki.stfu]

I gonna commit it right now.

source/Target/Process.cpp

Process::ResumeSynchronous (Stream *stream)
     }
 
     ListenerSP listener_sp (new Listener("lldb.Process.ResumeSynchronous.hijack"));
     HijackProcessEvents(listener_sp.get());
 
     Error error = PrivateResume();
     if (error.Success())
     {
-        StateType state = WaitForProcessToStop (NULL, NULL, true, listener_sp.get(), stream);
+        EventSP event_sp;
+        StateType state = WaitForProcessToStop (NULL, &event_sp, true, listener_sp.get(), stream);
         const bool must_be_alive = false; // eStateExited is ok, so this must be false
         if (!StateIsStoppedState(state, must_be_alive))
             error.SetErrorStringWithFormat("process not in stopped state after synchronous resume: %s", StateAsCString(state));
+        else
+        {
+            // Target was stopped. Need to notify listeners about it.
+            RestoreProcessEvents();
+            BroadcastEvent(event_sp);
+        }
     }
 
     // Undo the hijacking of process events...
     RestoreProcessEvents();
 
     return error;
 }
 
 StateType
 Process::GetPrivateState ()
 {
     return m_private_state.GetValue();

source/Target/Target.cpp

if (error.Success())
             {
                 if (!launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
                 {
                     if (synchronous_execution)
                     {
                         error = m_process_sp->PrivateResume();
                         if (error.Success())
                         {
-                            state = m_process_sp->WaitForProcessToStop (NULL, NULL, true, hijack_listener_sp.get(), stream);
+                            state = m_process_sp->WaitForProcessToStop (NULL, &event_sp, true, hijack_listener_sp.get(), stream);
                             const bool must_be_alive = false; // eStateExited is ok, so this must be false
                             if (!StateIsStoppedState(state, must_be_alive))
                             {
                                 error.SetErrorStringWithFormat("process isn't stopped: %s", StateAsCString(state));
                             }
+                            else
+                            {
+                                // Target was stopped. Need to notify listeners about it.
+                                m_process_sp->RestoreProcessEvents();
+                                m_process_sp->BroadcastEvent(event_sp);
+                            }
                         }
                     }
                     else
                     {
                         m_process_sp->RestoreProcessEvents();
                         error = m_process_sp->PrivateResume();
                     }
                     if (!error.Success())
                     {
                         Error error2;
                         error2.SetErrorStringWithFormat("process resume at entry point failed: %s", error.AsCString());
                         error = error2;
                     }
                 }
                 else
                 {
                     assert(synchronous_execution && launch_info.GetFlags().Test(eLaunchFlagStopAtEntry));
 
-                    // Target was stopped at entry as was intended. Need to notify the listeners about it.
+                    // Target was stopped at entry as was intended. Need to notify listeners about it.
                     m_process_sp->RestoreProcessEvents();
-                    m_process_sp->HandlePrivateEvent(event_sp);
+                    m_process_sp->BroadcastEvent(event_sp);
                 }
             }
             else if (state == eStateExited)
             {
                 bool with_shell = !!launch_info.GetShell();
                 const int exit_status = m_process_sp->GetExitStatus();
                 const char *exit_desc = m_process_sp->GetExitDescription();
 #define LAUNCH_SHELL_MESSAGE "\n'r' and 'run' are aliases that default to launching through a shell.\nTry launching without going through a shell by using 'process launch'."

test/functionalities/signal/TestSendSignal.py

def match_state(self, process_listener, expected_state):
         broadcaster = self.process().GetBroadcaster()
         event_type_mask = lldb.SBProcess.eBroadcastBitStateChanged
         event = lldb.SBEvent()
         got_event = process_listener.WaitForEventForBroadcasterWithType(
             num_seconds, broadcaster, event_type_mask, event)
         self.assertTrue(got_event, "Got an event")
         state = lldb.SBProcess.GetStateFromEvent(event)
         self.assertTrue(state == expected_state,
-                        "It was the %s state." %
-                        lldb.SBDebugger_StateAsCString(expected_state))
+                        "It was the %s state (act: %s)." % (
+                            lldb.SBDebugger_StateAsCString(expected_state),
+                            lldb.SBDebugger_StateAsCString(state)))
 
     def send_signal(self):
         """Test that lldb command 'process signal SIGUSR1' sends a signal to the inferior process."""
 
         exe = os.path.join(os.getcwd(), "a.out")
 
         # Create a target by the debugger.
         target = self.dbg.CreateTarget(exe)
def send_signal(self):
 
         self.setAsync(True)
 
         self.assertTrue(process_listener.IsValid(), "Got a good process listener")
 
         # Disable our breakpoint, we don't want to hit it anymore...
         breakpoint.SetEnabled(False)
 
+        self.match_state(process_listener, lldb.eStateStopped)
+
         # Now continue:
         process.Continue()
 
         # If running remote test, there should be a connected event
         if lldb.remote_platform:
             self.match_state(process_listener, lldb.eStateConnected)
 
         self.match_state(process_listener, lldb.eStateRunning)

test/tools/lldb-mi/startup_options/TestMiStartupOptions.py

def test_lldbmi_source_option_start_script(self):
 
         # After '-break-insert -f main'
         self.expect("-break-insert -f main")
         self.expect("\^done,bkpt={number=\"1\"")
 
         # After '-exec-run'
         self.expect("-exec-run")
         self.expect("\^running")
+        self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # After '-break-insert main.cpp:BP_return'
         line = line_number('main.cpp', '//BP_return')
         self.expect("-break-insert main.cpp:%d" % line)
         self.expect("\^done,bkpt={number=\"2\"")
 
         # After '-exec-continue'
         self.expect("-exec-continue")
         self.expect("\^running")
+        self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # Test that lldb-mi is ready after execution of --source start_script
         self.expect(self.child_prompt, exactly = True)
 
         # Try to evaluate 'a' expression
         self.runCmd("-data-evaluate-expression a")
         self.expect("\^done,value=\"10\"")
         self.expect(self.child_prompt, exactly = True)
def test_lldbmi_source_option_start_script_exit(self):
 
         # After '-break-insert -f main'
         self.expect("-break-insert -f main")
         self.expect("\^done,bkpt={number=\"1\"")
 
         # After '-exec-run'
         self.expect("-exec-run")
         self.expect("\^running")
+        self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # After '-break-insert main.cpp:BP_return'
         line = line_number('main.cpp', '//BP_return')
         self.expect("-break-insert main.cpp:%d" % line)
         self.expect("\^done,bkpt={number=\"2\"")
 
         # After '-exec-continue'
         self.expect("-exec-continue")
         self.expect("\^running")
+        self.expect("\*stopped,reason=\"breakpoint-hit\"")
 
         # After '-data-evaluate-expression a'
         self.expect("-data-evaluate-expression a")
         self.expect("\^done,value=\"10\"")
 
         # After '-gdb-exit'
         self.expect("-gdb-exit")
         self.expect("\^exit")