Yes, that all makes sense to me. It will somewhat complicate the JDWP thread logic, but i guess it will ultimately safe me quite a bit of pain.
Thanks! On Mon, Oct 27, 2014 at 9:23 PM, Greg Clayton <[email protected]> wrote: > > > On Oct 27, 2014, at 12:42 PM, Mario Zechner <[email protected]> > wrote: > > > > The piggy backing of events is fine from my point of view as long as i > can check the interrupted flag (for which i'll try to compose a patch). My > issues stem more from the fact that i have to concurrently operate on the > process (event loop thread, JDWP thread). The "hook" breakpoints are > actually handled on the event loop thread, including the auto-resume, which > is a requirement for our JDWP implementation to work correctly. The JDWP > client shouldn't know that the process hit a "hook" breakpoint. > > I would venture to say that trying to handle events on different threads > and doing a handoff is a bad idea (race conditions, etc) and you should > handle process events on one thread. > > > > My problem is really this: the JDWP thread stops the event loop thread. > Next it stops the process via SBProcess::Stop (which calls Process::Halt). > Sadly, Process::Halt doesn't set the state on the process. I have to poll > events on the JDWP thread for that to happen, on the JDWP thread, until i > get a stopped (or exited/crashed/...) event. I then have to re-broadcast > those events so the event loop thread can later process them once i resume > it. I'm very worried about re-broadcasting events as i don't know what > havoc that will wreck in the LLDB internal state. > > This what "sync" mode is for: > > debugger.SetAsync(false) > > When in this mode, any process events that resume the process will wait > until the process stops before returning: > > process.Continue() > > This continue won't return until the process actually stops. It will > consume all running/stopped events. > > But as you already have a thread that is handling events, I still say > this is a bad idea. What we do in Xcode is have a thread that is waiting > not only for process events, but also for other events like common things > to do like backtraces, get frame variables, etc). So you > > > > > Ideally, Process::Halt (and hence SBProcess::Stop()) would set things up > in a way where i don't have to poll events on the JDWP thread. It should > also set the process state accordingly. I also noticed strange timing bugs > when calling SBProcess::Stop() while a previous SBProcess::Continue() is > still being propagated to the inferior. That's actually what causes the > eStopReasonInvalid events i talked about in another thread. > > > > As always, really appreciate that you take the time to read and answer > my walls of text. > > So I would say the safest thing to do is to make your event handling > thread always handle all events. If you need the JDWP thread to do > something, it sends a event over to the event thread and has it be handled. > You can make your own broadcaster and then listen to it as well on the > thread that is listening for process events. This way you don't mix and > match. The event thread can also use that same broadcaster to send and > event back to the JDWP thread. > > Something like: > > SBBroadcaster g_broadcaster("JDWP"); > > enum JDWPEvents { > eEventThreadCommand = 1u << 0, > eJDWPThreadResponse = 1u << 1 > }; > > int event_thread(...) > { > SBDebugger debugger = lldb::SBDebugger::Create(); > debugger.SetAsync (true); > SBError error; > uint32_t event_timeout_seconds = UINT32_MAX; // Infinite wait > SBTarget target = debugger.CreateTarget (...) > if (target.IsValid()) > { > SBListener listener("event_thread.listener"); > > SBProcess process = target.Launch (listener, > NULL, // char const **argv, > NULL, // char const **envp, > NULL, // const char > *stdin_path, > NULL, // const char > *stdout_path, > NULL, // const char > *stderr_path, > NULL, // const char > *working_directory, > 0, // uint32_t > launch_flags > false, // bool stop_at_entry, > error); > > listener.StartListeningForEvents (g_broadcaster, > eEventThreadCommand); > > if (error.Success() && > process.IsValid() && > process.GetProcessID() != LLDB_INVALID_PROCESS_ID) > { > SBEvent event; > > if (listener.WaitForEvent (event_timeout_seconds, event)) > { > if (event.BroadcasterMatchesRef(g_broadcaster)) > { > // eEventThreadCommand > // Do the work needed by the JDWP thread, and then > send a response to let it know it is done... > > g_broadcaster.BroadcastEventByType(eJDWPThreadResponse); > } > else > { > // Process event... > } > } > } > } > } > > int jdwp_thread(...) > { > SBListener listener("jdwp_thread.listener"); > listener.StartListeningForEvents (g_broadcaster, eEventThreadCommand); > if (listener.WaitForEvent (event_timeout_seconds, event)) > { > } > } > > > One thing we need to add to: > > SBProcess > SBTarget::Launch (SBLaunchInfo &launch_info, SBError& error); > > Is the ability to set the SBListener in the SBLaunchInfo so we can specify > an alternate listener. Do you get where I am going with this? The key is to > listen for events and do all process control from one thread to ensure > there are no races. > > Greg > > > >
_______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
