From abae08a9fb596402eca1b36da722cbd384bb09f5 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Sun, 20 Jan 2013 22:23:27 +0100 Subject: [PATCH 01/13] create reader_data_t.interruptible flag and infrastructure to make it work. --- input.cpp | 2 +- reader.cpp | 25 +++++++++++++++++++++++-- reader.h | 14 +++++++++++++- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/input.cpp b/input.cpp index 5c178be8d..7ef8402a1 100644 --- a/input.cpp +++ b/input.cpp @@ -507,7 +507,7 @@ wint_t input_readch() /* Clear the interrupted flag */ - reader_interrupted(); + reader_reset_interrupted(); /* Search for sequence in mapping tables diff --git a/reader.cpp b/reader.cpp index f90df311f..b7d531125 100644 --- a/reader.cpp +++ b/reader.cpp @@ -323,6 +323,9 @@ public: /** Whether a screen reset is needed after a repaint. */ bool screen_reset_needed; + /** Whether the reader should exit on ^C. */ + bool interruptible; + /** Constructor */ reader_data_t() : allow_autosuggestion(0), @@ -339,7 +342,8 @@ public: next(0), search_mode(0), repaint_needed(0), - screen_reset_needed(0) + screen_reset_needed(0), + interruptible(0) { } }; @@ -373,7 +377,7 @@ static pid_t original_pid; /** This variable is set to true by the signal handler when ^C is pressed */ -static int interrupted=0; +static volatile int interrupted=0; /* @@ -632,11 +636,23 @@ static void remove_duplicates(std::vector &l) l.erase(std::unique(l.begin(), l.end()), l.end()); } + +void reader_reset_interrupted() +{ + interrupted = 0; +} + int reader_interrupted() { int res=interrupted; if (res) + { interrupted=0; + } + if (res && data && data->interruptible) + { + reader_exit(1, 0); + } return res; } @@ -2380,6 +2396,11 @@ void reader_set_test_function(int (*f)(const wchar_t *)) data->test_func = f; } +void reader_set_interruptible(bool i) +{ + data->interruptible = i; +} + void reader_import_history_if_necessary(void) { /* Import history from bash, etc. if our current history is empty */ diff --git a/reader.h b/reader.h index ba024e865..f94d8a896 100644 --- a/reader.h +++ b/reader.h @@ -111,9 +111,18 @@ void reader_set_buffer(const wcstring &b, size_t p); */ size_t reader_get_cursor_pos(); +/** + Clear the interrupted flag unconditionally without handling anything. The + flag could have been set e.g. when an interrupt arrived just as we were + ending an earlier \c reader_readline invocation but before the + \c is_interactive_read flag was cleared. +*/ +void reader_reset_interrupted(); + /** Return the value of the interrupted flag, which is set by the sigint - handler, and clear it if it was set. + handler, and clear it if it was set. If the current reader is interruptible, + call \c reader_exit(). */ int reader_interrupted(); @@ -181,6 +190,9 @@ void reader_set_right_prompt(const wcstring &prompt); /** Sets whether autosuggesting is allowed. */ void reader_set_allow_autosuggesting(bool flag); +/** Sets whether the reader should exit on ^C. */ +void reader_set_interruptible(bool flag); + /** Returns true if the shell is exiting, 0 otherwise. */ From 970d05df39e9a62f306e0147cd1732b3f11666f9 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Sun, 20 Jan 2013 23:35:41 +0100 Subject: [PATCH 02/13] make the `read` builtin respect ctrl-C --- builtin.cpp | 1 + reader.cpp | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/builtin.cpp b/builtin.cpp index d20dd27ea..3307431fb 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -2399,6 +2399,7 @@ static int builtin_read(parser_t &parser, wchar_t **argv) } /* No autosuggestions in builtin_read */ reader_set_allow_autosuggesting(false); + reader_set_interruptible(true); reader_set_buffer(commandline, wcslen(commandline)); proc_push_interactive(1); diff --git a/reader.cpp b/reader.cpp index b7d531125..49030aded 100644 --- a/reader.cpp +++ b/reader.cpp @@ -580,10 +580,23 @@ static void reader_kill(size_t begin_idx, size_t length, int mode, int newv) } + +/* + Called from a signal handler, so make sure to check \c data exists. + This is in fact racey as there is no guarantee that \c *data's members are + written to memory before \c data is. But signal handling is currently racey + anyway, so this should be fixed together with the rest of the signal + handling infrastructure. +*/ +static bool get_interruptible() +{ + return data ? data->interruptible : false; +} + /* This is called from a signal handler! */ void reader_handle_int(int sig) { - if (!is_interactive_read) + if (!is_interactive_read || get_interruptible()) { parser_t::skip_all_blocks(); } From c58278758c0aad2254aec671945b9e85057667ce Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 22 Jan 2013 10:27:14 +0100 Subject: [PATCH 03/13] reader_interrupted() should only be called on the main thread. fixes #537 --- wildcard.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wildcard.cpp b/wildcard.cpp index e21eb9548..1241e5e73 100644 --- a/wildcard.cpp +++ b/wildcard.cpp @@ -728,7 +728,7 @@ static int wildcard_expand_internal(const wchar_t *wc, // debug( 3, L"WILDCARD_EXPAND %ls in %ls", wc, base_dir ); - if (reader_interrupted()) + if (is_main_thread() && reader_interrupted()) { return -1; } From eb1c00c56b59daf1799fdb0d6ee8e34d76bde419 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 22 Jan 2013 11:00:02 +0100 Subject: [PATCH 04/13] fix comments on #516. Split `reader_interrupted` into a `reader_interrupted` and a `reader_reading_interrupted` --- input.cpp | 2 +- reader.cpp | 8 +++++++- reader.h | 8 +++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/input.cpp b/input.cpp index 7ef8402a1..1c60b93c2 100644 --- a/input.cpp +++ b/input.cpp @@ -292,7 +292,7 @@ static int interrupt_handler() /* Tell the reader an event occured */ - if (reader_interrupted()) + if (reader_reading_interrupted()) { /* Return 3, i.e. the character read by a Control-C. diff --git a/reader.cpp b/reader.cpp index 49030aded..2e4867b19 100644 --- a/reader.cpp +++ b/reader.cpp @@ -657,11 +657,17 @@ void reader_reset_interrupted() int reader_interrupted() { - int res=interrupted; + int res = interrupted; if (res) { interrupted=0; } + return res; +} + +int reader_reading_interrupted() +{ + int res = reader_interrupted(); if (res && data && data->interruptible) { reader_exit(1, 0); diff --git a/reader.h b/reader.h index f94d8a896..0c10fb695 100644 --- a/reader.h +++ b/reader.h @@ -111,6 +111,12 @@ void reader_set_buffer(const wcstring &b, size_t p); */ size_t reader_get_cursor_pos(); +/** + Return the value of the interrupted flag, which is set by the sigint + handler, and clear it if it was set. +*/ +int reader_interrupted(); + /** Clear the interrupted flag unconditionally without handling anything. The flag could have been set e.g. when an interrupt arrived just as we were @@ -124,7 +130,7 @@ void reader_reset_interrupted(); handler, and clear it if it was set. If the current reader is interruptible, call \c reader_exit(). */ -int reader_interrupted(); +int reader_reading_interrupted(); /** Read one line of input. Before calling this function, reader_push() From 55b3cf4627293ef91a061ba4c4bdc55810caacf5 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 22 Jan 2013 11:19:01 +0100 Subject: [PATCH 05/13] move work out of interrupt handler (which is safer as well) --- reader.cpp | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/reader.cpp b/reader.cpp index 2e4867b19..4e2685f17 100644 --- a/reader.cpp +++ b/reader.cpp @@ -581,22 +581,10 @@ static void reader_kill(size_t begin_idx, size_t length, int mode, int newv) } -/* - Called from a signal handler, so make sure to check \c data exists. - This is in fact racey as there is no guarantee that \c *data's members are - written to memory before \c data is. But signal handling is currently racey - anyway, so this should be fixed together with the rest of the signal - handling infrastructure. -*/ -static bool get_interruptible() -{ - return data ? data->interruptible : false; -} - /* This is called from a signal handler! */ void reader_handle_int(int sig) { - if (!is_interactive_read || get_interruptible()) + if (!is_interactive_read) { parser_t::skip_all_blocks(); } @@ -671,6 +659,7 @@ int reader_reading_interrupted() if (res && data && data->interruptible) { reader_exit(1, 0); + parser_t::skip_all_blocks(); } return res; } From a3b497b271fd65a7d823a5e17d76fa2b3e2ed5fd Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 22 Jan 2013 11:25:17 +0100 Subject: [PATCH 06/13] rename reader_data_t.interruptible to exit_on_interrupt --- builtin.cpp | 2 +- reader.cpp | 10 +++++----- reader.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index 3307431fb..70cb94a24 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -2399,7 +2399,7 @@ static int builtin_read(parser_t &parser, wchar_t **argv) } /* No autosuggestions in builtin_read */ reader_set_allow_autosuggesting(false); - reader_set_interruptible(true); + reader_set_exit_on_interrupt(true); reader_set_buffer(commandline, wcslen(commandline)); proc_push_interactive(1); diff --git a/reader.cpp b/reader.cpp index 4e2685f17..6b778eb5f 100644 --- a/reader.cpp +++ b/reader.cpp @@ -324,7 +324,7 @@ public: bool screen_reset_needed; /** Whether the reader should exit on ^C. */ - bool interruptible; + bool exit_on_interrupt; /** Constructor */ reader_data_t() : @@ -343,7 +343,7 @@ public: search_mode(0), repaint_needed(0), screen_reset_needed(0), - interruptible(0) + exit_on_interrupt(0) { } }; @@ -656,7 +656,7 @@ int reader_interrupted() int reader_reading_interrupted() { int res = reader_interrupted(); - if (res && data && data->interruptible) + if (res && data && data->exit_on_interrupt) { reader_exit(1, 0); parser_t::skip_all_blocks(); @@ -2404,9 +2404,9 @@ void reader_set_test_function(int (*f)(const wchar_t *)) data->test_func = f; } -void reader_set_interruptible(bool i) +void reader_set_exit_on_interrupt(bool i) { - data->interruptible = i; + data->exit_on_interrupt = i; } void reader_import_history_if_necessary(void) diff --git a/reader.h b/reader.h index 0c10fb695..0942bad08 100644 --- a/reader.h +++ b/reader.h @@ -197,7 +197,7 @@ void reader_set_right_prompt(const wcstring &prompt); void reader_set_allow_autosuggesting(bool flag); /** Sets whether the reader should exit on ^C. */ -void reader_set_interruptible(bool flag); +void reader_set_exit_on_interrupt(bool flag); /** Returns true if the shell is exiting, 0 otherwise. From b6bd6e399d97fb107bb05bc16df6ff32fb2f6877 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 22 Jan 2013 11:57:47 +0100 Subject: [PATCH 07/13] tweak reader interrupt behavior --- reader.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/reader.cpp b/reader.cpp index 6b778eb5f..07d58a31d 100644 --- a/reader.cpp +++ b/reader.cpp @@ -660,6 +660,9 @@ int reader_reading_interrupted() { reader_exit(1, 0); parser_t::skip_all_blocks(); + // We handled the interrupt ourselves, our caller doesn't need to + // handle it. + return 0; } return res; } From 70a75dc88aad2beb9c0007eba4af5154ebe58c8d Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Wed, 23 Jan 2013 00:19:29 +0100 Subject: [PATCH 08/13] implement reader_cancel_thread using __thread thread-local storage --- reader.cpp | 13 ++++++++++++- reader.h | 8 ++++++++ wildcard.cpp | 2 +- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/reader.cpp b/reader.cpp index 07d58a31d..8fe3a66d9 100644 --- a/reader.cpp +++ b/reader.cpp @@ -181,7 +181,10 @@ commence. #define SEARCH_FORWARD 1 /* Any time the contents of a buffer changes, we update the generation count. This allows for our background highlighting thread to notice it and skip doing work that it would otherwise have to do. */ -static unsigned int s_generation_count; +static volatile unsigned int s_generation_count; + +/* This threadlocal generation count is set when an autosuggestion background thread starts up, so it can easily check if the work it is doing is no longer useful. */ +static __thread unsigned int thread_generation_count; /* A color is an int */ typedef int color_t; @@ -667,6 +670,12 @@ int reader_reading_interrupted() return res; } +bool reader_cancel_thread() +{ + ASSERT_IS_BACKGROUND_THREAD(); + return s_generation_count != thread_generation_count; +} + void reader_write_title() { const wchar_t *title; @@ -1225,6 +1234,8 @@ struct autosuggestion_context_t return 0; } + thread_generation_count = generation_count; + /* Let's make sure we aren't using the empty string */ if (search_string.empty()) { diff --git a/reader.h b/reader.h index 0942bad08..f26f27405 100644 --- a/reader.h +++ b/reader.h @@ -132,6 +132,14 @@ void reader_reset_interrupted(); */ int reader_reading_interrupted(); +/** + Returns true if the current reader generation count does not equal the + generation count the current thread was started with. + Note: currently only valid for autocompletion threads! Other threads don't + set the threadlocal generation count when they start up. +*/ +bool reader_cancel_thread(); + /** Read one line of input. Before calling this function, reader_push() must have been called in order to set up a valid reader diff --git a/wildcard.cpp b/wildcard.cpp index 1241e5e73..b3d6e57aa 100644 --- a/wildcard.cpp +++ b/wildcard.cpp @@ -728,7 +728,7 @@ static int wildcard_expand_internal(const wchar_t *wc, // debug( 3, L"WILDCARD_EXPAND %ls in %ls", wc, base_dir ); - if (is_main_thread() && reader_interrupted()) + if (is_main_thread() ? reader_interrupted() : reader_cancel_thread()) { return -1; } From d12b1650aa0e58f79e3b2c05c16b58025e3ec0ea Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Wed, 23 Jan 2013 01:03:12 +0100 Subject: [PATCH 09/13] make threadlocal use conditional on configure check. NB: This revision mixes GPLv2-only and GPLv3+ code so should be considered proof of concept. --- configure.ac | 32 ++++++++++++++++++++++++++++++++ reader.cpp | 10 +++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index bf3defcff..0b4ba0dea 100644 --- a/configure.ac +++ b/configure.ac @@ -40,6 +40,38 @@ AC_SUBST(XSEL_BIN) AC_SUBST(XSEL_MAN_PATH) +# Copied from http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_tls.m4, +# This definition is licenced under GPLv3+. +AC_DEFUN([AX_TLS], [ + AC_MSG_CHECKING(for thread local storage (TLS) class) + AC_CACHE_VAL(ac_cv_tls, [ + ax_tls_keywords="__thread __declspec(thread) none" + for ax_tls_keyword in $ax_tls_keywords; do + AS_CASE([$ax_tls_keyword], + [none], [ac_cv_tls=none ; break], + [AC_TRY_COMPILE( + [#include + static void + foo(void) { + static ] $ax_tls_keyword [ int bar; + exit(1); + }], + [], + [ac_cv_tls=$ax_tls_keyword ; break], + ac_cv_tls=none + )]) + done + ]) + AC_MSG_RESULT($ac_cv_tls) + + AS_IF([test "$ac_cv_tls" != "none"], + AC_DEFINE_UNQUOTED([TLS], $ac_cv_tls, [If the compiler supports a TLS storage class define it to that here]) + m4_ifnblank([$1], [$1]), + m4_ifnblank([$2], [$2]) + ) +]) + +AX_TLS # # If needed, run autoconf to regenerate the configure file diff --git a/reader.cpp b/reader.cpp index 8fe3a66d9..6caca362b 100644 --- a/reader.cpp +++ b/reader.cpp @@ -184,7 +184,9 @@ commence. static volatile unsigned int s_generation_count; /* This threadlocal generation count is set when an autosuggestion background thread starts up, so it can easily check if the work it is doing is no longer useful. */ -static __thread unsigned int thread_generation_count; +#ifdef TLS +static TLS unsigned int thread_generation_count; +#endif /* A color is an int */ typedef int color_t; @@ -672,8 +674,12 @@ int reader_reading_interrupted() bool reader_cancel_thread() { +#ifdef TLS ASSERT_IS_BACKGROUND_THREAD(); return s_generation_count != thread_generation_count; +#else + return 0; +#endif } void reader_write_title() @@ -1234,7 +1240,9 @@ struct autosuggestion_context_t return 0; } +#ifdef TLS thread_generation_count = generation_count; +#endif /* Let's make sure we aren't using the empty string */ if (search_string.empty()) From 1e65e7c996d6d21ebe6b1ebdcdc3f5340913ec24 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Wed, 23 Jan 2013 01:29:24 +0100 Subject: [PATCH 10/13] extra comments --- reader.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/reader.h b/reader.h index f26f27405..f7cc7fdca 100644 --- a/reader.h +++ b/reader.h @@ -135,8 +135,10 @@ int reader_reading_interrupted(); /** Returns true if the current reader generation count does not equal the generation count the current thread was started with. - Note: currently only valid for autocompletion threads! Other threads don't + Note 1: currently only valid for autocompletion threads! Other threads don't set the threadlocal generation count when they start up. + Note 2: This function uses conditional compilation for thread-local storage. + If the compiler doesn't support that this function always returns `false`. */ bool reader_cancel_thread(); From e7b3f5745cd92c3d02a7cc8b8f8ae8a469975f49 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Thu, 24 Jan 2013 14:29:13 +0100 Subject: [PATCH 11/13] replace compiler-supported TLS with pthread_get/setspecific; remove GPLv3 code from two commits ago --- configure.ac | 32 -------------------------------- reader.cpp | 24 ++++++++++++------------ 2 files changed, 12 insertions(+), 44 deletions(-) diff --git a/configure.ac b/configure.ac index 0b4ba0dea..bf3defcff 100644 --- a/configure.ac +++ b/configure.ac @@ -40,38 +40,6 @@ AC_SUBST(XSEL_BIN) AC_SUBST(XSEL_MAN_PATH) -# Copied from http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_tls.m4, -# This definition is licenced under GPLv3+. -AC_DEFUN([AX_TLS], [ - AC_MSG_CHECKING(for thread local storage (TLS) class) - AC_CACHE_VAL(ac_cv_tls, [ - ax_tls_keywords="__thread __declspec(thread) none" - for ax_tls_keyword in $ax_tls_keywords; do - AS_CASE([$ax_tls_keyword], - [none], [ac_cv_tls=none ; break], - [AC_TRY_COMPILE( - [#include - static void - foo(void) { - static ] $ax_tls_keyword [ int bar; - exit(1); - }], - [], - [ac_cv_tls=$ax_tls_keyword ; break], - ac_cv_tls=none - )]) - done - ]) - AC_MSG_RESULT($ac_cv_tls) - - AS_IF([test "$ac_cv_tls" != "none"], - AC_DEFINE_UNQUOTED([TLS], $ac_cv_tls, [If the compiler supports a TLS storage class define it to that here]) - m4_ifnblank([$1], [$1]), - m4_ifnblank([$2], [$2]) - ) -]) - -AX_TLS # # If needed, run autoconf to regenerate the configure file diff --git a/reader.cpp b/reader.cpp index 6caca362b..244fe28de 100644 --- a/reader.cpp +++ b/reader.cpp @@ -45,6 +45,7 @@ commence. #include #include #include +#include #if HAVE_NCURSES_H #include @@ -183,10 +184,8 @@ commence. /* Any time the contents of a buffer changes, we update the generation count. This allows for our background highlighting thread to notice it and skip doing work that it would otherwise have to do. */ static volatile unsigned int s_generation_count; -/* This threadlocal generation count is set when an autosuggestion background thread starts up, so it can easily check if the work it is doing is no longer useful. */ -#ifdef TLS -static TLS unsigned int thread_generation_count; -#endif +/* This pthreads generation count is set when an autosuggestion background thread starts up, so it can easily check if the work it is doing is no longer useful. */ +static pthread_key_t generation_count_key; /* A color is an int */ typedef int color_t; @@ -674,12 +673,8 @@ int reader_reading_interrupted() bool reader_cancel_thread() { -#ifdef TLS ASSERT_IS_BACKGROUND_THREAD(); - return s_generation_count != thread_generation_count; -#else - return 0; -#endif + return ((size_t) s_generation_count) != (size_t) pthread_getspecific(generation_count_key); } void reader_write_title() @@ -800,6 +795,10 @@ static void exec_prompt() void reader_init() { + // We store unsigned ints cast to size_t's cast to void* in pthreads tls storage + assert(sizeof(size_t) >= sizeof(unsigned int)); + assert(sizeof(void*) >= sizeof(size_t)); + VOMIT_ON_FAILURE(pthread_key_create(&generation_count_key, NULL)); tcgetattr(0,&shell_modes); /* get the current terminal modes */ memcpy(&saved_modes, @@ -826,6 +825,7 @@ void reader_init() void reader_destroy() { tcsetattr(0, TCSANOW, &saved_modes); + pthread_key_delete(generation_count_key); } @@ -1240,9 +1240,9 @@ struct autosuggestion_context_t return 0; } -#ifdef TLS - thread_generation_count = generation_count; -#endif + // The manpage doesn't list any errors pthread_setspecific can return, + // so I'm assuming it can not fail. + pthread_setspecific(generation_count_key, (void*)(size_t) generation_count); /* Let's make sure we aren't using the empty string */ if (search_string.empty()) From 29fda9cb6c1866c447892185e5cd29b9f2358b11 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Thu, 24 Jan 2013 14:57:20 +0100 Subject: [PATCH 12/13] make the casting magic standards compliant and avoid compiler warnings; add error check --- reader.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/reader.cpp b/reader.cpp index 244fe28de..2328cb522 100644 --- a/reader.cpp +++ b/reader.cpp @@ -674,7 +674,7 @@ int reader_reading_interrupted() bool reader_cancel_thread() { ASSERT_IS_BACKGROUND_THREAD(); - return ((size_t) s_generation_count) != (size_t) pthread_getspecific(generation_count_key); + return (void*)(uintptr_t) s_generation_count != pthread_getspecific(generation_count_key); } void reader_write_title() @@ -795,9 +795,6 @@ static void exec_prompt() void reader_init() { - // We store unsigned ints cast to size_t's cast to void* in pthreads tls storage - assert(sizeof(size_t) >= sizeof(unsigned int)); - assert(sizeof(void*) >= sizeof(size_t)); VOMIT_ON_FAILURE(pthread_key_create(&generation_count_key, NULL)); tcgetattr(0,&shell_modes); /* get the current terminal modes */ @@ -1240,9 +1237,7 @@ struct autosuggestion_context_t return 0; } - // The manpage doesn't list any errors pthread_setspecific can return, - // so I'm assuming it can not fail. - pthread_setspecific(generation_count_key, (void*)(size_t) generation_count); + VOMIT_ON_FAILURE(pthread_setspecific(generation_count_key, (void*)(uintptr_t) generation_count)); /* Let's make sure we aren't using the empty string */ if (search_string.empty()) From 3f5c02bf926b238512f7275f3344944daf416229 Mon Sep 17 00:00:00 2001 From: Jan Kanis Date: Tue, 5 Feb 2013 21:18:22 +0100 Subject: [PATCH 13/13] rename reader_cancel_thread to reader_thread_job_is_stale, update comments --- reader.cpp | 4 ++-- reader.h | 4 +--- wildcard.cpp | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/reader.cpp b/reader.cpp index 2328cb522..385c5967a 100644 --- a/reader.cpp +++ b/reader.cpp @@ -181,7 +181,7 @@ commence. */ #define SEARCH_FORWARD 1 -/* Any time the contents of a buffer changes, we update the generation count. This allows for our background highlighting thread to notice it and skip doing work that it would otherwise have to do. */ +/* Any time the contents of a buffer changes, we update the generation count. This allows for our background highlighting thread to notice it and skip doing work that it would otherwise have to do. This variable should really be of some kind of interlocked or atomic type that guarantees we're not reading stale cache values. With C++11 we should use atomics, but until then volatile should work as well, at least on x86.*/ static volatile unsigned int s_generation_count; /* This pthreads generation count is set when an autosuggestion background thread starts up, so it can easily check if the work it is doing is no longer useful. */ @@ -671,7 +671,7 @@ int reader_reading_interrupted() return res; } -bool reader_cancel_thread() +bool reader_thread_job_is_stale() { ASSERT_IS_BACKGROUND_THREAD(); return (void*)(uintptr_t) s_generation_count != pthread_getspecific(generation_count_key); diff --git a/reader.h b/reader.h index f7cc7fdca..d0f878a30 100644 --- a/reader.h +++ b/reader.h @@ -137,10 +137,8 @@ int reader_reading_interrupted(); generation count the current thread was started with. Note 1: currently only valid for autocompletion threads! Other threads don't set the threadlocal generation count when they start up. - Note 2: This function uses conditional compilation for thread-local storage. - If the compiler doesn't support that this function always returns `false`. */ -bool reader_cancel_thread(); +bool reader_thread_job_is_stale(); /** Read one line of input. Before calling this function, reader_push() diff --git a/wildcard.cpp b/wildcard.cpp index b3d6e57aa..0673860b3 100644 --- a/wildcard.cpp +++ b/wildcard.cpp @@ -728,7 +728,7 @@ static int wildcard_expand_internal(const wchar_t *wc, // debug( 3, L"WILDCARD_EXPAND %ls in %ls", wc, base_dir ); - if (is_main_thread() ? reader_interrupted() : reader_cancel_thread()) + if (is_main_thread() ? reader_interrupted() : reader_thread_job_is_stale()) { return -1; }