GN args threading and error enhancements.

The locking in the args object was not very consistent which is a likely cause of flakyness. This patch makes it clear what should be locked and what shouldn't be.

It also enhances the error reporting to list all possible args on error to help me debug an issue on a bot.

TBR=scottmg

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@249300 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 34 insertions, 15 deletions

diff --git a/tools/gn/args.cc b/tools/gn/args.cc
index ca55b3d..5009bdb 100644
--- a/tools/gn/args.cc
+++ b/tools/gn/args.cc
@@ -67,11 +67,15 @@ Args::~Args() {
 }
 
 void Args::AddArgOverride(const char* name, const Value& value) {
+  base::AutoLock lock(lock_);
+
   overrides_[base::StringPiece(name)] = value;
   all_overrides_[base::StringPiece(name)] = value;
 }
 
 void Args::AddArgOverrides(const Scope::KeyValueMap& overrides) {
+  base::AutoLock lock(lock_);
+
   for (Scope::KeyValueMap::const_iterator i = overrides.begin();
        i != overrides.end(); ++i) {
     overrides_[i->first] = i->second;
@@ -80,6 +84,8 @@ void Args::AddArgOverrides(const Scope::KeyValueMap& overrides) {
 }
 
 const Value* Args::GetArgOverride(const char* name) const {
+  base::AutoLock lock(lock_);
+
   Scope::KeyValueMap::const_iterator found =
       all_overrides_.find(base::StringPiece(name));
   if (found == all_overrides_.end())
@@ -89,10 +95,12 @@ const Value* Args::GetArgOverride(const char* name) const {
 
 void Args::SetupRootScope(Scope* dest,
                           const Scope::KeyValueMap& toolchain_overrides) const {
-  SetSystemVars(dest);
-  ApplyOverrides(overrides_, dest);
-  ApplyOverrides(toolchain_overrides, dest);
-  SaveOverrideRecord(toolchain_overrides);
+  base::AutoLock lock(lock_);
+
+  SetSystemVarsLocked(dest);
+  ApplyOverridesLocked(overrides_, dest);
+  ApplyOverridesLocked(toolchain_overrides, dest);
+  SaveOverrideRecordLocked(toolchain_overrides);
 }
 
 bool Args::DeclareArgs(const Scope::KeyValueMap& args,
@@ -160,19 +168,30 @@ bool Args::VerifyAllOverridesUsed(Err* err) const {
   for (Scope::KeyValueMap::const_iterator i = all_overrides_.begin();
        i != all_overrides_.end(); ++i) {
     if (declared_arguments_.find(i->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(i->second.origin(), "Build argument has no effect.",
           "The variable \"" + i->first.as_string() + "\" was set as a build "
           "argument\nbut never appeared in a declare_args() block in any "
-          "buildfile.");
+          "buildfile.\n\nPossible arguments: " + all_declared_str);
       return false;
     }
   }
   return true;
 }
 
-void Args::SetSystemVars(Scope* dest) const {
-  base::AutoLock lock(lock_);
-
+void Args::SetSystemVarsLocked(Scope* dest) const {
   // Host OS.
   const char* os = NULL;
 #if defined(OS_WIN)
@@ -243,15 +262,14 @@ void Args::SetSystemVars(Scope* dest) const {
   dest->MarkUsed(variables::kOs);
 }
 
-void Args::ApplyOverrides(const Scope::KeyValueMap& values,
-                          Scope* scope) const {
+void Args::ApplyOverridesLocked(const Scope::KeyValueMap& values,
+                                Scope* scope) const {
   for (Scope::KeyValueMap::const_iterator i = values.begin();
        i != values.end(); ++i)
     scope->SetValue(i->first, i->second, i->second.origin());
 }
 
-void Args::SaveOverrideRecord(const Scope::KeyValueMap& values) const {
-  base::AutoLock lock(lock_);
+void Args::SaveOverrideRecordLocked(const Scope::KeyValueMap& values) const {
   for (Scope::KeyValueMap::const_iterator i = values.begin();
        i != values.end(); ++i)
     all_overrides_[i->first] = i->second;
diff --git a/tools/gn/args.h b/tools/gn/args.h
index 6acebf3..78a3473 100644
--- a/tools/gn/args.h
+++ b/tools/gn/args.h
@@ -66,12 +66,13 @@ class Args {
 
  private:
   // Sets the default config based on the current system.
-  void SetSystemVars(Scope* scope) const;
+  void SetSystemVarsLocked(Scope* scope) const;
 
   // Sets the given vars on the given scope.
-  void ApplyOverrides(const Scope::KeyValueMap& values, Scope* scope) const;
+  void ApplyOverridesLocked(const Scope::KeyValueMap& values,
+                            Scope* scope) const;
 
-  void SaveOverrideRecord(const Scope::KeyValueMap& values) const;
+  void SaveOverrideRecordLocked(const Scope::KeyValueMap& values) 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.