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 Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 81ca22ea8eeafe919df5a6599884b6a664a08c62
diff --git a/tools/gn/config.cc b/tools/gn/config.cc index 850d596..5b89e2b 100644 --- a/tools/gn/config.cc +++ b/tools/gn/config.cc
@@ -10,7 +10,8 @@ #include "tools/gn/item_tree.h" #include "tools/gn/scheduler.h" -Config::Config(const Label& label) : Item(label) { +Config::Config(const Settings* settings, const Label& label) + : Item(settings, label) { } Config::~Config() { @@ -38,7 +39,7 @@ ItemNode* node = tree->GetExistingNodeLocked(label); Config* config = NULL; if (!node) { - config = new Config(label); + config = new Config(settings, 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 0ee9b7b..e5a1df6 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 Label& label); + Config(const Settings* settings, 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 bf55165..b382575 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 933d55e..dde0306 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(label); + Toolchain toolchain(scope->settings(), label); Scope block_scope(scope); block_scope.SetProperty(&kToolchainPropertyKey, &toolchain);
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc index acc8fe9..0e27297 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 f69ed8f..fb669af 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 { - const Toolchain* toolchain = target->settings()->toolchain(); - if (toolchain->is_default()) + if (target->settings()->is_default()) return target->label().name(); // Default toolchain has no suffix. - return target->label().name() + "_" + toolchain->label().name(); + return target->label().name() + "_" + + target->settings()->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 19a8047..c40bdba 100644 --- a/tools/gn/input_conversion.cc +++ b/tools/gn/input_conversion.cc
@@ -117,9 +117,7 @@ } BuildSettings build_settings; - Label empty_label; - Toolchain toolchain(empty_label); - Settings settings(&build_settings, &toolchain, std::string()); + Settings settings(&build_settings, std::string()); Scope scope(&settings); Err nested_err;
diff --git a/tools/gn/item.cc b/tools/gn/item.cc index 747183c..fa16975 100644 --- a/tools/gn/item.cc +++ b/tools/gn/item.cc
@@ -6,7 +6,9 @@ #include "base/logging.h" -Item::Item(const Label& label) : label_(label) { +Item::Item(const Settings* settings, const Label& label) + : settings_(settings), + label_(label) { } Item::~Item() {
diff --git a/tools/gn/item.h b/tools/gn/item.h index 2538c50..69115bd 100644 --- a/tools/gn/item.h +++ b/tools/gn/item.h
@@ -11,6 +11,7 @@ class Config; class ItemNode; +class Settings; class Target; class Toolchain; @@ -18,9 +19,11 @@ // graph. class Item { public: - Item(const Label& label); + Item(const Settings* settings, 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_; } @@ -49,6 +52,7 @@ 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 3a1d620..c404006 100644 --- a/tools/gn/ninja_binary_target_writer.cc +++ b/tools/gn/ninja_binary_target_writer.cc
@@ -86,8 +86,9 @@ } // namespace NinjaBinaryTargetWriter::NinjaBinaryTargetWriter(const Target* target, + const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, out), + : NinjaTargetWriter(target, toolchain, out), tool_type_(GetToolTypeForTarget(target)){ } @@ -145,7 +146,6 @@ 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_, toolchain, input_file_type); + helper_.GetRuleForSourceType(settings_, input_file_type); if (command.empty()) continue; // Skip files not needing compilation. @@ -265,8 +265,7 @@ RecursiveTargetConfigStringsToStream(target_, &ConfigValues::ldflags, flag_options, out_); - const Toolchain* toolchain = GetToolchain(); - const Toolchain::Tool& tool = toolchain->GetTool(tool_type_); + const Toolchain::Tool& tool = toolchain_->GetTool(tool_type_); // Followed by library search paths that have been recursively pushed // through the dependency tree. @@ -306,7 +305,7 @@ path_output_.WriteFile(out_, external_output_file); } out_ << ": " - << helper_.GetRulePrefix(GetToolchain()) + << helper_.GetRulePrefix(target_->settings()) << Toolchain::ToolTypeToName(tool_type_); std::set<OutputFile> extra_object_files; @@ -346,7 +345,7 @@ out_ << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()->toolchain()) + << helper_.GetRulePrefix(target_->settings()) << "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 8c57ffb..c188335 100644 --- a/tools/gn/ninja_binary_target_writer.h +++ b/tools/gn/ninja_binary_target_writer.h
@@ -13,7 +13,9 @@ // library, or a static library). class NinjaBinaryTargetWriter : public NinjaTargetWriter { public: - NinjaBinaryTargetWriter(const Target* target, std::ostream& out); + NinjaBinaryTargetWriter(const Target* target, + const Toolchain* toolchain, + 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 a7de4a1..96a7448 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, out); + NinjaBinaryTargetWriter writer(&target, setup.toolchain(), 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: tc_cxx ../../foo/input1.cc\n" - "build obj/foo/bar.input2.obj: tc_cxx ../../foo/input2.cc\n" + "build obj/foo/bar.input1.obj: cxx ../../foo/input1.cc\n" + "build obj/foo/bar.input2.obj: cxx ../../foo/input2.cc\n" "\n" - "build obj/foo/bar.stamp: tc_stamp obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n"; + "build obj/foo/bar.stamp: 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, out); + NinjaBinaryTargetWriter writer(&shlib_target, setup.toolchain(), 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: tc_solink obj/foo/bar.input1.obj obj/foo/bar.input2.obj\n" + "build shlib.dll shlib.dll.lib: 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 5de249f..89b3e3f 100644 --- a/tools/gn/ninja_copy_target_writer.cc +++ b/tools/gn/ninja_copy_target_writer.cc
@@ -9,8 +9,9 @@ #include "tools/gn/string_utils.h" NinjaCopyTargetWriter::NinjaCopyTargetWriter(const Target* target, + const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, out) { + : NinjaTargetWriter(target, toolchain, out) { } NinjaCopyTargetWriter::~NinjaCopyTargetWriter() { @@ -22,8 +23,7 @@ std::vector<OutputFile> output_files; - std::string rule_prefix = - helper_.GetRulePrefix(target_->settings()->toolchain()); + std::string rule_prefix = helper_.GetRulePrefix(target_->settings()); 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 99bbaff..cadbc38 100644 --- a/tools/gn/ninja_copy_target_writer.h +++ b/tools/gn/ninja_copy_target_writer.h
@@ -11,7 +11,9 @@ // Writes a .ninja file for a copy target type. class NinjaCopyTargetWriter : public NinjaTargetWriter { public: - NinjaCopyTargetWriter(const Target* target, std::ostream& out); + NinjaCopyTargetWriter(const Target* target, + const Toolchain* toolchain, + 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 c31cbd2..5642d9d 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, out); + NinjaCopyTargetWriter writer(&target, setup.toolchain(), out); writer.Run(); const char expected_linux[] = - "build input1.out: tc_copy ../../foo/input1.txt\n" - "build input2.out: tc_copy ../../foo/input2.txt\n" + "build input1.out: copy ../../foo/input1.txt\n" + "build input2.out: copy ../../foo/input2.txt\n" "\n" - "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: 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, out); + NinjaCopyTargetWriter writer(&target, setup.toolchain(), 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: tc_copy ../../foo/input1.txt\n" - "build input2.out: tc_copy ../../foo/input2.txt\n" + "build input1.out: copy ../../foo/input1.txt\n" + "build input2.out: copy ../../foo/input2.txt\n" "\n" - "build obj/foo/bar.stamp: tc_stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: 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 606f916..d29f917 100644 --- a/tools/gn/ninja_group_target_writer.cc +++ b/tools/gn/ninja_group_target_writer.cc
@@ -8,8 +8,9 @@ #include "tools/gn/string_utils.h" NinjaGroupTargetWriter::NinjaGroupTargetWriter(const Target* target, + const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, out) { + : NinjaTargetWriter(target, toolchain, out) { } NinjaGroupTargetWriter::~NinjaGroupTargetWriter() { @@ -21,7 +22,7 @@ out_ << std::endl << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()->toolchain()) + << helper_.GetRulePrefix(target_->settings()) << "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 31625f8..862b920 100644 --- a/tools/gn/ninja_group_target_writer.h +++ b/tools/gn/ninja_group_target_writer.h
@@ -11,7 +11,9 @@ // Writes a .ninja file for a group target type. class NinjaGroupTargetWriter : public NinjaTargetWriter { public: - NinjaGroupTargetWriter(const Target* target, std::ostream& out); + NinjaGroupTargetWriter(const Target* target, + const Toolchain* toolchain, + std::ostream& out); virtual ~NinjaGroupTargetWriter(); virtual void Run() OVERRIDE;
diff --git a/tools/gn/ninja_helper.cc b/tools/gn/ninja_helper.cc index d90b698..e872c24 100644 --- a/tools/gn/ninja_helper.cc +++ b/tools/gn/ninja_helper.cc
@@ -189,23 +189,20 @@ return ret; } -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::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::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(toolchain); + std::string prefix = GetRulePrefix(settings); if (type == SOURCE_C) return prefix + "cc";
diff --git a/tools/gn/ninja_helper.h b/tools/gn/ninja_helper.h index bef2fcd..84a2c89 100644 --- a/tools/gn/ninja_helper.h +++ b/tools/gn/ninja_helper.h
@@ -52,12 +52,11 @@ // Returns the prefix for rules on the given toolchain. We need this to // disambiguate a given rule for each toolchain. - std::string GetRulePrefix(const Toolchain* toolchain) const; + std::string GetRulePrefix(const Settings* settings) 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 6e3b25c..63082bc 100644 --- a/tools/gn/ninja_helper_unittest.cc +++ b/tools/gn/ninja_helper_unittest.cc
@@ -17,9 +17,10 @@ public: HelperSetterUpper() : build_settings(), - toolchain(Label(SourceDir("//"), "tc")), - settings(&build_settings, &toolchain, std::string()), + settings(&build_settings, std::string()), + toolchain(&settings, Label(SourceDir("//"), "tc")), 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 1b8358c..e8cdef4 100644 --- a/tools/gn/ninja_script_target_writer.cc +++ b/tools/gn/ninja_script_target_writer.cc
@@ -11,8 +11,9 @@ #include "tools/gn/target.h" NinjaScriptTargetWriter::NinjaScriptTargetWriter(const Target* target, + const Toolchain* toolchain, std::ostream& out) - : NinjaTargetWriter(target, out), + : NinjaTargetWriter(target, toolchain, out), path_output_no_escaping_( target->settings()->build_settings()->build_dir(), ESCAPE_NONE, false) { @@ -152,7 +153,7 @@ out_ << "build "; path_output_.WriteFile(out_, helper_.GetTargetOutputFile(target_)); out_ << ": " - << helper_.GetRulePrefix(target_->settings()->toolchain()) + << helper_.GetRulePrefix(target_->settings()) << "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 e84d618..9c51a6a 100644 --- a/tools/gn/ninja_script_target_writer.h +++ b/tools/gn/ninja_script_target_writer.h
@@ -17,7 +17,9 @@ // Writes a .ninja file for a custom script target type. class NinjaScriptTargetWriter : public NinjaTargetWriter { public: - NinjaScriptTargetWriter(const Target* target, std::ostream& out); + NinjaScriptTargetWriter(const Target* target, + const Toolchain* toolchain, + 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 1394717..f2c8a93 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, out); + NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); FileTemplate output_template = writer.GetOutputTemplate(); @@ -42,7 +42,7 @@ Target target(setup.settings(), Label(SourceDir("//foo/"), "bar")); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, out); + NinjaScriptTargetWriter writer(&target, setup.toolchain(), out); std::vector<std::string> args; args.push_back("-i"); @@ -88,7 +88,7 @@ "/usr/bin/python"))); std::ostringstream out; - NinjaScriptTargetWriter writer(&target, out); + NinjaScriptTargetWriter writer(&target, setup.toolchain(), 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: tc_stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: 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, out); + NinjaScriptTargetWriter writer(&target, setup.toolchain(), 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: tc_stamp input1.out input2.out\n"; + "build obj/foo/bar.stamp: 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 d73c873..bdc0472 100644 --- a/tools/gn/ninja_target_writer.cc +++ b/tools/gn/ninja_target_writer.cc
@@ -19,9 +19,12 @@ #include "tools/gn/target.h" #include "tools/gn/trace.h" -NinjaTargetWriter::NinjaTargetWriter(const Target* target, std::ostream& out) +NinjaTargetWriter::NinjaTargetWriter(const Target* target, + const Toolchain* toolchain, + std::ostream& out) : settings_(target->settings()), target_(target), + toolchain_(toolchain), out_(out), path_output_(settings_->build_settings()->build_dir(), ESCAPE_NINJA, true), @@ -45,7 +48,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( @@ -54,6 +57,10 @@ 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 @@ -62,19 +69,19 @@ // Call out to the correct sub-type of writer. if (target->output_type() == Target::COPY_FILES) { - NinjaCopyTargetWriter writer(target, file); + NinjaCopyTargetWriter writer(target, tc, file); writer.Run(); } else if (target->output_type() == Target::CUSTOM) { - NinjaScriptTargetWriter writer(target, file); + NinjaScriptTargetWriter writer(target, tc, file); writer.Run(); } else if (target->output_type() == Target::GROUP) { - NinjaGroupTargetWriter writer(target, file); + NinjaGroupTargetWriter writer(target, tc, 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, file); + NinjaBinaryTargetWriter writer(target, tc, file); writer.Run(); } else { CHECK(0); @@ -85,10 +92,6 @@ 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 c9c72a7..e1c3bf8 100644 --- a/tools/gn/ninja_target_writer.h +++ b/tools/gn/ninja_target_writer.h
@@ -19,7 +19,9 @@ // generated by the NinjaBuildWriter. class NinjaTargetWriter { public: - NinjaTargetWriter(const Target* target, std::ostream& out); + NinjaTargetWriter(const Target* target, + const Toolchain* toolchain, + std::ostream& out); virtual ~NinjaTargetWriter(); static void RunAndWriteFile(const Target* target); @@ -27,9 +29,6 @@ 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 @@ -41,6 +40,7 @@ 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 310caba..5f33e69 100644 --- a/tools/gn/ninja_toolchain_writer.cc +++ b/tools/gn/ninja_toolchain_writer.cc
@@ -13,6 +13,7 @@ #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( @@ -62,11 +63,14 @@ } void NinjaToolchainWriter::WriteRules() { - const Toolchain* tc = settings_->toolchain(); + const Toolchain* tc = settings_->build_settings()->toolchain_manager() + .GetToolchainDefinitionUnlocked(settings_->toolchain_label()); + CHECK(tc); + std::string indent(" "); NinjaHelper helper(settings_->build_settings()); - std::string rule_prefix = helper.GetRulePrefix(tc); + std::string rule_prefix = helper.GetRulePrefix(settings_); 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 525aab6..1f4bc25 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 1137d3b..776a0aa 100644 --- a/tools/gn/scope_per_file_provider_unittest.cc +++ b/tools/gn/scope_per_file_provider_unittest.cc
@@ -6,34 +6,26 @@ #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) { - 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/")); + TestWithScope test; // Prevent horrible wrapping of calls below. #define GPV(val) provider.GetProgrammaticValue(val)->string_value() // Test the default toolchain. { - Toolchain toolchain(Label(SourceDir("//toolchain/"), "tc")); - Settings settings(&build_settings, &toolchain, std::string()); - - Scope scope(&settings); + Scope scope(test.settings()); scope.set_source_dir(SourceDir("//source/")); ScopePerFileProvider provider(&scope); - EXPECT_EQ("//toolchain:tc", GPV(variables::kCurrentToolchain)); - EXPECT_EQ("//toolchain:default", GPV(variables::kDefaultToolchain)); + 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("//out/Debug", GPV(variables::kRootBuildDir)); EXPECT_EQ("//out/Debug/gen", GPV(variables::kRootGenDir)); EXPECT_EQ("//out/Debug", GPV(variables::kRootOutDir)); @@ -43,13 +35,17 @@ // Test some with an alternate toolchain. { - Toolchain toolchain(Label(SourceDir("//toolchain/"), "tc")); - Settings settings(&build_settings, &toolchain, "tc"); + Settings settings(test.build_settings(), "tc"); + Toolchain toolchain(&settings, Label(SourceDir("//toolchain/"), "tc")); + settings.set_toolchain_label(toolchain.label()); 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 bfab85d..b8de976 100644 --- a/tools/gn/settings.cc +++ b/tools/gn/settings.cc
@@ -9,10 +9,9 @@ #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), - toolchain_(toolchain), + is_default_(false), import_manager_(), base_config_(this), greedy_target_generation_(false) {
diff --git a/tools/gn/settings.h b/tools/gn/settings.h index 1305c58..df6a3d6 100644 --- a/tools/gn/settings.h +++ b/tools/gn/settings.h
@@ -38,18 +38,17 @@ // 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_; } - // 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_; } + 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; } bool IsMac() const { return target_os_ == MAC; } bool IsLinux() const { return target_os_ == LINUX; } @@ -94,7 +93,8 @@ private: const BuildSettings* build_settings_; - const Toolchain* toolchain_; + Label toolchain_label_; + bool is_default_; TargetOS target_os_;
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc index 8bce9b5..47a243a 100644 --- a/tools/gn/setup.cc +++ b/tools/gn/setup.cc
@@ -202,9 +202,9 @@ Setup::Setup() : CommonSetup(), - empty_toolchain_(Label()), - empty_settings_(&empty_build_settings_, &empty_toolchain_, std::string()), + empty_settings_(&empty_build_settings_, 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 c2b3d82..8b6d714 100644 --- a/tools/gn/setup.h +++ b/tools/gn/setup.h
@@ -91,7 +91,6 @@ // 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 291d579..28dcf91 100644 --- a/tools/gn/target.cc +++ b/tools/gn/target.cc
@@ -57,8 +57,7 @@ } // namespace Target::Target(const Settings* settings, const Label& label) - : Item(label), - settings_(settings), + : Item(settings, label), output_type_(UNKNOWN), hard_dep_(false), external_(false), @@ -155,9 +154,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 d5c1af3..13b278c 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h
@@ -58,8 +58,6 @@ 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; } @@ -149,7 +147,6 @@ // 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 ea1700f..ef213b5 100644 --- a/tools/gn/target_unittest.cc +++ b/tools/gn/target_unittest.cc
@@ -15,16 +15,17 @@ public: TargetTest() : build_settings_(), - toolchain_(Label(SourceDir("//tc/"), "tc")), - settings_(&build_settings_, &toolchain_, std::string()) { + settings_(&build_settings_, std::string()), + toolchain_(&settings_, Label(SourceDir("//tc/"), "tc")) { + settings_.set_toolchain_label(toolchain_.label()); } virtual ~TargetTest() { } protected: BuildSettings build_settings_; - Toolchain toolchain_; Settings settings_; + Toolchain toolchain_; }; } // namespace @@ -122,15 +123,15 @@ b.deps().push_back(LabelTargetPair(&c)); // Normal non-inherited config. - Config config(Label(SourceDir("//foo/"), "config")); + Config config(&settings_, Label(SourceDir("//foo/"), "config")); c.configs().push_back(LabelConfigPair(&config)); // All dependent config. - Config all(Label(SourceDir("//foo/"), "all")); + Config all(&settings_, Label(SourceDir("//foo/"), "all")); c.all_dependent_configs().push_back(LabelConfigPair(&all)); // Direct dependent config. - Config direct(Label(SourceDir("//foo/"), "direct")); + Config direct(&settings_, 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 e83c99c..e149e92 100644 --- a/tools/gn/test_with_scope.cc +++ b/tools/gn/test_with_scope.cc
@@ -6,9 +6,13 @@ TestWithScope::TestWithScope() : build_settings_(), - toolchain_(Label(SourceDir("//toolchain/"), "tc")), - settings_(&build_settings_, &toolchain_, std::string()), + settings_(&build_settings_, std::string()), + toolchain_(&settings_, Label(SourceDir("//toolchain/"), "default")), 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 7d512c4..df2c93e 100644 --- a/tools/gn/test_with_scope.h +++ b/tools/gn/test_with_scope.h
@@ -20,12 +20,13 @@ BuildSettings* build_settings() { return &build_settings_; } Settings* settings() { return &settings_; } + Toolchain* toolchain() { return &toolchain_; } Scope* scope() { return &scope_; } private: BuildSettings build_settings_; - Toolchain toolchain_; Settings settings_; + Toolchain toolchain_; Scope scope_; DISALLOW_COPY_AND_ASSIGN(TestWithScope);
diff --git a/tools/gn/toolchain.cc b/tools/gn/toolchain.cc index 887fba2..eee458a 100644 --- a/tools/gn/toolchain.cc +++ b/tools/gn/toolchain.cc
@@ -25,7 +25,8 @@ Toolchain::Tool::~Tool() { } -Toolchain::Toolchain(const Label& label) : Item(label), is_default_(false) { +Toolchain::Toolchain(const Settings* settings, const Label& label) + : Item(settings, label) { } Toolchain::~Toolchain() {
diff --git a/tools/gn/toolchain.h b/tools/gn/toolchain.h index 106f2f4..210c2c0 100644 --- a/tools/gn/toolchain.h +++ b/tools/gn/toolchain.h
@@ -70,7 +70,7 @@ std::string rspfile_content; }; - Toolchain(const Label& label); + Toolchain(const Settings* settings, const Label& label); virtual ~Toolchain(); // Item overrides. @@ -87,9 +87,6 @@ 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 @@ -100,7 +97,6 @@ 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 affdec9..4edf212 100644 --- a/tools/gn/toolchain_manager.cc +++ b/tools/gn/toolchain_manager.cc
@@ -78,11 +78,17 @@ const Label& toolchain_name, const std::string& output_subdir_name) : state(TOOLCHAIN_NOT_LOADED), - toolchain(toolchain_name), + settings(build_settings, output_subdir_name), + toolchain(new Toolchain(&settings, 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 @@ -95,17 +101,13 @@ 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. @@ -118,6 +120,13 @@ // 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 @@ -202,7 +211,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( @@ -261,13 +270,10 @@ } // 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, 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); + // Save the toolchain. We can just overwrite our definition. + *info->toolchain = tc; if (info->toolchain_set) { *err = Err(defined_from, "Duplicate toolchain definition."); @@ -423,8 +429,9 @@ // 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(default_toolchain_); - info->toolchain.set_is_default(true); + *info->toolchain = Toolchain(&info->settings, default_toolchain_); + info->settings.set_is_default(true); + info->settings.set_toolchain_label(default_toolchain_); info->EnsureItemNode(); // The default toolchain is loaded in greedy mode so all targets we @@ -486,8 +493,8 @@ Scope* base_config = info->settings.base_config(); base_config->set_source_dir(SourceDir("//")); - info->settings.build_settings()->build_args().SetupRootScope(base_config, - info->settings.toolchain()->args()); + info->settings.build_settings()->build_args().SetupRootScope( + base_config, info->toolchain->args()); base_config->SetProcessingBuildConfig(); if (is_default) @@ -495,7 +502,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; @@ -540,7 +547,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()); @@ -548,7 +555,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);