This is an archive of the discontinued LLVM Phabricator instance.

Fix handling of hijacked events in synchronous mode
ClosedPublic

Authored by ki.stfu on Apr 30 2015, 6:28 AM.

Details

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

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 24702.Apr 30 2015, 6:28 AM
ki.stfu retitled this revision from to Fix handling of hijacked events in synchronous mode.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: jingham, clayborg.
ki.stfu added subscribers: jingham, clayborg, Unknown Object (MLST).
ki.stfu added a subscriber: zturner.

Friendly ping.

clayborg accepted this revision.May 5 2015, 10:16 AM
clayborg edited edge metadata.

I am ok as long as Jim is OK.

This revision is now accepted and ready to land.May 5 2015, 10:16 AM
jingham edited edge metadata.May 5 2015, 10:44 AM

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.

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 added inline comments.May 5 2015, 11:18 AM
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 updated this revision to Diff 25134.May 7 2015, 12:02 AM
ki.stfu edited edge metadata.

Rebase against ToT

ki.stfu updated this object.May 7 2015, 12:20 AM
ki.stfu updated this revision to Diff 25135.May 7 2015, 12:21 AM

Improve MiStartupOptionsTestCase tests to expect *stopped (MI)

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?

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)

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

@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

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

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.

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 added a subscriber: abidh.May 8 2015, 10:16 AM

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.

*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

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

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 added a comment.May 11 2015, 5:18 AM

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.

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

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

Jim

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?

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.

ki.stfu planned changes to this revision.May 11 2015, 10:54 PM
abidh added a comment.May 12 2015, 2:48 AM
  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 added a comment.May 12 2015, 2:50 AM

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

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 updated this revision to Diff 25696.May 13 2015, 8:21 AM

Rebase against ToT

This revision is now accepted and ready to land.May 13 2015, 8:21 AM
ki.stfu planned changes to this revision.May 13 2015, 8:22 AM
ki.stfu updated this revision to Diff 25699.May 13 2015, 9:36 AM

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

This revision is now accepted and ready to land.May 13 2015, 9:36 AM
ki.stfu requested a review of this revision.May 13 2015, 9:38 AM
ki.stfu edited edge metadata.

@clayborg, @jingham, @abidh,

Take a look again please.

ki.stfu updated this revision to Diff 25700.May 13 2015, 9:40 AM
ki.stfu edited edge metadata.

Remove unused event_sp in Target::Launch

clayborg accepted this revision.May 13 2015, 9:56 AM
clayborg edited edge metadata.

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

This revision is now accepted and ready to land.May 13 2015, 9:56 AM

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.

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.

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

@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 planned changes to this revision.May 15 2015, 11:15 AM
ki.stfu edited the test plan for this revision. (Show Details)May 18 2015, 7:35 AM
ki.stfu edited edge metadata.
ki.stfu updated this revision to Diff 25966.May 18 2015, 7:42 AM

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

This revision is now accepted and ready to land.May 18 2015, 7:42 AM
ki.stfu requested a review of this revision.May 18 2015, 7:44 AM
ki.stfu edited edge metadata.

@jingham, take a look again please.

ki.stfu updated this revision to Diff 25971.May 18 2015, 8:40 AM
ki.stfu edited edge metadata.

Refactor CMICmnLLDBDebugger

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.

@abidh, is it ok for you too?

ki.stfu updated this revision to Diff 26043.May 19 2015, 3:32 AM

Add a comment to SBProcess::GetStopEventForStopID

ki.stfu updated this object.May 19 2015, 3:39 AM

I gonna commit it right now.

This revision was automatically updated to reflect the committed changes.