From cb2ff8558ced3ddc021823a3c86bc7ce02dcbdc3 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Tue, 1 Nov 2016 14:49:08 -0700 Subject: Fix some possible performance issues found by clang-tidy No obvious time differences when building AOSP, but these all seem like reasonable changes. --- Android.bp | 5 +++++ dep.cc | 2 +- rule.cc | 2 +- rule.h | 2 +- strutil.cc | 2 +- strutil.h | 2 +- 6 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Android.bp b/Android.bp index f95b366..8dbb9cb 100644 --- a/Android.bp +++ b/Android.bp @@ -25,6 +25,11 @@ cc_defaults { host_ldlibs: ["-lrt", "-lpthread"], }, }, + tidy_checks: [ + "-google-global-names-in-headers", + "-google-build-using-namespace", + "-google-explicit-constructor", + ], } cc_library_host_static { diff --git a/dep.cc b/dep.cc index 502ec47..4bdf9ab 100644 --- a/dep.cc +++ b/dep.cc @@ -534,7 +534,7 @@ class DepBuilder { if (found == suffix_rules_.end()) return rule_merger; - for (shared_ptr irule : found->second) { + for (const shared_ptr &irule : found->second) { CHECK(irule->inputs.size() == 1); Symbol input = ReplaceSuffix(output, irule->inputs[0]); if (!Exists(input)) diff --git a/rule.cc b/rule.cc index 68ec7f5..6942fb5 100644 --- a/rule.cc +++ b/rule.cc @@ -54,7 +54,7 @@ Rule::Rule() } void ParseRule(Loc& loc, StringPiece line, char term, - function after_term_fn, + const function &after_term_fn, Rule** out_rule, RuleVarAssignment* rule_var) { size_t index = line.find(':'); if (index == string::npos) { diff --git a/rule.h b/rule.h index 4d72bb7..84412c1 100644 --- a/rule.h +++ b/rule.h @@ -65,7 +65,7 @@ struct RuleVarAssignment { // |term| is '='), |after_term_fn| will be called to obtain the right // hand side. void ParseRule(Loc& loc, StringPiece line, char term, - function after_term_fn, + const function &after_term_fn, Rule** rule, RuleVarAssignment* rule_var); #endif // RULE_H_ diff --git a/strutil.cc b/strutil.cc index 1b99dff..b094198 100644 --- a/strutil.cc +++ b/strutil.cc @@ -500,7 +500,7 @@ string ConcatDir(StringPiece b, StringPiece n) { return r; } -string EchoEscape(const string str) { +string EchoEscape(const string &str) { const char *in = str.c_str(); string buf; for (; *in; in++) { diff --git a/strutil.h b/strutil.h index 12e46a5..d61160d 100644 --- a/strutil.h +++ b/strutil.h @@ -140,7 +140,7 @@ string SortWordsInString(StringPiece s); string ConcatDir(StringPiece b, StringPiece n); -string EchoEscape(const string str); +string EchoEscape(const string &str); void EscapeShell(string* s); -- cgit v1.2.3 From 2f75ffadfbd550b2efd68816c33918c86554ed4b Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Fri, 4 Nov 2016 16:57:57 -0700 Subject: Allow rules to specify custom ninja pools Setting .KATI_NINJA_POOL as a rule variable will set the corresponding pool variable in the ninja file. There's no way to define custom pools in Kati, Android is planning on scaling the pool depth in a parent ninja file without re-running Kati. --- dep.cc | 7 ++++++- dep.h | 1 + ninja.cc | 7 ++++++- testcase/ninja_pool.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 testcase/ninja_pool.sh diff --git a/dep.cc b/dep.cc index 4bdf9ab..4aebae0 100644 --- a/dep.cc +++ b/dep.cc @@ -231,6 +231,7 @@ DepNode::DepNode(Symbol o, bool p, bool r) is_restat(r), rule_vars(NULL), depfile_var(NULL), + ninja_pool_var(NULL), output_pattern(Symbol::IsUninitialized()) { g_dep_node_pool->push_back(this); } @@ -244,7 +245,8 @@ class DepBuilder { rule_vars_(rule_vars), implicit_rules_(new RuleTrie()), first_rule_(Symbol::IsUninitialized{}), - depfile_var_name_(Intern(".KATI_DEPFILE")) { + depfile_var_name_(Intern(".KATI_DEPFILE")), + ninja_pool_var_name_(Intern(".KATI_NINJA_POOL")) { ScopedTimeReporter tr("make dep (populate)"); PopulateRules(rules); // TODO? @@ -607,6 +609,8 @@ class DepBuilder { if (name == depfile_var_name_) { n->depfile_var = new_var; + } else if (name == ninja_pool_var_name_) { + n->ninja_pool_var = new_var; } else { sv.emplace_back(new ScopedVar(cur_rule_vars_.get(), name, new_var)); } @@ -651,6 +655,7 @@ class DepBuilder { unordered_set phony_; unordered_set restat_; Symbol depfile_var_name_; + Symbol ninja_pool_var_name_; }; void MakeDep(Evaluator* ev, diff --git a/dep.h b/dep.h index 4302997..5e879e3 100644 --- a/dep.h +++ b/dep.h @@ -45,6 +45,7 @@ struct DepNode { vector actual_order_only_inputs; Vars* rule_vars; Var* depfile_var; + Var* ninja_pool_var; Symbol output_pattern; Loc loc; }; diff --git a/ninja.cc b/ninja.cc index 579cea3..fca3ace 100644 --- a/ninja.cc +++ b/ninja.cc @@ -561,8 +561,13 @@ class NinjaGenerator { } } *o << "\n"; - if (use_local_pool) + if (node->ninja_pool_var) { + string pool; + node->ninja_pool_var->Eval(ev_, &pool); + *o << " pool = " << pool << "\n"; + } else if (use_local_pool) { *o << " pool = local_pool\n"; + } if (node->is_default_target) { unique_lock lock(mu_); default_target_ = node; diff --git a/testcase/ninja_pool.sh b/testcase/ninja_pool.sh new file mode 100644 index 0000000..5fed8e4 --- /dev/null +++ b/testcase/ninja_pool.sh @@ -0,0 +1,43 @@ +#!/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 + +test: .KATI_NINJA_POOL := test_pool +test: + echo "PASS" +EOF + +${mk} 2>${log} +if [ -e ninja.sh ]; then + mv build.ninja kati.ninja + cat < build.ninja +pool test_pool + depth = 1 +include kati.ninja +EOF + ./ninja.sh +fi +if [ -e ninja.sh ]; then + if ! grep -q "pool = test_pool" kati.ninja; then + echo "Pool not present in build.ninja" + fi +fi -- cgit v1.2.3 From e99f57f8b49cf662e9149c962b7ffbcafc9a07fc Mon Sep 17 00:00:00 2001 From: Koichi Shiraishi Date: Tue, 6 Sep 2016 14:56:35 +0900 Subject: [go] fix stat.Dev type to uint64 Signed-off-by: Koichi Shiraishi --- pathutil.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pathutil.go b/pathutil.go index 0d3aa05..ad11c22 100644 --- a/pathutil.go +++ b/pathutil.go @@ -191,7 +191,7 @@ func (c *fsCacheT) readdir(dir string, id fileid) (fileid, []dirent) { return invalidFileid, nil } if stat, ok := fi.Sys().(*syscall.Stat_t); ok { - id = fileid{dev: stat.Dev, ino: stat.Ino} + id = fileid{dev: uint64(stat.Dev), ino: stat.Ino} } names, _ := d.Readdirnames(-1) // need sort? @@ -209,7 +209,7 @@ func (c *fsCacheT) readdir(dir string, id fileid) (fileid, []dirent) { mode := lmode var id fileid if stat, ok := fi.Sys().(*syscall.Stat_t); ok { - id = fileid{dev: stat.Dev, ino: stat.Ino} + id = fileid{dev: uint64(stat.Dev), ino: stat.Ino} } if lmode&os.ModeSymlink == os.ModeSymlink { fi, err = os.Stat(path) @@ -218,7 +218,7 @@ func (c *fsCacheT) readdir(dir string, id fileid) (fileid, []dirent) { } else { mode = fi.Mode() if stat, ok := fi.Sys().(*syscall.Stat_t); ok { - id = fileid{dev: stat.Dev, ino: stat.Ino} + id = fileid{dev: uint64(stat.Dev), ino: stat.Ino} } } } -- cgit v1.2.3 From 0a2453a1bcd3ee9113a8d695113c5de6cb1ed542 Mon Sep 17 00:00:00 2001 From: Koichi Shiraishi Date: Tue, 6 Sep 2016 14:56:59 +0900 Subject: [go] fix ldflags foramt to add '=' for -X flag - go has been changed ldflags parse format. Must be -X key=value. Signed-off-by: Koichi Shiraishi --- Makefile.kati | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.kati b/Makefile.kati index fa29d62..df91cbb 100644 --- a/Makefile.kati +++ b/Makefile.kati @@ -22,7 +22,7 @@ endif kati: go_src_stamp -rm -f out/bin/kati - GOPATH=${KATI_GOPATH} go install -ldflags "-X github.com/google/kati.gitVersion $(shell git rev-parse HEAD)" github.com/google/kati/cmd/kati + GOPATH=${KATI_GOPATH} go install -ldflags "-X github.com/google/kati.gitVersion=$(shell git rev-parse HEAD)" github.com/google/kati/cmd/kati cp out/bin/kati $@ go_src_stamp: $(GO_SRCS) $(wildcard cmd/*/*.go) -- cgit v1.2.3 From df8cd0596bc8b7a92767d9290fdce59fd4355460 Mon Sep 17 00:00:00 2001 From: Koichi Shiraishi Date: Tue, 6 Sep 2016 15:05:35 +0900 Subject: [C++] add +build ignore magic comment for go build Signed-off-by: Koichi Shiraishi --- affinity.cc | 2 ++ fileutil_bench.cc | 2 ++ regen.cc | 2 ++ strutil_bench.cc | 2 ++ thread_pool.cc | 2 ++ 5 files changed, 10 insertions(+) diff --git a/affinity.cc b/affinity.cc index 0743f94..2101fdb 100644 --- a/affinity.cc +++ b/affinity.cc @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build ignore + #include "affinity.h" #include "flags.h" diff --git a/fileutil_bench.cc b/fileutil_bench.cc index 4b1f033..77a0d35 100644 --- a/fileutil_bench.cc +++ b/fileutil_bench.cc @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build ignore + #include "fileutil.h" #include #include diff --git a/regen.cc b/regen.cc index 137d205..5b42d8f 100644 --- a/regen.cc +++ b/regen.cc @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build ignore + #include "regen.h" #include diff --git a/strutil_bench.cc b/strutil_bench.cc index c52e43c..d3b2e6c 100644 --- a/strutil_bench.cc +++ b/strutil_bench.cc @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build ignore + #include #include diff --git a/thread_pool.cc b/thread_pool.cc index 0a12bfa..b2429cb 100644 --- a/thread_pool.cc +++ b/thread_pool.cc @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build ignore + #include "thread_pool.h" #include -- cgit v1.2.3 From d02813b7baf3ab6e7c7b156e89b315445a454fce Mon Sep 17 00:00:00 2001 From: Koichi Shiraishi Date: Tue, 29 Nov 2016 17:48:11 +0900 Subject: Add Koichi Shiraishi to AUTHORS/CONTRIBUTORS Signed-off-by: Koichi Shiraishi --- AUTHORS | 1 + CONTRIBUTORS | 1 + 2 files changed, 2 insertions(+) diff --git a/AUTHORS b/AUTHORS index 3ca8fe5..553b76d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -9,5 +9,6 @@ # Please keep the list sorted. Google Inc. +Koichi Shiraishi Kouhei Sutou Po Hu diff --git a/CONTRIBUTORS b/CONTRIBUTORS index c153787..3c2e7d4 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -25,6 +25,7 @@ Colin Cross Dan Willemsen Fumitoshi Ukai +Koichi Shiraishi Kouhei Sutou Po Hu Ryo Hashimoto -- cgit v1.2.3 From e80373b37bb338b778071c8cd22706752351a4fd Mon Sep 17 00:00:00 2001 From: Koichi Shiraishi Date: Fri, 2 Dec 2016 04:40:00 +0900 Subject: [AUTHORS] Fix kati instead of glog Signed-off-by: Koichi Shiraishi --- AUTHORS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 553b76d..0acaa9e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1,4 +1,4 @@ -# This is the official list of glog authors for copyright purposes. +# This is the official list of kati authors for copyright purposes. # This file is distinct from the CONTRIBUTORS files. # See the latter for an explanation. # -- cgit v1.2.3 From e41c7556c22bda359c2b97cd98d59082110add95 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 22 Feb 2017 14:31:16 -0800 Subject: Add --color_warnings to make warnings/errors like clang This adds new (WARN|KATI_WARN|ERROR)_LOC log macro variants that take a location as the first argument, and will prefix that location information to the warning/error lines. When --color_warnings is enabled, it reformats them to have a standard warning:/error: infix, and adds colors in order to match the warnings/errors produced by clang. --- dep.cc | 19 ++++++++++--------- eval.cc | 2 +- expr.cc | 24 ++++++++++++------------ flags.cc | 2 ++ flags.h | 1 + func.cc | 17 ++++++++--------- log.cc | 41 +++++++++++++++++++++++++++++++++++++++++ log.h | 19 +++++++++++++++++++ main.cc | 4 ++-- parser.cc | 12 ++++++------ rule.cc | 22 ++++++++++------------ rule.h | 2 +- runtest.rb | 2 +- strutil.cc | 7 +++++++ strutil.h | 2 ++ strutil_test.cc | 16 ++++++++++++++++ 16 files changed, 139 insertions(+), 53 deletions(-) diff --git a/dep.cc b/dep.cc index 4aebae0..75dff76 100644 --- a/dep.cc +++ b/dep.cc @@ -156,16 +156,18 @@ struct RuleMerger { if (rules.empty()) { is_double_colon = r->is_double_colon; } else if (is_double_colon != r->is_double_colon) { - ERROR("%s:%d: *** target file `%s' has both : and :: entries.", - LOCF(r->loc), output.c_str()); + ERROR_LOC(r->loc, "*** target file `%s' has both : and :: entries.", + output.c_str()); } if (primary_rule && !r->cmds.empty() && !IsSuffixRule(output) && !r->is_double_colon) { - WARN("%s:%d: warning: overriding commands for target `%s'", - LOCF(r->cmd_loc()), output.c_str()); - WARN("%s:%d: warning: ignoring old commands for target `%s'", - LOCF(primary_rule->cmd_loc()), output.c_str()); + WARN_LOC(r->cmd_loc(), + "warning: overriding commands for target `%s'", + output.c_str()); + WARN_LOC(primary_rule->cmd_loc(), + "warning: ignoring old commands for target `%s'", + output.c_str()); primary_rule = r; } if (!primary_rule && !r->cmds.empty()) { @@ -274,8 +276,7 @@ class DepBuilder { if (targets.empty()) { suffix_rules_.clear(); } else { - WARN("%s:%d: kati doesn't support .SUFFIXES with prerequisites", - LOCF(loc)); + WARN_LOC(loc, "kati doesn't support .SUFFIXES with prerequisites"); } } @@ -296,7 +297,7 @@ class DepBuilder { }; for (const char** p = kUnsupportedBuiltinTargets; *p; p++) { if (GetRuleInputs(Intern(*p), &targets, &loc)) { - WARN("%s:%d: kati doesn't support %s", LOCF(loc), *p); + WARN_LOC(loc, "kati doesn't support %s", *p); } } } diff --git a/eval.cc b/eval.cc index 4b56f1f..7a08be7 100644 --- a/eval.cc +++ b/eval.cc @@ -383,7 +383,7 @@ string Evaluator::GetShellAndFlag() { } void Evaluator::Error(const string& msg) { - ERROR("%s:%d: %s", LOCF(loc_), msg.c_str()); + ERROR_LOC(loc_, "%s", msg.c_str()); } unordered_set Evaluator::used_undefined_vars_; diff --git a/expr.cc b/expr.cc index 5e7afe6..30a25b5 100644 --- a/expr.cc +++ b/expr.cc @@ -317,9 +317,9 @@ void ParseFunc(const Loc& loc, f->AddArg(v); i += n; if (i == s.size()) { - ERROR("%s:%d: *** unterminated call to function '%s': " - "missing '%c'.", - LOCF(loc), f->name(), terms[0]); + ERROR_LOC(loc, "*** unterminated call to function '%s': " + "missing '%c'.", + f->name(), terms[0]); } nargs++; if (s[i] == terms[0]) { @@ -332,8 +332,8 @@ void ParseFunc(const Loc& loc, } if (nargs <= f->min_arity()) { - ERROR("%s:%d: *** insufficient number of arguments (%d) to function `%s'.", - LOCF(loc), nargs - 1, f->name()); + ERROR_LOC(loc, "*** insufficient number of arguments (%d) to function `%s'.", + nargs - 1, f->name()); } *index_out = i; @@ -365,8 +365,8 @@ Value* ParseDollar(const Loc& loc, StringPiece s, size_t* index_out) { if (g_flags.enable_kati_warnings) { size_t found = sym.str().find_first_of(" ({"); if (found != string::npos) { - KATI_WARN("%s:%d: *warning*: variable lookup with '%c': %.*s", - loc, sym.str()[found], SPF(s)); + KATI_WARN_LOC(loc, "*warning*: variable lookup with '%c': %.*s", + sym.str()[found], SPF(s)); } } Value* r = new SymRef(sym); @@ -386,8 +386,8 @@ Value* ParseDollar(const Loc& loc, StringPiece s, size_t* index_out) { ParseFunc(loc, func, s, i+1, terms, index_out); return func; } else { - KATI_WARN("%s:%d: *warning*: unknown make function '%.*s': %.*s", - loc, SPF(lit->val()), SPF(s)); + KATI_WARN_LOC(loc, "*warning*: unknown make function '%.*s': %.*s", + SPF(lit->val()), SPF(s)); } } @@ -428,12 +428,12 @@ Value* ParseDollar(const Loc& loc, StringPiece s, size_t* index_out) { // for detail. size_t found = s.find(cp); if (found != string::npos) { - KATI_WARN("%s:%d: *warning*: unmatched parentheses: %.*s", - loc, SPF(s)); + KATI_WARN_LOC(loc, "*warning*: unmatched parentheses: %.*s", + SPF(s)); *index_out = s.size(); return new SymRef(Intern(s.substr(2, found-2))); } - ERROR("%s:%d: *** unterminated variable reference.", LOCF(loc)); + ERROR_LOC(loc, "*** unterminated variable reference."); } } diff --git a/flags.cc b/flags.cc index 3ad8e3c..5c14af7 100644 --- a/flags.cc +++ b/flags.cc @@ -97,6 +97,8 @@ void Flags::Parse(int argc, char** argv) { detect_android_echo = true; } else if (!strcmp(arg, "--detect_depfiles")) { detect_depfiles = true; + } else if (!strcmp(arg, "--color_warnings")) { + color_warnings = true; } else if (ParseCommandLineOptionWithArg( "-j", argv, &i, &num_jobs_str)) { num_jobs = strtol(num_jobs_str, NULL, 10); diff --git a/flags.h b/flags.h index d22d0cd..cd4090a 100644 --- a/flags.h +++ b/flags.h @@ -39,6 +39,7 @@ struct Flags { bool regen_debug; bool regen_ignoring_kati_binary; bool use_find_emulator; + bool color_warnings; const char* goma_dir; const char* ignore_dirty_pattern; const char* no_ignore_dirty_pattern; diff --git a/func.cc b/func.cc index 716cb2b..2031319 100644 --- a/func.cc +++ b/func.cc @@ -466,8 +466,8 @@ void EvalFunc(const vector& args, Evaluator* ev, string*) { string* text = new string; args[0]->Eval(ev, text); if (ev->avoid_io()) { - KATI_WARN("%s:%d: *warning*: $(eval) in a recipe is not recommended: %s", - LOCF(ev->loc()), text->c_str()); + KATI_WARN_LOC(ev->loc(), "*warning*: $(eval) in a recipe is not recommended: %s", + text->c_str()); } vector stmts; Parse(*text, ev->loc(), &stmts); @@ -555,9 +555,9 @@ void ShellFunc(const vector& args, Evaluator* ev, string* s) { string cmd = args[0]->Eval(ev); if (ev->avoid_io() && !HasNoIoInShellScript(cmd)) { if (ev->eval_depth() > 1) { - ERROR("%s:%d: kati doesn't support passing results of $(shell) " - "to other make constructs: %s", - LOCF(ev->loc()), cmd.c_str()); + ERROR_LOC(ev->loc(), "kati doesn't support passing results of $(shell) " + "to other make constructs: %s", + cmd.c_str()); } StripShellComment(&cmd); *s += "$("; @@ -595,8 +595,8 @@ void CallFunc(const vector& args, Evaluator* ev, string* s) { const StringPiece func_name = TrimSpace(func_name_buf); Var* func = ev->LookupVar(Intern(func_name)); if (!func->IsDefined()) { - KATI_WARN("%s:%d: *warning*: undefined user function: %s", - ev->loc(), func_name.as_string().c_str()); + KATI_WARN_LOC(ev->loc(), "*warning*: undefined user function: %s", + func_name.as_string().c_str()); } vector> av; for (size_t i = 1; i < args.size(); i++) { @@ -676,8 +676,7 @@ void WarningFunc(const vector& args, Evaluator* ev, string*) { StringPrintf("echo -e \"%s:%d: %s\" 2>&1", LOCF(ev->loc()), EchoEscape(a).c_str())); return; } - printf("%s:%d: %s\n", LOCF(ev->loc()), a.c_str()); - fflush(stdout); + WARN_LOC(ev->loc(), "%s", a.c_str()); } void ErrorFunc(const vector& args, Evaluator* ev, string*) { diff --git a/log.cc b/log.cc index fb9fcd4..b7141f6 100644 --- a/log.cc +++ b/log.cc @@ -16,5 +16,46 @@ #include "log.h" +#include "flags.h" +#include "strutil.h" + +#define BOLD "\033[1m" +#define RESET "\033[0m" +#define MAGENTA "\033[35m" +#define RED "\033[31m" + +void ColorErrorLog(const char* file, int line, const char* msg) { + if (file == nullptr) { + ERROR("%s", msg); + return; + } + + if (g_flags.color_warnings) { + StringPiece filtered = TrimPrefix(msg, "*** "); + + ERROR(BOLD "%s:%d: " RED "error: " RESET BOLD "%s" RESET, + file, line, filtered.as_string().c_str()); + } else { + ERROR("%s:%d: %s", file, line, msg); + } +} + +void ColorWarnLog(const char* file, int line, const char* msg) { + if (file == nullptr) { + fprintf(stderr, "%s\n", msg); + return; + } + + if (g_flags.color_warnings) { + StringPiece filtered = TrimPrefix(msg, "*warning*: "); + filtered = TrimPrefix(filtered, "warning: "); + + fprintf(stderr, BOLD "%s:%d: " MAGENTA "warning: " RESET BOLD "%s" RESET "\n", + file, line, filtered.as_string().c_str()); + } else { + fprintf(stderr, "%s:%d: %s\n", file, line, msg); + } +} + bool g_log_no_exit; string* g_last_error; diff --git a/log.h b/log.h index 11cf0e5..58c733a 100644 --- a/log.h +++ b/log.h @@ -21,6 +21,7 @@ #include #include "flags.h" +#include "log.h" #include "stringprintf.h" using namespace std; @@ -73,4 +74,22 @@ extern string* g_last_error; #define CHECK(c) if (!(c)) ERROR("%s:%d: %s", __FILE__, __LINE__, #c) +// Set of logging functions that will automatically colorize lines that have +// location information when --color_warnings is set. +void ColorWarnLog(const char* file, int line, const char *msg); +void ColorErrorLog(const char* file, int line, const char *msg); + +#define WARN_LOC(loc, ...) do { \ + ColorWarnLog(LOCF(loc), StringPrintf(__VA_ARGS__).c_str()); \ + } while (0) + +#define KATI_WARN_LOC(loc, ...) do { \ + if (g_flags.enable_kati_warnings) \ + ColorWarnLog(LOCF(loc), StringPrintf(__VA_ARGS__).c_str()); \ + } while(0) + +#define ERROR_LOC(loc, ...) do { \ + ColorErrorLog(LOCF(loc), StringPrintf(__VA_ARGS__).c_str()); \ + } while (0) + #endif // LOG_H_ diff --git a/main.cc b/main.cc index 5afe2b1..a4df3c0 100644 --- a/main.cc +++ b/main.cc @@ -173,8 +173,8 @@ static int Run(const vector& targets, } for (ParseErrorStmt* err : GetParseErrors()) { - WARN("%s:%d: warning for parse error in an unevaluated line: %s", - LOCF(err->loc()), err->msg.c_str()); + WARN_LOC(err->loc(), "warning for parse error in an unevaluated line: %s", + err->msg.c_str()); } vector nodes; diff --git a/parser.cc b/parser.cc index 61d1ac5..3822dd8 100644 --- a/parser.cc +++ b/parser.cc @@ -92,10 +92,10 @@ class Parser { } if (!if_stack_.empty()) - ERROR("%s:%d: *** missing `endif'.", loc_.filename, loc_.lineno + 1); + ERROR_LOC(Loc(loc_.filename, loc_.lineno + 1), "*** missing `endif'."); if (!define_name_.empty()) - ERROR("%s:%d: *** missing `endef', unterminated `define'.", - loc_.filename, define_start_line_); + ERROR_LOC(Loc(loc_.filename, define_start_line_), + "*** missing `endef', unterminated `define'."); } static void Init() { @@ -304,7 +304,7 @@ class Parser { StringPiece rest = TrimRightSpace(RemoveComment(TrimLeftSpace( line.substr(sizeof("endef"))))); if (!rest.empty()) { - WARN("%s:%d: extraneous text after `endef' directive", LOCF(loc_)); + WARN_LOC(loc_, "extraneous text after `endef' directive"); } AssignStmt* stmt = new AssignStmt(); @@ -374,7 +374,7 @@ class Parser { } } if (!s.empty()) { - WARN("%s:%d: extraneous text after `ifeq' directive", LOCF(loc_)); + WARN_LOC(loc_, "extraneous text after `ifeq' directive"); return true; } return true; @@ -411,7 +411,7 @@ class Parser { num_if_nest_ = st->num_nest + 1; if (!HandleDirective(next_if, else_if_directives_)) { - WARN("%s:%d: extraneous text after `else' directive", LOCF(loc_)); + WARN_LOC(loc_, "extraneous text after `else' directive"); } num_if_nest_ = 0; } diff --git a/rule.cc b/rule.cc index 6942fb5..717d0f0 100644 --- a/rule.cc +++ b/rule.cc @@ -58,7 +58,7 @@ void ParseRule(Loc& loc, StringPiece line, char term, Rule** out_rule, RuleVarAssignment* rule_var) { size_t index = line.find(':'); if (index == string::npos) { - ERROR("%s:%d: *** missing separator.", LOCF(loc)); + ERROR_LOC(loc, "*** missing separator."); } StringPiece first = line.substr(0, index); @@ -71,8 +71,7 @@ void ParseRule(Loc& loc, StringPiece line, char term, !outputs.empty() && IsPatternRule(outputs[0].str())); for (size_t i = 1; i < outputs.size(); i++) { if (IsPatternRule(outputs[i].str()) != is_first_pattern) { - ERROR("%s:%d: *** mixed implicit and normal rules: deprecated syntax", - LOCF(loc)); + ERROR_LOC(loc, "*** mixed implicit and normal rules: deprecated syntax"); } } @@ -94,8 +93,8 @@ void ParseRule(Loc& loc, StringPiece line, char term, // target specific variable). // See https://github.com/google/kati/issues/83 if (term_index == 0) { - KATI_WARN("%s:%d: defining a target which starts with `=', " - "which is not probably what you meant", LOCF(loc)); + KATI_WARN_LOC(loc, "defining a target which starts with `=', " + "which is not probably what you meant"); buf = line.as_string(); if (term) buf += term; @@ -136,8 +135,7 @@ void ParseRule(Loc& loc, StringPiece line, char term, } if (is_first_pattern) { - ERROR("%s:%d: *** mixed implicit and normal rules: deprecated syntax", - LOCF(loc)); + ERROR_LOC(loc, "*** mixed implicit and normal rules: deprecated syntax"); } StringPiece second = rest.substr(0, index); @@ -147,8 +145,8 @@ void ParseRule(Loc& loc, StringPiece line, char term, tok = TrimLeadingCurdir(tok); for (Symbol output : rule->outputs) { if (!Pattern(tok).Match(output.str())) { - WARN("%s:%d: target `%s' doesn't match the target pattern", - LOCF(loc), output.c_str()); + WARN_LOC(loc, "target `%s' doesn't match the target pattern", + output.c_str()); } } @@ -156,13 +154,13 @@ void ParseRule(Loc& loc, StringPiece line, char term, } if (rule->output_patterns.empty()) { - ERROR("%s:%d: *** missing target pattern.", LOCF(loc)); + ERROR_LOC(loc, "*** missing target pattern."); } if (rule->output_patterns.size() > 1) { - ERROR("%s:%d: *** multiple target patterns.", LOCF(loc)); + ERROR_LOC(loc, "*** multiple target patterns."); } if (!IsPatternRule(rule->output_patterns[0].str())) { - ERROR("%s:%d: *** target pattern contains no '%%'.", LOCF(loc)); + ERROR_LOC(loc, "*** target pattern contains no '%%'."); } ParseInputs(rule, third); } diff --git a/rule.h b/rule.h index 84412c1..48394bb 100644 --- a/rule.h +++ b/rule.h @@ -49,7 +49,7 @@ class Rule { private: void Error(const string& msg) { - ERROR("%s:%d: %s", loc.filename, loc.lineno, msg.c_str()); + ERROR_LOC(loc, "%s", msg.c_str()); } }; diff --git a/runtest.rb b/runtest.rb index fc594d5..190c11f 100755 --- a/runtest.rb +++ b/runtest.rb @@ -170,7 +170,7 @@ def normalize_kati_log(output) output.gsub!(/\/bin\/sh: ([^:]*): command not found/, "\\1: Command not found") output.gsub!(/.*: warning for parse error in an unevaluated line: .*\n/, '') - output.gsub!(/^FindEmulator: /, '') + output.gsub!(/^([^ ]+: )?FindEmulator: /, '') output.gsub!(/^\/bin\/sh: line 0: /, '') output.gsub!(/ (\.\/+)+kati\.\S+/, '') # kati log files in find_command.mk output.gsub!(/ (\.\/+)+test\S+.json/, '') # json files in find_command.mk diff --git a/strutil.cc b/strutil.cc index b094198..632d151 100644 --- a/strutil.cc +++ b/strutil.cc @@ -174,6 +174,13 @@ bool HasWord(StringPiece str, StringPiece w) { return true; } +StringPiece TrimPrefix(StringPiece str, StringPiece prefix) { + ssize_t size_diff = str.size() - prefix.size(); + if (size_diff < 0 || str.substr(0, prefix.size()) != prefix) + return str; + return str.substr(prefix.size()); +} + StringPiece TrimSuffix(StringPiece str, StringPiece suffix) { ssize_t size_diff = str.size() - suffix.size(); if (size_diff < 0 || str.substr(size_diff) != suffix) diff --git a/strutil.h b/strutil.h index d61160d..ff3d6a1 100644 --- a/strutil.h +++ b/strutil.h @@ -89,6 +89,8 @@ bool HasSuffix(StringPiece str, StringPiece suffix); bool HasWord(StringPiece str, StringPiece w); +StringPiece TrimPrefix(StringPiece str, StringPiece suffix); + StringPiece TrimSuffix(StringPiece str, StringPiece suffix); class Pattern { diff --git a/strutil_test.cc b/strutil_test.cc index 7a5a47d..a26cf4d 100644 --- a/strutil_test.cc +++ b/strutil_test.cc @@ -56,6 +56,20 @@ void TestHasSuffix() { assert(!HasSuffix("bar", "bbar")); } +void TestTrimPrefix() { + ASSERT_EQ(TrimPrefix("foo", "foo"), ""); + ASSERT_EQ(TrimPrefix("foo", "fo"), "o"); + ASSERT_EQ(TrimPrefix("foo", ""), "foo"); + ASSERT_EQ(TrimPrefix("foo", "fooo"), "foo"); +} + +void TestTrimSuffix() { + ASSERT_EQ(TrimSuffix("bar", "bar"), ""); + ASSERT_EQ(TrimSuffix("bar", "ar"), "b"); + ASSERT_EQ(TrimSuffix("bar", ""), "bar"); + ASSERT_EQ(TrimSuffix("bar", "bbar"), "bar"); +} + string SubstPattern(StringPiece str, StringPiece pat, StringPiece subst) { string r; Pattern(pat).AppendSubst(str, subst, &r); @@ -190,6 +204,8 @@ int main() { TestWordScanner(); TestHasPrefix(); TestHasSuffix(); + TestTrimPrefix(); + TestTrimSuffix(); TestSubstPattern(); TestNoLineBreak(); TestHasWord(); -- cgit v1.2.3 From 692e64ebf83bc87baca6e3c90c3d0b2849655d75 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 22 Feb 2017 15:38:33 -0800 Subject: Pass a Loc into FindEmulator for better warnings Before this change, we'd only get a warning from FindEmulator, with no idea which makefile caused it: FindEmulator: find: `tests': No such file or directory With this change, we'll get a better idea of which line triggered that problem: cts/tests/tests/content/Android.mk:43: FindEmulator: find: `test': No such file or directory And it will be colorized like any other location-based warning with the previous patch if --color_warnings is turned on. --- find.cc | 40 +++++++++++++++++++--------------------- find.h | 3 ++- find_test.cc | 2 +- func.cc | 9 +++++---- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/find.cc b/find.cc index 5d9cab6..0860847 100644 --- a/find.cc +++ b/find.cc @@ -150,7 +150,7 @@ class DirentNode { virtual const DirentNode* FindDir(StringPiece) const { return NULL; } - virtual bool RunFind(const FindCommand& fc, int d, + virtual bool RunFind(const FindCommand& fc, const Loc& loc, int d, string* path, unordered_map* cur_read_dirs, vector& out) const = 0; @@ -185,7 +185,7 @@ class DirentFileNode : public DirentNode { : DirentNode(name), type_(type) { } - virtual bool RunFind(const FindCommand& fc, int d, + virtual bool RunFind(const FindCommand& fc, const Loc&, int d, string* path, unordered_map*, vector& out) const override { @@ -255,15 +255,15 @@ class DirentDirNode : public DirentNode { return NULL; } - virtual bool RunFind(const FindCommand& fc, int d, + virtual bool RunFind(const FindCommand& fc, const Loc& loc, int d, string* path, unordered_map* cur_read_dirs, vector& out) const override { ScopedReadDirTracker srdt(this, *path, cur_read_dirs); if (!srdt.ok()) { - fprintf(stderr, "FindEmulator: find: File system loop detected; `%s' is " - "part of the same file system loop as `%s'.\n", - path->c_str(), srdt.conflicted().c_str()); + WARN_LOC(loc, "FindEmulator: find: File system loop detected; `%s' " + "is part of the same file system loop as `%s'.", + path->c_str(), srdt.conflicted().c_str()); return true; } @@ -292,7 +292,7 @@ class DirentDirNode : public DirentNode { if ((*path)[path->size()-1] != '/') *path += '/'; *path += c->base(); - if (!c->RunFind(fc, d + 1, path, cur_read_dirs, out)) + if (!c->RunFind(fc, loc, d + 1, path, cur_read_dirs, out)) return false; path->resize(orig_path_size); } @@ -320,7 +320,7 @@ class DirentDirNode : public DirentNode { if ((*path)[path->size()-1] != '/') *path += '/'; *path += c->base(); - if (!c->RunFind(fc, d + 1, path, cur_read_dirs, out)) + if (!c->RunFind(fc, loc, d + 1, path, cur_read_dirs, out)) return false; path->resize(orig_path_size); } @@ -330,7 +330,7 @@ class DirentDirNode : public DirentNode { if ((*path)[path->size()-1] != '/') *path += '/'; *path += c->base(); - if (!c->RunFind(fc, d + 1, path, cur_read_dirs, out)) + if (!c->RunFind(fc, loc, d + 1, path, cur_read_dirs, out)) return false; path->resize(orig_path_size); } @@ -360,7 +360,7 @@ class DirentSymlinkNode : public DirentNode { return NULL; } - virtual bool RunFind(const FindCommand& fc, int d, + virtual bool RunFind(const FindCommand& fc, const Loc& loc, int d, string* path, unordered_map* cur_read_dirs, vector& out) const override { @@ -368,8 +368,8 @@ class DirentSymlinkNode : public DirentNode { if (fc.follows_symlinks && errno_ != ENOENT) { if (errno_) { if (fc.type != FindCommandType::FINDLEAVES) { - fprintf(stderr, "FindEmulator: find: `%s': %s\n", - path->c_str(), strerror(errno_)); + WARN_LOC(loc, "FindEmulator: find: `%s': %s", + path->c_str(), strerror(errno_)); } return true; } @@ -379,7 +379,7 @@ class DirentSymlinkNode : public DirentNode { return false; } - return to_->RunFind(fc, d, path, cur_read_dirs, out); + return to_->RunFind(fc, loc, d, path, cur_read_dirs, out); } PrintIfNecessary(fc, *path, type, d, out); return true; @@ -783,7 +783,7 @@ class FindEmulatorImpl : public FindEmulator { } virtual bool HandleFind(const string& cmd UNUSED, const FindCommand& fc, - string* out) override { + const Loc& loc, string* out) override { if (!CanHandle(fc.chdir)) { LOG("FindEmulator: Cannot handle chdir (%.*s): %s", SPF(fc.chdir), cmd.c_str()); @@ -823,9 +823,8 @@ class FindEmulatorImpl : public FindEmulator { if (should_fallback) return false; if (!fc.redirect_to_devnull) { - fprintf(stderr, - "FindEmulator: cd: %.*s: No such file or directory\n", - SPF(fc.chdir)); + WARN_LOC(loc, "FindEmulator: cd: %.*s: No such file or directory", + SPF(fc.chdir)); } return true; } @@ -848,16 +847,15 @@ class FindEmulatorImpl : public FindEmulator { return false; } if (!fc.redirect_to_devnull) { - fprintf(stderr, - "FindEmulator: find: `%s': No such file or directory\n", - ConcatDir(fc.chdir, finddir).c_str()); + WARN_LOC(loc, "FindEmulator: find: `%s': No such file or directory", + ConcatDir(fc.chdir, finddir).c_str()); } continue; } string path = finddir; unordered_map cur_read_dirs; - if (!base->RunFind(fc, 0, &path, &cur_read_dirs, results)) { + if (!base->RunFind(fc, loc, 0, &path, &cur_read_dirs, results)) { LOG("FindEmulator: RunFind failed: %s", cmd.c_str()); return false; } diff --git a/find.h b/find.h index 1eee1e6..9af261a 100644 --- a/find.h +++ b/find.h @@ -20,6 +20,7 @@ #include #include +#include "loc.h" #include "string_piece.h" using namespace std; @@ -62,7 +63,7 @@ class FindEmulator { virtual ~FindEmulator() = default; virtual bool HandleFind(const string& cmd, const FindCommand& fc, - string* out) = 0; + const Loc& loc, string* out) = 0; static FindEmulator* Get(); diff --git a/find_test.cc b/find_test.cc index 1ba451a..ebdfa98 100644 --- a/find_test.cc +++ b/find_test.cc @@ -39,7 +39,7 @@ int main(int argc, char* argv[]) { return 1; } string out; - if (!FindEmulator::Get()->HandleFind(cmd, fc, &out)) { + if (!FindEmulator::Get()->HandleFind(cmd, fc, Loc(), &out)) { fprintf(stderr, "Find emulator does not support this command\n"); return 1; } diff --git a/func.cc b/func.cc index 2031319..7d50219 100644 --- a/func.cc +++ b/func.cc @@ -494,7 +494,8 @@ static bool HasNoIoInShellScript(const string& cmd) { } static void ShellFuncImpl(const string& shell, const string& shellflag, - const string& cmd, string* s, FindCommand** fc) { + const string& cmd, const Loc& loc, string* s, + FindCommand** fc) { LOG("ShellFunc: %s", cmd.c_str()); #ifdef TEST_FIND_EMULATOR @@ -505,11 +506,11 @@ static void ShellFuncImpl(const string& shell, const string& shellflag, *fc = new FindCommand(); if ((*fc)->Parse(cmd)) { #ifdef TEST_FIND_EMULATOR - if (FindEmulator::Get()->HandleFind(cmd, **fc, &out2)) { + if (FindEmulator::Get()->HandleFind(cmd, **fc, loc, &out2)) { need_check = true; } #else - if (FindEmulator::Get()->HandleFind(cmd, **fc, s)) { + if (FindEmulator::Get()->HandleFind(cmd, **fc, loc, s)) { return; } #endif @@ -571,7 +572,7 @@ void ShellFunc(const vector& args, Evaluator* ev, string* s) { string out; FindCommand* fc = NULL; - ShellFuncImpl(shell, shellflag, cmd, &out, &fc); + ShellFuncImpl(shell, shellflag, cmd, ev->loc(), &out, &fc); if (ShouldStoreCommandResult(cmd)) { CommandResult* cr = new CommandResult(); cr->op = (fc == NULL) ? CommandOp::SHELL : CommandOp::FIND, -- cgit v1.2.3 From 6f3f0f41f364a3ed7533cd075792ca38aef8ecf6 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Thu, 23 Feb 2017 00:10:04 -0800 Subject: Be more resilient to directories disappearing during startup In multiproduct_build, we run a lot of kati instances in the same source tree, but pointing to different out folders. I'm removing the out folders of successful runs so that too much disk space isn't used (unless the user requests it). But I often see a kati PERROR about opendir failing, since it's trying to set up the find emulator while a different configuration's output directory is being removed. This was also hit by #109 in similar circumstances. --- find.cc | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/find.cc b/find.cc index 5d9cab6..04da1f9 100644 --- a/find.cc +++ b/find.cc @@ -793,6 +793,9 @@ class FindEmulatorImpl : public FindEmulator { if (!is_initialized_) { ScopedTimeReporter tr("init find emulator time"); root_.reset(ConstructDirectoryTree("")); + if (!root_) { + ERROR("FindEmulator: Cannot open root directory"); + } ResolveSymlinks(); LOG_STAT("%d find nodes", node_cnt_); is_initialized_ = true; @@ -916,8 +919,14 @@ class FindEmulatorImpl : public FindEmulator { DirentNode* ConstructDirectoryTree(const string& path) { DIR* dir = opendir(path.empty() ? "." : path.c_str()); - if (!dir) - PERROR("opendir failed: %s", path.c_str()); + if (!dir) { + if (errno == ENOENT) { + LOG("opendir failed: %s", path.c_str()); + return NULL; + } else { + PERROR("opendir failed: %s", path.c_str()); + } + } DirentDirNode* n = new DirentDirNode(path); @@ -942,6 +951,9 @@ class FindEmulatorImpl : public FindEmulator { } if (d_type == DT_DIR) { c = ConstructDirectoryTree(npath); + if (c == NULL) { + continue; + } } else if (d_type == DT_LNK) { auto s = new DirentSymlinkNode(npath); symlinks_.push_back(make_pair(npath, s)); @@ -994,7 +1006,12 @@ class FindEmulatorImpl : public FindEmulator { if (type == DT_DIR) { if (path.find('/') == string::npos) { - s->set_to(ConstructDirectoryTree(path)); + DirentNode* dir = ConstructDirectoryTree(path); + if (dir != NULL) { + s->set_to(dir); + } else { + s->set_errno(errno); + } } } else if (type != DT_LNK && type != DT_UNKNOWN) { s->set_to(new DirentFileNode(path, type)); -- cgit v1.2.3 From 5db7f7c46b94424be56db9aa5af4ae537252a703 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 22 Feb 2017 23:59:06 -0800 Subject: Randomize cpu affinity Continue to lock kati to one or two CPUs, but pick one CPU at random, and pick another CPU next to it. This dramatically speeds up cases where more than one Kati instance is running at a time. There's a multiproduct_build tool in Android that attempts to run Kati on every build configuration present in the tree. My machine has 48 (including hyperthreading) cpu cores, with 64GB of ram. Before this change, it would take ~12 minutes to build all 59 configurations, 12 at a time. After this change it takes 3 minutes. There doesn't seem to be any significant change in timings for standalone builds. It looks like there was 1-2% difference with my previous change that chose two completely random CPUs, but choosing two that are likely hyperthreaded, or at least on the same chip helps. --- affinity.cc | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/affinity.cc b/affinity.cc index 2101fdb..5448b29 100644 --- a/affinity.cc +++ b/affinity.cc @@ -21,15 +21,29 @@ #ifdef __linux__ +#include + #include +#include +#include void SetAffinityForSingleThread() { cpu_set_t cs; CPU_ZERO(&cs); - int n = g_flags.num_cpus / 2; - CPU_SET(n, &cs); - if (n > 1) - CPU_SET(n + 1, &cs); + std::default_random_engine generator(getpid()); + std::uniform_int_distribution distribution(0,g_flags.num_cpus-1); + int cpu = distribution(generator); + + // Try to come up with a CPU and one close to it. This should work on most + // hyperthreaded system, but may be less optimal under stranger setups. + // Choosing two completely different CPUs would work here as well, it's just a + // couple percent faster if they're close (and still faster than letting the + // scheduler do whatever it wants). + cpu = cpu - (cpu % 2); + CPU_SET(cpu, &cs); + if (g_flags.num_cpus > 1) + CPU_SET(cpu+1, &cs); + if (sched_setaffinity(0, sizeof(cs), &cs) < 0) WARN("sched_setaffinity: %s", strerror(errno)); } -- cgit v1.2.3