Makes declared_arguments per toolchain

This means each toolchain can declare arguments as it sees fit. Prior to this change each toolchain needed to declare the argument at exactly the same place, which is not always possible. For example, the following triggered an error:

declare_args() {
  foo = 1
  if (xxx) {
    foo = 2
  }

with xxx being specific to one toolchain.

TEST=covered by test
BUG=465029
R=brettw@chromium.org

Review URL: https://codereview.chromium.org/984353002

Cr-Commit-Position: refs/heads/master@{#320753}

tools/gn/BUILD.gn

@@ -222,6 +222,7 @@ executable("gn") {
 test("gn_unittests") {
   sources = [
     "action_target_generator_unittest.cc",
+    "args_unittest.cc",
     "builder_unittest.cc",
     "c_include_iterator_unittest.cc",
     "command_format_unittest.cc",

tools/gn/args.cc

@@ -66,13 +66,30 @@ const char kBuildArgs_Help[] =
     "  to specify build args in an \"import\"-ed file if you want such\n"
     "  arguments to apply to multiple buildfiles.\n";
 
+namespace {
+
+// Removes all entries in |overrides| that are in |declared_overrides|.
+void RemoveDeclaredOverrides(const Scope::KeyValueMap& declared_arguments,
+                             Scope::KeyValueMap* overrides) {
+  for (Scope::KeyValueMap::iterator override = overrides->begin();
+       override != overrides->end();) {
+    if (declared_arguments.find(override->first) == declared_arguments.end())
+      ++override;
+    else
+      overrides->erase(override++);
+  }
+}
+
+}  // namespace
+
 Args::Args() {
 }
 
 Args::Args(const Args& other)
     : overrides_(other.overrides_),
       all_overrides_(other.all_overrides_),
-      declared_arguments_(other.declared_arguments_) {
+      declared_arguments_per_toolchain_(
+          other.declared_arguments_per_toolchain_) {
 }
 
 Args::~Args() {
@@ -124,6 +141,8 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
                        Err* err) const {
   base::AutoLock lock(lock_);
 
+  Scope::KeyValueMap& declared_arguments(
+      DeclaredArgumentsForToolchainLocked(scope_to_set));
   for (const auto& arg : args) {
     // Verify that the value hasn't already been declared. We want each value
     // to be declared only once.
@@ -132,8 +151,8 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
     // when used from different toolchains, so we can't just check that we've
     // seen it before. Instead, we check that the location matches.
     Scope::KeyValueMap::iterator previously_declared =
-        declared_arguments_.find(arg.first);
-    if (previously_declared != declared_arguments_.end()) {
+        declared_arguments.find(arg.first);
+    if (previously_declared != declared_arguments.end()) {
       if (previously_declared->second.origin() != arg.second.origin()) {
         // Declaration location mismatch.
         *err = Err(arg.second.origin(),
@@ -151,7 +170,7 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
         return false;
       }
     } else {
-      declared_arguments_.insert(arg);
+      declared_arguments.insert(arg);
     }
 
     // Only set on the current scope to the new value if it hasn't been already
@@ -168,43 +187,41 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
 
 bool Args::VerifyAllOverridesUsed(Err* err) const {
   base::AutoLock lock(lock_);
-  return VerifyAllOverridesUsed(all_overrides_, declared_arguments_, err);
-}
-
-bool Args::VerifyAllOverridesUsed(
-    const Scope::KeyValueMap& overrides,
-    const Scope::KeyValueMap& declared_arguments,
-    Err* err) {
-  for (const auto& override : overrides) {
-    if (declared_arguments.find(override.first) == declared_arguments.end()) {
-      // Get a list of all possible overrides for help with error finding.
-      //
-      // It might be nice to do edit distance checks to see if we can find one
-      // close to what you typed.
-      std::string all_declared_str;
-      for (Scope::KeyValueMap::const_iterator cur_str =
-               declared_arguments.begin();
-           cur_str != declared_arguments.end(); ++cur_str) {
-        if (cur_str != declared_arguments.begin())
-          all_declared_str += ", ";
-        all_declared_str += cur_str->first.as_string();
-      }
-
-      *err = Err(override.second.origin(), "Build argument has no effect.",
-          "The variable \"" + override.first.as_string() +
-          "\" was set as a build "
-          "argument\nbut never appeared in a declare_args() block in any "
-          "buildfile.\n\nPossible arguments: " + all_declared_str);
-      return false;
+  Scope::KeyValueMap all_overrides(all_overrides_);
+  for (const auto& map_pair : declared_arguments_per_toolchain_)
+    RemoveDeclaredOverrides(map_pair.second, &all_overrides);
+
+  if (all_overrides.empty())
+    return true;
+
+  // Get a list of all possible overrides for help with error finding.
+  //
+  // It might be nice to do edit distance checks to see if we can find one close
+  // to what you typed.
+  std::string all_declared_str;
+  for (const auto& map_pair : declared_arguments_per_toolchain_) {
+    for (const auto& cur_str : map_pair.second) {
+      if (!all_declared_str.empty())
+        all_declared_str += ", ";
+      all_declared_str += cur_str.first.as_string();
     }
   }
-  return true;
+
+  *err = Err(
+      all_overrides.begin()->second.origin(), "Build argument has no effect.",
+      "The variable \"" + all_overrides.begin()->first.as_string() +
+          "\" was set as a build argument\nbut never appeared in a " +
+          "declare_args() block in any buildfile.\n\nPossible arguments: " +
+          all_declared_str);
+  return false;
 }
 
 void Args::MergeDeclaredArguments(Scope::KeyValueMap* dest) const {
   base::AutoLock lock(lock_);
-  for (const auto& arg : declared_arguments_)
-    (*dest)[arg.first] = arg.second;
+  for (const auto& map_pair : declared_arguments_per_toolchain_) {
+    for (const auto& arg : map_pair.second)
+      (*dest)[arg.first] = arg.second;
+  }
 }
 
 void Args::SetSystemVarsLocked(Scope* dest) const {
@@ -257,12 +274,14 @@ void Args::SetSystemVarsLocked(Scope* dest) const {
   dest->SetValue(variables::kTargetCpu, empty_string, nullptr);
   dest->SetValue(variables::kCurrentCpu, empty_string, nullptr);
 
-  declared_arguments_[variables::kHostOs] = os_val;
-  declared_arguments_[variables::kCurrentOs] = empty_string;
-  declared_arguments_[variables::kTargetOs] = empty_string;
-  declared_arguments_[variables::kHostCpu] = arch_val;
-  declared_arguments_[variables::kCurrentCpu] = empty_string;
-  declared_arguments_[variables::kTargetCpu] = empty_string;
+  Scope::KeyValueMap& declared_arguments(
+      DeclaredArgumentsForToolchainLocked(dest));
+  declared_arguments[variables::kHostOs] = os_val;
+  declared_arguments[variables::kCurrentOs] = empty_string;
+  declared_arguments[variables::kTargetOs] = empty_string;
+  declared_arguments[variables::kHostCpu] = arch_val;
+  declared_arguments[variables::kCurrentCpu] = empty_string;
+  declared_arguments[variables::kTargetCpu] = empty_string;
 
   // Mark these variables used so the build config file can override them
   // without geting a warning about overwriting an unused variable.
@@ -286,3 +305,9 @@ void Args::SaveOverrideRecordLocked(const Scope::KeyValueMap& values) const {
   for (const auto& val : values)
     all_overrides_[val.first] = val.second;
 }
+
+Scope::KeyValueMap& Args::DeclaredArgumentsForToolchainLocked(
+    Scope* scope) const {
+  lock_.AssertAcquired();
+  return declared_arguments_per_toolchain_[scope->settings()];
+}

tools/gn/args.h

@@ -61,19 +61,15 @@ class Args {
   // arguments. If there are, this returns false and sets the error.
   bool VerifyAllOverridesUsed(Err* err) const;
 
-  // Like VerifyAllOverridesUsed but takes the lists of overrides specified and
-  // parameters declared.
-  static bool VerifyAllOverridesUsed(
-      const Scope::KeyValueMap& overrides,
-      const Scope::KeyValueMap& declared_arguments,
-      Err* err);
-
   // Adds all declared arguments to the given output list. If the values exist
   // in the list already, their values will be overwriten, but other values
   // already in the list will remain.
   void MergeDeclaredArguments(Scope::KeyValueMap* dest) const;
 
  private:
+  using DeclaredArgumentsPerToolchain =
+      base::hash_map<const Settings*, Scope::KeyValueMap>;
+
   // Sets the default config based on the current system.
   void SetSystemVarsLocked(Scope* scope) const;
 
@@ -83,6 +79,10 @@ class Args {
 
   void SaveOverrideRecordLocked(const Scope::KeyValueMap& values) const;
 
+  // Returns the KeyValueMap used for arguments declared for the specified
+  // toolchain.
+  Scope::KeyValueMap& DeclaredArgumentsForToolchainLocked(Scope* scope) const;
+
   // Since this is called during setup which we assume is single-threaded,
   // this is not protected by the lock. It should be set only during init.
   Scope::KeyValueMap overrides_;
@@ -94,9 +94,12 @@ class Args {
   // check for overrides that were specified but never used.
   mutable Scope::KeyValueMap all_overrides_;
 
-  // Tracks all variables declared in any buildfile. This is so we can see if
-  // the user set variables on the command line that are not used anywhere.
-  mutable Scope::KeyValueMap declared_arguments_;
+  // Maps from Settings (which corresponds to a toolchain) to the map of
+  // declared variables. This is used to tracks all variables declared in any
+  // buildfile. This is so we can see if the user set variables on the command
+  // line that are not used anywhere. Each map is toolchain specific as each
+  // toolchain may define variables in different locations.
+  mutable DeclaredArgumentsPerToolchain declared_arguments_per_toolchain_;
 
   Args& operator=(const Args& other);  // Disallow assignment.
 };

tools/gn/args_unittest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "tools/gn/args.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "tools/gn/scheduler.h"
+#include "tools/gn/test_with_scope.h"
+
+// Assertions for VerifyAllOverridesUsed() and DeclareArgs() with multiple
+// toolchains.
+TEST(ArgsTest, VerifyAllOverridesUsed) {
+  TestWithScope setup1, setup2;
+  Args args;
+  Scope::KeyValueMap key_value_map1;
+  Err err;
+  LiteralNode assignment1;
+
+  setup1.scope()->SetValue("a", Value(nullptr, true), &assignment1);
+  setup1.scope()->GetCurrentScopeValues(&key_value_map1);
+  EXPECT_TRUE(args.DeclareArgs(key_value_map1, setup1.scope(), &err));
+
+  LiteralNode assignment2;
+  setup2.scope()->SetValue("b", Value(nullptr, true), &assignment2);
+  Scope::KeyValueMap key_value_map2;
+  setup2.scope()->GetCurrentScopeValues(&key_value_map2);
+  EXPECT_TRUE(args.DeclareArgs(key_value_map2, setup2.scope(), &err));
+
+  // Override "a", shouldn't see any errors as "a" was defined.
+  args.AddArgOverride("a", Value(nullptr, true));
+  EXPECT_TRUE(args.VerifyAllOverridesUsed(&err));
+
+  // Override "a", & "b", shouldn't see any errors as both were defined.
+  args.AddArgOverride("b", Value(nullptr, true));
+  EXPECT_TRUE(args.VerifyAllOverridesUsed(&err));
+
+  // Override "a", "b" and "c", should fail as "c" was not defined.
+  args.AddArgOverride("c", Value(nullptr, true));
+  EXPECT_FALSE(args.VerifyAllOverridesUsed(&err));
+}

tools/gn/config_values_extractors.h

@@ -43,7 +43,7 @@ class ConfigValuesIterator {
     return target_->configs()[cur_index_].ptr->config_values();
   }
 
-  // Returns the origin of who added this config, if any. This will alwsya be
+  // Returns the origin of who added this config, if any. This will always be
   // null for the config values of a target itself.
   const ParseNode* origin() const {
     if (cur_index_ == -1)