Skip to content

🍒/austria/53e9ee3027db+10222764a9a3+14bd14f9f92f+453f8c87ff20 #4091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 18, 2022
Merged

🍒/austria/53e9ee3027db+10222764a9a3+14bd14f9f92f+453f8c87ff20 #4091

merged 4 commits into from
Mar 18, 2022

Conversation

JDevlieghere
Copy link

  • Revert "[lldb] Fix ^C handling in IOHandlerProcessSTDIO"
  • [lldb] Avoid data race in IOHandlerProcessSTDIO
  • [lldb] Fix ^C handling in IOHandlerProcessSTDIO
  • [lldb] Remove lldb/lldb subdir created by wrong patch level

labath and others added 4 commits March 18, 2022 09:18
@labath @JDevlieghere
Revert "[lldb] Fix ^C handling in IOHandlerProcessSTDIO"
0aadad8 
This reverts commit f93d861 -- looks
like a bad application of `patch -p<level>`

(cherry picked from commit 53e9ee3)
@JDevlieghere
[lldb] Avoid data race in IOHandlerProcessSTDIO
1de082f 
This patch fixes a data race in IOHandlerProcessSTDIO. The race is
happens between the main thread and the event handling thread. The main
thread is running the IOHandler (IOHandlerProcessSTDIO::Run()) when an
event comes in that makes us pop the process IO handler which involves
cancelling the IOHandler (IOHandlerProcessSTDIO::Cancel). The latter
calls SetIsDone(true) which modifies m_is_done. At the same time, we
have the main thread reading the variable through GetIsDone().

This patch avoids the race by using a mutex to synchronize the two
threads. On the event thread, in IOHandlerProcessSTDIO ::Cancel method,
we obtain the lock before changing the value of m_is_done. On the main
thread, in IOHandlerProcessSTDIO::Run(), we obtain the lock before
reading the value of m_is_done. Additionally, we delay calling SetIsDone
until after the loop exists, to avoid a potential race between the two
writes.

  Write of size 1 at 0x00010b66bb68 by thread T7 (mutexes: write M2862, write M718324145051843688):
    #0 lldb_private::IOHandler::SetIsDone(bool) IOHandler.h:90 (liblldb.15.0.0git.dylib:arm64+0x971d84)
    #1 IOHandlerProcessSTDIO::Cancel() Process.cpp:4382 (liblldb.15.0.0git.dylib:arm64+0x5ddfec)
    #2 lldb_private::Debugger::PopIOHandler(std::__1::shared_ptr<lldb_private::IOHandler> const&) Debugger.cpp:1156 (liblldb.15.0.0git.dylib:arm64+0x3cb2a8)
    #3 lldb_private::Debugger::RemoveIOHandler(std::__1::shared_ptr<lldb_private::IOHandler> const&) Debugger.cpp:1063 (liblldb.15.0.0git.dylib:arm64+0x3cbd2c)
    #4 lldb_private::Process::PopProcessIOHandler() Process.cpp:4487 (liblldb.15.0.0git.dylib:arm64+0x5c583c)
    #5 lldb_private::Debugger::HandleProcessEvent(std::__1::shared_ptr<lldb_private::Event> const&) Debugger.cpp:1549 (liblldb.15.0.0git.dylib:arm64+0x3ceabc)
    #6 lldb_private::Debugger::DefaultEventHandler() Debugger.cpp:1622 (liblldb.15.0.0git.dylib:arm64+0x3cf2c0)
    #7 std::__1::__function::__func<lldb_private::Debugger::StartEventHandlerThread()::$_2, std::__1::allocator<lldb_private::Debugger::StartEventHandlerThread()::$_2>, void* ()>::operator()() function.h:352 (liblldb.15.0.0git.dylib:arm64+0x3d1bd8)
    #8 lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) HostNativeThreadBase.cpp:62 (liblldb.15.0.0git.dylib:arm64+0x4c71ac)
    #9 lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(void*) HostThreadMacOSX.mm:18 (liblldb.15.0.0git.dylib:arm64+0x29ef544)

  Previous read of size 1 at 0x00010b66bb68 by main thread:
    #0 lldb_private::IOHandler::GetIsDone() IOHandler.h:92 (liblldb.15.0.0git.dylib:arm64+0x971db8)
    #1 IOHandlerProcessSTDIO::Run() Process.cpp:4339 (liblldb.15.0.0git.dylib:arm64+0x5ddc7c)
    #2 lldb_private::Debugger::RunIOHandlers() Debugger.cpp:982 (liblldb.15.0.0git.dylib:arm64+0x3cb48c)
    #3 lldb_private::CommandInterpreter::RunCommandInterpreter(lldb_private::CommandInterpreterRunOptions&) CommandInterpreter.cpp:3298 (liblldb.15.0.0git.dylib:arm64+0x506478)
    #4 lldb::SBDebugger::RunCommandInterpreter(bool, bool) SBDebugger.cpp:1166 (liblldb.15.0.0git.dylib:arm64+0x53604)
    #5 Driver::MainLoop() Driver.cpp:634 (lldb:arm64+0x100006294)
    #6 main Driver.cpp:853 (lldb:arm64+0x100007344)

Differential revision: https://reviews.llvm.org/D120762

(cherry picked from commit 1022276)
@labath @JDevlieghere
[lldb] Fix ^C handling in IOHandlerProcessSTDIO
4ce4f17 
D120762 accidentally moved the interrupt check into the block which was
reading stdio. This meant that a ^C only took effect after a regular
character has been pressed.

This patch fixes that and adds a (pexpect) test.

Differential Revision: https://reviews.llvm.org/D121912

(cherry picked from commit 14bd14f)
@JDevlieghere
[lldb] Remove lldb/lldb subdir created by wrong patch level
1172d84 
Bad application of patch -p<level>.

(cherry picked from commit 453f8c8)
@JDevlieghere
Copy link
Author

@swift-ci please test

@JDevlieghere JDevlieghere merged commit eb5ac23 into stable/20211026 Mar 18, 2022
@JDevlieghere JDevlieghere deleted the 🍒/austria/53e9ee3027db+10222764a9a3+14bd14f9f92f+453f8c87ff20 branch March 18, 2022 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JDevlieghere @labath