Commit 36d377a3 authored by jan.koester's avatar jan.koester
Browse files

jobs

parent 4e097be3
Loading
Loading
Loading
Loading
+113 −12
Changes for executor.cpp: 113 added lines, 12 removed lines.
Original line number Diff line number Diff line
@@ -3,7 +3,13 @@
#include <filesystem>
#include <algorithm>
#include <cstdlib>
#include <cstring>
#include <cerrno>
#include <sys/wait.h>
#include <unistd.h>
#include <spawn.h>

extern char** environ;

namespace fs = std::filesystem;
using namespace makeplus;
@@ -248,18 +254,65 @@ bool Executor::needs_rebuild(const std::string& target, const std::vector<std::s

// ======================== Command Execution ========================

int Executor::run_command(const std::string& cmd) {
    int status = std::system(cmd.c_str());
int Executor::run_command(const std::string& cmd, const std::vector<std::string>& extra_env) {
    // Build the child environment explicitly (a copy of our environment with the
    // recipe's exported variables layered on top) and hand it to posix_spawn.
    // This avoids mutating the global environ via setenv, which is not safe when
    // several recipes run concurrently in parallel (-j) builds.
    std::vector<std::string> env_storage;
    {
        // Names overridden by extra_env, so we drop the inherited copy.
        std::set<std::string> overridden;
        for (const auto& kv : extra_env) {
            size_t eq = kv.find('=');
            if (eq != std::string::npos) overridden.insert(kv.substr(0, eq));
        }
        for (char** e = environ; e && *e; ++e) {
            std::string entry(*e);
            size_t eq = entry.find('=');
            std::string name = (eq == std::string::npos) ? entry : entry.substr(0, eq);
            if (!overridden.count(name)) env_storage.push_back(std::move(entry));
        }
        for (const auto& kv : extra_env) env_storage.push_back(kv);
    }
    std::vector<char*> envp;
    envp.reserve(env_storage.size() + 1);
    for (auto& s : env_storage) envp.push_back(const_cast<char*>(s.c_str()));
    envp.push_back(nullptr);

    const char* shell = "/bin/sh";
    char* const argv[] = {
        const_cast<char*>("sh"),
        const_cast<char*>("-c"),
        const_cast<char*>(cmd.c_str()),
        nullptr
    };

    pid_t pid = 0;
    int rc = posix_spawn(&pid, shell, nullptr, nullptr, argv, envp.data());
    if (rc != 0) return 127;

    int status = 0;
    while (waitpid(pid, &status, 0) < 0 && errno == EINTR) {}
    if (WIFEXITED(status)) return WEXITSTATUS(status);
    if (WIFSIGNALED(status)) return 128 + WTERMSIG(status);
    return 1;
}

void Executor::apply_exports(const Makefile& mf) {
    for (const auto& kv : collect_exports(mf)) {
        size_t eq = kv.find('=');
        if (eq == std::string::npos) continue;
        setenv(kv.substr(0, eq).c_str(), kv.substr(eq + 1).c_str(), 1);
    }
}

std::vector<std::string> Executor::collect_exports(const Makefile& mf) {
    std::vector<std::string> out;
    auto export_one = [&](const std::string& name, const Variable& var) {
        std::string val = (var.flavor == VarFlavor::Recursive)
            ? parser_.expand(var.value, mf) : var.value;
        setenv(name.c_str(), val.c_str(), 1);
        out.push_back(name + "=" + val);
    };
    if (mf.export_all) {
        for (const auto& kv : mf.variables) {
@@ -272,6 +325,37 @@ void Executor::apply_exports(const Makefile& mf) {
            if (it != mf.variables.end()) export_one(name, it->second);
        }
    }
    return out;
}

// Acquire one job token. The process's implicit free token is used first; any
// further concurrency is granted by reading a byte from the jobserver pipe,
// blocking until another job (anywhere in the recursive tree) releases one.
bool Executor::js_acquire(bool& used_free) {
    used_free = false;
    if (!js_active_) return true;
    bool expected = true;
    if (js_free_token_.compare_exchange_strong(expected, false)) {
        used_free = true;
        return true;
    }
    for (;;) {
        char tok;
        ssize_t n = ::read(js_read_fd_, &tok, 1);
        if (n == 1) return true;
        if (n < 0 && errno == EINTR) continue;
        // Pipe closed/error: fall back to running without a token rather than
        // deadlocking the build.
        return true;
    }
}

void Executor::js_release(bool used_free) {
    if (!js_active_) return;
    if (used_free) { js_free_token_.store(true); return; }
    char tok = '+';
    ssize_t n;
    do { n = ::write(js_write_fd_, &tok, 1); } while (n < 0 && errno == EINTR);
}

std::map<std::string, std::string> Executor::compute_target_scope(
@@ -340,9 +424,10 @@ int Executor::execute_rule(const std::string& target, const Rule* rule,
            local_copy.variables[kv.first] = {kv.second, VarFlavor::Recursive, false};
        mfp = &local_copy;
    }
    // Export marked variables into the environment so recipes and any
    // recursive sub-makes ($(MAKE)) inherit them.
    apply_exports(*mfp);
    // Compute the variables to export into each recipe's child process. Passing
    // them explicitly (rather than setenv on the shared environment) keeps
    // concurrent recipes from clobbering one another's environment under -j.
    std::vector<std::string> recipe_env = collect_exports(*mfp);

    std::vector<std::string> newer;
    if (file_exists(target)) {
@@ -394,7 +479,7 @@ int Executor::execute_rule(const std::string& target, const Rule* rule,
            if (question_) return 1; // question mode: return 1 if would rebuild

            if (!dry_run_ || force_exec) {
                int rc = run_command(cmd);
                int rc = run_command(cmd, recipe_env);
                if (rc != 0 && !ignore_error) {
                    std::cerr << "makeplus: *** [" << target << "] Error " << rc << std::endl;
                    return rc;
@@ -460,7 +545,8 @@ int Executor::build_sequential(const std::string& target_in,

// ======================== Parallel Build ========================

void Executor::add_to_dag(const std::string& target, std::set<std::string>& visited) {
void Executor::add_to_dag(const std::string& target, std::set<std::string>& visited,
                          const std::map<std::string, std::string>& inherited) {
    if (visited.count(target)) return;
    visited.insert(target);

@@ -470,12 +556,19 @@ void Executor::add_to_dag(const std::string& target, std::set<std::string>& visi
    node.target = target;
    node.rule = find_rule(target, node.stem);

    // Effective target-specific variable scope: this target's own bindings
    // merged with those inherited from ancestors. Propagated to prerequisites,
    // mirroring the sequential build path so vdso and similar targets that rely
    // on inherited target-specific variables still build correctly under -j.
    std::map<std::string, std::string> scope = compute_target_scope(target, inherited);
    node.scope = scope;

    if (node.rule) {
        node.prereqs = expand_prereqs(target, node.rule, node.stem);
        node.unresolved_deps = 0;

        for (const auto& p : node.prereqs) {
            add_to_dag(p, visited);
            add_to_dag(p, visited, scope);
            node.unresolved_deps++;
        }
    }
@@ -512,14 +605,22 @@ void Executor::worker_thread() {
        bool success = true;

        if (!node.rule) {
            if (!file_exists(target)) {
            // A target declared .PHONY but lacking any rule/recipe (e.g. Kbuild's
            // in-tree `outputmakefile`) is a no-op, mirroring the sequential path.
            if (!mf_.phony_targets.count(target) && !file_exists(target)) {
                std::lock_guard<std::mutex> lock(mutex_);
                std::cerr << "makeplus: *** No rule to make target '" << target << "'. Stop." << std::endl;
                success = false;
            }
        } else {
            if (needs_rebuild(target, node.prereqs)) {
                int rc = execute_rule(target, node.rule, node.stem, node.prereqs);
                // Hold a jobserver token for the duration of this recipe so the
                // total number of concurrent jobs across the whole recursive
                // sub-make tree stays within the -j limit.
                bool used_free = false;
                js_acquire(used_free);
                int rc = execute_rule(target, node.rule, node.stem, node.prereqs, node.scope);
                js_release(used_free);
                if (rc != 0) success = false;
            }
        }
@@ -553,7 +654,7 @@ int Executor::build_parallel(const std::vector<std::string>& targets) {
    // Phase 1: Build DAG
    std::set<std::string> visited;
    for (const auto& t : targets) {
        add_to_dag(t, visited);
        add_to_dag(t, visited, {});
    }

    // Phase 2: Find initially ready nodes
+28 −2
Changes for executor.h: 28 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -21,6 +21,14 @@ public:
    void set_keep_going(bool b) { keep_going_ = b; }
    void set_silent(bool b) { silent_ = b; }
    void set_question(bool b) { question_ = b; }
    // GNU-compatible jobserver: a pool of tokens (bytes in a pipe) shared by the
    // whole recursive sub-make tree caps the TOTAL number of concurrent recipe
    // jobs. Each make also has one implicit "free" token for its first job.
    void set_jobserver(int read_fd, int write_fd) {
        js_read_fd_ = read_fd;
        js_write_fd_ = write_fd;
        js_active_ = (read_fd >= 0 && write_fd >= 0);
    }

    int build(const std::vector<std::string>& targets);

@@ -34,6 +42,16 @@ private:
    bool silent_ = false;
    bool question_ = false;

    // Jobserver token pool (see set_jobserver).
    int js_read_fd_ = -1;
    int js_write_fd_ = -1;
    bool js_active_ = false;
    std::atomic<bool> js_free_token_{true};
    // Acquire/release one job token. used_free reports whether the process's
    // implicit free token was taken (released without touching the pipe).
    bool js_acquire(bool& used_free);
    void js_release(bool used_free);

    // Sequential build
    std::map<std::string, int> visited_;
    std::set<std::string> rebuilt_;
@@ -48,6 +66,9 @@ private:
        std::vector<std::string> prereqs;
        std::vector<std::string> dependents;
        int unresolved_deps = 0;
        // Target-specific variable scope (own bindings merged with those
        // inherited from ancestors), mirroring the sequential build path.
        std::map<std::string, std::string> scope;
        enum class State { Pending, Ready, Running, Done, Failed };
        State state = State::Pending;
    };
@@ -59,7 +80,8 @@ private:
    std::atomic<int> remaining_{0};

    int build_parallel(const std::vector<std::string>& targets);
    void add_to_dag(const std::string& target, std::set<std::string>& visited);
    void add_to_dag(const std::string& target, std::set<std::string>& visited,
                    const std::map<std::string, std::string>& inherited);
    void worker_thread();

    // Shared helpers
@@ -77,7 +99,11 @@ private:
                     const std::string& stem, const std::vector<std::string>& prereqs,
                     const std::map<std::string, std::string>& scope = {});
    void apply_exports(const Makefile& mf);
    int run_command(const std::string& cmd);
    // Collect the variables a recipe should export as "NAME=value" entries,
    // instead of mutating the global environment (which is not thread-safe when
    // multiple recipes run concurrently).
    std::vector<std::string> collect_exports(const Makefile& mf);
    int run_command(const std::string& cmd, const std::vector<std::string>& extra_env);
    bool file_exists(const std::string& path);
    long long file_mtime(const std::string& path);
};
+72 −1
Changes for main.cpp: 72 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -7,7 +7,9 @@
#include <cstring>
#include <filesystem>
#include <thread>
#include <sstream>
#include <unistd.h>
#include <fcntl.h>

extern char** environ;

@@ -259,7 +261,18 @@ int main(int argc, char* argv[]) {
    // (the kernel tools/ build relies on this for its include flags).
    if (const char* mflags = getenv("MAKEFLAGS")) {
        std::string env_flags(mflags);
        size_t sep = env_flags.find("--");
        // Locate the standalone "--" token that separates short/long flags from
        // the NAME=value override section. Must NOT match the "--" inside long
        // options such as "--jobserver-auth", so require space/boundary on both
        // sides.
        size_t sep = std::string::npos;
        for (size_t i = 0; i + 1 < env_flags.size(); ++i) {
            if (env_flags[i] == '-' && env_flags[i + 1] == '-') {
                bool left = (i == 0 || env_flags[i - 1] == ' ' || env_flags[i - 1] == '\t');
                bool right = (i + 2 == env_flags.size() || env_flags[i + 2] == ' ' || env_flags[i + 2] == '\t');
                if (left && right) { sep = i; break; }
            }
        }
        if (sep != std::string::npos) {
            std::string overrides = env_flags.substr(sep + 2);
            std::vector<std::pair<std::string, std::string>> inherited;
@@ -289,6 +302,57 @@ int main(int argc, char* argv[]) {
        }
    }

    // Inherit parallelism and the jobserver from MAKEFLAGS. Recursive kernel
    // sub-make *processes* carry no -j on their argv; like GNU make they pick up
    // the job count and the jobserver pipe fds from the MAKEFLAGS flags portion
    // (everything before the standalone "--"): short option "-jN" and the long
    // option "--jobserver-auth=R,W".
    int js_read_fd = -1, js_write_fd = -1;
    if (const char* mflags = getenv("MAKEFLAGS")) {
        std::istringstream iss(mflags);
        std::string tok;
        while (iss >> tok) {
            if (tok == "--") break;  // start of NAME=value overrides
            if (tok.rfind("-j", 0) == 0) {
                if (tok.size() > 2) {
                    int j = std::atoi(tok.c_str() + 2);
                    if (j > jobs) jobs = j;
                } else if (jobs < 2) {
                    jobs = 2;  // bare "-j"; jobserver caps real concurrency
                }
            } else if (tok.rfind("--jobserver-auth=", 0) == 0 ||
                       tok.rfind("--jobserver-fds=", 0) == 0) {
                std::string v = tok.substr(tok.find('=') + 1);
                int r = -1, w = -1;
                if (std::sscanf(v.c_str(), "%d,%d", &r, &w) == 2) {
                    js_read_fd = r;
                    js_write_fd = w;
                }
            }
        }
    }

    // Drop a stale inherited jobserver whose fds are not actually open (e.g. the
    // parent closed them); fall back to acting as our own jobserver master.
    if (js_read_fd >= 0 &&
        (fcntl(js_read_fd, F_GETFD) == -1 || fcntl(js_write_fd, F_GETFD) == -1)) {
        js_read_fd = js_write_fd = -1;
    }

    // No jobserver inherited but -j requested: become the jobserver master.
    // Create the pipe, preload (jobs-1) tokens (this make keeps one implicit
    // free token), and advertise the fds to every sub-make via MAKEFLAGS.
    if (js_read_fd < 0 && jobs > 1) {
        int fds[2];
        if (pipe(fds) == 0) {
            js_read_fd = fds[0];
            js_write_fd = fds[1];
            std::string tokens(static_cast<size_t>(jobs - 1), '+');
            ssize_t wn = write(js_write_fd, tokens.data(), tokens.size());
            (void)wn;
        }
    }

    // Apply command-line variables (highest priority). Also export them into the
    // environment, like GNU make, so plain $$VAR references in recipes and any
    // child process observe the value.
@@ -320,6 +384,12 @@ int main(int argc, char* argv[]) {
        std::string combined = flags;
        if (!words.empty()) combined += words;
        if (jobs > 1) combined += " -j" + std::to_string(jobs);
        // Advertise the jobserver pipe so recursive sub-makes share the global
        // job-count limit instead of each spawning -j jobs (which would otherwise
        // fork-bomb a deep recursive build).
        if (js_read_fd >= 0 && js_write_fd >= 0)
            combined += " --jobserver-auth=" + std::to_string(js_read_fd) +
                        "," + std::to_string(js_write_fd);
        // Append command-line variable overrides after "--" so they propagate to
        // sub-makes (GNU make format; spaces/backslashes in a value are escaped).
        if (!cmdline_vars.empty()) {
@@ -360,6 +430,7 @@ int main(int argc, char* argv[]) {
    // Execute
    makeplus::Executor executor(mf, parser);
    executor.set_jobs(jobs);
    executor.set_jobserver(js_read_fd, js_write_fd);
    executor.set_dry_run(dry_run);
    executor.set_always_make(always_make);
    executor.set_keep_going(keep_going);