From b7871c7368d19d344865e97de3e62f52f3092be4 Mon Sep 17 00:00:00 2001 From: Pauli Date: Mon, 11 Jun 2018 22:36:27 +0300 Subject: [PATCH 1/9] Console-posix: Use lowest possible nfds parameter to the select call --- library/Console-posix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/Console-posix.cpp b/library/Console-posix.cpp index ef74e67d2..9c244de10 100644 --- a/library/Console-posix.cpp +++ b/library/Console-posix.cpp @@ -157,7 +157,7 @@ namespace DFHack FD_SET(STDIN_FILENO, &descriptor_set); FD_SET(exit_pipe[0], &descriptor_set); int ret = TMP_FAILURE_RETRY( - select (FD_SETSIZE,&descriptor_set, NULL, NULL, NULL) + select (std::max(STDIN_FILENO,exit_pipe[0])+1,&descriptor_set, NULL, NULL, NULL) ); if(ret == -1) return false; From 19a169ba656a7c134b10d97fd8eb291ea25dcf98 Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 12 Jun 2018 17:33:12 +0300 Subject: [PATCH 2/9] Convert Core.cpp to use c++11 thread I noticed that tthread is missing some c++11 features that make thread handling code a bit easier. To be able to use those features I decided to convert Core.cpp to use equivalent standard classes. This patch has no functional changes. --- library/Core.cpp | 115 ++++++++++++++++---------------------- library/PluginManager.cpp | 13 ++--- library/include/Core.h | 20 +++---- 3 files changed, 61 insertions(+), 87 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 62ede1162..fccf3fa0e 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -77,7 +77,9 @@ using namespace DFHack; #include #include #include -#include "tinythread.h" +#include +#include +#include #include "md5wrapper.h" #include "SDL_events.h" @@ -86,7 +88,6 @@ using namespace DFHack; #include #endif -using namespace tthread; using namespace df::enums; using df::global::init; using df::global::world; @@ -96,40 +97,35 @@ using df::global::world; static bool parseKeySpec(std::string keyspec, int *psym, int *pmod, std::string *pfocus = NULL); size_t loadScriptFiles(Core* core, color_ostream& out, const vector& prefix, const std::string& folder); -struct Core::Cond +struct Core::Cond : public std::condition_variable { - Cond() + Cond() : + std::condition_variable{}, + predicate{false} { - predicate = false; - wakeup = new tthread::condition_variable(); } ~Cond() { - delete wakeup; } - bool Lock(tthread::mutex * m) + bool Lock(std::unique_lock& lock) { - while(!predicate) - { - wakeup->wait(*m); - } + wait(lock, [this]() -> bool {return this->predicate;}); predicate = false; return true; } bool Unlock() { predicate = true; - wakeup->notify_one(); + notify_one(); return true; } - tthread::condition_variable * wakeup; bool predicate; }; struct Core::Private { - tthread::mutex AccessMutex; - tthread::mutex StackMutex; + std::mutex AccessMutex; + std::mutex StackMutex; std::stack suspended_tools; Core::Cond core_cond; thread::id df_suspend_thread; @@ -510,7 +506,7 @@ static bool try_autocomplete(color_ostream &con, const std::string &first, std:: bool Core::addScriptPath(string path, bool search_before) { - lock_guard lock(*script_path_mutex); + lock_guard lock(script_path_mutex); vector &vec = script_paths[search_before ? 0 : 1]; if (std::find(vec.begin(), vec.end(), path) != vec.end()) return false; @@ -522,7 +518,7 @@ bool Core::addScriptPath(string path, bool search_before) bool Core::removeScriptPath(string path) { - lock_guard lock(*script_path_mutex); + lock_guard lock(script_path_mutex); bool found = false; for (int i = 0; i < 2; i++) { @@ -541,7 +537,7 @@ bool Core::removeScriptPath(string path) void Core::getScriptPaths(std::vector *dest) { - lock_guard lock(*script_path_mutex); + lock_guard lock(script_path_mutex); dest->clear(); string df_path = this->p->getPath(); for (auto it = script_paths[0].begin(); it != script_paths[0].end(); ++it) @@ -1480,10 +1476,15 @@ void fIOthread(void * iodata) } } -Core::Core() -{ - d = new Private(); +Core::Core() : + d{new Private}, + script_path_mutex{}, + HotkeyMutex{}, + HotkeyCond{}, + alias_mutex{}, + misc_data_mutex{} +{ // init the console. This must be always the first step! plug_mgr = 0; vif = 0; @@ -1495,10 +1496,6 @@ Core::Core() // set up hotkey capture hotkey_set = false; - HotkeyMutex = 0; - HotkeyCond = 0; - alias_mutex = 0; - misc_data_mutex = 0; last_world_data_ptr = NULL; last_local_map_ptr = NULL; last_pause_state = false; @@ -1508,7 +1505,6 @@ Core::Core() color_ostream::log_errors_to_stderr = true; - script_path_mutex = new mutex(); }; void Core::fatal (std::string output) @@ -1642,7 +1638,6 @@ bool Core::Init() // Init global object pointers df::global::InitGlobals(); - alias_mutex = new recursive_mutex(); cerr << "Initializing Console.\n"; // init the console. @@ -1732,7 +1727,6 @@ bool Core::Init() } // create mutex for syncing with interactive tasks - misc_data_mutex=new mutex(); cerr << "Initializing Plugins.\n"; // create plugin manager plug_mgr = new PluginManager(this); @@ -1741,21 +1735,16 @@ bool Core::Init() temp->core = this; temp->plug_mgr = plug_mgr; - HotkeyMutex = new mutex(); - HotkeyCond = new condition_variable(); - if (!is_text_mode || is_headless) { cerr << "Starting IO thread.\n"; // create IO thread - thread * IO = new thread(fIOthread, (void *) temp); - (void)IO; + new thread(fIOthread, (void *) temp); } else { cerr << "Starting dfhack.init thread.\n"; - thread * init = new thread(fInitthread, (void *) temp); - (void)init; + new thread(fInitthread, (void *) temp); } cerr << "Starting DF input capture thread.\n"; @@ -1840,28 +1829,21 @@ bool Core::Init() bool Core::setHotkeyCmd( std::string cmd ) { // access command - HotkeyMutex->lock(); - { - hotkey_set = true; - hotkey_cmd = cmd; - HotkeyCond->notify_all(); - } - HotkeyMutex->unlock(); + std::lock_guard lock(HotkeyMutex); + hotkey_set = true; + hotkey_cmd = cmd; + HotkeyCond.notify_all(); return true; } /// removes the hotkey command and gives it to the caller thread std::string Core::getHotkeyCmd( void ) { string returner; - HotkeyMutex->lock(); - while ( ! hotkey_set ) - { - HotkeyCond->wait(*HotkeyMutex); - } + std::unique_lock lock(HotkeyMutex); + HotkeyCond.wait(lock, [this]() -> bool {return this->hotkey_set;}); hotkey_set = false; returner = hotkey_cmd; hotkey_cmd.clear(); - HotkeyMutex->unlock(); return returner; } @@ -1887,25 +1869,22 @@ void Core::printerr(const char *format, ...) void Core::RegisterData( void *p, std::string key ) { - misc_data_mutex->lock(); + std::lock_guard lock(misc_data_mutex); misc_data_map[key] = p; - misc_data_mutex->unlock(); } void *Core::GetData( std::string key ) { - misc_data_mutex->lock(); + std::lock_guard lock(misc_data_mutex); std::map::iterator it=misc_data_map.find(key); if ( it != misc_data_map.end() ) { void *p=it->second; - misc_data_mutex->unlock(); return p; } else { - misc_data_mutex->unlock(); return 0;// or throw an error. } } @@ -1943,9 +1922,9 @@ void Core::Suspend() // wait until Core::Update() wakes up the tool { - lock_guard lock(d->AccessMutex); + unique_lock lock(d->AccessMutex); - nc->Lock(&d->AccessMutex); + nc->Lock(lock); assert(d->df_suspend_depth == 0); d->df_suspend_thread = tid; @@ -2135,11 +2114,11 @@ int Core::Update() Core::Cond * nc = d->suspended_tools.top(); d->suspended_tools.pop(); - lock_guard lock(d->AccessMutex); + std::unique_lock lock(d->AccessMutex); // wake tool nc->Unlock(); // wait for tool to wake us - d->core_cond.Lock(&d->AccessMutex); + d->core_cond.Lock(lock); // verify assert(d->df_suspend_depth == 0); // destroy condition @@ -2530,7 +2509,7 @@ bool Core::SelectHotkey(int sym, int modifiers) std::string cmd; { - tthread::lock_guard lock(*HotkeyMutex); + std::lock_guard lock(HotkeyMutex); // Check the internal keybindings std::vector &bindings = key_bindings[sym]; @@ -2629,7 +2608,7 @@ bool Core::ClearKeyBindings(std::string keyspec) if (!parseKeySpec(keyspec, &sym, &mod, &focus)) return false; - tthread::lock_guard lock(*HotkeyMutex); + std::lock_guard lock(HotkeyMutex); std::vector &bindings = key_bindings[sym]; for (int i = bindings.size()-1; i >= 0; --i) { @@ -2668,7 +2647,7 @@ bool Core::AddKeyBinding(std::string keyspec, std::string cmdline) if (binding.command.empty()) return false; - tthread::lock_guard lock(*HotkeyMutex); + std::lock_guard lock(HotkeyMutex); // Don't add duplicates std::vector &bindings = key_bindings[sym]; @@ -2692,7 +2671,7 @@ std::vector Core::ListKeyBindings(std::string keyspec) if (!parseKeySpec(keyspec, &sym, &mod, &focus)) return rv; - tthread::lock_guard lock(*HotkeyMutex); + std::lock_guard lock(HotkeyMutex); std::vector &bindings = key_bindings[sym]; for (int i = bindings.size()-1; i >= 0; --i) { @@ -2712,7 +2691,7 @@ std::vector Core::ListKeyBindings(std::string keyspec) bool Core::AddAlias(const std::string &name, const std::vector &command, bool replace) { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); if (!IsAlias(name) || replace) { aliases[name] = command; @@ -2723,7 +2702,7 @@ bool Core::AddAlias(const std::string &name, const std::vector &com bool Core::RemoveAlias(const std::string &name) { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); if (IsAlias(name)) { aliases.erase(name); @@ -2734,14 +2713,14 @@ bool Core::RemoveAlias(const std::string &name) bool Core::IsAlias(const std::string &name) { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); return aliases.find(name) != aliases.end(); } bool Core::RunAlias(color_ostream &out, const std::string &name, const std::vector ¶meters, command_result &result) { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); if (!IsAlias(name)) { return false; @@ -2756,13 +2735,13 @@ bool Core::RunAlias(color_ostream &out, const std::string &name, std::map> Core::ListAliases() { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); return aliases; } std::string Core::GetAliasCommand(const std::string &name, const std::string &default_) { - tthread::lock_guard lock(*alias_mutex); + std::lock_guard lock(alias_mutex); if (IsAlias(name)) return join_strings(" ", aliases[name]); else diff --git a/library/PluginManager.cpp b/library/PluginManager.cpp index 8e3f436cb..b17f8e214 100644 --- a/library/PluginManager.cpp +++ b/library/PluginManager.cpp @@ -49,7 +49,6 @@ using namespace DFHack; using namespace std; #include "tinythread.h" -using namespace tthread; #include @@ -83,8 +82,8 @@ struct Plugin::RefLock RefLock() { refcount = 0; - wakeup = new condition_variable(); - mut = new mutex(); + wakeup = new tthread::condition_variable(); + mut = new tthread::mutex(); } ~RefLock() { @@ -119,8 +118,8 @@ struct Plugin::RefLock wakeup->wait(*mut); } } - condition_variable * wakeup; - mutex * mut; + tthread::condition_variable * wakeup; + tthread::mutex * mut; int refcount; }; @@ -786,8 +785,8 @@ void Plugin::push_function(lua_State *state, LuaFunction *fn) PluginManager::PluginManager(Core * core) : core(core) { - plugin_mutex = new recursive_mutex(); - cmdlist_mutex = new mutex(); + plugin_mutex = new tthread::recursive_mutex(); + cmdlist_mutex = new tthread::mutex(); ruby = NULL; } diff --git a/library/include/Core.h b/library/include/Core.h index 7d51e7613..a8bbb9c41 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -34,6 +34,9 @@ distribution. #include "Console.h" #include "modules/Graphic.h" +#include +#include + #include "RemoteClient.h" #define DFH_MOD_SHIFT 1 @@ -42,13 +45,6 @@ distribution. struct WINDOW; -namespace tthread -{ - class mutex; - class condition_variable; - class thread; -} - namespace df { struct viewscreen; @@ -246,7 +242,7 @@ namespace DFHack DFHack::PluginManager * plug_mgr; std::vector script_paths[2]; - tthread::mutex *script_path_mutex; + std::mutex script_path_mutex; // hotkey-related stuff struct KeyBinding { @@ -261,11 +257,11 @@ namespace DFHack std::map hotkey_states; std::string hotkey_cmd; bool hotkey_set; - tthread::mutex * HotkeyMutex; - tthread::condition_variable * HotkeyCond; + std::mutex HotkeyMutex; + std::condition_variable HotkeyCond; std::map> aliases; - tthread::recursive_mutex * alias_mutex; + std::recursive_mutex alias_mutex; bool SelectHotkey(int key, int modifiers); @@ -280,7 +276,7 @@ namespace DFHack // Additional state change scripts std::vector state_change_scripts; - tthread::mutex * misc_data_mutex; + std::mutex misc_data_mutex; std::map misc_data_map; friend class CoreService; From 1acb60daa2cff9303aa11953d77131b7794da0fe Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 12 Jun 2018 17:53:43 +0300 Subject: [PATCH 3/9] Prevent data races during console/init thread shutdown There is a minor chance that console or init thread would access already freed memory when core is shutting down and cleaning up state. To avoid any danger of having random bugs caused by the potential data race I decided to make sure the shutdown code waits for the thread to exit first. Windows change is completely untested. It is purely based on msdn documentation. --- library/Console-posix.cpp | 18 ++++++++++-------- library/Console-windows.cpp | 5 ++++- library/Core.cpp | 21 ++++++++++++++++----- library/include/Core.h | 1 + 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/library/Console-posix.cpp b/library/Console-posix.cpp index 9c244de10..a2f59077d 100644 --- a/library/Console-posix.cpp +++ b/library/Console-posix.cpp @@ -730,8 +730,7 @@ Console::Console() } Console::~Console() { - if(inited) - shutdown(); + assert(!inited); if(wlock) delete wlock; if(d) @@ -768,11 +767,6 @@ bool Console::shutdown(void) if(!d) return true; lock_guard g(*wlock); - if(d->rawmode) - d->disable_raw(); - d->print("\n"); - inited = false; - // kill the thing close(d->exit_pipe[1]); return true; } @@ -854,8 +848,16 @@ int Console::lineedit(const std::string & prompt, std::string & output, CommandH { lock_guard g(*wlock); int ret = -2; - if(inited) + if(inited) { ret = d->lineedit(prompt,output,wlock,ch); + if (ret == -2) { + // kill the thing + if(d->rawmode) + d->disable_raw(); + d->print("\n"); + inited = false; + } + } return ret; } diff --git a/library/Console-windows.cpp b/library/Console-windows.cpp index 3caacd924..f3771f4ff 100644 --- a/library/Console-windows.cpp +++ b/library/Console-windows.cpp @@ -285,7 +285,10 @@ namespace DFHack INPUT_RECORD rec; DWORD count; lock->unlock(); - ReadConsoleInputA(console_in, &rec, 1, &count); + if (ReadConsoleInputA(console_in, &rec, 1, &count) != 0) { + lock->lock(); + return -2; + } lock->lock(); if (rec.EventType != KEY_EVENT || !rec.Event.KeyEvent.bKeyDown) continue; diff --git a/library/Core.cpp b/library/Core.cpp index fccf3fa0e..d584cb891 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -130,6 +130,7 @@ struct Core::Private Core::Cond core_cond; thread::id df_suspend_thread; int df_suspend_depth; + std::thread iothread; Private() { df_suspend_depth = 0; @@ -1476,6 +1477,12 @@ void fIOthread(void * iodata) } } +Core::~Core() +{ + if (d->iothread.joinable()) + con.shutdown(); + delete d; +} Core::Core() : d{new Private}, @@ -1739,12 +1746,12 @@ bool Core::Init() { cerr << "Starting IO thread.\n"; // create IO thread - new thread(fIOthread, (void *) temp); + d->iothread = std::thread{fIOthread, (void*)temp}; } else { - cerr << "Starting dfhack.init thread.\n"; - new thread(fInitthread, (void *) temp); + std::cerr << "Starting dfhack.init thread.\n"; + d->iothread = std::thread{fInitthread, (void*)temp}; } cerr << "Starting DF input capture thread.\n"; @@ -2339,9 +2346,14 @@ void Core::onStateChange(color_ostream &out, state_change_event event) handleLoadAndUnloadScripts(out, event); } -// FIXME: needs to terminate the IO threads and properly dismantle all the machinery involved. int Core::Shutdown ( void ) { + // Make sure the console thread shutdowns before clean up to avoid any + // unlikely data races. + if (d->iothread.joinable()) { + con.shutdown(); + d->iothread.join(); + } if(errorstate) return true; errorstate = 1; @@ -2358,7 +2370,6 @@ int Core::Shutdown ( void ) } allModules.clear(); memset(&(s_mods), 0, sizeof(s_mods)); - con.shutdown(); return -1; } diff --git a/library/include/Core.h b/library/include/Core.h index a8bbb9c41..6e3bd718e 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -198,6 +198,7 @@ namespace DFHack DFHack::Console con; Core(); + ~Core(); struct Private; Private *d; From 820b787cd008c5dbd8107b421795157907db4e2e Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 12 Jun 2018 17:58:45 +0300 Subject: [PATCH 4/9] Add multibyte character handling to posix console I noticed that multibyte characters can mess up the console state variables. I decided to add a minimal multibyte support to make sure the input only collects complete valid multibyte characters in case user enters them to console. This change assumes that UTF-32 has one to one mapping between printed characters and char32 indexes. But I remember reading there is a few rare corner cases with accents where character might require multiple 4byte characters too. But this patch at least changes correct handling from about 100 characters to 99% of unicode characters. --- library/Console-posix.cpp | 86 ++++++++++++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 19 deletions(-) diff --git a/library/Console-posix.cpp b/library/Console-posix.cpp index a2f59077d..6184bd011 100644 --- a/library/Console-posix.cpp +++ b/library/Console-posix.cpp @@ -60,6 +60,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include // George Vulov for MacOSX #ifndef __LINUX__ @@ -133,6 +134,42 @@ const char * getANSIColor(const int c) } } +std::u32string fromLocaleMB(const std::string& str) +{ + std::u32string rv; + char32_t ch; + size_t pos = 0; + ssize_t sz; + std::mbstate_t state{}; + while ((sz = std::mbrtoc32(&ch,&str[pos], str.size() - pos, &state)) != 0) { + if (sz == -1 || sz == -2) + break; + rv.push_back(ch); + if (sz == -3) + continue; + pos += sz; + } + rv.push_back('\0'); + return rv; +} + +std::string toLocaleMB(const std::u32string& wstr) +{ + std::stringstream ss{}; + char mb[MB_CUR_MAX]; + std::mbstate_t state{}; + const size_t err = -1; + for (char32_t ch: wstr) { + size_t sz = std::c32rtomb(mb, ch, &state); + if (sz == err) { + ss << '\0'; + break; + } + ss.write(mb, sz); + } + return ss.str(); +} + namespace DFHack { class Private @@ -364,7 +401,7 @@ namespace DFHack print("\n"); if(count != -1) { - output = raw_buffer; + output = toLocaleMB(raw_buffer); } return count; } @@ -414,26 +451,24 @@ namespace DFHack char seq[64]; int cols = get_columns(); int plen = prompt.size(); - const char * buf = raw_buffer.c_str(); int len = raw_buffer.size(); + int begin = 0; int cooked_cursor = raw_cursor; - // Use math! This is silly. - while((plen+cooked_cursor) >= cols) - { - buf++; - len--; - cooked_cursor--; - } - while (plen+len > cols) + if ((plen+cooked_cursor) >= cols) { - len--; + begin = plen+cooked_cursor-cols-1; + len -= plen+cooked_cursor-cols-1; + cooked_cursor -= plen+cooked_cursor-cols-1; } + if (plen+len > cols) + len -= plen+len - cols; + std::string mbstr = toLocaleMB(raw_buffer.substr(begin,len)); /* Cursor to left edge */ snprintf(seq,64,"\x1b[1G"); if (::write(STDIN_FILENO,seq,strlen(seq)) == -1) return; /* Write the prompt and the current buffer content */ if (::write(STDIN_FILENO,prompt.c_str(),plen) == -1) return; - if (::write(STDIN_FILENO,buf,len) == -1) return; + if (::write(STDIN_FILENO,mbstr.c_str(),mbstr.length()) == -1) return; /* Erase to right */ snprintf(seq,64,"\x1b[0K"); if (::write(STDIN_FILENO,seq,strlen(seq)) == -1) return; @@ -555,7 +590,7 @@ namespace DFHack { /* Update the current history entry before to * overwrite it with tne next one. */ - history[history_index] = raw_buffer; + history[history_index] = toLocaleMB(raw_buffer); /* Show the new entry */ history_index += (seq[1] == 'A') ? 1 : -1; if (history_index < 0) @@ -568,7 +603,7 @@ namespace DFHack history_index = history.size()-1; break; } - raw_buffer = history[history_index]; + raw_buffer = fromLocaleMB(history[history_index]); raw_cursor = raw_buffer.size(); prompt_refresh(); } @@ -672,15 +707,28 @@ namespace DFHack default: if (c >= 32) // Space { + char32_t c32; + char mb[MB_CUR_MAX]; + size_t count = 1; + mb[0] = c; + ssize_t sz; + std::mbstate_t state{}; + while ((sz = std::mbrtoc32(&c32,reinterpret_cast(&c),1, &state)) < 0) { + if (sz == -1 || sz == -3) + return -1; /* mbrtoc32 error (not valid utf-32 character */ + if(!read_char(c)) + return -2; + mb[count++] = c; + } if (raw_buffer.size() == size_t(raw_cursor)) { - raw_buffer.append(1,c); + raw_buffer.append(1,c32); raw_cursor++; if (plen+raw_buffer.size() < size_t(get_columns())) { /* Avoid a full update of the line in the * trivial case. */ - if (::write(fd,&c,1) == -1) return -1; + if (::write(fd,mb,count) == -1) return -1; } else { @@ -689,7 +737,7 @@ namespace DFHack } else { - raw_buffer.insert(raw_cursor,1,c); + raw_buffer.insert(raw_cursor,1,c32); raw_cursor++; prompt_refresh(); } @@ -712,8 +760,8 @@ namespace DFHack } state; bool in_batch; std::string prompt; // current prompt string - std::string raw_buffer; // current raw mode buffer - std::string yank_buffer; // last text deleted with Ctrl-K/Ctrl-U + std::u32string raw_buffer; // current raw mode buffer + std::u32string yank_buffer; // last text deleted with Ctrl-K/Ctrl-U int raw_cursor; // cursor position in the buffer // thread exit mechanism int exit_pipe[2]; From 84f8a75a2ec980af83afef5590b5f637a3b321bf Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 12 Jun 2018 20:25:40 +0300 Subject: [PATCH 5/9] Add cuchar fallback implementation for gcc 4 and 5 --- CMakeLists.txt | 6 ++++ library/Console-posix.cpp | 64 +++++++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6e8663f88..7a8cb42ea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,6 +137,12 @@ ${CMAKE_MODULE_PATH} # generates compile_commands.json, used for autocompletion by some editors SET(CMAKE_EXPORT_COMPILE_COMMANDS ON) +include(CheckIncludeFileCXX) +CHECK_INCLUDE_FILE_CXX("cuchar" HAVE_CUCHAR) +if(HAVE_CUCHAR) + add_definitions("-DHAVE_CUCHAR") +endif() + # mixing the build system with the source code is ugly and stupid. enforce the opposite :) if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_BINARY_DIR}") message(FATAL_ERROR "In-source builds are not allowed.") diff --git a/library/Console-posix.cpp b/library/Console-posix.cpp index 6184bd011..a06008cba 100644 --- a/library/Console-posix.cpp +++ b/library/Console-posix.cpp @@ -60,7 +60,11 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include +#ifdef HAVE_CUCHAR #include +#else +#include +#endif // George Vulov for MacOSX #ifndef __LINUX__ @@ -134,37 +138,63 @@ const char * getANSIColor(const int c) } } -std::u32string fromLocaleMB(const std::string& str) + +#ifdef HAVE_CUCHAR +// Use u32string for GCC 6 and later and msvc to allow potable implementation +using u32string = std::u32string; +using std::c32rtomb; +using std::mbrtoc32; +#else +// Fallback for gcc 4 and 5 that don't have cuchar header +// But wchar_t is 4 bytes that is a good fallback implementation +using u32string = std::wstring; +size_t mbrtoc32(u32string::value_type* c, + const char* s, + std::size_t n, + std::mbstate_t* ps) +{ + return std::mbrtowc(c, s, n, ps); +} + +size_t c32rtomb(char* mb, + u32string::value_type c, + std::mbstate_t* ps) +{ + return std::wcrtomb(mb, c, ps); +} +#endif + +//! Convert a locale defined multibyte coding to UTF-32 string for easier +//! character processing. +static u32string fromLocaleMB(const std::string& str) { - std::u32string rv; - char32_t ch; + u32string rv; + u32string::value_type ch; size_t pos = 0; ssize_t sz; std::mbstate_t state{}; - while ((sz = std::mbrtoc32(&ch,&str[pos], str.size() - pos, &state)) != 0) { + while ((sz = mbrtoc32(&ch,&str[pos], str.size() - pos, &state)) != 0) { if (sz == -1 || sz == -2) break; rv.push_back(ch); - if (sz == -3) + if (sz == -3) /* multi value character */ continue; pos += sz; } - rv.push_back('\0'); return rv; } -std::string toLocaleMB(const std::u32string& wstr) +//! Convert a UTF-32 string back to locale defined multibyte coding. +static std::string toLocaleMB(const u32string& wstr) { std::stringstream ss{}; char mb[MB_CUR_MAX]; std::mbstate_t state{}; const size_t err = -1; - for (char32_t ch: wstr) { - size_t sz = std::c32rtomb(mb, ch, &state); - if (sz == err) { - ss << '\0'; + for (auto ch: wstr) { + size_t sz = c32rtomb(mb, ch, &state); + if (sz == err) break; - } ss.write(mb, sz); } return ss.str(); @@ -707,13 +737,15 @@ namespace DFHack default: if (c >= 32) // Space { - char32_t c32; + u32string::value_type c32; char mb[MB_CUR_MAX]; size_t count = 1; mb[0] = c; ssize_t sz; std::mbstate_t state{}; - while ((sz = std::mbrtoc32(&c32,reinterpret_cast(&c),1, &state)) < 0) { + // Read all bytes belonging to a multi byte + // character starting from the first bye already red + while ((sz = mbrtoc32(&c32,&mb[count-1],1, &state)) < 0) { if (sz == -1 || sz == -3) return -1; /* mbrtoc32 error (not valid utf-32 character */ if(!read_char(c)) @@ -760,8 +792,8 @@ namespace DFHack } state; bool in_batch; std::string prompt; // current prompt string - std::u32string raw_buffer; // current raw mode buffer - std::u32string yank_buffer; // last text deleted with Ctrl-K/Ctrl-U + u32string raw_buffer; // current raw mode buffer + u32string yank_buffer; // last text deleted with Ctrl-K/Ctrl-U int raw_cursor; // cursor position in the buffer // thread exit mechanism int exit_pipe[2]; From 84b1361387c6b6b5aafbb246db5feb274b3464be Mon Sep 17 00:00:00 2001 From: Pauli Date: Wed, 13 Jun 2018 18:36:57 +0300 Subject: [PATCH 6/9] Make cuchar check test if c32rtomb is present in system Fix for mac build where cuchar is present but uchar.h is not. --- CMakeLists.txt | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7a8cb42ea..27b9ccc13 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,10 +137,19 @@ ${CMAKE_MODULE_PATH} # generates compile_commands.json, used for autocompletion by some editors SET(CMAKE_EXPORT_COMPILE_COMMANDS ON) -include(CheckIncludeFileCXX) -CHECK_INCLUDE_FILE_CXX("cuchar" HAVE_CUCHAR) -if(HAVE_CUCHAR) - add_definitions("-DHAVE_CUCHAR") +include(CheckCXXSourceCompiles) +CHECK_CXX_SOURCE_COMPILES(" +#include +#include +int main(void) { + char32_t in = 0; + char out[MB_CUR_MAX]; + std::mbstate_t state{}; + std::c32rtomb(out, in, &state); + return 0; +}" HAVE_CUCHAR2) +if(HAVE_CUCHAR2) + add_definitions("-DHAVE_CUCHAR") endif() # mixing the build system with the source code is ugly and stupid. enforce the opposite :) From 1fc37f8ddcf15130d2057e2f6ae46c46861ee27b Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 21 Jun 2018 16:57:36 +0300 Subject: [PATCH 7/9] Checke Console::lineedit error return values Console::lineedit can return -1 to indicate input error and -2 to indicate the program is closing. But most users don't check possible unusual return values which can lead to exit hang. --- library/LuaTools.cpp | 19 +++++++++++++------ library/lua/utils.lua | 27 +++++++++++++++------------ plugins/Brushes.h | 11 +++++++---- plugins/liquids.cpp | 5 +++-- plugins/mode.cpp | 19 ++++++++++--------- plugins/tiletypes.cpp | 5 +++-- 6 files changed, 51 insertions(+), 35 deletions(-) diff --git a/library/LuaTools.cpp b/library/LuaTools.cpp index 24d6917ff..8e05fbe22 100644 --- a/library/LuaTools.cpp +++ b/library/LuaTools.cpp @@ -283,9 +283,6 @@ static int lua_dfhack_is_interactive(lua_State *S) static int dfhack_lineedit_sync(lua_State *S, Console *pstream) { - if (!pstream) - return 2; - const char *prompt = luaL_optstring(S, 1, ">> "); const char *hfile = luaL_optstring(S, 2, NULL); @@ -299,7 +296,10 @@ static int dfhack_lineedit_sync(lua_State *S, Console *pstream) if (rv < 0) { lua_pushnil(S); - lua_pushstring(S, "input error"); + if (rv == -2) + lua_pushstring(S, "shutdown requested"); + else + lua_pushstring(S, "input error"); return 2; } else @@ -333,8 +333,11 @@ static int dfhack_lineedit(lua_State *S) lua_settop(S, 2); Console *pstream = get_console(S); - if (!pstream) + if (!pstream) { + lua_pushnil(S); + lua_pushstring(S, "no console"); return 2; + } lua_rawgetp(S, LUA_REGISTRYINDEX, &DFHACK_QUERY_COROTABLE_TOKEN); lua_rawgetp(S, -1, S); @@ -1058,7 +1061,11 @@ bool DFHack::Lua::RunCoreQueryLoop(color_ostream &out, lua_State *state, prompt = ">> "; std::string curline; - con.lineedit(prompt,curline,hist); + rv = con.lineedit(prompt,curline,hist); + if (rv < 0) { + rv = rv == -2 ? LUA_OK : LUA_ERRRUN; + break; + } hist.add(curline); { diff --git a/library/lua/utils.lua b/library/lua/utils.lua index a6007a08f..60830e82f 100644 --- a/library/lua/utils.lua +++ b/library/lua/utils.lua @@ -505,17 +505,17 @@ function prompt_yes_no(msg,default) prompt = prompt..' (y/n)[n]: ' end while true do - local rv = dfhack.lineedit(prompt) - if rv then - if string.match(rv,'^[Yy]') then - return true - elseif string.match(rv,'^[Nn]') then - return false - elseif rv == 'abort' then - qerror('User abort') - elseif rv == '' and default ~= nil then - return default - end + local rv,err = dfhack.lineedit(prompt) + if not rv then + qerror(err); + elseif string.match(rv,'^[Yy]') then + return true + elseif string.match(rv,'^[Nn]') then + return false + elseif rv == 'abort' then + qerror('User abort') + elseif rv == '' and default ~= nil then + return default end end end @@ -524,7 +524,10 @@ end function prompt_input(prompt,check,quit_str) quit_str = quit_str or '~~~' while true do - local rv = dfhack.lineedit(prompt) + local rv,err = dfhack.lineedit(prompt) + if not rv then + qerror(err); + end if rv == quit_str then qerror('User abort') end diff --git a/plugins/Brushes.h b/plugins/Brushes.h index 6095a03da..0ef9c147e 100644 --- a/plugins/Brushes.h +++ b/plugins/Brushes.h @@ -214,7 +214,7 @@ DFHack::command_result parseRectangle(DFHack::color_ostream & out, bool hasConsole = true) { using namespace DFHack; - int newWidth = 0, newHeight = 0, newZLevels = 0; + int newWidth = 0, newHeight = 0, newZLevels = 0, rv = 0; if (end > start + 1) { @@ -237,7 +237,8 @@ DFHack::command_result parseRectangle(DFHack::color_ostream & out, str.str(""); str << "Set range width <" << width << "> "; - con.lineedit(str.str(), command, hist); + if ((rv = con.lineedit(str.str(), command, hist)) < 0) + return rv == -2 ? CR_OK : CR_FAILURE; hist.add(command); newWidth = command.empty() ? width : atoi(command.c_str()); } else { @@ -251,7 +252,8 @@ DFHack::command_result parseRectangle(DFHack::color_ostream & out, str.str(""); str << "Set range height <" << height << "> "; - con.lineedit(str.str(), command, hist); + if ((rv = con.lineedit(str.str(), command, hist)) < 0) + return rv == -2 ? CR_OK : CR_FAILURE; hist.add(command); newHeight = command.empty() ? height : atoi(command.c_str()); } else { @@ -265,7 +267,8 @@ DFHack::command_result parseRectangle(DFHack::color_ostream & out, str.str(""); str << "Set range z-levels <" << zLevels << "> "; - con.lineedit(str.str(), command, hist); + if ((rv = con.lineedit(str.str(), command, hist)) < 0) + return rv == -2 ? CR_OK : CR_FAILURE; hist.add(command); newZLevels = command.empty() ? zLevels : atoi(command.c_str()); } else { diff --git a/plugins/liquids.cpp b/plugins/liquids.cpp index f0dcf1ce3..12e92a573 100644 --- a/plugins/liquids.cpp +++ b/plugins/liquids.cpp @@ -188,8 +188,9 @@ command_result df_liquids (color_ostream &out_, vector & parameters) std::stringstream str; print_prompt(str, cur_mode); str << "# "; - if(out.lineedit(str.str(),input,liquids_hist) == -1) - return CR_FAILURE; + int rv; + if((rv = out.lineedit(str.str(),input,liquids_hist)) < 0) + return rv == -2 ? CR_OK : CR_FAILURE; liquids_hist.add(input); commands.clear(); diff --git a/plugins/mode.cpp b/plugins/mode.cpp index 66fb488a6..5258e2007 100644 --- a/plugins/mode.cpp +++ b/plugins/mode.cpp @@ -96,6 +96,7 @@ command_result mode (color_ostream &out_, vector & parameters) string command = ""; bool set = false; bool abuse = false; + int rv = 0; t_gamemodes gm; for(auto iter = parameters.begin(); iter != parameters.end(); iter++) { @@ -139,9 +140,9 @@ command_result mode (color_ostream &out_, vector & parameters) string selected; input_again: CommandHistory hist; - out.lineedit("Enter new mode: ",selected, hist); - if(selected == "c") - return CR_OK; + rv = out.lineedit("Enter new mode: ",selected, hist); + if(rv < 0 || selected == "c") + return rv == -2 ? CR_OK : CR_FAILURE; const char * start = selected.c_str(); char * end = 0; select = strtol(start, &end, 10); @@ -178,14 +179,14 @@ command_result mode (color_ostream &out_, vector & parameters) { CommandHistory hist; string selected; - out.lineedit("Enter new game mode number (c for exit): ",selected, hist); - if(selected == "c") - return CR_OK; + rv = out.lineedit("Enter new game mode number (c for exit): ",selected, hist); + if(rv < 0 || selected == "c") + return rv == -2 ? CR_OK : CR_FAILURE; const char * start = selected.c_str(); gm.g_mode = (GameMode) strtol(start, 0, 10); - out.lineedit("Enter new game type number (c for exit): ",selected, hist); - if(selected == "c") - return CR_OK; + rv = out.lineedit("Enter new game type number (c for exit): ",selected, hist); + if(rv < 0 || selected == "c") + return rv == -2 ? CR_OK : CR_FAILURE; start = selected.c_str(); gm.g_type = (GameType) strtol(start, 0, 10); } diff --git a/plugins/tiletypes.cpp b/plugins/tiletypes.cpp index 74fcbe1b9..1e3362b0b 100644 --- a/plugins/tiletypes.cpp +++ b/plugins/tiletypes.cpp @@ -984,9 +984,10 @@ command_result df_tiletypes (color_ostream &out_, vector & parameters) printState(out); std::string input = ""; + int rv = 0; - if (out.lineedit("tiletypes> ",input,tiletypes_hist) == -1) - return CR_FAILURE; + if ((rv = out.lineedit("tiletypes> ",input,tiletypes_hist)) < 0) + return rv == -2 ? CR_OK : CR_FAILURE; tiletypes_hist.add(input); commands.clear(); From 0bc1db4f07aec9ffded06a4cc11cbd8f54b4c623 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 21 Jun 2018 17:13:22 +0300 Subject: [PATCH 8/9] Make sure hotkeythread exits before cleanup --- library/Core.cpp | 34 ++++++++++++++++++++++++++-------- library/include/Core.h | 9 +++++++-- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index d584cb891..d16b5378d 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -131,6 +131,7 @@ struct Core::Private thread::id df_suspend_thread; int df_suspend_depth; std::thread iothread; + std::thread hotkeythread; Private() { df_suspend_depth = 0; @@ -224,9 +225,10 @@ void fHKthread(void * iodata) cerr << "Hotkey thread has croaked." << endl; return; } - while(1) + bool keep_going = true; + while(keep_going) { - std::string stuff = core->getHotkeyCmd(); // waits on mutex! + std::string stuff = core->getHotkeyCmd(keep_going); // waits on mutex! if(!stuff.empty()) { color_ostream_proxy out(core->getConsole()); @@ -1479,6 +1481,11 @@ void fIOthread(void * iodata) Core::~Core() { + if (d->hotkeythread.joinable()) { + std::lock_guard lock(HotkeyMutex); + hotkey_set = SHUTDOWN; + HotkeyCond.notify_one(); + } if (d->iothread.joinable()) con.shutdown(); delete d; @@ -1502,7 +1509,7 @@ Core::Core() : memset(&(s_mods), 0, sizeof(s_mods)); // set up hotkey capture - hotkey_set = false; + hotkey_set = NO; last_world_data_ptr = NULL; last_local_map_ptr = NULL; last_pause_state = false; @@ -1756,8 +1763,7 @@ bool Core::Init() cerr << "Starting DF input capture thread.\n"; // set up hotkey capture - thread * HK = new thread(fHKthread, (void *) temp); - (void)HK; + d->hotkeythread = std::thread(fHKthread, (void *) temp); screen_window = new Windows::top_level_window(); screen_window->addChild(new Windows::dfhack_dummy(5,10)); started = true; @@ -1837,18 +1843,22 @@ bool Core::setHotkeyCmd( std::string cmd ) { // access command std::lock_guard lock(HotkeyMutex); - hotkey_set = true; + hotkey_set = SET; hotkey_cmd = cmd; HotkeyCond.notify_all(); return true; } /// removes the hotkey command and gives it to the caller thread -std::string Core::getHotkeyCmd( void ) +std::string Core::getHotkeyCmd( bool &keep_going ) { string returner; std::unique_lock lock(HotkeyMutex); HotkeyCond.wait(lock, [this]() -> bool {return this->hotkey_set;}); - hotkey_set = false; + if (hotkey_set == SHUTDOWN) { + keep_going = false; + return returner; + } + hotkey_set = NO; returner = hotkey_cmd; hotkey_cmd.clear(); return returner; @@ -2357,6 +2367,14 @@ int Core::Shutdown ( void ) if(errorstate) return true; errorstate = 1; + if (d->hotkeythread.joinable()) { + std::unique_lock hot_lock(HotkeyMutex); + hotkey_set = SHUTDOWN; + HotkeyCond.notify_one(); + } + + d->hotkeythread.join(); + CoreSuspendClaimer suspend; if(plug_mgr) { diff --git a/library/include/Core.h b/library/include/Core.h index 6e3bd718e..2621bac21 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -145,7 +145,7 @@ namespace DFHack /// sets the current hotkey command bool setHotkeyCmd( std::string cmd ); /// removes the hotkey command and gives it to the caller thread - std::string getHotkeyCmd( void ); + std::string getHotkeyCmd( bool &keep_going ); /// adds a named pointer (for later or between plugins) void RegisterData(void *p,std::string key); @@ -257,7 +257,12 @@ namespace DFHack std::map > key_bindings; std::map hotkey_states; std::string hotkey_cmd; - bool hotkey_set; + enum hotkey_set_t { + NO, + SET, + SHUTDOWN, + }; + hotkey_set_t hotkey_set; std::mutex HotkeyMutex; std::condition_variable HotkeyCond; From f6b0ac78196474c99f92c50bc275e4dd5855f3bc Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 21 Jun 2018 18:58:16 +0300 Subject: [PATCH 9/9] Refactor CoreSuspender to fix Console::lineedit exit hangs The old CoreSuspender requires processing from Core::Update to allow commands execute. But that causes issues if Core::Shutdown wants quarentee cleanup order with std::thread::join. Fixing shutdown ordering adds too many branches to already fairly complex code. I decided to try to refactor CoreSuspender to use simpler locking locking using a std::recusive_muted as primary synchronization primitive. To help control when Core::Update unlocks the primary mutex there is std::contition_variable_any and std::atomic queue lenght counter. The last state variable is std::atomic that is used to keep track of owner thread for Core::IsSuspended query. This should be merged only just after a release to make sure that it gets maximum testing in develop branch before next release. Fixes #1066 --- library/Core.cpp | 178 +++++++--------------------------- library/RemoteTools.cpp | 17 ++-- library/include/Core.h | 103 +++++++++++++++----- library/include/LuaTools.h | 2 +- library/include/RemoteTools.h | 1 + 5 files changed, 124 insertions(+), 177 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index d16b5378d..463175e0f 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -97,45 +97,18 @@ using df::global::world; static bool parseKeySpec(std::string keyspec, int *psym, int *pmod, std::string *pfocus = NULL); size_t loadScriptFiles(Core* core, color_ostream& out, const vector& prefix, const std::string& folder); -struct Core::Cond : public std::condition_variable -{ - Cond() : - std::condition_variable{}, - predicate{false} - { - } - ~Cond() - { - } - bool Lock(std::unique_lock& lock) - { - wait(lock, [this]() -> bool {return this->predicate;}); - predicate = false; - return true; - } - bool Unlock() - { - predicate = true; - notify_one(); - return true; - } - bool predicate; -}; +//! mainThreadSuspend keeps the main DF thread suspended from Core::Init to +//! thread exit. +template +static std::unique_lock& mainThreadSuspend(M& mutex) { + static thread_local std::unique_lock lock(mutex, std::defer_lock); + return lock; +} struct Core::Private { - std::mutex AccessMutex; - std::mutex StackMutex; - std::stack suspended_tools; - Core::Cond core_cond; - thread::id df_suspend_thread; - int df_suspend_depth; std::thread iothread; std::thread hotkeythread; - - Private() { - df_suspend_depth = 0; - } }; struct CommandDepthCounter @@ -1481,6 +1454,9 @@ void fIOthread(void * iodata) Core::~Core() { + if (mainThreadSuspend(CoreSuspendMutex).owns_lock()) + mainThreadSuspend(CoreSuspendMutex).unlock(); + if (d->hotkeythread.joinable()) { std::lock_guard lock(HotkeyMutex); hotkey_set = SHUTDOWN; @@ -1497,7 +1473,11 @@ Core::Core() : HotkeyMutex{}, HotkeyCond{}, alias_mutex{}, - misc_data_mutex{} + misc_data_mutex{}, + CoreSuspendMutex{}, + CoreWakeup{}, + ownerThread{}, + toolCount{0} { // init the console. This must be always the first step! plug_mgr = 0; @@ -1562,6 +1542,10 @@ bool Core::Init() if(errorstate) return false; + // Lock the CoreSuspendMutex until the thread exits or call Core::Shutdown + // Core::Update will temporary unlock when there is any commands queued + mainThreadSuspend(CoreSuspendMutex).lock(); + // Re-route stdout and stderr again - DF seems to set up stdout and // stderr.txt on Windows as of 0.43.05. Also, log before switching files to // make it obvious what's going on if someone checks the *.txt files. @@ -1908,57 +1892,7 @@ void *Core::GetData( std::string key ) bool Core::isSuspended(void) { - lock_guard lock(d->AccessMutex); - - return (d->df_suspend_depth > 0 && d->df_suspend_thread == this_thread::get_id()); -} - -void Core::Suspend() -{ - auto tid = this_thread::get_id(); - - // If recursive, just increment the count - { - lock_guard lock(d->AccessMutex); - - if (d->df_suspend_depth > 0 && d->df_suspend_thread == tid) - { - d->df_suspend_depth++; - return; - } - } - - // put the condition on a stack - Core::Cond *nc = new Core::Cond(); - - { - lock_guard lock2(d->StackMutex); - - d->suspended_tools.push(nc); - } - - // wait until Core::Update() wakes up the tool - { - unique_lock lock(d->AccessMutex); - - nc->Lock(lock); - - assert(d->df_suspend_depth == 0); - d->df_suspend_thread = tid; - d->df_suspend_depth = 1; - } -} - -void Core::Resume() -{ - auto tid = this_thread::get_id(); - lock_guard lock(d->AccessMutex); - - assert(d->df_suspend_depth > 0 && d->df_suspend_thread == tid); - (void)tid; - - if (--d->df_suspend_depth == 0) - d->core_cond.Unlock(); + return ownerThread.load() == std::this_thread::get_id(); } int Core::TileUpdate() @@ -1969,40 +1903,6 @@ int Core::TileUpdate() return true; } -int Core::ClaimSuspend(bool force_base) -{ - auto tid = this_thread::get_id(); - lock_guard lock(d->AccessMutex); - - if (force_base || d->df_suspend_depth <= 0) - { - assert(d->df_suspend_depth == 0); - - d->df_suspend_thread = tid; - d->df_suspend_depth = 1000000; - return 1000000; - } - else - { - assert(d->df_suspend_thread == tid); - return ++d->df_suspend_depth; - } -} - -void Core::DisclaimSuspend(int level) -{ - auto tid = this_thread::get_id(); - lock_guard lock(d->AccessMutex); - - assert(d->df_suspend_depth == level && d->df_suspend_thread == tid); - (void)tid; - - if (level == 1000000) - d->df_suspend_depth = 0; - else - --d->df_suspend_depth; -} - void Core::doUpdate(color_ostream &out, bool first_update) { Lua::Core::Reset(out, "DF code execution"); @@ -2122,27 +2022,9 @@ int Core::Update() doUpdate(out, first_update); } - // wake waiting tools - // do not allow more tools to join in while we process stuff here - lock_guard lock_stack(d->StackMutex); - - while (!d->suspended_tools.empty()) - { - Core::Cond * nc = d->suspended_tools.top(); - d->suspended_tools.pop(); - - std::unique_lock lock(d->AccessMutex); - // wake tool - nc->Unlock(); - // wait for tool to wake us - d->core_cond.Lock(lock); - // verify - assert(d->df_suspend_depth == 0); - // destroy condition - delete nc; - // check lua stack depth - Lua::Core::Reset(out, "suspend"); - } + // Let all commands run that require CoreSuspender + CoreWakeup.wait(mainThreadSuspend(CoreSuspendMutex), + [this]() -> bool {return this->toolCount.load() == 0;}); return 0; }; @@ -2358,15 +2240,20 @@ void Core::onStateChange(color_ostream &out, state_change_event event) int Core::Shutdown ( void ) { + if(errorstate) + return true; + errorstate = 1; + + // Make sure we release main thread if this is called from main thread + if (mainThreadSuspend(CoreSuspendMutex).owns_lock()) + mainThreadSuspend(CoreSuspendMutex).unlock(); + // Make sure the console thread shutdowns before clean up to avoid any // unlikely data races. if (d->iothread.joinable()) { con.shutdown(); - d->iothread.join(); } - if(errorstate) - return true; - errorstate = 1; + if (d->hotkeythread.joinable()) { std::unique_lock hot_lock(HotkeyMutex); hotkey_set = SHUTDOWN; @@ -2374,6 +2261,7 @@ int Core::Shutdown ( void ) } d->hotkeythread.join(); + d->iothread.join(); CoreSuspendClaimer suspend; if(plug_mgr) diff --git a/library/RemoteTools.cpp b/library/RemoteTools.cpp index 26833a3a3..1d45a19c0 100644 --- a/library/RemoteTools.cpp +++ b/library/RemoteTools.cpp @@ -652,8 +652,10 @@ static command_result SetUnitLabors(color_ostream &stream, const SetUnitLaborsIn return CR_OK; } -CoreService::CoreService() { - suspend_depth = 0; +CoreService::CoreService() : + suspend_depth{0}, + coreSuspender{nullptr} +{ // These 2 methods must be first, so that they get id 0 and 1 addMethod("BindMethod", &CoreService::BindMethod, SF_DONT_SUSPEND | SF_ALLOW_REMOTE); @@ -683,8 +685,7 @@ CoreService::CoreService() { CoreService::~CoreService() { - while (suspend_depth-- > 0) - Core::getInstance().Resume(); + delete coreSuspender; } command_result CoreService::BindMethod(color_ostream &stream, @@ -725,7 +726,8 @@ command_result CoreService::RunCommand(color_ostream &stream, command_result CoreService::CoreSuspend(color_ostream &stream, const EmptyMessage*, IntMessage *cnt) { - Core::getInstance().Suspend(); + if (suspend_depth == 0) + coreSuspender = new CoreSuspender(); cnt->set_value(++suspend_depth); return CR_OK; } @@ -735,8 +737,11 @@ command_result CoreService::CoreResume(color_ostream &stream, const EmptyMessage if (suspend_depth <= 0) return CR_WRONG_USAGE; - Core::getInstance().Resume(); cnt->set_value(--suspend_depth); + if (suspend_depth == 0) { + delete coreSuspender; + coreSuspender = nullptr; + } return CR_OK; } diff --git a/library/include/Core.h b/library/include/Core.h index 2621bac21..8c778fa08 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -34,8 +34,10 @@ distribution. #include "Console.h" #include "modules/Graphic.h" -#include +#include #include +#include +#include #include "RemoteClient.h" @@ -61,6 +63,11 @@ namespace DFHack class PluginManager; class Core; class ServerMain; + class CoreSuspender; + + namespace Lua { namespace Core { + DFHACK_EXPORT void Reset(color_ostream &out, const char *where); + } } namespace Windows { class df_window; @@ -129,10 +136,6 @@ namespace DFHack } /// check if the activity lock is owned by this thread bool isSuspended(void); - /// try to acquire the activity lock - void Suspend(void); - /// return activity lock - void Resume(void); /// Is everything OK? bool isValid(void) { return !errorstate; } @@ -203,10 +206,6 @@ namespace DFHack struct Private; Private *d; - friend class CoreSuspendClaimer; - int ClaimSuspend(bool force_base); - void DisclaimSuspend(int level); - bool Init(); int Update (void); int TileUpdate (void); @@ -285,32 +284,86 @@ namespace DFHack std::mutex misc_data_mutex; std::map misc_data_map; + /*! + * \defgroup core_suspend CoreSuspender state handling serialization to + * DF memory. + * \sa DFHack::CoreSuspender + * \{ + */ + std::recursive_mutex CoreSuspendMutex; + std::condition_variable_any CoreWakeup; + std::atomic ownerThread; + std::atomic toolCount; + //! \} + friend class CoreService; friend class ServerConnection; + friend class CoreSuspender; ServerMain *server; }; - class CoreSuspender { - Core *core; - public: - CoreSuspender() : core(&Core::getInstance()) { core->Suspend(); } - CoreSuspender(Core *core) : core(core) { core->Suspend(); } - ~CoreSuspender() { core->Resume(); } + template + struct ToolIncrement { + ToolIncrement(std::atomic& toolCount) { + toolCount += 1; + } }; - /** Claims the current thread already has the suspend lock. - * Strictly for use in callbacks from DF. + /*! + * CoreSuspender allows serialization to DF data with std::unique_lock like + * interface. It includes handling for recursive CoreSuspender calls and + * notification to main thread after all queue tools have been handled. + * + * State transitions are: + * - Startup setups Core::SuspendMutex to unlocked states + * - Core::Init locks Core::SuspendMutex until the thread exits or that thread + * calls Core::Shutdown or Core::~Core. + * - Other thread request core suspend by atomic incrementation of Core::toolCount + * and then locking Core::CoreSuspendMutex. After locking CoreSuspendMutex + * success callers exchange their std::thread::id to Core::ownerThread. + * - Core::Update() makes sure that queued tools are run when it calls + * Core::CoreWakup::wait. The wait keeps Core::CoreSuspendMutex unlocked + * and waits until Core::toolCount is reduced back to zero. + * - CoreSuspender::~CoreSuspender() first stores the previous Core::ownerThread + * back. In case of recursive call Core::ownerThread equals tid. If tis is + * zero then we are releasing the recursive_mutex which means suspend + * context is over. It is time to reset lua. + * The last step is to decrement Core::toolCount and wakeup main thread if + * no more tools are queued trying to acquire the + * Core::CoreSuspenderMutex. */ - class CoreSuspendClaimer { + class CoreSuspender : protected ToolIncrement, + public std::unique_lock { + using parent_t = std::unique_lock; Core *core; - int level; + std::thread::id tid; public: - CoreSuspendClaimer(bool base = false) : core(&Core::getInstance()) { - level = core->ClaimSuspend(base); - } - CoreSuspendClaimer(Core *core, bool base = false) : core(core) { - level = core->ClaimSuspend(base); + CoreSuspender() : CoreSuspender(&Core::getInstance()) { } + CoreSuspender(bool) : CoreSuspender(&Core::getInstance()) { } + CoreSuspender(Core* core, bool) : CoreSuspender(core) { } + CoreSuspender(Core* core) : + /* Increment the wait count */ + ToolIncrement{core->toolCount}, + /* Lock the core */ + parent_t{core->CoreSuspendMutex}, + core{core}, + /* Mark this thread to be the core owner */ + tid{core->ownerThread.exchange(std::this_thread::get_id())} + { } + ~CoreSuspender() { + /* Restore core owner to previous value */ + core->ownerThread.store(tid); + if (tid == std::thread::id{}) + Lua::Core::Reset(core->getConsole(), "suspend"); + /* Notify core to continue when all queued tools have completed + * 0 = None wants to own the core + * 1+ = There are tools waiting core access + * fetch_add returns old value before subtraction + */ + if (core->toolCount.fetch_add(-1) == 1) + core->CoreWakeup.notify_one(); } - ~CoreSuspendClaimer() { core->DisclaimSuspend(level); } }; + + using CoreSuspendClaimer = CoreSuspender; } diff --git a/library/include/LuaTools.h b/library/include/LuaTools.h index e1d828271..814afe179 100644 --- a/library/include/LuaTools.h +++ b/library/include/LuaTools.h @@ -395,7 +395,7 @@ namespace DFHack {namespace Lua { // Not exported; for use by the Core class bool Init(color_ostream &out); - void Reset(color_ostream &out, const char *where); + DFHACK_EXPORT void Reset(color_ostream &out, const char *where); // Events signalled by the core void onStateChange(color_ostream &out, int code); diff --git a/library/include/RemoteTools.h b/library/include/RemoteTools.h index ead1c0aa1..d7d4eb4fe 100644 --- a/library/include/RemoteTools.h +++ b/library/include/RemoteTools.h @@ -133,6 +133,7 @@ namespace DFHack class CoreService : public RPCService { int suspend_depth; + CoreSuspender* coreSuspender; static int doRunLuaFunction(lua_State *L); public: