ilya-nozhkin created this revision.
ilya-nozhkin added reviewers: jingham, clayborg, labath.
ilya-nozhkin added a project: LLDB.
Herald added subscribers: lldb-commits, JDevlieghere.
ilya-nozhkin requested review of this revision.
Process does something with its state and threads when an event is
removed from some public listener's event queue. The comment at the
beginning of DoOnRemoval and the condition checking m_update_state
tell that all these actions on the process state and threads must
be executed only once and after the private listener has already
handled an event. However, there is no any modification of
m_update_state except for invocation of SetUpdateStateOnRemoval
in HandlePrivateEvent. It means that m_update_state becomes 1 after an
event is handled by the private listener and is not changed anymore.
So, if more than one listener listen for process events
(for example, if someone add a listener following the example [1])
then the code in DoOnRemoval may be executed more than once. Moreover,
since events are handled asynchronously, DoOnRemoval may be executed by
two threads simultaneously that leads to data race.
This commit replaces m_update_state-based mechanism with a check of the
event broadcaster at the beginning of DoOnRemoval and a boolean flag
telling whether DoOnRemoval's logic has been already executed.
Also, there added a mutex which is locked during the whole execution of
DoOnRemoval. It makes DoOnRemoval thread-safe and ensures that the
public state of a process will be correct for all threads invoking
DoOnRemoval.
[1] https://lldb.llvm.org/python_reference/lldb.SBEvent-class.html
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D86652
Files:
lldb/include/lldb/Target/Process.h
lldb/source/Target/Process.cpp
lldb/unittests/Process/ProcessEventDataTest.cpp
Index: lldb/unittests/Process/ProcessEventDataTest.cpp
===================================================================
--- lldb/unittests/Process/ProcessEventDataTest.cpp
+++ lldb/unittests/Process/ProcessEventDataTest.cpp
@@ -169,7 +169,7 @@
ProcessEventDataSP event_data_sp =
std::make_shared<DummyProcessEventData>(process_sp, eStateStopped);
EventSP event_sp = std::make_shared<Event>(0, event_data_sp);
- event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+ process_sp->BroadcastEvent(event_sp);
event_data_sp->DoOnRemoval(event_sp.get());
bool result = static_cast<DummyProcessEventData *>(event_data_sp.get())
->m_should_stop_hit_count == 1;
@@ -181,7 +181,7 @@
event_data_sp =
std::make_shared<DummyProcessEventData>(process_sp, eStateStepping);
event_sp = std::make_shared<Event>(0, event_data_sp);
- event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+ process_sp->BroadcastEvent(event_sp);
event_data_sp->DoOnRemoval(event_sp.get());
result = static_cast<DummyProcessEventData *>(event_data_sp.get())
->m_should_stop_hit_count == 0;
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3774,7 +3774,6 @@
StateAsCString(GetState()),
is_hijacked ? "hijacked" : "public");
}
- Process::ProcessEventData::SetUpdateStateOnRemoval(event_sp.get());
if (StateIsRunningState(new_state)) {
// Only push the input handler if we aren't fowarding events, as this
// means the curses GUI is in use... Or don't push it if we are launching
@@ -3987,12 +3986,12 @@
Process::ProcessEventData::ProcessEventData()
: EventData(), m_process_wp(), m_state(eStateInvalid), m_restarted(false),
- m_update_state(0), m_interrupted(false) {}
+ m_do_on_removal_executed(false), m_interrupted(false) {}
Process::ProcessEventData::ProcessEventData(const ProcessSP &process_sp,
StateType state)
: EventData(), m_process_wp(), m_state(state), m_restarted(false),
- m_update_state(0), m_interrupted(false) {
+ m_do_on_removal_executed(false), m_interrupted(false) {
if (process_sp)
m_process_wp = process_sp;
}
@@ -4117,20 +4116,34 @@
}
void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) {
- ProcessSP process_sp(m_process_wp.lock());
+ // This function gets called each time an event gets pulled off of some
+ // listener's event queue. However, all the code below must be executed only
+ // once and after an event is already handled by the private state listener.
+ // So, here are two guards: the first of them checks that this event has been
+ // broadcasted by the public broadcaster and the second checks that this code
+ // hasn't been executed yet.
+
+ Broadcaster *broadcaster = event_ptr->GetBroadcaster();
+ if (!broadcaster ||
+ broadcaster->GetBroadcasterName() != Process::GetStaticBroadcasterClass())
+ return;
- if (!process_sp)
+ // Since different listeners are usually monitored by different threads, two
+ // threads may enter this function simultaneously. This lock ensures that
+ // only one thread executes all the code below in this case. This lock must
+ // be taken during the whole DoOnRemoval execution to ensure that other
+ // threads (which don't execute the code below) won't resume while DoOnRemoval
+ // does all the stuff about public process state and thus until the public
+ // state becomes correct.
+ std::lock_guard<std::mutex> do_on_removal_lock(m_do_on_removal_mutex);
+ if (m_do_on_removal_executed)
return;
- // This function gets called twice for each event, once when the event gets
- // pulled off of the private process event queue, and then any number of
- // times, first when it gets pulled off of the public event queue, then other
- // times when we're pretending that this is where we stopped at the end of
- // expression evaluation. m_update_state is used to distinguish these three
- // cases; it is 0 when we're just pulling it off for private handling, and >
- // 1 for expression evaluation, and we don't want to do the breakpoint
- // command handling then.
- if (m_update_state != 1)
+ m_do_on_removal_executed = true;
+
+ ProcessSP process_sp(m_process_wp.lock());
+
+ if (!process_sp)
return;
process_sp->SetPublicState(
@@ -4287,16 +4300,6 @@
data->SetInterrupted(new_value);
}
-bool Process::ProcessEventData::SetUpdateStateOnRemoval(Event *event_ptr) {
- ProcessEventData *data =
- const_cast<ProcessEventData *>(GetEventDataFromEvent(event_ptr));
- if (data) {
- data->SetUpdateStateOnRemoval();
- return true;
- }
- return false;
-}
-
lldb::TargetSP Process::CalculateTarget() { return m_target_wp.lock(); }
void Process::CalculateExecutionContext(ExecutionContext &exe_ctx) {
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -479,11 +479,7 @@
static void SetInterruptedInEvent(Event *event_ptr, bool new_value);
- static bool SetUpdateStateOnRemoval(Event *event_ptr);
-
private:
- void SetUpdateStateOnRemoval() { m_update_state++; }
-
void SetRestarted(bool new_value) { m_restarted = new_value; }
void SetInterrupted(bool new_value) { m_interrupted = new_value; }
@@ -497,7 +493,8 @@
std::vector<std::string> m_restarted_reasons;
bool m_restarted; // For "eStateStopped" events, this is true if the target
// was automatically restarted.
- int m_update_state;
+ std::mutex m_do_on_removal_mutex;
+ bool m_do_on_removal_executed;
bool m_interrupted;
ProcessEventData(const ProcessEventData &) = delete;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
