[Lldb-commits] [PATCH] D48463: Prevent dead locking when calling PrintAsync

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/D48463#1145434, @teemperor wrote:

> Well in theory we could poke more holes in our guard from some nested function, but this only fixes the deadlock symptom. PrintAsync would still be blocking whenever the mutex is hold by someone.


Yes, but for me the real question is do really need to be calling PrintAsync while holding the output mutex. The output mutex is only used within the editline support code, and I think it's fair to require that this code be wary of when it is printing stuff to stdout. Making sure the mutex is dropped (and the rest of the output machinery is in a sane state) when calling "foreign" code is a part of its job.

"Poking holes" in the mutex, as you have so aptly phrased it, is not nice, but neither is your solution (you're doing the exact same thing with the `DisableMessageDelayScope` object), and I'd rather have one sub-optimal solution than two.

Instead of coming up with another solution which does something-similar-but-not-quite, I think we should invest more time improving the existing solution. For example the `DisableMessageDelayScope` approach would be useful for the scoped disabling of the output mutex. This will make it easy to unlock the mutex in any editline callback where we deem it safe to print text (and any place where it isn't safe to print text shouldn't be calling random code anyway). We're never going to be able to have a nice scoped locking strategy for this because the thing we want to lock is not scoped, but this should improve the situation somewhat. It will also make the thing fully contained inside the Editline class, instead of spreading it out over Core and Host modules. That also means the solution will be unit-testable, at least to some extent (right now, you don't have any tests for this code)



================
Comment at: source/Host/common/Editline.cpp:14
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
----------------
This is wrong layering. Editline should not know anything about the debugger. If we really want to proceed with this, then this needs to be abstracted and re-layered. However, I don't think that is really necessary. More on that below.


https://reviews.llvm.org/D48463