[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

This revision was automatically updated to reflect the committed changes. Closed by commit rG10222764a9a3: [lldb] Avoid data race in IOHandlerProcessSTDIO (authored by JDevlieghere). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D120762?vs=412479&id=412567#toc

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere added a comment. I think you might be looking at a combination of the old and the new patch. The new mutex protects the whole `Cancel` and `SetIsRunning`. I don't think this needs to be a recursive mutex because these functions are not calling each other. They both indirectly prote

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

clayborg added a comment. How is using m_mutex better than just using the std::atomic? Just protecting the modification of the value with a mutex doesn't seem like it would do much more than the atomic already did unless we are using the lock the protect the value for longer that just the modif

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good. Thanks for the test. Comment at: lldb/test/API/iohandler/stdio/TestIOHandlerProcessSTDIO.py:16 +self.build() +self.launch(executable=self.getBuild

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere updated this revision to Diff 412479. JDevlieghere added a comment. Unsurprisingly no tests failed with the typo. Added a test case to cover reading from stdin through the `IOHandlerProcessSTDIO`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120762/new/ https://reviews.ll

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

labath added a comment. In D120762#3353686 , @JDevlieghere wrote: > In D120762#3353655 , @labath wrote: > >> Are you sure that we're still sending input to the process (I'm not sure how >> much test coverage for

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere added a comment. In D120762#3353655 , @labath wrote: > Are you sure that we're still sending input to the process (I'm not sure how > much test coverage for this do we have)? I'll rerun the tests tomorrow with my typo and see if anything ca

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere updated this revision to Diff 412337. JDevlieghere marked an inline comment as done. JDevlieghere added a comment. Fix typo in `SetIsRunning`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120762/new/ https://reviews.llvm.org/D120762 Files: lldb/source/Target/Process.cpp

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere marked an inline comment as done. JDevlieghere added inline comments. Comment at: lldb/source/Target/Process.cpp:4343 +std::lock_guard guard(m_mutex); +if (GetIsDone()) + break; labath wrote: > I'm confused. How come this doe

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

labath added a comment. This seems like a better approach, but I'm not sure how does it actually work (see inline comment). Are you sure that we're still sending input to the process (I'm not sure how much test coverage for this do we have)? Comment at: lldb/source/Target/Pro

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere added a comment. In D120762#3352628 , @labath wrote: > These kinds of changes rarely fix a bug -- usually they just change a > (detectable) data race into some nondeterministic runtime behavior. > > That's because, even though e.g. `IsActive

[Lldb-commits] [PATCH] D120762: [lldb] Avoid data race in IOHandlerProcessSTDIO

JDevlieghere updated this revision to Diff 412316. JDevlieghere retitled this revision from "[lldb] Protect control variables in IOHandler with a mutex to avoid a data race" to "[lldb] Avoid data race in IOHandlerProcessSTDIO". JDevlieghere edited the summary of this revision. CHANGES SINCE LAST