From 1a71ed813a8808032b73822605aec7f80f491f1f Mon Sep 17 00:00:00 2001 From: Jack Andersen Date: Fri, 1 Jun 2018 14:01:47 -1000 Subject: [PATCH] Fix TSan-reported race conditions --- lib/audiodev/LinuxMidi.hpp | 1 + lib/graphicsdev/GL.cpp | 2 +- lib/inputdev/HIDListenerUdev.cpp | 33 ++++++++++++++-------------- lib/win/ApplicationWin32.cpp | 2 ++ lib/win/Win32Common.hpp | 2 ++ lib/x11/ApplicationUnix.cpp | 2 +- lib/x11/ApplicationXlib.hpp | 4 +++- lib/x11/WindowXlib.cpp | 37 ++++++++++++++++---------------- logvisor | 2 +- 9 files changed, 45 insertions(+), 40 deletions(-) diff --git a/lib/audiodev/LinuxMidi.hpp b/lib/audiodev/LinuxMidi.hpp index eb5e082..b1d5674 100644 --- a/lib/audiodev/LinuxMidi.hpp +++ b/lib/audiodev/LinuxMidi.hpp @@ -70,6 +70,7 @@ struct LinuxMidi : BaseAudioVoiceEngine static void MIDIReceiveProc(snd_rawmidi_t* midi, const ReceiveFunctor& receiver) { + logvisor::RegisterThreadName("Boo MIDI"); snd_rawmidi_status_t* midiStatus; snd_rawmidi_status_malloc(&midiStatus); pthread_cleanup_push(MIDIFreeProc, midiStatus); diff --git a/lib/graphicsdev/GL.cpp b/lib/graphicsdev/GL.cpp index b10e563..9d0fce9 100644 --- a/lib/graphicsdev/GL.cpp +++ b/lib/graphicsdev/GL.cpp @@ -1329,7 +1329,7 @@ struct GLCommandQueue : IGraphicsCommandQueue #if _WIN32 std::string thrName = WCSTMBS(APP->getFriendlyName().data()) + " GL Rendering Thread"; #else - std::string thrName = std::string(APP->getFriendlyName()) + " GL Rendering Thread"; + std::string thrName = std::string(APP->getFriendlyName()) + " Render"; #endif logvisor::RegisterThreadName(thrName.c_str()); GLDataFactoryImpl* dataFactory = static_cast(self->m_parent->getDataFactory()); diff --git a/lib/inputdev/HIDListenerUdev.cpp b/lib/inputdev/HIDListenerUdev.cpp index 699560e..2be0924 100644 --- a/lib/inputdev/HIDListenerUdev.cpp +++ b/lib/inputdev/HIDListenerUdev.cpp @@ -1,6 +1,7 @@ #include "boo/inputdev/IHIDListener.hpp" #include "boo/inputdev/DeviceFinder.hpp" #include "boo/inputdev/HIDParser.hpp" +#include "logvisor/logvisor.hpp" #include #include #include @@ -30,15 +31,14 @@ class HIDListenerUdev final : public IHIDListener std::thread m_udevThread; bool m_scanningEnabled; - static void deviceConnected(HIDListenerUdev* listener, - udev_device* device) + void deviceConnected(udev_device* device) { - if (!listener->m_scanningEnabled) + if (!m_scanningEnabled) return; /* Prevent redundant registration */ const char* devPath = udev_device_get_syspath(device); - if (listener->m_finder._hasToken(devPath)) + if (m_finder._hasToken(devPath)) return; /* Filter to USB/BT */ @@ -140,21 +140,20 @@ class HIDListenerUdev final : public IHIDListener fprintf(stderr, "\n\n"); #endif - listener->m_finder._insertToken( - std::make_unique(type, vid, pid, manuf, product, devPath)); + m_finder._insertToken(std::make_unique(type, vid, pid, manuf, product, devPath)); } - static void deviceDisconnected(HIDListenerUdev* listener, - udev_device* device) + void deviceDisconnected(udev_device* device) { const char* devPath = udev_device_get_syspath(device); - listener->m_finder._removeToken(devPath); + m_finder._removeToken(devPath); } - static void _udevProc(HIDListenerUdev* listener) + void _udevProc() { - udev_monitor_enable_receiving(listener->m_udevMon); - int fd = udev_monitor_get_fd(listener->m_udevMon); + logvisor::RegisterThreadName("Boo udev"); + udev_monitor_enable_receiving(m_udevMon); + int fd = udev_monitor_get_fd(m_udevMon); while (true) { fd_set fds; @@ -168,14 +167,14 @@ class HIDListenerUdev final : public IHIDListener } int oldtype; pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldtype); - udev_device* dev = udev_monitor_receive_device(listener->m_udevMon); + udev_device* dev = udev_monitor_receive_device(m_udevMon); if (dev) { const char* action = udev_device_get_action(dev); if (!strcmp(action, "add")) - deviceConnected(listener, dev); + deviceConnected(dev); else if (!strcmp(action, "remove")) - deviceDisconnected(listener, dev); + deviceDisconnected(dev); udev_device_unref(dev); } pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &oldtype); @@ -205,7 +204,7 @@ public: m_scanningEnabled = false; /* Start hotplug thread */ - m_udevThread = std::thread(_udevProc, this); + m_udevThread = std::thread(std::bind(&HIDListenerUdev::_udevProc, this), this); } ~HIDListenerUdev() @@ -243,7 +242,7 @@ public: const char* devPath = udev_list_entry_get_name(uenumItem); udev_device* dev = udev_device_new_from_syspath(UDEV_INST, devPath); if (dev) - deviceConnected(this, dev); + deviceConnected(dev); udev_device_unref(dev); } udev_enumerate_unref(uenum); diff --git a/lib/win/ApplicationWin32.cpp b/lib/win/ApplicationWin32.cpp index ae7f8b2..f476864 100644 --- a/lib/win/ApplicationWin32.cpp +++ b/lib/win/ApplicationWin32.cpp @@ -441,10 +441,12 @@ public: /* SetFullscreen call for OpenGL window */ DoSetFullscreen(*reinterpret_cast(msg.wParam), msg.lParam); continue; +#if BOO_HAS_VULKAN case WM_USER+6: /* SetFullscreen call for Vulkan window */ DoSetFullscreen(*reinterpret_cast(msg.wParam), msg.lParam); continue; +#endif default: break; } } diff --git a/lib/win/Win32Common.hpp b/lib/win/Win32Common.hpp index c77650e..57ab0f0 100644 --- a/lib/win/Win32Common.hpp +++ b/lib/win/Win32Common.hpp @@ -57,6 +57,7 @@ static inline void SetFullscreen(OGLContext::Window& win, bool fs) g_nwcv.wait(lk); } +#if BOO_HAS_VULKAN static inline void SetFullscreen(boo::VulkanContext::Window& win, bool fs) { std::unique_lock lk(g_nwmt); @@ -64,6 +65,7 @@ static inline void SetFullscreen(boo::VulkanContext::Window& win, bool fs) g_nwcv.wait(lk); } #endif +#endif struct Boo3DAppContextWin32 : Boo3DAppContext { diff --git a/lib/x11/ApplicationUnix.cpp b/lib/x11/ApplicationUnix.cpp index 10ed5c3..056632d 100644 --- a/lib/x11/ApplicationUnix.cpp +++ b/lib/x11/ApplicationUnix.cpp @@ -64,7 +64,7 @@ int ApplicationRun(IApplication::EPlatformType platform, bool deepColor, bool singleInstance) { - std::string thrName = std::string(friendlyName) + " Main Thread"; + std::string thrName = std::string(friendlyName) + " Main"; logvisor::RegisterThreadName(thrName.c_str()); if (APP) return 1; diff --git a/lib/x11/ApplicationXlib.hpp b/lib/x11/ApplicationXlib.hpp index 1846e78..dc6e4c8 100644 --- a/lib/x11/ApplicationXlib.hpp +++ b/lib/x11/ApplicationXlib.hpp @@ -467,7 +467,7 @@ public: std::unique_lock innerLk(initmt); innerLk.unlock(); initcv.notify_one(); - std::string thrName = std::string(getFriendlyName()) + " Client Thread"; + std::string thrName = std::string(getFriendlyName()) + " Client"; logvisor::RegisterThreadName(thrName.c_str()); clientReturn = m_callback.appMain(this); pthread_kill(mainThread, SIGUSR2); @@ -574,6 +574,7 @@ public: std::shared_ptr newWindow(std::string_view title) { + XLockDisplay(m_xDisp); #if BOO_HAS_VULKAN std::shared_ptr newWindow = _WindowXlibNew(title, m_xDisp, m_xcbConn, m_xDefaultScreen, m_xIM, m_bestStyle, m_fontset, m_lastGlxCtx, (void*)m_getVkProc, &m_glContext); @@ -582,6 +583,7 @@ public: m_bestStyle, m_fontset, m_lastGlxCtx, nullptr, &m_glContext); #endif m_windows[(Window)newWindow->getPlatformHandle()] = newWindow; + XUnlockDisplay(m_xDisp); return newWindow; } diff --git a/lib/x11/WindowXlib.cpp b/lib/x11/WindowXlib.cpp index eb73928..cd3581d 100644 --- a/lib/x11/WindowXlib.cpp +++ b/lib/x11/WindowXlib.cpp @@ -298,6 +298,8 @@ struct GraphicsContextXlib : IGraphicsContext GLContext* m_glCtx; Display* m_xDisp; + std::mutex m_initmt; + std::condition_variable m_initcv; std::mutex m_vsyncmt; std::condition_variable m_vsynccv; @@ -506,15 +508,13 @@ public: /* Spawn vsync thread */ m_vsyncRunning = true; - std::mutex initmt; - std::condition_variable initcv; - std::unique_lock outerLk(initmt); + std::unique_lock outerLk(m_initmt); m_vsyncThread = std::thread([&]() { Display* vsyncDisp; GLXContext vsyncCtx; { - std::unique_lock innerLk(initmt); + std::unique_lock innerLk(m_initmt); vsyncDisp = XOpenDisplay(0); if (!vsyncDisp) @@ -531,7 +531,7 @@ public: if (!glXMakeCurrent(vsyncDisp, DefaultRootWindow(vsyncDisp), vsyncCtx)) Log.report(logvisor::Fatal, "unable to make vsync context current"); } - initcv.notify_one(); + m_initcv.notify_one(); while (m_vsyncRunning) { @@ -549,7 +549,7 @@ public: XUnlockDisplay(vsyncDisp); XCloseDisplay(vsyncDisp); }); - initcv.wait(outerLk); + m_initcv.wait(outerLk); XUnlockDisplay(m_xDisp); m_commandQueue = _NewGLCommandQueue(this, m_glCtx); @@ -642,7 +642,7 @@ struct GraphicsContextXlibVulkan : GraphicsContextXlib std::unique_ptr m_commandQueue; std::thread m_vsyncThread; - bool m_vsyncRunning; + std::atomic_bool m_vsyncRunning; static void ThrowIfFailed(VkResult res) { @@ -676,9 +676,9 @@ public: m_surface = VK_NULL_HANDLE; } - if (m_vsyncRunning) + if (m_vsyncRunning.load()) { - m_vsyncRunning = false; + m_vsyncRunning.store(false); if (m_vsyncThread.joinable()) m_vsyncThread.join(); } @@ -832,16 +832,15 @@ public: m_ctx->initSwapChain(*m_windowCtx, m_surface, m_format, m_colorspace); /* Spawn vsync thread */ - m_vsyncRunning = true; - std::mutex initmt; - std::condition_variable initcv; - std::unique_lock outerLk(initmt); + m_vsyncRunning.store(true); + std::unique_lock outerLk(m_initmt); m_vsyncThread = std::thread([&]() { + logvisor::RegisterThreadName("Boo VSync"); Display* vsyncDisp; GLXContext vsyncCtx; { - std::unique_lock innerLk(initmt); + std::unique_lock innerLk(m_initmt); vsyncDisp = XOpenDisplay(0); if (!vsyncDisp) @@ -858,9 +857,9 @@ public: if (!glXMakeCurrent(vsyncDisp, DefaultRootWindow(vsyncDisp), vsyncCtx)) Log.report(logvisor::Fatal, "unable to make vsync context current"); } - initcv.notify_one(); + m_initcv.notify_one(); - while (m_vsyncRunning) + while (m_vsyncRunning.load()) { unsigned int sync; int err = glXWaitVideoSyncSGI(1, 0, &sync); @@ -874,7 +873,7 @@ public: XUnlockDisplay(vsyncDisp); XCloseDisplay(vsyncDisp); }); - initcv.wait(outerLk); + m_initcv.wait(outerLk); m_dataFactory = _NewVulkanDataFactory(this, m_ctx); m_commandQueue = _NewVulkanCommandQueue(m_ctx, m_ctx->m_windows[m_parentWindow].get(), this); @@ -1118,7 +1117,9 @@ public: void setCallback(IWindowCallback* cb) { + XLockDisplay(m_xDisp); m_callback = cb; + XUnlockDisplay(m_xDisp); } void closeWindow() @@ -2064,11 +2065,9 @@ std::shared_ptr _WindowXlibNew(std::string_view title, int defaultScreen, XIM xIM, XIMStyle bestInputStyle, XFontSet fontset, GLXContext lastCtx, void* vulkanHandle, GLContext* glCtx) { - XLockDisplay(display); std::shared_ptr ret = std::make_shared(title, display, xcbConn, defaultScreen, xIM, bestInputStyle, fontset, lastCtx, vulkanHandle, glCtx); - XUnlockDisplay(display); return ret; } diff --git a/logvisor b/logvisor index 2352699..82f1df9 160000 --- a/logvisor +++ b/logvisor @@ -1 +1 @@ -Subproject commit 2352699c6598866548b81c3886337b4a8bb91472 +Subproject commit 82f1df9c40edc99a2d68499f363161e5b0bef849