From 14fb38f952367ac6876eec18d6c3444aa6b77a2d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 26 Jan 2017 14:47:32 -0800 Subject: [PATCH] Switch job handling to use shared pointers instead of raw pointers Clarifies memory management around allocation of job_ts --- src/builtin.cpp | 10 +++------- src/expand.cpp | 13 ++++++------- src/parse_execution.cpp | 28 +++++++++++++--------------- src/parser.cpp | 26 +++++++++++++++----------- src/parser.h | 6 +++--- src/proc.cpp | 22 +++++++++------------- src/proc.h | 18 ++++++++---------- src/reader.cpp | 2 +- 8 files changed, 58 insertions(+), 67 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 12252bde5..7ba07970e 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -1681,7 +1681,7 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis event_t e(EVENT_ANY); if ((opt == 'j') && (wcscasecmp(w.woptarg, L"caller") == 0)) { - int job_id = -1; + job_id_t job_id = -1; if (is_subshell) { size_t block_idx = 0; @@ -2861,10 +2861,6 @@ static int builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **arg return res; } -/// Make the specified job the first job of the job list. Moving jobs around in the list makes the -/// list reflect the order in which the jobs were used. -static void make_first(job_t *j) { job_promote(j); } - /// Builtin for putting a job in the foreground. static int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { job_t *j = NULL; @@ -2941,7 +2937,7 @@ static int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { if (!ft.empty()) env_set(L"_", ft.c_str(), ENV_EXPORT); reader_write_title(j->command()); - make_first(j); + job_promote(j); job_set_flag(j, JOB_FOREGROUND, 1); job_continue(j, job_is_stopped(j)); @@ -2964,7 +2960,7 @@ static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j, const w streams.err.append_format(_(L"Send job %d '%ls' to background\n"), j->job_id, j->command_wcstr()); - make_first(j); + job_promote(j); job_set_flag(j, JOB_FOREGROUND, 0); job_continue(j, job_is_stopped(j)); return STATUS_BUILTIN_OK; diff --git a/src/expand.cpp b/src/expand.cpp index 768b49085..7ab2a10ba 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -435,7 +435,6 @@ bool process_iterator_t::next_process(wcstring *out_str, pid_t *out_pid) { static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vector *completions) { ASSERT_IS_MAIN_THREAD(); - const job_t *j; bool found = false; // If we are not doing tab completion, we first check for the single '%' character, because an // empty string will pass the numeric check below. But if we are doing tab completion, we want @@ -444,7 +443,7 @@ static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vectorcommand_is_empty()) { append_completion(completions, to_string(j->pgid)); break; @@ -457,7 +456,7 @@ static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vectorcommand_is_empty()) continue; @@ -471,8 +470,8 @@ static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vector 0) { - j = job_get(jid); - if ((j != 0) && (j->command_wcstr() != 0) && (!j->command_is_empty())) { + const job_t *j = job_get(jid); + if (j && !j->command_is_empty()) { append_completion(completions, to_string(j->pgid)); } } @@ -487,7 +486,7 @@ static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vectorcommand_is_empty()) continue; size_t offset; @@ -507,7 +506,7 @@ static bool find_job(const wchar_t *proc, expand_flags_t flags, std::vectorcommand_is_empty()) continue; for (const process_ptr_t &p : j->processes) { if (p->actual_cmd.empty()) continue; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5e312d5a0..0284a8fc9 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1265,34 +1265,32 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t return result; } - job_t *j = new job_t(acquire_job_id(), block_io); - j->tmodes = tmodes; - job_set_flag(j, JOB_CONTROL, + shared_ptr job = std::make_shared(acquire_job_id(), block_io); + job->tmodes = tmodes; + job_set_flag(job.get(), JOB_CONTROL, (job_control_mode == JOB_CONTROL_ALL) || ((job_control_mode == JOB_CONTROL_INTERACTIVE) && shell_is_interactive())); - job_set_flag(j, JOB_FOREGROUND, !tree.job_should_be_backgrounded(job_node)); + job_set_flag(job.get(), JOB_FOREGROUND, !tree.job_should_be_backgrounded(job_node)); - job_set_flag(j, JOB_TERMINAL, job_get_flag(j, JOB_CONTROL) && !is_subshell && !is_event); + job_set_flag(job.get(), JOB_TERMINAL, job_get_flag(job.get(), JOB_CONTROL) && !is_subshell && !is_event); - job_set_flag(j, JOB_SKIP_NOTIFICATION, + job_set_flag(job.get(), JOB_SKIP_NOTIFICATION, is_subshell || is_block || is_event || !shell_is_interactive()); // Tell the current block what its job is. This has to happen before we populate it (#1394). - parser->current_block()->job = j; + parser->current_block()->job = job; // Populate the job. This may fail for reasons like command_not_found. If this fails, an error // will have been printed. parse_execution_result_t pop_result = - this->populate_job_from_job_node(j, job_node, associated_block); + this->populate_job_from_job_node(job.get(), job_node, associated_block); // Clean up the job on failure or cancellation. bool populated_job = (pop_result == parse_execution_success); if (!populated_job || this->should_cancel_execution(associated_block)) { - assert(parser->current_block()->job == j); + assert(parser->current_block()->job == job); parser->current_block()->job = NULL; - delete j; - j = NULL; populated_job = false; } @@ -1303,11 +1301,11 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t if (populated_job) { // Success. Give the job to the parser - it will clean it up. - parser->job_add(j); + parser->job_add(job); // Check to see if this contained any external commands. bool job_contained_external_command = false; - for (const auto &proc : j->processes) { + for (const auto &proc : job->processes) { if (proc->type == EXTERNAL) { job_contained_external_command = true; break; @@ -1315,7 +1313,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t } // Actually execute the job. - exec_job(*this->parser, j); + exec_job(*this->parser, job.get()); // Only external commands require a new fishd barrier. if (job_contained_external_command) { @@ -1328,7 +1326,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t profile_item->level = eval_level; profile_item->parse = (int)(parse_time - start_time); profile_item->exec = (int)(exec_time - parse_time); - profile_item->cmd = j ? j->command() : wcstring(); + profile_item->cmd = job ? job->command() : wcstring(); profile_item->skipped = !populated_job; } diff --git a/src/parser.cpp b/src/parser.cpp index 0f2c56035..b1169a64b 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -523,17 +523,18 @@ wcstring parser_t::current_line() { return line_info; } -void parser_t::job_add(job_t *job) { +void parser_t::job_add(shared_ptr job) { assert(job != NULL); assert(! job->processes.empty()); - this->my_job_list.push_front(job); + this->my_job_list.push_front(std::move(job)); } bool parser_t::job_remove(job_t *job) { - job_list_t::iterator iter = std::find(my_job_list.begin(), my_job_list.end(), job); - if (iter != my_job_list.end()) { - my_job_list.erase(iter); - return true; + for (auto iter = my_job_list.begin(); iter != my_job_list.end(); ++iter) { + if (iter->get() == job) { + my_job_list.erase(iter); + return true; + } } debug(1, _(L"Job inconsistency")); @@ -542,7 +543,12 @@ bool parser_t::job_remove(job_t *job) { } void parser_t::job_promote(job_t *job) { - job_list_t::iterator loc = std::find(my_job_list.begin(), my_job_list.end(), job); + job_list_t::iterator loc; + for (loc = my_job_list.begin(); loc != my_job_list.end(); ++loc) { + if (loc->get() == job) { + break; + } + } assert(loc != my_job_list.end()); // Move the job to the beginning. @@ -550,10 +556,8 @@ void parser_t::job_promote(job_t *job) { } job_t *parser_t::job_get(job_id_t id) { - job_iterator_t jobs(my_job_list); - job_t *job; - while ((job = jobs.next())) { - if (id <= 0 || job->job_id == id) return job; + for (const auto &job : my_job_list) { + if (id <= 0 || job->job_id == id) return job.get(); } return NULL; } diff --git a/src/parser.h b/src/parser.h index 1e2841262..3385526e1 100644 --- a/src/parser.h +++ b/src/parser.h @@ -78,7 +78,7 @@ struct block_t { /// Status for the current loop block. Can be any of the values from the loop_status enum. enum loop_status_t loop_status; /// The job that is currently evaluated in the specified block. - job_t *job; + shared_ptr job; /// Name of file that created this block. This string is intern'd. const wchar_t *src_filename; /// Line number where this block was created. @@ -207,7 +207,7 @@ class parser_t { parser_t &operator=(const parser_t &); /// Adds a job to the beginning of the job list. - void job_add(job_t *job); + void job_add(shared_ptr job); /// Returns the name of the currently evaluated function if we are currently evaluating a /// function, null otherwise. This is tested by moving down the block-scope-stack, checking @@ -310,7 +310,7 @@ class parser_t { void job_promote(job_t *job); /// Return the job with the specified job id. If id is 0 or less, return the last job used. - job_t *job_get(int job_id); + job_t *job_get(job_id_t job_id); /// Returns the job with the given pid. job_t *job_get_from_pid(int pid); diff --git a/src/proc.cpp b/src/proc.cpp index 4c4f92826..8be007fb6 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -138,18 +138,12 @@ void job_promote(job_t *job) { parser_t::principal_parser().job_promote(job); } -/// Remove job from the job list and free all memory associated with it. -void job_free(job_t *j) { - job_remove(j); - delete j; -} - void proc_destroy() { job_list_t &jobs = parser_t::principal_parser().job_list(); while (!jobs.empty()) { - job_t *job = jobs.front(); + job_t *job = jobs.front().get(); debug(2, L"freeing leaked job %ls", job->command_wcstr()); - job_free(job); + job_remove(job); } } @@ -237,7 +231,7 @@ bool job_is_completed(const job_t *j) { return result; } -void job_set_flag(job_t *j, unsigned int flag, int set) { +void job_set_flag(job_t *j, job_flag_t flag, bool set) { if (set) { j->flags |= flag; } else { @@ -245,7 +239,9 @@ void job_set_flag(job_t *j, unsigned int flag, int set) { } } -int job_get_flag(const job_t *j, unsigned int flag) { return static_cast(j->flags & flag); } +bool job_get_flag(const job_t *j, job_flag_t flag) { + return !! (j->flags & flag); +} int job_signal(job_t *j, int signal) { pid_t my_pid = getpid(); @@ -630,7 +626,7 @@ int job_reap(bool allow_interactive) { proc_fire_event(L"JOB_EXIT", EVENT_EXIT, -j->pgid, 0); proc_fire_event(L"JOB_EXIT", EVENT_JOB_ID, j->job_id, 0); - job_free(j); + job_remove(j); } else if (job_is_stopped(j) && !job_get_flag(j, JOB_NOTIFIED)) { // Notify the user about newly stopped jobs. if (!job_get_flag(j, JOB_SKIP_NOTIFICATION)) { @@ -983,10 +979,10 @@ int proc_format_status(int status) { } void proc_sanity_check() { - job_t *fg_job = NULL; + const job_t *fg_job = NULL; job_iterator_t jobs; - while (job_t *j = jobs.next()) { + while (const job_t *j = jobs.next()) { if (!job_get_flag(j, JOB_CONSTRUCTED)) continue; diff --git a/src/proc.h b/src/proc.h index f3f59c74c..1183de0dc 100644 --- a/src/proc.h +++ b/src/proc.h @@ -155,7 +155,7 @@ typedef std::unique_ptr process_ptr_t; typedef std::vector process_list_t; /// Constants for the flag variable in the job struct. -enum { +enum job_flag_t { /// Whether the user has been told about stopped job. JOB_NOTIFIED = 1 << 0, /// Whether this job is in the foreground. @@ -189,8 +189,8 @@ class job_t { const io_chain_t block_io; // No copying. - job_t(const job_t &rhs); - void operator=(const job_t &); + job_t(const job_t &rhs) = delete; + void operator=(const job_t &) = delete; public: job_t(job_id_t jobid, const io_chain_t &bio); @@ -249,7 +249,8 @@ extern int is_login; /// Whether we are running an event handler. extern int is_event; -typedef std::list job_list_t; +// List of jobs. We sometimes mutate this while iterating - hence it must be a list, not a vector +typedef std::list> job_list_t; bool job_list_is_empty(void); @@ -264,7 +265,7 @@ class job_iterator_t { job_t *next() { job_t *job = NULL; if (current != end) { - job = *current; + job = current->get(); ++current; } return job; @@ -298,10 +299,10 @@ extern int job_control_mode; extern int no_exec; /// Add the specified flag to the bitset of flags for the specified job. -void job_set_flag(job_t *j, unsigned int flag, int set); +void job_set_flag(job_t *j, job_flag_t flag, bool set); /// Returns one if the specified flag is set in the specified job, 0 otherwise. -int job_get_flag(const job_t *j, unsigned int flag); +bool job_get_flag(const job_t *j, job_flag_t flag); /// Sets the status of the last process to exit. void proc_set_last_status(int s); @@ -309,9 +310,6 @@ void proc_set_last_status(int s); /// Returns the status of the last process to exit. int proc_get_last_status(); -/// Remove the specified job. -void job_free(job_t *j); - /// Promotes a job to the front of the job list. void job_promote(job_t *job); diff --git a/src/reader.cpp b/src/reader.cpp index 59b02d530..957da8fa3 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2184,7 +2184,7 @@ static void handle_end_loop() { } bool bg_jobs = false; - while (job_t *j = jobs.next()) { + while (const job_t *j = jobs.next()) { if (!job_is_completed(j)) { bg_jobs = true; break;