Revert 232657 "GN: toolchain threading cleanup" Broke linux clang bots: ../../tools/gn/ninja_helper_unittest.cc:20:9:error: field 'settings' will be initialized after field 'toolchain' [-Werror,-Wreorder] This happened on your try jobs too. > GN: toolchain threading cleanup > > Remove the thread-unsafe toolchain pointer on the otherwise-threadsafe Settings object. I replaced it with the toolchain label, and moved the is_default flag from the toolchain to the Settings object. > > This required that I pass the toolchain around in a few more places, but also simplifies some other cases. > > I removed the toolchain prefix from Ninja rules for the default toolchain since that's not necessary any more for GYP compat. > > This fixes an annoying double-free in the toolchain manager. I think my current refactor will clean this up in a later phase. > > R=scottmg@chromium.org > > Review URL: https://codereview.chromium.org/51693002 TBR=brettw@chromium.org Review URL: https://codereview.chromium.org/46313006 Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 404b9863075145cde48e00b47169e3d0446266b6
diff --git a/tools/gn/config.cc b/tools/gn/config.cc index 5b89e2b..850d596 100644 --- a/tools/gn/config.cc +++ b/tools/gn/config.cc
@@ -10,8 +10,7 @@ #include "tools/gn/item_tree.h" #include "tools/gn/scheduler.h" -Config::Config(const Settings* settings, const Label& label) - : Item(settings, label) { +Config::Config(const Label& label) : Item(label) { } Config::~Config() { @@ -39,7 +38,7 @@ ItemNode* node = tree->GetExistingNodeLocked(label); Config* config = NULL; if (!node) { - config = new Config(settings, label); + config = new Config(label); node = new ItemNode(config); node->set_originally_referenced_from_here(specified_from_here); tree->AddNodeLocked(node);
diff --git a/tools/gn/config.h b/tools/gn/config.h index e5a1df6..0ee9b7b 100644 --- a/tools/gn/config.h +++ b/tools/gn/config.h
@@ -17,7 +17,7 @@ // Represents a named config in the dependency graph. class Config : public Item { public: - Config(const Settings* settings, const Label& label); + Config(const Label& label); virtual ~Config(); virtual Config* AsConfig() OVERRIDE;
diff --git a/tools/gn/function_exec_script.cc b/tools/gn/function_exec_script.cc index b382575..bf55165 100644 --- a/tools/gn/function_exec_script.cc +++ b/tools/gn/function_exec_script.cc
@@ -301,7 +301,7 @@ } ScopedTrace trace(TraceItem::TRACE_SCRIPT_EXECUTE, script_source.value()); - trace.SetToolchain(settings->toolchain_label()); + trace.SetToolchain(settings->toolchain()->label()); // Add all dependencies of this script, including the script itself, to the // build deps.
diff --git a/tools/gn/function_toolchain.cc b/tools/gn/function_toolchain.cc index dde0306..933d55e 100644 --- a/tools/gn/function_toolchain.cc +++ b/tools/gn/function_toolchain.cc
@@ -102,7 +102,7 @@ // This object will actually be copied into the one owned by the toolchain // manager, but that has to be done in the lock. - Toolchain toolchain(scope->settings(), label); + Toolchain toolchain(label); Scope block_scope(scope); block_scope.SetProperty(&kToolchainPropertyKey, &toolchain);
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc index 0e27297..acc8fe9 100644 --- a/tools/gn/functions.cc +++ b/tools/gn/functions.cc
@@ -134,7 +134,7 @@ } const Label& ToolchainLabelForScope(const Scope* scope) { - return scope->settings()->toolchain_label(); + return scope->settings()->toolchain()->label(); } Label MakeLabelForScope(const Scope* scope,
diff --git a/tools/gn/gyp_helper.cc b/tools/gn/gyp_helper.cc index fb669af..f69ed8f 100644 --- a/tools/gn/gyp_helper.cc +++ b/tools/gn/gyp_helper.cc
@@ -45,10 +45,10 @@ } std::string GypHelper::GetNameForTarget(const Target* target) const { - if (target->settings()->is_default()) + const Toolchain* toolchain = target->settings()->toolchain(); + if (toolchain->is_default()) return target->label().name(); // Default toolchain has no suffix. - return target->label().name() + "_" + - target->settings()->toolchain_label().name(); + return target->label().name() + "_" + toolchain->label().name(); } std::string GypHelper::GetFullRefForTarget(const Target* target) const {
diff --git a/tools/gn/input_conversion.cc b/tools/gn/input_conversion.cc index c40bdba..19a8047 100644 --- a/tools/gn/input_conversion.cc +++ b/tools/gn/input_conversion.cc
@@ -117,7 +117,9 @@ } BuildSettings build_settings; - Settings settings(&build_settings, std::string()); + Label empty_label; + Toolchain toolchain(empty_label); + Settings settings(&build_settings, &toolchain, std::string()); Scope scope(&settings); Err nested_err;
diff --git a/tools/gn/item.cc b/tools/gn/item.cc index fa16975..747183c 100644 --- a/tools/gn/item.cc +++ b/tools/gn/item.cc
@@ -6,9 +6,7 @@ #include "base/logging.h" -Item::Item(const Settings* settings, const Label& label) - : settings_(settings), - label_(label) { +Item::Item(const Label& label) : label_(label) { } Item::~Item() {
diff --git a/tools/gn/item.h b/tools/gn/item.h index 69115bd..2538c50 100644 --- a/tools/gn/item.h +++ b/tools/gn/item.h
@@ -11,7 +11,6 @@ class Config; class ItemNode; -class Settings; class Target; class Toolchain; @@ -19,11 +18,9 @@ // graph. class Item { public: - Item(const Settings* settings, const Label& label); + Item(const Label& label); virtual ~Item(); - const Settings* settings() const { return settings_; } - // This is guaranteed to never change after construction so this can be // accessed from any thread with no locking once the item is constructed. const Label& label() const { return label_; } @@ -52,7 +49,6 @@ virtual void OnResolved() {} private: - const Settings* settings_; Label label_; ItemNode* item_node_;
diff --git a/tools/gn/ninja_binary_target_writer.cc b/tools/gn/ninja_binary_target_writer.cc index c404006..3a1d620 100644 --- a/tools/gn/ninja_binary_target_writer.cc +++ b/tools/gn/ninja_binary_target_writer.cc
@@ -86,9 +86,8 @@ } // namespace NinjaBinaryTargetWriter::NinjaBinaryTargetWriter(const Target* target, - const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, toolchain, out), + : NinjaTargetWriter(target, out), tool_type_(GetToolTypeForTarget(target)){ } @@ -146,6 +145,7 @@ const Target::FileList& sources = target_->sources(); object_files->reserve(sources.size()); + const Toolchain* toolchain = GetToolchain(); std::string implicit_deps = GetSourcesImplicitDeps(); for (size_t i = 0; i < sources.size(); i++) { @@ -156,7 +156,7 @@ if (input_file_type == SOURCE_UNKNOWN) continue; // Skip unknown file types. std::string command = - helper_.GetRuleForSourceType(settings_, input_file_type); + helper_.GetRuleForSourceType(settings_, toolchain, input_file_type); if (command.empty()) continue; // Skip files not needing compilation. @@ -265,7 +265,8 @@ RecursiveTargetConfigStringsToStream(target_, &ConfigValues::ldflags, flag_options, out_); - const Toolchain::Tool& tool = toolchain_->GetTool(tool_type_); + const Toolchain* toolchain = GetToolchain(); + const Toolchain::Tool& tool = toolchain->GetTool(tool_type_); // Followed by library search paths that have been recursively pushed // through the dependency tree. @@ -305,7 +306,7 @@ path_output_.WriteFile(out_, external_output_file); } out_ << ": " - << helper_.GetRulePrefix(target_->settings()) + << helper_.GetRulePrefix(GetToolchain()) << Toolchain::ToolTypeToName(tool_type_); std::set<OutputFile> extra_object_files; @@ -345,7 +346,7 @@ out_ << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()) + << helper_.GetRulePrefix(target_->settings()->toolchain()) << "stamp"; std::set<OutputFile> extra_object_files;
diff --git a/tools/gn/ninja_binary_target_writer.h b/tools/gn/ninja_binary_target_writer.h index c188335..8c57ffb 100644 --- a/tools/gn/ninja_binary_target_writer.h +++ b/tools/gn/ninja_binary_target_writer.h
@@ -13,9 +13,7 @@ // library, or a static library). class NinjaBinaryTargetWriter : public NinjaTargetWriter { public: - NinjaBinaryTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out); + NinjaBinaryTargetWriter(const Target* target, std::ostream& out); virtual ~NinjaBinaryTargetWriter(); virtual void Run() OVERRIDE;
diff --git a/tools/gn/ninja_binary_target_writer_unittest.cc b/tools/gn/ninja_binary_target_writer_unittest.cc index 96a7448..a7de4a1 100644 --- a/tools/gn/ninja_binary_target_writer_unittest.cc +++ b/tools/gn/ninja_binary_target_writer_unittest.cc
@@ -22,7 +22,7 @@ // Source set itself. { std::ostringstream out; - NinjaBinaryTargetWriter writer(&target, setup.toolchain(), out); + NinjaBinaryTargetWriter writer(&target, out); writer.Run(); // TODO(brettw) I think we'll need to worry about backslashes here @@ -36,10 +36,10 @@ "cflags_objc =\n" "cflags_objcc =\n" "\n" - "build obj/foo/bar.input1.obj: cxx ../../foo/input1.cc\n" - "build obj/foo/bar.input2.obj: cxx ../../foo/input2.cc\n" + "build obj/foo/bar.input1.obj: tc_cxx ../../foo/input1.cc\n" + "build obj/foo/bar.input2.obj: tc_cxx ../../foo/input2.cc\n" "\n" - "build obj/foo/bar.stamp: stamp obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n"; + "build obj/foo/bar.stamp: tc_stamp obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n"; std::string out_str = out.str(); #if defined(OS_WIN) std::replace(out_str.begin(), out_str.end(), '\\', '/'); @@ -55,7 +55,7 @@ { std::ostringstream out; - NinjaBinaryTargetWriter writer(&shlib_target, setup.toolchain(), out); + NinjaBinaryTargetWriter writer(&shlib_target, out); writer.Run(); // TODO(brettw) I think we'll need to worry about backslashes here @@ -73,7 +73,7 @@ "manifests = obj/foo/shlib.intermediate.manifest\n" "ldflags = /MANIFEST /ManifestFile:obj/foo/shlib.intermediate.manifest\n" "libs =\n" - "build shlib.dll shlib.dll.lib: solink obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n" + "build shlib.dll shlib.dll.lib: tc_solink obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n" " soname = shlib.dll\n" " lib = shlib.dll\n" " dll = shlib.dll\n"
diff --git a/tools/gn/ninja_copy_target_writer.cc b/tools/gn/ninja_copy_target_writer.cc index 89b3e3f..5de249f 100644 --- a/tools/gn/ninja_copy_target_writer.cc +++ b/tools/gn/ninja_copy_target_writer.cc
@@ -9,9 +9,8 @@ #include "tools/gn/string_utils.h" NinjaCopyTargetWriter::NinjaCopyTargetWriter(const Target* target, - const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, toolchain, out) { + : NinjaTargetWriter(target, out) { } NinjaCopyTargetWriter::~NinjaCopyTargetWriter() { @@ -23,7 +22,8 @@ std::vector<OutputFile> output_files; - std::string rule_prefix = helper_.GetRulePrefix(target_->settings()); + std::string rule_prefix = + helper_.GetRulePrefix(target_->settings()->toolchain()); for (size_t i = 0; i < target_->sources().size(); i++) { const SourceFile& input_file = target_->sources()[i];
diff --git a/tools/gn/ninja_copy_target_writer.h b/tools/gn/ninja_copy_target_writer.h index cadbc38..99bbaff 100644 --- a/tools/gn/ninja_copy_target_writer.h +++ b/tools/gn/ninja_copy_target_writer.h
@@ -11,9 +11,7 @@ // Writes a .ninja file for a copy target type. class NinjaCopyTargetWriter : public NinjaTargetWriter { public: - NinjaCopyTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out); + NinjaCopyTargetWriter(const Target* target, std::ostream& out); virtual ~NinjaCopyTargetWriter(); virtual void Run() OVERRIDE;
diff --git a/tools/gn/ninja_copy_target_writer_unittest.cc b/tools/gn/ninja_copy_target_writer_unittest.cc index 5642d9d..c31cbd2 100644 --- a/tools/gn/ninja_copy_target_writer_unittest.cc +++ b/tools/gn/ninja_copy_target_writer_unittest.cc
@@ -27,14 +27,14 @@ setup.settings()->set_target_os(Settings::LINUX); std::ostringstream out; - NinjaCopyTargetWriter writer(&target, setup.toolchain(), out); + NinjaCopyTargetWriter writer(&target, out); writer.Run(); const char expected_linux[] = - "build input1.out: copy ../../foo/input1.txt\n" - "build input2.out: copy ../../foo/input2.txt\n" + "build input1.out: tc_copy ../../foo/input1.txt\n" + "build input2.out: tc_copy ../../foo/input2.txt\n" "\n" - "build obj/foo/bar.stamp: stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; std::string out_str = out.str(); #if defined(OS_WIN) std::replace(out_str.begin(), out_str.end(), '\\', '/'); @@ -47,16 +47,16 @@ setup.settings()->set_target_os(Settings::WIN); std::ostringstream out; - NinjaCopyTargetWriter writer(&target, setup.toolchain(), out); + NinjaCopyTargetWriter writer(&target, out); writer.Run(); // TODO(brettw) I think we'll need to worry about backslashes here // depending if we're on actual Windows or Linux pretending to be Windows. const char expected_win[] = - "build input1.out: copy ../../foo/input1.txt\n" - "build input2.out: copy ../../foo/input2.txt\n" + "build input1.out: tc_copy ../../foo/input1.txt\n" + "build input2.out: tc_copy ../../foo/input2.txt\n" "\n" - "build obj/foo/bar.stamp: stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; std::string out_str = out.str(); #if defined(OS_WIN) std::replace(out_str.begin(), out_str.end(), '\\', '/');
diff --git a/tools/gn/ninja_group_target_writer.cc b/tools/gn/ninja_group_target_writer.cc index d29f917..606f916 100644 --- a/tools/gn/ninja_group_target_writer.cc +++ b/tools/gn/ninja_group_target_writer.cc
@@ -8,9 +8,8 @@ #include "tools/gn/string_utils.h" NinjaGroupTargetWriter::NinjaGroupTargetWriter(const Target* target, - const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, toolchain, out) { + : NinjaTargetWriter(target, out) { } NinjaGroupTargetWriter::~NinjaGroupTargetWriter() { @@ -22,7 +21,7 @@ out_ << std::endl << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()) + << helper_.GetRulePrefix(target_->settings()->toolchain()) << "stamp"; const LabelTargetVector& deps = target_->deps();
diff --git a/tools/gn/ninja_group_target_writer.h b/tools/gn/ninja_group_target_writer.h index 862b920..31625f8 100644 --- a/tools/gn/ninja_group_target_writer.h +++ b/tools/gn/ninja_group_target_writer.h
@@ -11,9 +11,7 @@ // Writes a .ninja file for a group target type. class NinjaGroupTargetWriter : public NinjaTargetWriter { public: - NinjaGroupTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out); + NinjaGroupTargetWriter(const Target* target, std::ostream& out); virtual ~NinjaGroupTargetWriter(); virtual void Run() OVERRIDE;
diff --git a/tools/gn/ninja_helper.cc b/tools/gn/ninja_helper.cc index e872c24..d90b698 100644 --- a/tools/gn/ninja_helper.cc +++ b/tools/gn/ninja_helper.cc
@@ -189,20 +189,23 @@ return ret; } -std::string NinjaHelper::GetRulePrefix(const Settings* settings) const { - // Don't prefix the default toolchain so it looks prettier, prefix everything - // else. - if (settings->is_default()) - return std::string(); // Default toolchain has no prefix. - return settings->toolchain_label().name() + "_"; +std::string NinjaHelper::GetRulePrefix(const Toolchain* toolchain) const { + // This code doesn't prefix the default toolchain commands. This is disabled + // so we can coexist with GYP's commands (which aren't prefixed). If we don't + // need to coexist with GYP anymore, we can uncomment this to make things a + // bit prettier. + //if (toolchain->is_default()) + // return std::string(); // Default toolchain has no prefix. + return toolchain->label().name() + "_"; } std::string NinjaHelper::GetRuleForSourceType(const Settings* settings, + const Toolchain* toolchain, SourceFileType type) const { // This function may be hot since it will be called for every source file // in the tree. We could cache the results to avoid making a string for // every invocation. - std::string prefix = GetRulePrefix(settings); + std::string prefix = GetRulePrefix(toolchain); if (type == SOURCE_C) return prefix + "cc";
diff --git a/tools/gn/ninja_helper.h b/tools/gn/ninja_helper.h index 84a2c89..bef2fcd 100644 --- a/tools/gn/ninja_helper.h +++ b/tools/gn/ninja_helper.h
@@ -52,11 +52,12 @@ // Returns the prefix for rules on the given toolchain. We need this to // disambiguate a given rule for each toolchain. - std::string GetRulePrefix(const Settings* settings) const; + std::string GetRulePrefix(const Toolchain* toolchain) const; // Returns the name of the rule name for the given toolchain and file/target // type. Returns the empty string for source files with no command. std::string GetRuleForSourceType(const Settings* settings, + const Toolchain* toolchain, SourceFileType type) const; // Returns the relative directory in either slashes or the system separator
diff --git a/tools/gn/ninja_helper_unittest.cc b/tools/gn/ninja_helper_unittest.cc index 63082bc..6e3b25c 100644 --- a/tools/gn/ninja_helper_unittest.cc +++ b/tools/gn/ninja_helper_unittest.cc
@@ -17,10 +17,9 @@ public: HelperSetterUpper() : build_settings(), - settings(&build_settings, std::string()), - toolchain(&settings, Label(SourceDir("//"), "tc")), + toolchain(Label(SourceDir("//"), "tc")), + settings(&build_settings, &toolchain, std::string()), target(&settings, Label(SourceDir("//tools/gn/"), "name")) { - settings.set_toolchain_label(toolchain.label()); settings.set_target_os(Settings::WIN); // Output going to "out/Debug".
diff --git a/tools/gn/ninja_script_target_writer.cc b/tools/gn/ninja_script_target_writer.cc index e8cdef4..1b8358c 100644 --- a/tools/gn/ninja_script_target_writer.cc +++ b/tools/gn/ninja_script_target_writer.cc
@@ -11,9 +11,8 @@ #include "tools/gn/target.h" NinjaScriptTargetWriter::NinjaScriptTargetWriter(const Target* target, - const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, toolchain, out), + : NinjaTargetWriter(target, out), path_output_no_escaping_( target->settings()->build_settings()->build_dir(), ESCAPE_NONE, false) { @@ -153,7 +152,7 @@ out_ << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()) + << helper_.GetRulePrefix(target_->settings()->toolchain()) << "stamp"; for (size_t i = 0; i < output_files.size(); i++) { out_ << " ";
diff --git a/tools/gn/ninja_script_target_writer.h b/tools/gn/ninja_script_target_writer.h index 9c51a6a..e84d618 100644 --- a/tools/gn/ninja_script_target_writer.h +++ b/tools/gn/ninja_script_target_writer.h
@@ -17,9 +17,7 @@ // Writes a .ninja file for a custom script target type. class NinjaScriptTargetWriter : public NinjaTargetWriter { public: - NinjaScriptTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out); + NinjaScriptTargetWriter(const Target* target, std::ostream& out); virtual ~NinjaScriptTargetWriter(); virtual void Run() OVERRIDE;
diff --git a/tools/gn/ninja_script_target_writer_unittest.cc b/tools/gn/ninja_script_target_writer_unittest.cc index f2c8a93..1394717 100644 --- a/tools/gn/ninja_script_target_writer_unittest.cc +++ b/tools/gn/ninja_script_target_writer_unittest.cc
@@ -21,7 +21,7 @@ SourceFile("//out/Debug/gen/{{source_name_part}}.cc")); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); + NinjaScriptTargetWriter writer(&target, out); FileTemplate output_template = writer.GetOutputTemplate(); @@ -42,7 +42,7 @@ Target target(setup.settings(), Label(SourceDir("//foo/"), "bar")); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); + NinjaScriptTargetWriter writer(&target, out); std::vector<std::string> args; args.push_back("-i"); @@ -88,7 +88,7 @@ "/usr/bin/python"))); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); + NinjaScriptTargetWriter writer(&target, out); writer.Run(); const char expected_linux[] = @@ -107,7 +107,7 @@ " source = ../../foo/input2.txt\n" " source_name_part = input2\n" "\n" - "build obj/foo/bar.stamp: stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; std::string out_str = out.str(); #if defined(OS_WIN) @@ -125,7 +125,7 @@ setup.settings()->set_target_os(Settings::WIN); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); + NinjaScriptTargetWriter writer(&target, out); writer.Run(); // TODO(brettw) I think we'll need to worry about backslashes here @@ -151,7 +151,7 @@ " source = ../../foo/input2.txt\n" " source_name_part = input2\n" "\n" - "build obj/foo/bar.stamp: stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; std::string out_str = out.str(); #if defined(OS_WIN) std::replace(out_str.begin(), out_str.end(), '\\', '/');
diff --git a/tools/gn/ninja_target_writer.cc b/tools/gn/ninja_target_writer.cc index bdc0472..d73c873 100644 --- a/tools/gn/ninja_target_writer.cc +++ b/tools/gn/ninja_target_writer.cc
@@ -19,12 +19,9 @@ #include "tools/gn/target.h" #include "tools/gn/trace.h" -NinjaTargetWriter::NinjaTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out) +NinjaTargetWriter::NinjaTargetWriter(const Target* target, std::ostream& out) : settings_(target->settings()), target_(target), - toolchain_(toolchain), out_(out), path_output_(settings_->build_settings()->build_dir(), ESCAPE_NINJA, true), @@ -48,7 +45,7 @@ ScopedTrace trace(TraceItem::TRACE_FILE_WRITE, target->label().GetUserVisibleName(false)); - trace.SetToolchain(settings->toolchain_label()); + trace.SetToolchain(settings->toolchain()->label()); base::FilePath ninja_file(settings->build_settings()->GetFullPath( helper.GetNinjaFileForTarget(target).GetSourceFile( @@ -57,10 +54,6 @@ if (g_scheduler->verbose_logging()) g_scheduler->Log("Writing", FilePathToUTF8(ninja_file)); - const Toolchain* tc = settings->build_settings()->toolchain_manager() - .GetToolchainDefinitionUnlocked(settings->toolchain_label()); - CHECK(tc); - file_util::CreateDirectory(ninja_file.DirName()); // It's rediculously faster to write to a string and then write that to @@ -69,19 +62,19 @@ // Call out to the correct sub-type of writer. if (target->output_type() == Target::COPY_FILES) { - NinjaCopyTargetWriter writer(target, tc, file); + NinjaCopyTargetWriter writer(target, file); writer.Run(); } else if (target->output_type() == Target::CUSTOM) { - NinjaScriptTargetWriter writer(target, tc, file); + NinjaScriptTargetWriter writer(target, file); writer.Run(); } else if (target->output_type() == Target::GROUP) { - NinjaGroupTargetWriter writer(target, tc, file); + NinjaGroupTargetWriter writer(target, file); writer.Run(); } else if (target->output_type() == Target::EXECUTABLE || target->output_type() == Target::STATIC_LIBRARY || target->output_type() == Target::SHARED_LIBRARY || target->output_type() == Target::SOURCE_SET) { - NinjaBinaryTargetWriter writer(target, tc, file); + NinjaBinaryTargetWriter writer(target, file); writer.Run(); } else { CHECK(0); @@ -92,6 +85,10 @@ static_cast<int>(contents.size())); } +const Toolchain* NinjaTargetWriter::GetToolchain() const { + return target_->settings()->toolchain(); +} + std::string NinjaTargetWriter::GetSourcesImplicitDeps() const { std::ostringstream ret; ret << " |";
diff --git a/tools/gn/ninja_target_writer.h b/tools/gn/ninja_target_writer.h index e1c3bf8..c9c72a7 100644 --- a/tools/gn/ninja_target_writer.h +++ b/tools/gn/ninja_target_writer.h
@@ -19,9 +19,7 @@ // generated by the NinjaBuildWriter. class NinjaTargetWriter { public: - NinjaTargetWriter(const Target* target, - const Toolchain* toolchain, - std::ostream& out); + NinjaTargetWriter(const Target* target, std::ostream& out); virtual ~NinjaTargetWriter(); static void RunAndWriteFile(const Target* target); @@ -29,6 +27,9 @@ virtual void Run() = 0; protected: + // Returns the toolchain associated with the target. + const Toolchain* GetToolchain() const; + // Returns the string to be appended to source rules that encodes the // order-only dependencies for the current target. This will include the // "|" character so can just be appended to the source rules. If there are no @@ -40,7 +41,6 @@ const Settings* settings_; // Non-owning. const Target* target_; // Non-owning. - const Toolchain* toolchain_; // Non-owning. std::ostream& out_; PathOutput path_output_;
diff --git a/tools/gn/ninja_toolchain_writer.cc b/tools/gn/ninja_toolchain_writer.cc index 5f33e69..310caba 100644 --- a/tools/gn/ninja_toolchain_writer.cc +++ b/tools/gn/ninja_toolchain_writer.cc
@@ -13,7 +13,6 @@ #include "tools/gn/settings.h" #include "tools/gn/target.h" #include "tools/gn/toolchain.h" -#include "tools/gn/toolchain_manager.h" #include "tools/gn/trace.h" NinjaToolchainWriter::NinjaToolchainWriter( @@ -63,14 +62,11 @@ } void NinjaToolchainWriter::WriteRules() { - const Toolchain* tc = settings_->build_settings()->toolchain_manager() - .GetToolchainDefinitionUnlocked(settings_->toolchain_label()); - CHECK(tc); - + const Toolchain* tc = settings_->toolchain(); std::string indent(" "); NinjaHelper helper(settings_->build_settings()); - std::string rule_prefix = helper.GetRulePrefix(settings_); + std::string rule_prefix = helper.GetRulePrefix(tc); for (int i = Toolchain::TYPE_NONE + 1; i < Toolchain::TYPE_NUMTYPES; i++) { Toolchain::ToolType tool_type = static_cast<Toolchain::ToolType>(i);
diff --git a/tools/gn/scope_per_file_provider.cc b/tools/gn/scope_per_file_provider.cc index 1f4bc25..525aab6 100644 --- a/tools/gn/scope_per_file_provider.cc +++ b/tools/gn/scope_per_file_provider.cc
@@ -43,7 +43,7 @@ const Value* ScopePerFileProvider::GetCurrentToolchain() { if (!current_toolchain_) { current_toolchain_.reset(new Value(NULL, - scope_->settings()->toolchain_label().GetUserVisibleName(false))); + scope_->settings()->toolchain()->label().GetUserVisibleName(false))); } return current_toolchain_.get(); }
diff --git a/tools/gn/scope_per_file_provider_unittest.cc b/tools/gn/scope_per_file_provider_unittest.cc index 776a0aa..1137d3b 100644 --- a/tools/gn/scope_per_file_provider_unittest.cc +++ b/tools/gn/scope_per_file_provider_unittest.cc
@@ -6,26 +6,34 @@ #include "tools/gn/build_settings.h" #include "tools/gn/scope_per_file_provider.h" #include "tools/gn/settings.h" -#include "tools/gn/test_with_scope.h" #include "tools/gn/toolchain.h" #include "tools/gn/variables.h" TEST(ScopePerFileProvider, Expected) { - TestWithScope test; + Err err; + + BuildSettings build_settings; + build_settings.toolchain_manager().SetDefaultToolchainUnlocked( + Label(SourceDir("//toolchain/"), "default", SourceDir(), ""), + LocationRange(), &err); + EXPECT_FALSE(err.has_error()); + + build_settings.SetBuildDir(SourceDir("//out/Debug/")); // Prevent horrible wrapping of calls below. #define GPV(val) provider.GetProgrammaticValue(val)->string_value() // Test the default toolchain. { - Scope scope(test.settings()); + Toolchain toolchain(Label(SourceDir("//toolchain/"), "tc")); + Settings settings(&build_settings, &toolchain, std::string()); + + Scope scope(&settings); scope.set_source_dir(SourceDir("//source/")); ScopePerFileProvider provider(&scope); - EXPECT_EQ("//toolchain:default", GPV(variables::kCurrentToolchain)); - // TODO(brettw) this test harness does not set up the Toolchain manager - // which is the source of this value, so we can't test this yet. - //EXPECT_EQ("//toolchain:default", GPV(variables::kDefaultToolchain)); + EXPECT_EQ("//toolchain:tc", GPV(variables::kCurrentToolchain)); + EXPECT_EQ("//toolchain:default", GPV(variables::kDefaultToolchain)); EXPECT_EQ("//out/Debug", GPV(variables::kRootBuildDir)); EXPECT_EQ("//out/Debug/gen", GPV(variables::kRootGenDir)); EXPECT_EQ("//out/Debug", GPV(variables::kRootOutDir)); @@ -35,17 +43,13 @@ // Test some with an alternate toolchain. { - Settings settings(test.build_settings(), "tc"); - Toolchain toolchain(&settings, Label(SourceDir("//toolchain/"), "tc")); - settings.set_toolchain_label(toolchain.label()); + Toolchain toolchain(Label(SourceDir("//toolchain/"), "tc")); + Settings settings(&build_settings, &toolchain, "tc"); Scope scope(&settings); scope.set_source_dir(SourceDir("//source/")); ScopePerFileProvider provider(&scope); - EXPECT_EQ("//toolchain:tc", GPV(variables::kCurrentToolchain)); - // See above. - //EXPECT_EQ("//toolchain:default", GPV(variables::kDefaultToolchain)); EXPECT_EQ("//out/Debug", GPV(variables::kRootBuildDir)); EXPECT_EQ("//out/Debug/tc/gen", GPV(variables::kRootGenDir)); EXPECT_EQ("//out/Debug/tc", GPV(variables::kRootOutDir));
diff --git a/tools/gn/settings.cc b/tools/gn/settings.cc index b8de976..bfab85d 100644 --- a/tools/gn/settings.cc +++ b/tools/gn/settings.cc
@@ -9,9 +9,10 @@ #include "tools/gn/filesystem_utils.h" Settings::Settings(const BuildSettings* build_settings, + const Toolchain* toolchain, const std::string& output_subdir_name) : build_settings_(build_settings), - is_default_(false), + toolchain_(toolchain), import_manager_(), base_config_(this), greedy_target_generation_(false) {
diff --git a/tools/gn/settings.h b/tools/gn/settings.h index df6a3d6..1305c58 100644 --- a/tools/gn/settings.h +++ b/tools/gn/settings.h
@@ -38,17 +38,18 @@ // toolchain's outputs. It should have no slashes in it. The default // toolchain should use an empty string. Settings(const BuildSettings* build_settings, + const Toolchain* toolchain, const std::string& output_subdir_name); ~Settings(); const BuildSettings* build_settings() const { return build_settings_; } - const Label& toolchain_label() const { return toolchain_label_; } - void set_toolchain_label(const Label& l) { toolchain_label_ = l; } - - // Indicates if this corresponds to the default toolchain. - bool is_default() const { return is_default_; } - void set_is_default(bool id) { is_default_ = id; } + // Danger: this must only be used for getting the toolchain label until the + // toolchain has been resolved. Otherwise, it will be modified on an + // arbitrary thread when the toolchain invocation is found. Generally, you + // will only read this from the target generation where we know everything + // has been resolved and won't change. + const Toolchain* toolchain() const { return toolchain_; } bool IsMac() const { return target_os_ == MAC; } bool IsLinux() const { return target_os_ == LINUX; } @@ -93,8 +94,7 @@ private: const BuildSettings* build_settings_; - Label toolchain_label_; - bool is_default_; + const Toolchain* toolchain_; TargetOS target_os_;
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc index 47a243a..8bce9b5 100644 --- a/tools/gn/setup.cc +++ b/tools/gn/setup.cc
@@ -202,9 +202,9 @@ Setup::Setup() : CommonSetup(), - empty_settings_(&empty_build_settings_, std::string()), + empty_toolchain_(Label()), + empty_settings_(&empty_build_settings_, &empty_toolchain_, std::string()), dotfile_scope_(&empty_settings_) { - empty_settings_.set_toolchain_label(Label()); } Setup::~Setup() {
diff --git a/tools/gn/setup.h b/tools/gn/setup.h index 8b6d714..c2b3d82 100644 --- a/tools/gn/setup.h +++ b/tools/gn/setup.h
@@ -91,6 +91,7 @@ // These empty settings and toolchain are used to interpret the command line // and dot file. BuildSettings empty_build_settings_; + Toolchain empty_toolchain_; Settings empty_settings_; Scope dotfile_scope_;
diff --git a/tools/gn/target.cc b/tools/gn/target.cc index 28dcf91..291d579 100644 --- a/tools/gn/target.cc +++ b/tools/gn/target.cc
@@ -57,7 +57,8 @@ } // namespace Target::Target(const Settings* settings, const Label& label) - : Item(settings, label), + : Item(label), + settings_(settings), output_type_(UNKNOWN), hard_dep_(false), external_(false), @@ -154,9 +155,9 @@ } // Mark as resolved. - if (!settings()->build_settings()->target_resolved_callback().is_null()) { + if (!settings_->build_settings()->target_resolved_callback().is_null()) { g_scheduler->ScheduleWork(base::Bind(&TargetResolvedThunk, - settings()->build_settings()->target_resolved_callback(), + settings_->build_settings()->target_resolved_callback(), this)); } }
diff --git a/tools/gn/target.h b/tools/gn/target.h index 13b278c..d5c1af3 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h
@@ -58,6 +58,8 @@ bool HasBeenGenerated() const; void SetGenerated(const Token* token); + const Settings* settings() const { return settings_; } + OutputType output_type() const { return output_type_; } void set_output_type(OutputType t) { output_type_ = t; } @@ -147,6 +149,7 @@ // dependencies have been resolved. void PullDependentTargetInfo(std::set<const Config*>* unique_configs); + const Settings* settings_; OutputType output_type_; std::string output_name_;
diff --git a/tools/gn/target_unittest.cc b/tools/gn/target_unittest.cc index ef213b5..ea1700f 100644 --- a/tools/gn/target_unittest.cc +++ b/tools/gn/target_unittest.cc
@@ -15,17 +15,16 @@ public: TargetTest() : build_settings_(), - settings_(&build_settings_, std::string()), - toolchain_(&settings_, Label(SourceDir("//tc/"), "tc")) { - settings_.set_toolchain_label(toolchain_.label()); + toolchain_(Label(SourceDir("//tc/"), "tc")), + settings_(&build_settings_, &toolchain_, std::string()) { } virtual ~TargetTest() { } protected: BuildSettings build_settings_; - Settings settings_; Toolchain toolchain_; + Settings settings_; }; } // namespace @@ -123,15 +122,15 @@ b.deps().push_back(LabelTargetPair(&c)); // Normal non-inherited config. - Config config(&settings_, Label(SourceDir("//foo/"), "config")); + Config config(Label(SourceDir("//foo/"), "config")); c.configs().push_back(LabelConfigPair(&config)); // All dependent config. - Config all(&settings_, Label(SourceDir("//foo/"), "all")); + Config all(Label(SourceDir("//foo/"), "all")); c.all_dependent_configs().push_back(LabelConfigPair(&all)); // Direct dependent config. - Config direct(&settings_, Label(SourceDir("//foo/"), "direct")); + Config direct(Label(SourceDir("//foo/"), "direct")); c.direct_dependent_configs().push_back(LabelConfigPair(&direct)); c.OnResolved();
diff --git a/tools/gn/test_with_scope.cc b/tools/gn/test_with_scope.cc index e149e92..e83c99c 100644 --- a/tools/gn/test_with_scope.cc +++ b/tools/gn/test_with_scope.cc
@@ -6,13 +6,9 @@ TestWithScope::TestWithScope() : build_settings_(), - settings_(&build_settings_, std::string()), - toolchain_(&settings_, Label(SourceDir("//toolchain/"), "default")), + toolchain_(Label(SourceDir("//toolchain/"), "tc")), + settings_(&build_settings_, &toolchain_, std::string()), scope_(&settings_) { - build_settings_.SetBuildDir(SourceDir("//out/Debug/")); - - settings_.set_toolchain_label(toolchain_.label()); - settings_.set_is_default(true); } TestWithScope::~TestWithScope() {
diff --git a/tools/gn/test_with_scope.h b/tools/gn/test_with_scope.h index df2c93e..7d512c4 100644 --- a/tools/gn/test_with_scope.h +++ b/tools/gn/test_with_scope.h
@@ -20,13 +20,12 @@ BuildSettings* build_settings() { return &build_settings_; } Settings* settings() { return &settings_; } - Toolchain* toolchain() { return &toolchain_; } Scope* scope() { return &scope_; } private: BuildSettings build_settings_; - Settings settings_; Toolchain toolchain_; + Settings settings_; Scope scope_; DISALLOW_COPY_AND_ASSIGN(TestWithScope);
diff --git a/tools/gn/toolchain.cc b/tools/gn/toolchain.cc index eee458a..887fba2 100644 --- a/tools/gn/toolchain.cc +++ b/tools/gn/toolchain.cc
@@ -25,8 +25,7 @@ Toolchain::Tool::~Tool() { } -Toolchain::Toolchain(const Settings* settings, const Label& label) - : Item(settings, label) { +Toolchain::Toolchain(const Label& label) : Item(label), is_default_(false) { } Toolchain::~Toolchain() {
diff --git a/tools/gn/toolchain.h b/tools/gn/toolchain.h index 210c2c0..106f2f4 100644 --- a/tools/gn/toolchain.h +++ b/tools/gn/toolchain.h
@@ -70,7 +70,7 @@ std::string rspfile_content; }; - Toolchain(const Settings* settings, const Label& label); + Toolchain(const Label& label); virtual ~Toolchain(); // Item overrides. @@ -87,6 +87,9 @@ const std::string& environment() const { return environment_; } void set_environment(const std::string& env) { environment_ = env; } + bool is_default() const { return is_default_; } + void set_is_default(bool id) { is_default_ = id; } + // Specifies build argument overrides that will be set on the base scope. It // will be as if these arguments were passed in on the command line. This // allows a toolchain to override the OS type of the default toolchain or @@ -97,6 +100,7 @@ private: Tool tools_[TYPE_NUMTYPES]; + bool is_default_; Scope::KeyValueMap args_; std::string environment_;
diff --git a/tools/gn/toolchain_manager.cc b/tools/gn/toolchain_manager.cc index 4edf212..affdec9 100644 --- a/tools/gn/toolchain_manager.cc +++ b/tools/gn/toolchain_manager.cc
@@ -78,17 +78,11 @@ const Label& toolchain_name, const std::string& output_subdir_name) : state(TOOLCHAIN_NOT_LOADED), - settings(build_settings, output_subdir_name), - toolchain(new Toolchain(&settings, toolchain_name)), + toolchain(toolchain_name), toolchain_set(false), + settings(build_settings, &toolchain, output_subdir_name), toolchain_file_loaded(false), item_node(NULL) { - settings.set_toolchain_label(toolchain_name); - } - - ~Info() { - if (!item_node) // See toolchain definition for more. - delete toolchain; } // Makes sure that an ItemNode is created for the toolchain, which lets @@ -101,13 +95,17 @@ void EnsureItemNode() { if (!item_node) { ItemTree& tree = settings.build_settings()->item_tree(); - item_node = new ItemNode(toolchain); + item_node = new ItemNode(&toolchain); tree.AddNodeLocked(item_node); } } ToolchainState state; + Toolchain toolchain; + bool toolchain_set; + LocationRange toolchain_definition_location; + // The first place in the build that we saw a reference for this toolchain. // This allows us to report errors if it can't be loaded and blame some // reasonable place of the code. This will be empty for the default toolchain. @@ -120,13 +118,6 @@ // is only ever read or written inside the lock. Settings settings; - // When we create an item node, this pointer will be owned by that node - // so it's lifetime is managed by the dependency graph. Before we've created - // the ItemNode, this class has to takre responsibility for this pointer. - Toolchain* toolchain; - bool toolchain_set; - LocationRange toolchain_definition_location; - // Set when the file corresponding to the toolchain definition is loaded. // This will normally be set right after "toolchain_set". However, if the // toolchain definition is missing, the file might be marked loaded but the @@ -211,7 +202,7 @@ // Since we don't allow defining a toolchain more than once, we know that // once it's set it won't be mutated, so we can safely return this pointer // for reading outside the lock. - return found->second->toolchain; + return &found->second->toolchain; } bool ToolchainManager::SetDefaultToolchainUnlocked( @@ -270,10 +261,13 @@ } // The labels should match or else we're setting the wrong one! - CHECK(info->toolchain->label() == tc.label()); + CHECK(info->toolchain.label() == tc.label()); - // Save the toolchain. We can just overwrite our definition. - *info->toolchain = tc; + // Save the toolchain. We can just overwrite our definition, but we need to + // be careful to preserve the is_default flag. + bool is_default = info->toolchain.is_default(); + info->toolchain = tc; + info->toolchain.set_is_default(is_default); if (info->toolchain_set) { *err = Err(defined_from, "Duplicate toolchain definition."); @@ -429,9 +423,8 @@ // to set the label, but we can assign the toolchain to a new one. Loading // the build config can not change the toolchain, so we won't be overwriting // anything useful. - *info->toolchain = Toolchain(&info->settings, default_toolchain_); - info->settings.set_is_default(true); - info->settings.set_toolchain_label(default_toolchain_); + info->toolchain = Toolchain(default_toolchain_); + info->toolchain.set_is_default(true); info->EnsureItemNode(); // The default toolchain is loaded in greedy mode so all targets we @@ -493,8 +486,8 @@ Scope* base_config = info->settings.base_config(); base_config->set_source_dir(SourceDir("//")); - info->settings.build_settings()->build_args().SetupRootScope( - base_config, info->toolchain->args()); + info->settings.build_settings()->build_args().SetupRootScope(base_config, + info->settings.toolchain()->args()); base_config->SetProcessingBuildConfig(); if (is_default) @@ -502,7 +495,7 @@ ScopedTrace trace(TraceItem::TRACE_FILE_EXECUTE, info->settings.build_settings()->build_config_file().value()); - trace.SetToolchain(info->settings.toolchain_label()); + trace.SetToolchain(info->settings.toolchain()->label()); const BlockNode* root_block = root->AsBlock(); Err err; @@ -547,7 +540,7 @@ if (root && !g_scheduler->is_failed()) { if (g_scheduler->verbose_logging()) { g_scheduler->Log("Running", file_name.value() + " with toolchain " + - info->toolchain->label().GetUserVisibleName(false)); + info->toolchain.label().GetUserVisibleName(false)); } Scope our_scope(info->settings.base_config()); @@ -555,7 +548,7 @@ our_scope.set_source_dir(file_name.GetDir()); ScopedTrace trace(TraceItem::TRACE_FILE_EXECUTE, file_name.value()); - trace.SetToolchain(info->settings.toolchain_label()); + trace.SetToolchain(info->settings.toolchain()->label()); Err err; root->Execute(&our_scope, &err);