From 36ef521c0e08e4920b29a3f72e337f21a9b5b807 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Sat, 26 Apr 2014 17:36:20 +0200 Subject: [PATCH 1/5] Fix filehandle leak in proc_get_jiffies --- proc.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/proc.cpp b/proc.cpp index 2ea0d1bd4..66a258270 100644 --- a/proc.cpp +++ b/proc.cpp @@ -845,15 +845,16 @@ unsigned long proc_get_jiffies(process_t *p) &cnswap, &exit_signal, &processor ); + /* + Don't need to check exit status of fclose on read-only streams + */ + fclose(f); + if (count < 17) { return 0; } - /* - Don't need to check exit status of fclose on read-only streams - */ - fclose(f); return utime+stime+cutime+cstime; } From 58ebdd4a7edf32a6db5f44f9a8bea45f6d12baa5 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 27 Apr 2014 17:23:19 -0700 Subject: [PATCH 2/5] Attempt to silence some warnings --- builtin.cpp | 4 ++-- env_universal_common.cpp | 2 +- fallback.cpp | 1 + input.cpp | 2 +- iothread.cpp | 13 +++++++++---- parse_tree.cpp | 2 +- parser.cpp | 3 +-- proc.cpp | 2 +- screen.cpp | 2 +- 9 files changed, 18 insertions(+), 13 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index 8a81648ee..70829acb4 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -428,7 +428,7 @@ static void builtin_bind_list(const wchar_t *bind_mode) if (input_terminfo_get_name(seq, tname)) { append_format(stdout_buffer, L"bind -k %ls -M %ls -m %ls", tname.c_str(), mode.c_str(), sets_mode.c_str()); - for (int i = 0; i < ecmds.size(); i++) + for (size_t i = 0; i < ecmds.size(); i++) { wcstring ecmd = ecmds.at(i); append_format(stdout_buffer, L" %ls", escape(ecmd.c_str(), 1)); @@ -439,7 +439,7 @@ static void builtin_bind_list(const wchar_t *bind_mode) { const wcstring eseq = escape_string(seq, 1); append_format(stdout_buffer, L"bind -k %ls -M %ls -m %ls", eseq.c_str(), mode.c_str(), sets_mode.c_str()); - for (int i = 0; i < ecmds.size(); i++) + for (size_t i = 0; i < ecmds.size(); i++) { wcstring ecmd = ecmds.at(i); append_format(stdout_buffer, L" %ls", escape(ecmd.c_str(), 1)); diff --git a/env_universal_common.cpp b/env_universal_common.cpp index 5194e8ff1..eb0e15ae7 100644 --- a/env_universal_common.cpp +++ b/env_universal_common.cpp @@ -464,7 +464,7 @@ static wcstring full_escape(const wchar_t *in) { out.push_back(c); } - else if (c <= ASCII_MAX) + else if (c <= (wchar_t)ASCII_MAX) { // See #1225 for discussion of use of ASCII_MAX here append_format(out, L"\\x%.2x", c); diff --git a/fallback.cpp b/fallback.cpp index d26907e2b..405c499f8 100644 --- a/fallback.cpp +++ b/fallback.cpp @@ -802,6 +802,7 @@ wchar_t *wcstok(wchar_t *wcs, const wchar_t *delim, wchar_t **save_ptr) /* Fallback implementations of wcsdup and wcscasecmp. On systems where these are not needed (e.g. building on Linux) these should end up just being stripped, as they are static functions that are not referenced in this file. */ +__attribute__((unused)) static wchar_t *wcsdup_fallback(const wchar_t *in) { size_t len=wcslen(in); diff --git a/input.cpp b/input.cpp index 41121ab3a..3b135cf19 100644 --- a/input.cpp +++ b/input.cpp @@ -156,7 +156,7 @@ static const wchar_t * const name_arr[] = wcstring describe_char(wint_t c) { - wchar_t initial_cmd_char = R_BEGINNING_OF_LINE; + wint_t initial_cmd_char = R_BEGINNING_OF_LINE; size_t name_count = sizeof name_arr / sizeof *name_arr; if (c >= initial_cmd_char && c < initial_cmd_char + name_count) { diff --git a/iothread.cpp b/iothread.cpp index da2e08903..65bc4144a 100644 --- a/iothread.cpp +++ b/iothread.cpp @@ -114,6 +114,11 @@ static void enqueue_thread_result(SpawnRequest_t *req) s_result_queue.push(req); } +static void *this_thread() +{ + return (void *)(intptr_t)pthread_self(); +} + /* The function that does thread work. */ static void *iothread_worker(void *unused) { @@ -121,7 +126,7 @@ static void *iothread_worker(void *unused) struct SpawnRequest_t *req; while ((req = dequeue_spawn_request()) != NULL) { - IOTHREAD_LOG fprintf(stderr, "pthread %p dequeued %p\n", pthread_self(), req); + IOTHREAD_LOG fprintf(stderr, "pthread %p dequeued %p\n", this_thread(), req); /* Unlock the queue while we execute the request */ locker.unlock(); @@ -150,7 +155,7 @@ static void *iothread_worker(void *unused) assert(s_active_thread_count > 0); s_active_thread_count -= 1; - IOTHREAD_LOG fprintf(stderr, "pthread %p exiting\n", pthread_self()); + IOTHREAD_LOG fprintf(stderr, "pthread %p exiting\n", this_thread()); /* We're done */ return NULL; @@ -165,13 +170,13 @@ static void iothread_spawn() VOMIT_ON_FAILURE(pthread_sigmask(SIG_BLOCK, &new_set, &saved_set)); /* Spawn a thread. If this fails, it means there's already a bunch of threads; it is very unlikely that they are all on the verge of exiting, so one is likely to be ready to handle extant requests. So we can ignore failure with some confidence. */ - pthread_t thread = NULL; + pthread_t thread = 0; pthread_create(&thread, NULL, iothread_worker, NULL); /* We will never join this thread */ VOMIT_ON_FAILURE(pthread_detach(thread)); - IOTHREAD_LOG fprintf(stderr, "pthread %p spawned\n", thread); + IOTHREAD_LOG fprintf(stderr, "pthread %p spawned\n", (void *)(intptr_t)thread); /* Restore our sigmask */ VOMIT_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &saved_set, NULL)); diff --git a/parse_tree.cpp b/parse_tree.cpp index 36c94544d..fe1e2527e 100644 --- a/parse_tree.cpp +++ b/parse_tree.cpp @@ -525,7 +525,7 @@ class parse_ll_t { PARSE_ASSERT(! symbol_stack.empty()); const parse_stack_element_t &top_symbol = symbol_stack.back(); - PARSE_ASSERT(top_symbol.node_idx != -1); + PARSE_ASSERT(top_symbol.node_idx != NODE_OFFSET_INVALID); PARSE_ASSERT(top_symbol.node_idx < nodes.size()); return nodes.at(top_symbol.node_idx); } diff --git a/parser.cpp b/parser.cpp index b75b1ec79..773cde6b7 100644 --- a/parser.cpp +++ b/parser.cpp @@ -866,10 +866,9 @@ int parser_t::eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t execution_contexts.push_back(ctx); /* Execute the first node */ - int result = 1; if (! tree.empty()) { - result = this->eval_block_node(0, io, block_type); + this->eval_block_node(0, io, block_type); } /* Clean up the execution context stack */ diff --git a/proc.cpp b/proc.cpp index 66a258270..8ebcdb01e 100644 --- a/proc.cpp +++ b/proc.cpp @@ -383,7 +383,7 @@ static void mark_process_status(const job_t *j, process_t *p, int status) } else { - ssize_t ignore; + ssize_t ignore __attribute__((unused)); /* This should never be reached */ p->completed = 1; diff --git a/screen.cpp b/screen.cpp index 1fb7ec2f6..27b10a5c2 100644 --- a/screen.cpp +++ b/screen.cpp @@ -48,7 +48,7 @@ efficient way for transforming that to the desired screen content. #include "pager.h" /** The number of characters to indent new blocks */ -#define INDENT_STEP 4 +#define INDENT_STEP 4u /** The initial screen width */ #define SCREEN_WIDTH_UNINITIALIZED -1 From fb89c762fc909a4b5a5c51f51990185b30ec6857 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 27 Apr 2014 17:27:20 -0700 Subject: [PATCH 3/5] Silence unused return value warning in FATAL_EXIT --- common.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/common.h b/common.h index 3c409e8f6..057db144d 100644 --- a/common.h +++ b/common.h @@ -180,10 +180,11 @@ extern const wchar_t *program_name; */ #define FATAL_EXIT() \ { \ - char exit_read_buff; \ + char exit_read_buff; \ show_stackframe(); \ - read( 0, &exit_read_buff, 1 ); \ - exit_without_destructors( 1 ); \ + int ignore __attribute__((unused)); \ + ignore = read( 0, &exit_read_buff, 1 ); \ + exit_without_destructors( 1 ); \ } \ From 4948508277e424264a1b6232a6c275dd03fd584d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 27 Apr 2014 18:27:34 -0700 Subject: [PATCH 4/5] Squelch some more warnings on Linux --- builtin.cpp | 2 -- common.cpp | 18 +++++++++++++++--- common.h | 13 ++++++++----- configure.ac | 3 ++- fish_tests.cpp | 4 ++-- key_reader.cpp | 2 +- proc.cpp | 4 +--- wutil.cpp | 2 +- 8 files changed, 30 insertions(+), 18 deletions(-) diff --git a/builtin.cpp b/builtin.cpp index 70829acb4..9f63e5155 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -3622,8 +3622,6 @@ static int builtin_history(parser_t &parser, wchar_t **argv) return STATUS_BUILTIN_ERROR; } -#pragma mark Simulator - int builtin_parse(parser_t &parser, wchar_t **argv) { struct sigaction act; diff --git a/common.cpp b/common.cpp index 0a247fe61..4bb151042 100644 --- a/common.cpp +++ b/common.cpp @@ -716,6 +716,18 @@ void print_stderr(const wcstring &str) fprintf(stderr, "%ls\n", str.c_str()); } +void read_ignore(int fd, void *buff, size_t count) +{ + size_t ignore __attribute__((unused)); + ignore = read(fd, buff, count); +} + +void write_ignore(int fd, const void *buff, size_t count) +{ + size_t ignore __attribute__((unused)); + ignore = write(fd, buff, count); +} + void debug_safe(int level, const char *msg, const char *param1, const char *param2, const char *param3, const char *param4, const char *param5, const char *param6, const char *param7, const char *param8, const char *param9, const char *param10, const char *param11, const char *param12) { @@ -736,7 +748,7 @@ void debug_safe(int level, const char *msg, const char *param1, const char *para if (end == NULL) end = cursor + strlen(cursor); - write(STDERR_FILENO, cursor, end - cursor); + write_ignore(STDERR_FILENO, cursor, end - cursor); if (end[0] == '%' && end[1] == 's') { @@ -745,7 +757,7 @@ void debug_safe(int level, const char *msg, const char *param1, const char *para const char *format = params[param_idx++]; if (! format) format = "(null)"; - write(STDERR_FILENO, format, strlen(format)); + write_ignore(STDERR_FILENO, format, strlen(format)); cursor = end + 2; } else if (end[0] == '\0') @@ -761,7 +773,7 @@ void debug_safe(int level, const char *msg, const char *param1, const char *para } // We always append a newline - write(STDERR_FILENO, "\n", 1); + write_ignore(STDERR_FILENO, "\n", 1); errno = errno_old; } diff --git a/common.h b/common.h index 057db144d..b22720b06 100644 --- a/common.h +++ b/common.h @@ -157,6 +157,10 @@ extern bool g_profiling_active; */ extern const wchar_t *program_name; +/* Variants of read() and write() that ignores return values, defeating a warning */ +void read_ignore(int fd, void *buff, size_t count); +void write_ignore(int fd, const void *buff, size_t count); + /** This macro is used to check that an input argument is not null. It is a bit lika a non-fatal form of assert. Instead of exit-ing on @@ -180,11 +184,10 @@ extern const wchar_t *program_name; */ #define FATAL_EXIT() \ { \ - char exit_read_buff; \ - show_stackframe(); \ - int ignore __attribute__((unused)); \ - ignore = read( 0, &exit_read_buff, 1 ); \ - exit_without_destructors( 1 ); \ + char exit_read_buff; \ + show_stackframe(); \ + read_ignore( 0, &exit_read_buff, 1 ); \ + exit_without_destructors( 1 ); \ } \ diff --git a/configure.ac b/configure.ac index 23c5b16a0..ae1a61836 100644 --- a/configure.ac +++ b/configure.ac @@ -200,9 +200,10 @@ CXXFLAGS="$CXXFLAGS -fno-exceptions" # # -Wall is there to keep me on my toes +# But signed comparison warnings are way too aggressive # -CXXFLAGS="$CXXFLAGS -Wall" +CXXFLAGS="$CXXFLAGS -Wall -Wno-sign-compare" # # This is needed in order to get the really cool backtraces on Linux diff --git a/fish_tests.cpp b/fish_tests.cpp index 382abb39c..7ec4ab8b4 100644 --- a/fish_tests.cpp +++ b/fish_tests.cpp @@ -1330,7 +1330,7 @@ static void test_expand() err(L"Expansion not correctly handling literal path components in dotfiles"); } - system("rm -Rf /tmp/fish_expand_test"); + if (system("rm -Rf /tmp/fish_expand_test")) err(L"rm failed"); } static void test_fuzzy_match(void) @@ -3117,7 +3117,7 @@ static void test_highlighting(void) // Generate the text wcstring text; - std::vector expected_colors; + std::vector expected_colors; for (size_t i=0; i < component_count; i++) { if (i > 0) diff --git a/key_reader.cpp b/key_reader.cpp index fae6f33fa..382f4b229 100644 --- a/key_reader.cpp +++ b/key_reader.cpp @@ -22,7 +22,7 @@ int writestr(char *str) { - write(1, str, strlen(str)); + write_ignore(1, str, strlen(str)); return 0; } diff --git a/proc.cpp b/proc.cpp index 8ebcdb01e..f62b00f64 100644 --- a/proc.cpp +++ b/proc.cpp @@ -383,8 +383,6 @@ static void mark_process_status(const job_t *j, process_t *p, int status) } else { - ssize_t ignore __attribute__((unused)); - /* This should never be reached */ p->completed = 1; @@ -398,7 +396,7 @@ static void mark_process_status(const job_t *j, process_t *p, int status) handler. If things aren't working properly, it's safer to give up. */ - ignore = write(2, mess, strlen(mess)); + write_ignore(2, mess, strlen(mess)); } } diff --git a/wutil.cpp b/wutil.cpp index ffb4f2b22..7750a3e43 100644 --- a/wutil.cpp +++ b/wutil.cpp @@ -364,7 +364,7 @@ void safe_perror(const char *message) safe_append(buff, safe_strerror(err), sizeof buff); safe_append(buff, "\n", sizeof buff); - write(STDERR_FILENO, buff, strlen(buff)); + write_ignore(STDERR_FILENO, buff, strlen(buff)); errno = err; } From 97c2ec8dcfe14882bafb2f2c56502427c0ffa1d0 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 27 Apr 2014 18:44:21 -0700 Subject: [PATCH 5/5] Fix a duplicated variable, and defeat some warnings in fish_tests --- fish_tests.cpp | 4 ++-- parse_execution.cpp | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/fish_tests.cpp b/fish_tests.cpp index 7ec4ab8b4..6e2935148 100644 --- a/fish_tests.cpp +++ b/fish_tests.cpp @@ -2012,8 +2012,8 @@ static void test_autosuggest_suggest_special() // A single quote should defeat tilde expansion perform_one_autosuggestion_test(L"cd '~/test_autosuggest_suggest_specia'", wd, L"", __LINE__); - system("rm -Rf '/tmp/autosuggest_test/'"); - system("rm -Rf ~/test_autosuggest_suggest_special/"); + if (system("rm -Rf '/tmp/autosuggest_test/'")) err(L"rm failed"); + if (system("rm -Rf ~/test_autosuggest_suggest_special/")) err(L"rm failed"); } static void test_autosuggestion_combining() diff --git a/parse_execution.cpp b/parse_execution.cpp index d5c63aba9..3c4dfe6c1 100644 --- a/parse_execution.cpp +++ b/parse_execution.cpp @@ -503,7 +503,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(const pars parse_execution_result_t parse_execution_context_t::run_switch_statement(const parse_node_t &statement) { assert(statement.type == symbol_switch_statement); - parse_execution_result_t ret = parse_execution_success; const parse_node_t *matching_case_item = NULL; parse_execution_result_t result = parse_execution_success; @@ -530,7 +529,7 @@ parse_execution_result_t parse_execution_context_t::run_switch_statement(const p { /* Store the node that failed to expand */ report_error(switch_value_node, WILDCARD_ERR_MSG, switch_value.c_str()); - ret = parse_execution_errored; + result = parse_execution_errored; break; }