[Lldb-commits] [PATCH] D121500: [lldb] Protect the debugger's output and error stream

Sat Mar 12 13:55:39 PST 2022

JDevlieghere added a comment.

In D121500#3377321 <https://reviews.llvm.org/D121500#3377321>, @labath wrote:

> Are you sure we don't want to handle this at a higher level? The way I understand it, the main reason for the existence of PrintAsync and StreamAsynchronousIO machinery is to provide precise control about when the various bits of output get printed. That's why the asynchronous output gets plumbed through the topmost iohandler and everything. In that setup, I think the right solution would be to set up some synchronization inside/near the iohandler object.

Yes, I considered doing the synchronization in the IO handler. We could have the IOHandler class hold onto a mutex. Locking it in `PrintAsync` takes care of the asynchronous output from the event thread. But everyone else is using the `File`, `StreamFile`, `FILE*` or file descriptor directly, so you need to hand out the mutex every time, in order to avoid colliding with the asynchronous output. That's certainly doable, but how long before someone forgets to lock the mutex? The benefit of doing it at a lower level is that as long as you're using the `Stream`, you're (somewhat) safe. Somewhat because you could still run into trouble when doing something that results in multiple calls to `Stream::WriteImpl`. The disadvantage is that we're still in the same boat for whoever is using the other accessors. I guess it's even slightly worse now, because there's no way to get the mutex.

I'm happy to update the patch if you think this is the way to go. Or maybe you had something else in mind?

It's possible and would solve the issues here. It also means that you have the same issue as we have for the debugger. That is, now every time you hand out the output File, FILE*, fd or StreamFileSP, you have to

> If we don't want to bother with that then, we might as well ditch the StreamAsynchronousIO completely.
>
> In D121500#3376501 <https://reviews.llvm.org/D121500#3376501>, @JDevlieghere wrote:
>
>> Yup, that's exactly what I started doing. I'm aware of at least one offender (https://reviews.llvm.org/D121502) because I just added that. But I'll see if there are other places that can be migrated to use the Stream instead of the File.
>
> Yeah, and I actually came very close to bringing that up (that it should be using StreamAsynchronousIO), but the patch was already submitted, and I did not want to bother with that. But if we're going to start introducing new concepts to make that work, then my calculus changes...

I don't think this patch and using `StreamAsynchronousIO` for the progress updates is mutually exclusive. It sounds like the right thing to use for the progress events regardless, so I'll update D121502 <https://reviews.llvm.org/D121502> to use that.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121500/new/

https://reviews.llvm.org/D121500