Page MenuHomePhabricator

Fix a race in Broadcaster/Listener interaction
ClosedPublic

Authored by labath on Aug 11 2016, 7:01 AM.

Details

Summary

The following problem was occuring:

  • broadcaster B had two listeners: L1 and L2 (thread T1)
  • (T1) B has started to broadcast an event, it has locked a shared_ptr to L1 (in ListenerIterator())
  • on another thread T2 the penultimate reference to L1 was destroyed (the transient object in B is now the last reference)
  • (T2) the last reference to L2 was destroyed as well
  • (T1) B has finished broadcasting the event to L1 and destroyed the last shared_ptr
  • (T1) this triggered the destructor, which called into B->RemoveListener()
  • (T1) all pointers in the m_listeners list were now stale, so RemoveListener emptied the list
  • (T1) Eventually control returned to the ListenerIterator() for doing broadcasting, which was still in the middle of iterating through the list
  • (T1) Only now, it was holding onto a dangling iterator. BOOM.

I fix this issue by making sure nothing can interfere with the
iterate-and-remove-expired-pointers loop, by moving this logic into a single function, which
first locks (or clears) the whole list and then returns the list of valid and locked Listeners
for further processing. Instead of std::list I use an llvm::SmallVector which should hopefully
offset the fact that we create a copy of the list for the common case where we have only a few
listeners (no heap allocations).

A slight difference in behaviour is that now RemoveListener does not remove an element from the
list -- it only sets it's mask to 0, which means it will be removed during the next iteration of
GetListeners(). This is purely an implementation detail and it should not be externally
noticable.

I was not able to reproduce this bug reliably without inserting sleep statements into the code,
so I do not add a test for it. Instead, I add some unit tests for the functions that I do modify.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 67686.Aug 11 2016, 7:01 AM
labath retitled this revision from to Fix a race in Broadcaster/Listener interaction.
labath updated this object.
labath added reviewers: clayborg, jingham.
labath added subscribers: lldb-commits, tberghammer.
clayborg edited edge metadata.Aug 11 2016, 2:34 PM

My only issue with this patch is that is uses llvm::SmallVector. Although the class is nice, we don't have visibility into this class when debugging like we do when we use STL collections. I would rather not move to SmallVector if we don't need to. Jim, thoughts?

jingham edited edge metadata.EditedAug 11 2016, 3:55 PM

The patch seems correct to me.

I don't have a strong opinion about std::vector vrs. SmallVector. These are temporary objects, so the size of the container doesn't matter, and I doubt this code is hot enough in normal lldb sessions that the difference in performance between the two will matter. Maybe the SmallVector data formatter (llvm/utils/lldbDataFormatters.py) works, or if it doesn't we should fix it?

clayborg accepted this revision.Aug 11 2016, 4:06 PM
clayborg edited edge metadata.

Ok. Jim says the patch is OK, so I will OK it also.

This revision is now accepted and ready to land.Aug 11 2016, 4:06 PM

Jim, just to double-check. Was that an "approve" then?

My feeling is that the lack of visibility into the SmallVector, means more that we should resurrect the formatter than that we should stop using it. Especially considering the whole "let's integrate closer with llvm" idea. (Although I agree than in this case in probably doesn't matter much.

jingham accepted this revision.Aug 12 2016, 10:10 AM
jingham edited edge metadata.

Yes, sorry. I seem to have trouble with remembering the Action dropdown... Thanks for figuring this out - and apologies for any hair loss we inadvertently caused you!

I doubt it is important to use SmallVector rather than std::vector in this case, but not doing it because the thing is opaque does not seem the right deciding factor. There will clearly be cases where it is beneficial. Along with all the other testing improvements we need to do in our spare time, it might be good to write some tests for these formatters so they don't regress if they have.

gdb's build process writes a .gdbinit file into the build products that set up a convenient environment for debugging gdb with gdb. To use the gdb ones, you have to cd into the gdb directory, and debug from there. That works but is a little inelegant. I don't have a better idea right now, but it would be cool to come up with a way to do the same thing in lldb, so that if you're debugging lldb you get both useful formatters for lldb & llvm/clang.

Another in the continuing series of "What other people should do with their spare time..."

Jim

Yes, sorry. I seem to have trouble with remembering the Action dropdown... Thanks for figuring this out - and apologies for any hair loss we inadvertently caused you!

No worries.

I doubt it is important to use SmallVector rather than std::vector in this case, but not doing it because the thing is opaque does not seem the right deciding factor. There will clearly be cases where it is beneficial. Along with all the other testing improvements we need to do in our spare time, it might be good to write some tests for these formatters so they don't regress if they have.

gdb's build process writes a .gdbinit file into the build products that set up a convenient environment for debugging gdb with gdb. To use the gdb ones, you have to cd into the gdb directory, and debug from there. That works but is a little inelegant. I don't have a better idea right now, but it would be cool to come up with a way to do the same thing in lldb, so that if you're debugging lldb you get both useful formatters for lldb & llvm/clang.

Another in the continuing series of "What other people should do with their spare time..."

:)

The .lldbinit thing would work for me pretty well. When doing debugging from the console, I generally cd into the build directory already. If xcode has some "initial debugger commands" option we could use it to load that file and make that use case work as well. I'll see if I can get around to doing that.

This revision was automatically updated to reflect the committed changes.