Fix broken handling of instrumentation lists.
- We cannot copy before iterating, as entries might be deleted.
- We cannot remove entries in the list, as mutators could be
currently iterating over it.
Solution in this change is to never remove list entries, but
put null when a listener is removed. When adding a listener, we
will either put it where there is a null slot, or at the end
of the list if there is no null slot.
Change-Id: Id94582fd971cd56bcb445caff64270d21987f700
diff --git a/runtime/instrumentation.cc b/runtime/instrumentation.cc
index 4db37e6..e937397 100644
--- a/runtime/instrumentation.cc
+++ b/runtime/instrumentation.cc
@@ -387,146 +387,151 @@
return (events & expected) != 0;
}
+static void PotentiallyAddListenerTo(Instrumentation::InstrumentationEvent event,
+ uint32_t events,
+ std::list<InstrumentationListener*>& list,
+ InstrumentationListener* listener,
+ bool* has_listener)
+ REQUIRES(Locks::mutator_lock_, !Locks::thread_list_lock_, !Locks::classlinker_classes_lock_) {
+ Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
+ if (!HasEvent(event, events)) {
+ return;
+ }
+ // If there is a free slot in the list, we insert the listener in that slot.
+ // Otherwise we add it to the end of the list.
+ auto it = std::find(list.begin(), list.end(), nullptr);
+ if (it != list.end()) {
+ *it = listener;
+ } else {
+ list.push_back(listener);
+ }
+ *has_listener = true;
+}
+
void Instrumentation::AddListener(InstrumentationListener* listener, uint32_t events) {
Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
- if (HasEvent(kMethodEntered, events)) {
- method_entry_listeners_.push_back(listener);
- have_method_entry_listeners_ = true;
- }
- if (HasEvent(kMethodExited, events)) {
- method_exit_listeners_.push_back(listener);
- have_method_exit_listeners_ = true;
- }
- if (HasEvent(kMethodUnwind, events)) {
- method_unwind_listeners_.push_back(listener);
- have_method_unwind_listeners_ = true;
- }
- if (HasEvent(kBackwardBranch, events)) {
- backward_branch_listeners_.push_back(listener);
- have_backward_branch_listeners_ = true;
- }
- if (HasEvent(kInvokeVirtualOrInterface, events)) {
- invoke_virtual_or_interface_listeners_.push_back(listener);
- have_invoke_virtual_or_interface_listeners_ = true;
- }
- if (HasEvent(kDexPcMoved, events)) {
- std::list<InstrumentationListener*>* modified;
- if (have_dex_pc_listeners_) {
- modified = new std::list<InstrumentationListener*>(*dex_pc_listeners_.get());
- } else {
- modified = new std::list<InstrumentationListener*>();
- }
- modified->push_back(listener);
- dex_pc_listeners_.reset(modified);
- have_dex_pc_listeners_ = true;
- }
- if (HasEvent(kFieldRead, events)) {
- std::list<InstrumentationListener*>* modified;
- if (have_field_read_listeners_) {
- modified = new std::list<InstrumentationListener*>(*field_read_listeners_.get());
- } else {
- modified = new std::list<InstrumentationListener*>();
- }
- modified->push_back(listener);
- field_read_listeners_.reset(modified);
- have_field_read_listeners_ = true;
- }
- if (HasEvent(kFieldWritten, events)) {
- std::list<InstrumentationListener*>* modified;
- if (have_field_write_listeners_) {
- modified = new std::list<InstrumentationListener*>(*field_write_listeners_.get());
- } else {
- modified = new std::list<InstrumentationListener*>();
- }
- modified->push_back(listener);
- field_write_listeners_.reset(modified);
- have_field_write_listeners_ = true;
- }
- if (HasEvent(kExceptionCaught, events)) {
- std::list<InstrumentationListener*>* modified;
- if (have_exception_caught_listeners_) {
- modified = new std::list<InstrumentationListener*>(*exception_caught_listeners_.get());
- } else {
- modified = new std::list<InstrumentationListener*>();
- }
- modified->push_back(listener);
- exception_caught_listeners_.reset(modified);
- have_exception_caught_listeners_ = true;
- }
+ PotentiallyAddListenerTo(kMethodEntered,
+ events,
+ method_entry_listeners_,
+ listener,
+ &have_method_entry_listeners_);
+ PotentiallyAddListenerTo(kMethodExited,
+ events,
+ method_exit_listeners_,
+ listener,
+ &have_method_exit_listeners_);
+ PotentiallyAddListenerTo(kMethodUnwind,
+ events,
+ method_unwind_listeners_,
+ listener,
+ &have_method_unwind_listeners_);
+ PotentiallyAddListenerTo(kBackwardBranch,
+ events,
+ backward_branch_listeners_,
+ listener,
+ &have_backward_branch_listeners_);
+ PotentiallyAddListenerTo(kInvokeVirtualOrInterface,
+ events,
+ invoke_virtual_or_interface_listeners_,
+ listener,
+ &have_invoke_virtual_or_interface_listeners_);
+ PotentiallyAddListenerTo(kDexPcMoved,
+ events,
+ dex_pc_listeners_,
+ listener,
+ &have_dex_pc_listeners_);
+ PotentiallyAddListenerTo(kFieldRead,
+ events,
+ field_read_listeners_,
+ listener,
+ &have_field_read_listeners_);
+ PotentiallyAddListenerTo(kFieldWritten,
+ events,
+ field_write_listeners_,
+ listener,
+ &have_field_write_listeners_);
+ PotentiallyAddListenerTo(kExceptionCaught,
+ events,
+ exception_caught_listeners_,
+ listener,
+ &have_exception_caught_listeners_);
UpdateInterpreterHandlerTable();
}
+static void PotentiallyRemoveListenerFrom(Instrumentation::InstrumentationEvent event,
+ uint32_t events,
+ std::list<InstrumentationListener*>& list,
+ InstrumentationListener* listener,
+ bool* has_listener)
+ REQUIRES(Locks::mutator_lock_, !Locks::thread_list_lock_, !Locks::classlinker_classes_lock_) {
+ Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
+ if (!HasEvent(event, events)) {
+ return;
+ }
+ auto it = std::find(list.begin(), list.end(), listener);
+ if (it != list.end()) {
+ // Just update the entry, do not remove from the list. Removing entries in the list
+ // is unsafe when mutators are iterating over it.
+ *it = nullptr;
+ }
+
+ // Check if the list contains any non-null listener, and update 'has_listener'.
+ for (InstrumentationListener* l : list) {
+ if (l != nullptr) {
+ *has_listener = true;
+ return;
+ }
+ }
+ *has_listener = false;
+}
+
void Instrumentation::RemoveListener(InstrumentationListener* listener, uint32_t events) {
Locks::mutator_lock_->AssertExclusiveHeld(Thread::Current());
-
- if (HasEvent(kMethodEntered, events) && have_method_entry_listeners_) {
- method_entry_listeners_.remove(listener);
- have_method_entry_listeners_ = !method_entry_listeners_.empty();
- }
- if (HasEvent(kMethodExited, events) && have_method_exit_listeners_) {
- method_exit_listeners_.remove(listener);
- have_method_exit_listeners_ = !method_exit_listeners_.empty();
- }
- if (HasEvent(kMethodUnwind, events) && have_method_unwind_listeners_) {
- method_unwind_listeners_.remove(listener);
- have_method_unwind_listeners_ = !method_unwind_listeners_.empty();
- }
- if (HasEvent(kBackwardBranch, events) && have_backward_branch_listeners_) {
- backward_branch_listeners_.remove(listener);
- have_backward_branch_listeners_ = !backward_branch_listeners_.empty();
- }
- if (HasEvent(kInvokeVirtualOrInterface, events) && have_invoke_virtual_or_interface_listeners_) {
- invoke_virtual_or_interface_listeners_.remove(listener);
- have_invoke_virtual_or_interface_listeners_ = !invoke_virtual_or_interface_listeners_.empty();
- }
- if (HasEvent(kDexPcMoved, events) && have_dex_pc_listeners_) {
- std::list<InstrumentationListener*>* modified =
- new std::list<InstrumentationListener*>(*dex_pc_listeners_.get());
- modified->remove(listener);
- have_dex_pc_listeners_ = !modified->empty();
- if (have_dex_pc_listeners_) {
- dex_pc_listeners_.reset(modified);
- } else {
- dex_pc_listeners_.reset();
- delete modified;
- }
- }
- if (HasEvent(kFieldRead, events) && have_field_read_listeners_) {
- std::list<InstrumentationListener*>* modified =
- new std::list<InstrumentationListener*>(*field_read_listeners_.get());
- modified->remove(listener);
- have_field_read_listeners_ = !modified->empty();
- if (have_field_read_listeners_) {
- field_read_listeners_.reset(modified);
- } else {
- field_read_listeners_.reset();
- delete modified;
- }
- }
- if (HasEvent(kFieldWritten, events) && have_field_write_listeners_) {
- std::list<InstrumentationListener*>* modified =
- new std::list<InstrumentationListener*>(*field_write_listeners_.get());
- modified->remove(listener);
- have_field_write_listeners_ = !modified->empty();
- if (have_field_write_listeners_) {
- field_write_listeners_.reset(modified);
- } else {
- field_write_listeners_.reset();
- delete modified;
- }
- }
- if (HasEvent(kExceptionCaught, events) && have_exception_caught_listeners_) {
- std::list<InstrumentationListener*>* modified =
- new std::list<InstrumentationListener*>(*exception_caught_listeners_.get());
- modified->remove(listener);
- have_exception_caught_listeners_ = !modified->empty();
- if (have_exception_caught_listeners_) {
- exception_caught_listeners_.reset(modified);
- } else {
- exception_caught_listeners_.reset();
- delete modified;
- }
- }
+ PotentiallyRemoveListenerFrom(kMethodEntered,
+ events,
+ method_entry_listeners_,
+ listener,
+ &have_method_entry_listeners_);
+ PotentiallyRemoveListenerFrom(kMethodExited,
+ events,
+ method_exit_listeners_,
+ listener,
+ &have_method_exit_listeners_);
+ PotentiallyRemoveListenerFrom(kMethodUnwind,
+ events,
+ method_unwind_listeners_,
+ listener,
+ &have_method_unwind_listeners_);
+ PotentiallyRemoveListenerFrom(kBackwardBranch,
+ events,
+ backward_branch_listeners_,
+ listener,
+ &have_backward_branch_listeners_);
+ PotentiallyRemoveListenerFrom(kInvokeVirtualOrInterface,
+ events,
+ invoke_virtual_or_interface_listeners_,
+ listener,
+ &have_invoke_virtual_or_interface_listeners_);
+ PotentiallyRemoveListenerFrom(kDexPcMoved,
+ events,
+ dex_pc_listeners_,
+ listener,
+ &have_dex_pc_listeners_);
+ PotentiallyRemoveListenerFrom(kFieldRead,
+ events,
+ field_read_listeners_,
+ listener,
+ &have_field_read_listeners_);
+ PotentiallyRemoveListenerFrom(kFieldWritten,
+ events,
+ field_write_listeners_,
+ listener,
+ &have_field_write_listeners_);
+ PotentiallyRemoveListenerFrom(kExceptionCaught,
+ events,
+ exception_caught_listeners_,
+ listener,
+ &have_exception_caught_listeners_);
UpdateInterpreterHandlerTable();
}
@@ -861,28 +866,24 @@
void Instrumentation::MethodEnterEventImpl(Thread* thread, mirror::Object* this_object,
ArtMethod* method,
uint32_t dex_pc) const {
- auto it = method_entry_listeners_.begin();
- bool is_end = (it == method_entry_listeners_.end());
- // Implemented this way to prevent problems caused by modification of the list while iterating.
- while (!is_end) {
- InstrumentationListener* cur = *it;
- ++it;
- is_end = (it == method_entry_listeners_.end());
- cur->MethodEntered(thread, this_object, method, dex_pc);
+ if (HasMethodEntryListeners()) {
+ for (InstrumentationListener* listener : method_entry_listeners_) {
+ if (listener != nullptr) {
+ listener->MethodEntered(thread, this_object, method, dex_pc);
+ }
+ }
}
}
void Instrumentation::MethodExitEventImpl(Thread* thread, mirror::Object* this_object,
ArtMethod* method,
uint32_t dex_pc, const JValue& return_value) const {
- auto it = method_exit_listeners_.begin();
- bool is_end = (it == method_exit_listeners_.end());
- // Implemented this way to prevent problems caused by modification of the list while iterating.
- while (!is_end) {
- InstrumentationListener* cur = *it;
- ++it;
- is_end = (it == method_exit_listeners_.end());
- cur->MethodExited(thread, this_object, method, dex_pc, return_value);
+ if (HasMethodExitListeners()) {
+ for (InstrumentationListener* listener : method_exit_listeners_) {
+ if (listener != nullptr) {
+ listener->MethodExited(thread, this_object, method, dex_pc, return_value);
+ }
+ }
}
}
@@ -891,7 +892,9 @@
uint32_t dex_pc) const {
if (HasMethodUnwindListeners()) {
for (InstrumentationListener* listener : method_unwind_listeners_) {
- listener->MethodUnwind(thread, this_object, method, dex_pc);
+ if (listener != nullptr) {
+ listener->MethodUnwind(thread, this_object, method, dex_pc);
+ }
}
}
}
@@ -899,16 +902,19 @@
void Instrumentation::DexPcMovedEventImpl(Thread* thread, mirror::Object* this_object,
ArtMethod* method,
uint32_t dex_pc) const {
- std::shared_ptr<std::list<InstrumentationListener*>> original(dex_pc_listeners_);
- for (InstrumentationListener* listener : *original.get()) {
- listener->DexPcMoved(thread, this_object, method, dex_pc);
+ for (InstrumentationListener* listener : dex_pc_listeners_) {
+ if (listener != nullptr) {
+ listener->DexPcMoved(thread, this_object, method, dex_pc);
+ }
}
}
void Instrumentation::BackwardBranchImpl(Thread* thread, ArtMethod* method,
int32_t offset) const {
for (InstrumentationListener* listener : backward_branch_listeners_) {
- listener->BackwardBranch(thread, method, offset);
+ if (listener != nullptr) {
+ listener->BackwardBranch(thread, method, offset);
+ }
}
}
@@ -918,25 +924,29 @@
uint32_t dex_pc,
ArtMethod* callee) const {
for (InstrumentationListener* listener : invoke_virtual_or_interface_listeners_) {
- listener->InvokeVirtualOrInterface(thread, this_object, caller, dex_pc, callee);
+ if (listener != nullptr) {
+ listener->InvokeVirtualOrInterface(thread, this_object, caller, dex_pc, callee);
+ }
}
}
void Instrumentation::FieldReadEventImpl(Thread* thread, mirror::Object* this_object,
ArtMethod* method, uint32_t dex_pc,
ArtField* field) const {
- std::shared_ptr<std::list<InstrumentationListener*>> original(field_read_listeners_);
- for (InstrumentationListener* listener : *original.get()) {
- listener->FieldRead(thread, this_object, method, dex_pc, field);
+ for (InstrumentationListener* listener : field_read_listeners_) {
+ if (listener != nullptr) {
+ listener->FieldRead(thread, this_object, method, dex_pc, field);
+ }
}
}
void Instrumentation::FieldWriteEventImpl(Thread* thread, mirror::Object* this_object,
ArtMethod* method, uint32_t dex_pc,
ArtField* field, const JValue& field_value) const {
- std::shared_ptr<std::list<InstrumentationListener*>> original(field_write_listeners_);
- for (InstrumentationListener* listener : *original.get()) {
- listener->FieldWritten(thread, this_object, method, dex_pc, field, field_value);
+ for (InstrumentationListener* listener : field_write_listeners_) {
+ if (listener != nullptr) {
+ listener->FieldWritten(thread, this_object, method, dex_pc, field, field_value);
+ }
}
}
@@ -945,9 +955,10 @@
if (HasExceptionCaughtListeners()) {
DCHECK_EQ(thread->GetException(), exception_object);
thread->ClearException();
- std::shared_ptr<std::list<InstrumentationListener*>> original(exception_caught_listeners_);
- for (InstrumentationListener* listener : *original.get()) {
- listener->ExceptionCaught(thread, exception_object);
+ for (InstrumentationListener* listener : exception_caught_listeners_) {
+ if (listener != nullptr) {
+ listener->ExceptionCaught(thread, exception_object);
+ }
}
thread->SetException(exception_object);
}
diff --git a/runtime/instrumentation.h b/runtime/instrumentation.h
index 8dd2357..726cf1b 100644
--- a/runtime/instrumentation.h
+++ b/runtime/instrumentation.h
@@ -517,20 +517,25 @@
InstrumentationLevelTable requested_instrumentation_levels_ GUARDED_BY(Locks::mutator_lock_);
// The event listeners, written to with the mutator_lock_ exclusively held.
+ // Mutators must be able to iterate over these lists concurrently, that is, with listeners being
+ // added or removed while iterating. The modifying thread holds exclusive lock,
+ // so other threads cannot iterate (i.e. read the data of the list) at the same time but they
+ // do keep iterators that need to remain valid. This is the reason these listeners are std::list
+ // and not for example std::vector: the existing storage for a std::list does not move.
+ // Note that mutators cannot make a copy of these lists before iterating, as the instrumentation
+ // listeners can also be deleted concurrently.
+ // As a result, these lists are never trimmed. That's acceptable given the low number of
+ // listeners we have.
std::list<InstrumentationListener*> method_entry_listeners_ GUARDED_BY(Locks::mutator_lock_);
std::list<InstrumentationListener*> method_exit_listeners_ GUARDED_BY(Locks::mutator_lock_);
std::list<InstrumentationListener*> method_unwind_listeners_ GUARDED_BY(Locks::mutator_lock_);
std::list<InstrumentationListener*> backward_branch_listeners_ GUARDED_BY(Locks::mutator_lock_);
std::list<InstrumentationListener*> invoke_virtual_or_interface_listeners_
GUARDED_BY(Locks::mutator_lock_);
- std::shared_ptr<std::list<InstrumentationListener*>> dex_pc_listeners_
- GUARDED_BY(Locks::mutator_lock_);
- std::shared_ptr<std::list<InstrumentationListener*>> field_read_listeners_
- GUARDED_BY(Locks::mutator_lock_);
- std::shared_ptr<std::list<InstrumentationListener*>> field_write_listeners_
- GUARDED_BY(Locks::mutator_lock_);
- std::shared_ptr<std::list<InstrumentationListener*>> exception_caught_listeners_
- GUARDED_BY(Locks::mutator_lock_);
+ std::list<InstrumentationListener*> dex_pc_listeners_ GUARDED_BY(Locks::mutator_lock_);
+ std::list<InstrumentationListener*> field_read_listeners_ GUARDED_BY(Locks::mutator_lock_);
+ std::list<InstrumentationListener*> field_write_listeners_ GUARDED_BY(Locks::mutator_lock_);
+ std::list<InstrumentationListener*> exception_caught_listeners_ GUARDED_BY(Locks::mutator_lock_);
// The set of methods being deoptimized (by the debugger) which must be executed with interpreter
// only.
