From f87d49e41a5dd57733d02f3990c91dc38e557dad Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Thu, 29 Sep 2016 20:09:47 -0700 Subject: Support marking variables as readonly When the magic variable .KATI_READONLY is set to a variable name, any further attempts to modify the named variable will result in an error. FOO := bar .KATI_READONLY := FOO FOO := baz # Error! This is useful to make some global configuration readonly so that another makefile cannot change it. In Android, we emulated this by backing up some global configuration before including the Android.mk files, then comparing the current values to the backed up values after they've been included. But this means we don't know the location that modified the variable, just that something did. And it's not perfect, since the backup can also be changed. Something similar to this could be implemented with `override`, but then setting the variable silently fails, and it still could be overriden with another override. --- eval.cc | 53 +++++++++++++++++++++++++++++++++---- eval.h | 2 ++ symtab.cc | 11 +++++++- symtab.h | 2 +- testcase/readonly_global.sh | 47 ++++++++++++++++++++++++++++++++ testcase/readonly_global_missing.sh | 32 ++++++++++++++++++++++ testcase/readonly_rule.sh | 48 +++++++++++++++++++++++++++++++++ testcase/readonly_rule_missing.sh | 32 ++++++++++++++++++++++ var.cc | 9 +++++-- var.h | 8 +++++- 10 files changed, 234 insertions(+), 10 deletions(-) create mode 100644 testcase/readonly_global.sh create mode 100644 testcase/readonly_global_missing.sh create mode 100644 testcase/readonly_rule.sh create mode 100644 testcase/readonly_rule_missing.sh diff --git a/eval.cc b/eval.cc index da183f7..115d3dd 100644 --- a/eval.cc +++ b/eval.cc @@ -36,7 +36,8 @@ Evaluator::Evaluator() avoid_io_(false), eval_depth_(0), posix_sym_(Intern(".POSIX")), - is_posix_(false) { + is_posix_(false), + kati_readonly_(Intern(".KATI_READONLY")) { } Evaluator::~Evaluator() { @@ -69,6 +70,8 @@ Var* Evaluator::EvalRHS(Symbol lhs, Value* rhs_v, StringPiece orig_rhs, Var* prev = LookupVarInCurrentScope(lhs); if (!prev->IsDefined()) { rhs = new RecursiveVar(rhs_v, origin, orig_rhs); + } else if (prev->ReadOnly()) { + Error(StringPrintf("*** cannot assign to readonly variable: %s", lhs.c_str())); } else { prev->AppendVar(this, rhs_v); rhs = prev; @@ -101,11 +104,31 @@ void Evaluator::EvalAssign(const AssignStmt* stmt) { Symbol lhs = stmt->GetLhsSymbol(this); if (lhs.empty()) Error("*** empty variable name."); + + if (lhs == kati_readonly_) { + string rhs; + stmt->rhs->Eval(this, &rhs); + for (auto const& name : WordScanner(rhs)) { + Var* var = Intern(name).GetGlobalVar(); + if (!var->IsDefined()) { + Error(StringPrintf("*** unknown variable: %s", name.as_string().c_str())); + } + var->SetReadOnly(); + } + return; + } + Var* rhs = EvalRHS(lhs, stmt->rhs, stmt->orig_rhs, stmt->op, stmt->directive == AssignDirective::OVERRIDE); - if (rhs) + if (rhs) { + bool readonly; lhs.SetGlobalVar(rhs, - stmt->directive == AssignDirective::OVERRIDE); + stmt->directive == AssignDirective::OVERRIDE, + &readonly); + if (readonly) { + Error(StringPrintf("*** cannot assign to readonly variable: %s", lhs.c_str())); + } + } } void Evaluator::EvalRule(const RuleStmt* stmt) { @@ -167,9 +190,29 @@ void Evaluator::EvalRule(const RuleStmt* stmt) { } current_scope_ = p.first->second; + + if (lhs == kati_readonly_) { + string rhs_value; + rhs->Eval(this, &rhs_value); + for (auto const& name : WordScanner(rhs_value)) { + Var* var = current_scope_->Lookup(Intern(name)); + if (!var->IsDefined()) { + Error(StringPrintf("*** unknown variable: %s", name.as_string().c_str())); + } + var->SetReadOnly(); + } + current_scope_ = NULL; + continue; + } + Var* rhs_var = EvalRHS(lhs, rhs, StringPiece("*TODO*"), rule_var.op); - if (rhs_var) - current_scope_->Assign(lhs, new RuleVar(rhs_var, rule_var.op)); + if (rhs_var) { + bool readonly; + current_scope_->Assign(lhs, new RuleVar(rhs_var, rule_var.op), &readonly); + if (readonly) { + Error(StringPrintf("*** cannot assign to readonly variable: %s", lhs.c_str())); + } + } current_scope_ = NULL; } } diff --git a/eval.h b/eval.h index bf8c98a..c0f27b9 100644 --- a/eval.h +++ b/eval.h @@ -125,6 +125,8 @@ class Evaluator { bool is_posix_; static unordered_set used_undefined_vars_; + + Symbol kati_readonly_; }; #endif // EVAL_H_ diff --git a/symtab.cc b/symtab.cc index cbaa4b4..0e51c21 100644 --- a/symtab.cc +++ b/symtab.cc @@ -59,11 +59,20 @@ Var* Symbol::GetGlobalVar() const { return v; } -void Symbol::SetGlobalVar(Var* v, bool is_override) const { +void Symbol::SetGlobalVar(Var* v, bool is_override, bool* readonly) const { if (static_cast(v_) >= g_symbol_data.size()) { g_symbol_data.resize(v_ + 1); } Var* orig = g_symbol_data[v_].gv; + if (orig->ReadOnly()) { + if (readonly != nullptr) + *readonly = true; + else + ERROR("*** cannot assign to readonly variable: %s", c_str()); + return; + } else if (readonly != nullptr) { + *readonly = false; + } if (!is_override && (orig->Origin() == VarOrigin::OVERRIDE || orig->Origin() == VarOrigin::ENVIRONMENT_OVERRIDE)) { diff --git a/symtab.h b/symtab.h index e7e71d5..7e39be9 100644 --- a/symtab.h +++ b/symtab.h @@ -56,7 +56,7 @@ class Symbol { bool IsValid() const { return v_ >= 0; } Var* GetGlobalVar() const; - void SetGlobalVar(Var* v, bool is_override = false) const; + void SetGlobalVar(Var* v, bool is_override = false, bool* readonly = nullptr) const; private: explicit Symbol(int v); diff --git a/testcase/readonly_global.sh b/testcase/readonly_global.sh new file mode 100644 index 0000000..33695f2 --- /dev/null +++ b/testcase/readonly_global.sh @@ -0,0 +1,47 @@ +#!/bin/bash +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -u + +mk="$@" + +function build() { + cat < Makefile +FOO $1 bar +.KATI_READONLY $2 FOO +FOO $3 baz +all: +EOF + + echo "Testcase: $1 $2 $3" + if echo "${mk}" | grep -q "^make"; then + # Make doesn't support .KATI_READONLY + echo "Makefile:3: *** cannot assign to readonly variable: FOO" + else + ${mk} 2>&1 && echo "Clean exit" + fi +} + +build "=" "=" "=" +build "=" "+=" "=" +build "=" ":=" "=" + +build "=" ":=" ":=" +build "=" ":=" "+=" + +build ":=" ":=" ":=" +build ":=" ":=" "+=" +build ":=" ":=" "=" diff --git a/testcase/readonly_global_missing.sh b/testcase/readonly_global_missing.sh new file mode 100644 index 0000000..cab86f2 --- /dev/null +++ b/testcase/readonly_global_missing.sh @@ -0,0 +1,32 @@ +#!/bin/bash +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -u + +mk="$@" + +cat < Makefile +all: FOO = bar +.KATI_READONLY = FOO +all: +EOF + +if echo "${mk}" | grep -q "^make"; then + # Make doesn't support .KATI_READONLY + echo "Makefile:2: *** unknown variable: FOO" +else + ${mk} 2>&1 && echo "Clean exit" +fi diff --git a/testcase/readonly_rule.sh b/testcase/readonly_rule.sh new file mode 100644 index 0000000..676d639 --- /dev/null +++ b/testcase/readonly_rule.sh @@ -0,0 +1,48 @@ +#!/bin/bash +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -u + +mk="$@" + +function build() { + cat < Makefile +all: FOO $1 bar +all: .KATI_READONLY $2 FOO +FOO $3 foo +all: FOO $3 baz +all: +EOF + + echo "Testcase: $1 $2 $3" + if echo "${mk}" | grep -q "^make"; then + # Make doesn't support .KATI_READONLY + echo "Makefile:4: *** cannot assign to readonly variable: FOO" + else + ${mk} 2>&1 && echo "Clean exit" + fi +} + +#build "=" "=" "=" +#build "=" "+=" "=" +#build "=" ":=" "=" +# +#build "=" ":=" ":=" +#build "=" ":=" "+=" +# +#build ":=" ":=" ":=" +build ":=" ":=" "+=" +#build ":=" ":=" "=" diff --git a/testcase/readonly_rule_missing.sh b/testcase/readonly_rule_missing.sh new file mode 100644 index 0000000..6940438 --- /dev/null +++ b/testcase/readonly_rule_missing.sh @@ -0,0 +1,32 @@ +#!/bin/bash +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -u + +mk="$@" + +cat < Makefile +FOO = bar +all: .KATI_READONLY = FOO +all: +EOF + +if echo "${mk}" | grep -q "^make"; then + # Make doesn't support .KATI_READONLY + echo "Makefile:2: *** unknown variable: FOO" +else + ${mk} 2>&1 && echo "Clean exit" +fi diff --git a/var.cc b/var.cc index 4b67c4f..8f46ff8 100644 --- a/var.cc +++ b/var.cc @@ -37,7 +37,7 @@ const char* GetOriginStr(VarOrigin origin) { return "*** broken origin ***"; } -Var::Var() { +Var::Var() : readonly_(false) { } Var::~Var() { @@ -130,10 +130,15 @@ Var* Vars::Lookup(Symbol name) const { return v; } -void Vars::Assign(Symbol name, Var* v) { +void Vars::Assign(Symbol name, Var* v, bool* readonly) { + *readonly = false; auto p = emplace(name, v); if (!p.second) { Var* orig = p.first->second; + if (orig->ReadOnly()) { + *readonly = true; + return; + } if (orig->Origin() == VarOrigin::OVERRIDE || orig->Origin() == VarOrigin::ENVIRONMENT_OVERRIDE) { return; diff --git a/var.h b/var.h index 5fc09fa..75653de 100644 --- a/var.h +++ b/var.h @@ -56,8 +56,14 @@ class Var : public Evaluable { virtual string DebugString() const = 0; + bool ReadOnly() const { return readonly_; } + void SetReadOnly() { readonly_ = true; } + protected: Var(); + + private: + bool readonly_; }; class SimpleVar : public Var { @@ -177,7 +183,7 @@ class Vars : public unordered_map { Var* Lookup(Symbol name) const; - void Assign(Symbol name, Var* v); + void Assign(Symbol name, Var* v, bool* readonly); static void add_used_env_vars(Symbol v); -- cgit v1.2.3 From 9862c2a1dc844d431cf3f9cca41603362cc077c9 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Fri, 30 Sep 2016 19:42:53 -0700 Subject: Add simple benchmark for RunCommand Change-Id: I7f3aabdd8fc01f9ddbcf23e586b6f5e81ab8cbce --- Android.bp | 13 +++++++++++++ Makefile.ckati | 3 ++- fileutil_bench.cc | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 fileutil_bench.cc diff --git a/Android.bp b/Android.bp index 91c2972..5dd8ef5 100644 --- a/Android.bp +++ b/Android.bp @@ -79,3 +79,16 @@ cc_test_host { }, }, } + +cc_benchmark_host { + name: "ckati_fileutil_bench", + srcs: [ + "fileutil_bench.cc", + ], + whole_static_libs: ["libckati"], + target: { + linux: { + host_ldlibs: ["-lrt", "-lpthread"], + }, + }, +} diff --git a/Makefile.ckati b/Makefile.ckati index df4c6a5..e4067bb 100644 --- a/Makefile.ckati +++ b/Makefile.ckati @@ -57,7 +57,8 @@ KATI_CXX_GENERATED_SRCS := \ KATI_CXX_SRCS := $(addprefix $(KATI_SRC_PATH)/,$(KATI_CXX_SRCS)) KATI_CXX_TEST_SRCS := \ $(wildcard $(KATI_SRC_PATH)/*_test.cc) \ - $(wildcard $(KATI_SRC_PATH)/*_bench.cc) + $(filter-out $(KATI_SRC_PATH)/fileutil_bench.cc,\ + $(wildcard $(KATI_SRC_PATH)/*_bench.cc)) KATI_CXX_OBJS := $(patsubst $(KATI_SRC_PATH)/%.cc,$(KATI_INTERMEDIATES_PATH)/%.o,\ $(KATI_CXX_SRCS)) diff --git a/fileutil_bench.cc b/fileutil_bench.cc new file mode 100644 index 0000000..dc07d9a --- /dev/null +++ b/fileutil_bench.cc @@ -0,0 +1,39 @@ +// Copyright 2016 Google Inc. All rights reserved +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "fileutil.h" +#include +#include + +static void BM_RunCommand(benchmark::State& state) { + std::string shell = "/bin/bash -c"; + std::string cmd = "echo $((1+3))"; + while (state.KeepRunning()) { + std::string result; + RunCommand(shell, cmd, RedirectStderr::NONE, &result); + } +} +BENCHMARK(BM_RunCommand); + +static void BM_RunCommand_ComplexShell(benchmark::State& state) { + std::string shell = "/bin/bash -c"; + std::string cmd = "echo $((1+3))"; + while (state.KeepRunning()) { + std::string result; + RunCommand(shell, cmd, RedirectStderr::NONE, &result); + } +} +BENCHMARK(BM_RunCommand_ComplexShell); + +BENCHMARK_MAIN(); -- cgit v1.2.3 From 064be227bfc5948a74e034fd613d556a49e8b49b Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Fri, 30 Sep 2016 20:17:14 -0700 Subject: Optimize RunCommand by removing /bin/sh wrapper when possible For every $(shell echo "test: $PATH") command, when SHELL is /bin/bash, we essentially run: (each arg wrapped in []) [/bin/sh] [-c] [/bin/bash -c "echo \"test: \$PATH\""] This is redundant, since we can just use SHELL, and then we don't need to do an extra level of shell escaping either. This change makes us run this instead: [/bin/bash] [-c] [echo "test: $PATH"] If SHELL is more complicated than an absolute path to a binary, then we'll fall back to /bin/sh. Using the benchmark introduced in the last change, this reduces a minimal RunCommand execution with a simple SHELL from 3.7ms to 1.3ms. For a more complex benchmark (though less normalized), for an AOSP Android build, this change shrinks the average time spent in $(shell) functions from 4.5ms to 3ms. Change-Id: I622116e33565e58bb123ee9e9bdd302616a6609c --- exec.cc | 7 +++++-- fileutil.cc | 25 +++++++++++++++++-------- fileutil.h | 4 ++-- fileutil_bench.cc | 10 ++++++---- func.cc | 12 +++++++----- func.h | 1 + ninja.cc | 1 + regen.cc | 4 +++- 8 files changed, 42 insertions(+), 22 deletions(-) diff --git a/exec.cc b/exec.cc index 6065169..1569519 100644 --- a/exec.cc +++ b/exec.cc @@ -46,7 +46,8 @@ class Executor { explicit Executor(Evaluator* ev) : ce_(ev), num_commands_(0) { - shell_ = ev->GetShellAndFlag(); + shell_ = ev->GetShell(); + shellflag_ = ev->GetShellFlag(); } double ExecNode(DepNode* n, DepNode* needed_by) { @@ -106,7 +107,8 @@ class Executor { } if (!g_flags.is_dry_run) { string out; - int result = RunCommand(shell_, command->cmd.c_str(), + int result = RunCommand(shell_, shellflag_, + command->cmd.c_str(), RedirectStderr::STDOUT, &out); printf("%s", out.c_str()); @@ -136,6 +138,7 @@ class Executor { CommandEvaluator ce_; unordered_map done_; string shell_; + string shellflag_; uint64_t num_commands_; }; diff --git a/fileutil.cc b/fileutil.cc index 0d3c2d6..4cf653b 100644 --- a/fileutil.cc +++ b/fileutil.cc @@ -60,15 +60,24 @@ double GetTimestamp(StringPiece filename) { return GetTimestampFromStat(st); } -int RunCommand(const string& shell, const string& cmd, - RedirectStderr redirect_stderr, +int RunCommand(const string& shell, const string& shellflag, + const string& cmd, RedirectStderr redirect_stderr, string* s) { - string cmd_escaped = cmd; - EscapeShell(&cmd_escaped); - string cmd_with_shell = shell + " \"" + cmd_escaped + "\""; - const char* argv[] = { - "/bin/sh", "-c", cmd_with_shell.c_str(), NULL - }; + const char* argv[] = { NULL, NULL, NULL, NULL }; + string cmd_with_shell; + if (shell[0] != '/' || shell.find_first_of(" $") != string::npos) { + string cmd_escaped = cmd; + EscapeShell(&cmd_escaped); + cmd_with_shell = shell + " " + shellflag + " \"" + cmd_escaped + "\""; + argv[0] = "/bin/sh"; + argv[1] = "-c"; + argv[2] = cmd_with_shell.c_str(); + } else { + // If the shell isn't complicated, we don't need to wrap in /bin/sh + argv[0] = shell.c_str(); + argv[1] = shellflag.c_str(); + argv[2] = cmd.c_str(); + } int pipefd[2]; if (pipe(pipefd) != 0) diff --git a/fileutil.h b/fileutil.h index 7cc3c8e..264d9e1 100644 --- a/fileutil.h +++ b/fileutil.h @@ -36,8 +36,8 @@ enum struct RedirectStderr { DEV_NULL, }; -int RunCommand(const string& shell, const string& cmd, - RedirectStderr redirect_stderr, +int RunCommand(const string& shell, const string& shellflag, + const string& cmd, RedirectStderr redirect_stderr, string* out); void GetExecutablePath(string* path); diff --git a/fileutil_bench.cc b/fileutil_bench.cc index dc07d9a..4b1f033 100644 --- a/fileutil_bench.cc +++ b/fileutil_bench.cc @@ -17,21 +17,23 @@ #include static void BM_RunCommand(benchmark::State& state) { - std::string shell = "/bin/bash -c"; + std::string shell = "/bin/bash"; + std::string shellflag = "-c"; std::string cmd = "echo $((1+3))"; while (state.KeepRunning()) { std::string result; - RunCommand(shell, cmd, RedirectStderr::NONE, &result); + RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result); } } BENCHMARK(BM_RunCommand); static void BM_RunCommand_ComplexShell(benchmark::State& state) { - std::string shell = "/bin/bash -c"; + std::string shell = "/bin/bash "; + std::string shellflag = "-c"; std::string cmd = "echo $((1+3))"; while (state.KeepRunning()) { std::string result; - RunCommand(shell, cmd, RedirectStderr::NONE, &result); + RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result); } } BENCHMARK(BM_RunCommand_ComplexShell); diff --git a/func.cc b/func.cc index 623e56e..7915aea 100644 --- a/func.cc +++ b/func.cc @@ -491,8 +491,8 @@ static bool HasNoIoInShellScript(const string& cmd) { return false; } -static void ShellFuncImpl(const string& shell, const string& cmd, - string* s, FindCommand** fc) { +static void ShellFuncImpl(const string& shell, const string& shellflag, + const string& cmd, string* s, FindCommand** fc) { LOG("ShellFunc: %s", cmd.c_str()); #ifdef TEST_FIND_EMULATOR @@ -517,7 +517,7 @@ static void ShellFuncImpl(const string& shell, const string& cmd, } COLLECT_STATS_WITH_SLOW_REPORT("func shell time", cmd.c_str()); - RunCommand(shell, cmd, RedirectStderr::NONE, s); + RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, s); FormatForCommandSubstitution(s); #ifdef TEST_FIND_EMULATOR @@ -564,14 +564,16 @@ void ShellFunc(const vector& args, Evaluator* ev, string* s) { return; } - const string&& shell = ev->GetShellAndFlag(); + const string&& shell = ev->GetShell(); + const string&& shellflag = ev->GetShellFlag(); string out; FindCommand* fc = NULL; - ShellFuncImpl(shell, cmd, &out, &fc); + ShellFuncImpl(shell, shellflag, cmd, &out, &fc); if (ShouldStoreCommandResult(cmd)) { CommandResult* cr = new CommandResult(); cr->shell = shell; + cr->shellflag = shellflag; cr->cmd = cmd; cr->find.reset(fc); cr->result = out; diff --git a/func.h b/func.h index e78deb7..82e821c 100644 --- a/func.h +++ b/func.h @@ -43,6 +43,7 @@ struct FindCommand; struct CommandResult { string shell; + string shellflag; string cmd; unique_ptr find; string result; diff --git a/ninja.cc b/ninja.cc index 90fe404..e2bc9cd 100644 --- a/ninja.cc +++ b/ninja.cc @@ -737,6 +737,7 @@ class NinjaGenerator { DumpInt(fp, crs.size()); for (CommandResult* cr : crs) { DumpString(fp, cr->shell); + DumpString(fp, cr->shellflag); DumpString(fp, cr->cmd); DumpString(fp, cr->result); if (!cr->find.get()) { diff --git a/regen.cc b/regen.cc index f5ed50a..a322691 100644 --- a/regen.cc +++ b/regen.cc @@ -53,6 +53,7 @@ class StampChecker { struct ShellResult { string shell; + string shellflag; string cmd; string result; vector missing_dirs; @@ -232,6 +233,7 @@ class StampChecker { ShellResult* sr = new ShellResult; commands_.push_back(sr); LOAD_STRING(fp, &sr->shell); + LOAD_STRING(fp, &sr->shellflag); LOAD_STRING(fp, &sr->cmd); LOAD_STRING(fp, &sr->result); sr->has_condition = LOAD_INT(fp); @@ -340,7 +342,7 @@ class StampChecker { COLLECT_STATS_WITH_SLOW_REPORT("shell time (regen)", sr->cmd.c_str()); string result; - RunCommand(sr->shell, sr->cmd, RedirectStderr::DEV_NULL, &result); + RunCommand(sr->shell, sr->shellflag, sr->cmd, RedirectStderr::DEV_NULL, &result); FormatForCommandSubstitution(&result); if (sr->result != result) { if (g_flags.dump_kati_stamp) { -- cgit v1.2.3 From d325f591edfafc34167a2f23a2295c434136940b Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Mon, 3 Oct 2016 01:00:08 -0700 Subject: Run regen commands sequentially When I remove --ignore-dirty=$(OUT_DIR)/% from an Android build, it turns out that we have $(shell) commands that have side effects and cannot be reordered in relationship to one another. The trivial case is a sequence of 'rm', 'write', 'write'..., 'cmp'. The 'rm' and 'write' commands can be collapsed into a single $(file) function call, but it still needs to be ordered for the 'cmp' function to work properly. I've been making changes to prevent this from slowing down regen too much, but it's still a 2-3x slowdown overall (0.3s -> 0.8s for aosp_arm64-eng on aosp master). --- regen.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/regen.cc b/regen.cc index f5ed50a..f7db27d 100644 --- a/regen.cc +++ b/regen.cc @@ -375,8 +375,8 @@ class StampChecker { } }); - for (ShellResult* sr : commands_) { - tp->Submit([this, sr]() { + tp->Submit([this]() { + for (ShellResult* sr : commands_) { string err; if (CheckShellResult(sr, &err)) { unique_lock lock(mu_); @@ -385,8 +385,8 @@ class StampChecker { msg_ = err; } } - }); - } + } + }); tp->Wait(); if (needs_regen_) { -- cgit v1.2.3 From f06d8019e99ae6aee1d2881f30315aee7b544cfb Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Mon, 3 Oct 2016 00:16:07 -0700 Subject: Implement the `file` function to read and write files This allows us to do file reading and writing without $(shell). Besides being simpler, this also allows faster regen times, since we can just stat the files to be read, or directly write to the files that need to be written. --- func.cc | 123 +++++++++++++++++++++++++++++++++ func.h | 10 +++ ninja.cc | 36 +++++----- regen.cc | 77 +++++++++++++++++---- testcase/file_func.sh | 47 +++++++++++++ testcase/ninja_regen_filefunc_read.sh | 74 ++++++++++++++++++++ testcase/ninja_regen_filefunc_write.sh | 46 ++++++++++++ 7 files changed, 379 insertions(+), 34 deletions(-) create mode 100755 testcase/file_func.sh create mode 100755 testcase/ninja_regen_filefunc_read.sh create mode 100755 testcase/ninja_regen_filefunc_write.sh diff --git a/func.cc b/func.cc index 623e56e..1c666ec 100644 --- a/func.cc +++ b/func.cc @@ -17,9 +17,11 @@ #include "func.h" #include +#include #include #include #include +#include #include #include @@ -571,6 +573,7 @@ void ShellFunc(const vector& args, Evaluator* ev, string* s) { ShellFuncImpl(shell, cmd, &out, &fc); if (ShouldStoreCommandResult(cmd)) { CommandResult* cr = new CommandResult(); + cr->op = (fc == NULL) ? CommandOp::SHELL : CommandOp::FIND, cr->shell = shell; cr->cmd = cmd; cr->find.reset(fc); @@ -686,6 +689,124 @@ void ErrorFunc(const vector& args, Evaluator* ev, string*) { ev->Error(StringPrintf("*** %s.", a.c_str())); } +static void FileReadFunc(Evaluator* ev, const string& filename, string* s) { + int fd = open(filename.c_str(), O_RDONLY); + if (fd < 0) { + if (errno == ENOENT) { + if (ShouldStoreCommandResult(filename)) { + CommandResult* cr = new CommandResult(); + cr->op = CommandOp::READ_MISSING; + cr->cmd = filename; + g_command_results.push_back(cr); + } + return; + } else { + ev->Error("*** open failed."); + } + } + + struct stat st; + if (fstat(fd, &st) < 0) { + ev->Error("*** fstat failed."); + } + + size_t len = st.st_size; + string out; + out.resize(len); + ssize_t r = HANDLE_EINTR(read(fd, &out[0], len)); + if (r != static_cast(len)) { + ev->Error("*** read failed."); + } + + if (close(fd) < 0) { + ev->Error("*** close failed."); + } + + if (out.back() == '\n') { + out.pop_back(); + } + + if (ShouldStoreCommandResult(filename)) { + CommandResult* cr = new CommandResult(); + cr->op = CommandOp::READ; + cr->cmd = filename; + g_command_results.push_back(cr); + } + *s += out; +} + +static void FileWriteFunc(Evaluator* ev, const string& filename, bool append, string text) { + FILE* f = fopen(filename.c_str(), append ? "ab" : "wb"); + if (f == NULL) { + ev->Error("*** fopen failed."); + } + + if (fwrite(&text[0], text.size(), 1, f) != 1) { + ev->Error("*** fwrite failed."); + } + + if (fclose(f) != 0) { + ev->Error("*** fclose failed."); + } + + if (ShouldStoreCommandResult(filename)) { + CommandResult* cr = new CommandResult(); + cr->op = CommandOp::WRITE; + cr->cmd = filename; + cr->result = text; + g_command_results.push_back(cr); + } +} + +void FileFunc(const vector& args, Evaluator* ev, string* s) { + if (ev->avoid_io()) { + ev->Error("*** $(file ...) is not supported in rules."); + } + + string arg = args[0]->Eval(ev); + StringPiece filename = TrimSpace(arg); + + if (filename.size() <= 1) { + ev->Error("*** Missing filename"); + } + + if (filename[0] == '<') { + filename = TrimLeftSpace(filename.substr(1)); + if (!filename.size()) { + ev->Error("*** Missing filename"); + } + if (args.size() > 1) { + ev->Error("*** invalid argument"); + } + + FileReadFunc(ev, filename.as_string(), s); + } else if (filename[0] == '>') { + bool append = false; + if (filename[1] == '>') { + append = true; + filename = filename.substr(2); + } else { + filename = filename.substr(1); + } + filename = TrimLeftSpace(filename); + if (!filename.size()) { + ev->Error("*** Missing filename"); + } + + string text; + if (args.size() > 1) { + text = args[1]->Eval(ev); + if (text.size() == 0 || text.back() != '\n') { + text.push_back('\n'); + } + } + + FileWriteFunc(ev, filename.as_string(), append, text); + } else { + ev->Error(StringPrintf("*** Invalid file operation: %s. Stop.", filename.as_string().c_str())); + } +} + FuncInfo g_func_infos[] = { { "patsubst", &PatsubstFunc, 3, 3, false, false }, { "strip", &StripFunc, 1, 1, false, false }, @@ -727,6 +848,8 @@ FuncInfo g_func_infos[] = { { "info", &InfoFunc, 1, 1, false, false }, { "warning", &WarningFunc, 1, 1, false, false }, { "error", &ErrorFunc, 1, 1, false, false }, + + { "file", &FileFunc, 2, 1, false, false }, }; unordered_map* g_func_info_map; diff --git a/func.h b/func.h index e78deb7..ab6affb 100644 --- a/func.h +++ b/func.h @@ -41,7 +41,17 @@ FuncInfo* GetFuncInfo(StringPiece name); struct FindCommand; +enum struct CommandOp { + SHELL, + FIND, + READ, + READ_MISSING, + WRITE, + APPEND, +}; + struct CommandResult { + CommandOp op; string shell; string cmd; unique_ptr find; diff --git a/ninja.cc b/ninja.cc index 90fe404..59e8712 100644 --- a/ninja.cc +++ b/ninja.cc @@ -736,31 +736,27 @@ class NinjaGenerator { const vector& crs = GetShellCommandResults(); DumpInt(fp, crs.size()); for (CommandResult* cr : crs) { + DumpInt(fp, static_cast(cr->op)); DumpString(fp, cr->shell); DumpString(fp, cr->cmd); DumpString(fp, cr->result); - if (!cr->find.get()) { - // Always re-run this command. - DumpInt(fp, 0); - continue; - } - DumpInt(fp, 1); - - vector missing_dirs; - for (StringPiece fd : cr->find->finddirs) { - const string& d = ConcatDir(cr->find->chdir, fd); - if (!Exists(d)) - missing_dirs.push_back(d); - } - DumpInt(fp, missing_dirs.size()); - for (const string& d : missing_dirs) { - DumpString(fp, d); - } + if (cr->op == CommandOp::FIND) { + vector missing_dirs; + for (StringPiece fd : cr->find->finddirs) { + const string& d = ConcatDir(cr->find->chdir, fd); + if (!Exists(d)) + missing_dirs.push_back(d); + } + DumpInt(fp, missing_dirs.size()); + for (const string& d : missing_dirs) { + DumpString(fp, d); + } - DumpInt(fp, cr->find->read_dirs->size()); - for (StringPiece s : *cr->find->read_dirs) { - DumpString(fp, ConcatDir(cr->find->chdir, s)); + DumpInt(fp, cr->find->read_dirs->size()); + for (StringPiece s : *cr->find->read_dirs) { + DumpString(fp, ConcatDir(cr->find->chdir, s)); + } } } diff --git a/regen.cc b/regen.cc index f5ed50a..bbae7c8 100644 --- a/regen.cc +++ b/regen.cc @@ -23,6 +23,7 @@ #include "fileutil.h" #include "find.h" +#include "func.h" #include "io.h" #include "log.h" #include "ninja.h" @@ -52,12 +53,12 @@ class StampChecker { }; struct ShellResult { + CommandOp op; string shell; string cmd; string result; vector missing_dirs; vector read_dirs; - bool has_condition; }; public: @@ -231,22 +232,22 @@ class StampChecker { for (int i = 0; i < num_crs; i++) { ShellResult* sr = new ShellResult; commands_.push_back(sr); + sr->op = static_cast(LOAD_INT(fp)); LOAD_STRING(fp, &sr->shell); LOAD_STRING(fp, &sr->cmd); LOAD_STRING(fp, &sr->result); - sr->has_condition = LOAD_INT(fp); - if (!sr->has_condition) - continue; - int num_missing_dirs = LOAD_INT(fp); - for (int j = 0; j < num_missing_dirs; j++) { - LOAD_STRING(fp, &s); - sr->missing_dirs.push_back(s); - } - int num_read_dirs = LOAD_INT(fp); - for (int j = 0; j < num_read_dirs; j++) { - LOAD_STRING(fp, &s); - sr->read_dirs.push_back(s); + if (sr->op == CommandOp::FIND) { + int num_missing_dirs = LOAD_INT(fp); + for (int j = 0; j < num_missing_dirs; j++) { + LOAD_STRING(fp, &s); + sr->missing_dirs.push_back(s); + } + int num_read_dirs = LOAD_INT(fp); + for (int j = 0; j < num_read_dirs; j++) { + LOAD_STRING(fp, &s); + sr->read_dirs.push_back(s); + } } } @@ -292,7 +293,7 @@ class StampChecker { } bool ShouldRunCommand(const ShellResult* sr) { - if (!sr->has_condition) + if (sr->op != CommandOp::FIND) return true; COLLECT_STATS("stat time (regen)"); @@ -324,6 +325,54 @@ class StampChecker { } bool CheckShellResult(const ShellResult* sr, string* err) { + if (sr->op == CommandOp::READ_MISSING) { + if (Exists(sr->cmd)) { + if (g_flags.dump_kati_stamp) + printf("file %s: dirty\n", sr->cmd.c_str()); + else + *err = StringPrintf("$(file <%s) was changed, regenerating...\n", + sr->cmd.c_str()); + return true; + } + if (g_flags.dump_kati_stamp) + printf("file %s: clean\n", sr->cmd.c_str()); + return false; + } + + if (sr->op == CommandOp::READ) { + double ts = GetTimestamp(sr->cmd); + if (gen_time_ < ts) { + if (g_flags.dump_kati_stamp) + printf("file %s: dirty\n", sr->cmd.c_str()); + else + *err = StringPrintf("$(file <%s) was changed, regenerating...\n", + sr->cmd.c_str()); + return true; + } + if (g_flags.dump_kati_stamp) + printf("file %s: clean\n", sr->cmd.c_str()); + return false; + } + + if (sr->op == CommandOp::WRITE || sr->op == CommandOp::APPEND) { + FILE* f = fopen(sr->cmd.c_str(), (sr->op == CommandOp::WRITE) ? "wb" : "ab"); + if (f == NULL) { + PERROR("fopen"); + } + + if (fwrite(&sr->result[0], sr->result.size(), 1, f) != 1) { + PERROR("fwrite"); + } + + if (fclose(f) != 0) { + PERROR("fclose"); + } + + if (g_flags.dump_kati_stamp) + printf("file %s: clean (write)\n", sr->cmd.c_str()); + return false; + } + if (!ShouldRunCommand(sr)) { if (g_flags.regen_debug) printf("shell %s: clean (no rerun)\n", sr->cmd.c_str()); diff --git a/testcase/file_func.sh b/testcase/file_func.sh new file mode 100755 index 0000000..72935dc --- /dev/null +++ b/testcase/file_func.sh @@ -0,0 +1,47 @@ +#!/bin/sh +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e + +mk="$@" + +echo "PASS" >testfile + +cat < Makefile +ifdef KATI +SUPPORTS_FILE := 1 +endif +ifneq (,\$(filter 4.2%,\$(MAKE_VERSION))) +SUPPORTS_FILE := 1 +endif + +ifdef SUPPORTS_FILE + \$(file >testwrite,PASS) + \$(info Read not found: \$(if \$(file &1 diff --git a/testcase/ninja_regen_filefunc_read.sh b/testcase/ninja_regen_filefunc_read.sh new file mode 100755 index 0000000..7fc3a9f --- /dev/null +++ b/testcase/ninja_regen_filefunc_read.sh @@ -0,0 +1,74 @@ +#!/bin/sh +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e + +log=/tmp/log +mk="$@" + +sleep_if_necessary() { + if [ x$(uname) != x"Linux" -o x"${TRAVIS}" != x"" ]; then + sleep "$@" + fi +} + +cat < Makefile +A := \$(file ${log} +if [ -e ninja.sh ]; then + ./ninja.sh +fi + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep regenerating ${log}; then + echo 'Should not be regenerated' + fi + ./ninja.sh +fi + +echo regen >file_a + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if ! grep regenerating ${log} >/dev/null; then + echo 'Should be regenerated' + fi + ./ninja.sh +fi + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep regenerating ${log}; then + echo 'Should not be regenerated' + fi + ./ninja.sh +fi + +sleep_if_necessary 1 +echo regen >>file_a + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if ! grep regenerating ${log} >/dev/null; then + echo 'Should be regenerated' + fi + ./ninja.sh +fi diff --git a/testcase/ninja_regen_filefunc_write.sh b/testcase/ninja_regen_filefunc_write.sh new file mode 100755 index 0000000..cb438cc --- /dev/null +++ b/testcase/ninja_regen_filefunc_write.sh @@ -0,0 +1,46 @@ +#!/bin/sh +# +# Copyright 2016 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e + +log=/tmp/log +mk="$@" + +cat < Makefile +\$(file >file_a,test) +all: + echo foo +EOF + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if [ ! -f file_a ]; then + echo 'file_a does not exist' + fi + ./ninja.sh + rm file_a +fi + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep regenerating ${log}; then + echo 'Should not be regenerated' + fi + if [ ! -f file_a ]; then + echo 'file_a does not exist' + fi + ./ninja.sh +fi -- cgit v1.2.3