GN: Don't define argument overrides globally

Previously, argument overrides (set via "gn args" or toolchain_args()) were
global, even though their default values (set inside declare_args()) were set
locally and thus only available to the file that set them, plus any file that
imported it.

Now, argument overrides are always applied locally, at the end of the
declare_args() block. This ensures that argument defaults and overrides
behave in the same way.

BUG=619963

Review-Url: https://codereview.chromium.org/2092623002
Cr-Commit-Position: refs/heads/master@{#404746}

tools/gn/args.cc

@@ -89,7 +89,8 @@ Args::Args(const Args& other)
     : overrides_(other.overrides_),
       all_overrides_(other.all_overrides_),
       declared_arguments_per_toolchain_(
-          other.declared_arguments_per_toolchain_) {
+          other.declared_arguments_per_toolchain_),
+      toolchain_overrides_(other.toolchain_overrides_) {
 }
 
 Args::~Args() {
@@ -131,8 +132,14 @@ void Args::SetupRootScope(Scope* dest,
   base::AutoLock lock(lock_);
 
   SetSystemVarsLocked(dest);
+
+  // Apply overrides for already declared args.
+  // (i.e. the system vars we set above)
   ApplyOverridesLocked(overrides_, dest);
   ApplyOverridesLocked(toolchain_overrides, dest);
+
+  OverridesForToolchainLocked(dest) = toolchain_overrides;
+
   SaveOverrideRecordLocked(toolchain_overrides);
 }
 
@@ -143,6 +150,10 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
 
   Scope::KeyValueMap& declared_arguments(
       DeclaredArgumentsForToolchainLocked(scope_to_set));
+
+  const Scope::KeyValueMap& toolchain_overrides(
+      OverridesForToolchainLocked(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.
@@ -173,13 +184,30 @@ bool Args::DeclareArgs(const Scope::KeyValueMap& args,
       declared_arguments.insert(arg);
     }
 
-    // Only set on the current scope to the new value if it hasn't been already
-    // set. Mark the variable used so the build script can override it in
-    // certain cases without getting unused value errors.
-    if (!scope_to_set->GetValue(arg.first)) {
-      scope_to_set->SetValue(arg.first, arg.second, arg.second.origin());
-      scope_to_set->MarkUsed(arg.first);
+    // Check whether this argument has been overridden on the toolchain level
+    // and use the override instead.
+    Scope::KeyValueMap::const_iterator toolchain_override =
+        toolchain_overrides.find(arg.first);
+    if (toolchain_override != toolchain_overrides.end()) {
+      scope_to_set->SetValue(toolchain_override->first,
+                             toolchain_override->second,
+                             toolchain_override->second.origin());
+      continue;
+    }
+
+    // Check whether this argument has been overridden and use the override
+    // instead.
+    Scope::KeyValueMap::const_iterator override = overrides_.find(arg.first);
+    if (override != overrides_.end()) {
+      scope_to_set->SetValue(override->first, override->second,
+                             override->second.origin());
+      continue;
     }
+
+    // Mark the variable used so the build script can override it in
+    // certain cases without getting unused value errors.
+    scope_to_set->SetValue(arg.first, arg.second, arg.second.origin());
+    scope_to_set->MarkUsed(arg.first);
   }
 
   return true;
@@ -286,8 +314,20 @@ void Args::SetSystemVarsLocked(Scope* dest) const {
 void Args::ApplyOverridesLocked(const Scope::KeyValueMap& values,
                                 Scope* scope) const {
   lock_.AssertAcquired();
-  for (const auto& val : values)
+
+  const Scope::KeyValueMap& declared_arguments(
+      DeclaredArgumentsForToolchainLocked(scope));
+
+  // Only set a value if it has been declared.
+  for (const auto& val : values) {
+    Scope::KeyValueMap::const_iterator declared =
+        declared_arguments.find(val.first);
+
+    if (declared == declared_arguments.end())
+      continue;
+
     scope->SetValue(val.first, val.second, val.second.origin());
+  }
 }
 
 void Args::SaveOverrideRecordLocked(const Scope::KeyValueMap& values) const {
@@ -301,3 +341,9 @@ Scope::KeyValueMap& Args::DeclaredArgumentsForToolchainLocked(
   lock_.AssertAcquired();
   return declared_arguments_per_toolchain_[scope->settings()];
 }
+
+Scope::KeyValueMap& Args::OverridesForToolchainLocked(
+    Scope* scope) const {
+  lock_.AssertAcquired();
+  return toolchain_overrides_[scope->settings()];
+}

tools/gn/args.h

@@ -40,8 +40,8 @@ class Args {
   Scope::KeyValueMap GetAllOverrides() const;
 
   // Sets up the root scope for a toolchain. This applies the default system
-  // flags, then any overrides stored in this object, then applies any
-  // toolchain overrides specified in the argument.
+  // flags and saves the toolchain overrides so they can be applied to
+  // declare_args blocks that appear when loading files in that toolchain.
   void SetupRootScope(Scope* dest,
                       const Scope::KeyValueMap& toolchain_overrides) const;
 
@@ -68,13 +68,13 @@ class Args {
   void MergeDeclaredArguments(Scope::KeyValueMap* dest) const;
 
  private:
-  using DeclaredArgumentsPerToolchain =
+  using ArgumentsPerToolchain =
       base::hash_map<const Settings*, Scope::KeyValueMap>;
 
   // Sets the default config based on the current system.
   void SetSystemVarsLocked(Scope* scope) const;
 
-  // Sets the given vars on the given scope.
+  // Sets the given already declared vars on the given scope.
   void ApplyOverridesLocked(const Scope::KeyValueMap& values,
                             Scope* scope) const;
 
@@ -84,6 +84,10 @@ class Args {
   // toolchain.
   Scope::KeyValueMap& DeclaredArgumentsForToolchainLocked(Scope* scope) const;
 
+  // Returns the KeyValueMap used for overrides for the specified
+  // toolchain.
+  Scope::KeyValueMap& OverridesForToolchainLocked(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_;
@@ -100,7 +104,12 @@ class Args {
   // 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_;
+  mutable ArgumentsPerToolchain declared_arguments_per_toolchain_;
+
+  // Overrides for individual toolchains. This is necessary so we
+  // can apply the correct override for the current toolchain, once
+  // we see an argument declaration.
+  mutable ArgumentsPerToolchain toolchain_overrides_;
 
   DISALLOW_ASSIGN(Args);
 };

tools/gn/args_unittest.cc

@@ -39,3 +39,43 @@ TEST(ArgsTest, VerifyAllOverridesUsed) {
   args.AddArgOverride("c", Value(nullptr, true));
   EXPECT_FALSE(args.VerifyAllOverridesUsed(&err));
 }
+
+// Ensure that arg overrides get only set after the they were declared.
+TEST(ArgsTest, VerifyOverrideScope) {
+  TestWithScope setup;
+  Args args;
+  Err err;
+
+  args.AddArgOverride("a", Value(nullptr, "avalue"));
+  args.AddArgOverride("current_os", Value(nullptr, "theiros"));
+
+  Scope::KeyValueMap toolchain_overrides;
+  toolchain_overrides["b"] = Value(nullptr, "bvalue");
+  toolchain_overrides["current_os"] = Value(nullptr, "myos");
+  args.SetupRootScope(setup.scope(), toolchain_overrides);
+
+  // Overrides of arguments not yet declared aren't applied yet.
+  EXPECT_EQ(nullptr, setup.scope()->GetValue("a"));
+  EXPECT_EQ(nullptr, setup.scope()->GetValue("b"));
+
+  // |current_os| is a system var. and already declared.
+  // Thus it should have our override value.
+  ASSERT_NE(nullptr, setup.scope()->GetValue("current_os"));
+  EXPECT_EQ(Value(nullptr, "myos"), *setup.scope()->GetValue("current_os"));
+
+  Scope::KeyValueMap key_value_map1;
+  key_value_map1["a"] = Value(nullptr, "avalue2");
+  key_value_map1["b"] = Value(nullptr, "bvalue2");
+  key_value_map1["c"] = Value(nullptr, "cvalue2");
+  EXPECT_TRUE(args.DeclareArgs(key_value_map1, setup.scope(), &err));
+
+  ASSERT_NE(nullptr, setup.scope()->GetValue("a"));
+  EXPECT_EQ(Value(nullptr, "avalue"), *setup.scope()->GetValue("a"));
+
+  ASSERT_NE(nullptr, setup.scope()->GetValue("b"));
+  EXPECT_EQ(Value(nullptr, "bvalue"), *setup.scope()->GetValue("b"));
+
+  // This wasn't overwritten, so it should have the default value.
+  ASSERT_NE(nullptr, setup.scope()->GetValue("c"));
+  EXPECT_EQ(Value(nullptr, "cvalue2"), *setup.scope()->GetValue("c"));
+}