Merge topic 'reduce-cmState-accumulation-for-master'

9342a4c2 Merge branch 'reduce-cmState-accumulation' into reduce-cmState-accumulation-for-master
5f860ebb cmState: Avoid accumulating snapshot storage for short-lived scopes
f21dc4a8 cmState: Avoid accumulating policy stack storage for short-lived scopes
bc1d3a8a cmListFileCache: Implement cmListFileBacktrace ctor/dtor out-of-line
85fe26b5 cmLinkedTree: Add Pop method
518d6b22 cmLinkedTree: Rename 'Extend' method to 'Push'
32edac6f cmState: Enforce policy scope balancing around variable scopes
0fa7f143 cmLocalGenerator: Use ScopePushPop RAII class to manage local variable scopes
d85c9176 cmMakefile: Remove unused PolicyPushPop interfaces
8e1be7bf cmMakefile: Clarify purpose of method that pops a scope snapshot

Source/cmLinkedTree.h

@@ -24,7 +24,7 @@
   needs of the cmState.  For example, the Truncate() method is a specific
   requirement of the cmState.
 
-  An empty cmLinkedTree provides a Root() method, and an Extend() method,
+  An empty cmLinkedTree provides a Root() method, and an Push() method,
   each of which return iterators.  A Tree can be built up by extending
   from the root, and then extending from any other iterator.
 
@@ -142,16 +142,37 @@ public:
     return iterator(const_cast<cmLinkedTree*>(this), 0);
   }
 
-  iterator Extend(iterator it)
+  iterator Push(iterator it)
   {
-    return Extend_impl(it, T());
+    return Push_impl(it, T());
   }
 
-  iterator Extend(iterator it, T t)
+  iterator Push(iterator it, T t)
   {
-    return Extend_impl(it, t);
+    return Push_impl(it, t);
   }
 
+  bool IsLast(iterator it)
+    {
+    return it.Position == this->Data.size();
+    }
+
+  iterator Pop(iterator it)
+    {
+    assert(!this->Data.empty());
+    assert(this->UpPositions.size() == this->Data.size());
+    bool const isLast = this->IsLast(it);
+    ++it;
+    // If this is the last entry then no other entry can refer
+    // to it so we can drop its storage.
+    if (isLast)
+      {
+      this->Data.pop_back();
+      this->UpPositions.pop_back();
+      }
+    return it;
+    }
+
   iterator Truncate()
   {
     assert(this->UpPositions.size() > 0);
@@ -179,7 +200,7 @@ private:
     return &this->Data[pos];
   }
 
-  iterator Extend_impl(iterator it, T t)
+  iterator Push_impl(iterator it, T t)
   {
     assert(this->UpPositions.size() == this->Data.size());
     assert(it.Position <= this->UpPositions.size());

Source/cmListFileCache.cxx

@@ -398,6 +398,21 @@ bool cmListFileParser::AddArgument(cmListFileLexer_Token* token,
     }
 }
 
+cmListFileBacktrace::cmListFileBacktrace(cmState::Snapshot snapshot,
+                                         cmCommandContext const& cc)
+  : Context(cc)
+  , Snapshot(snapshot)
+{
+  if (this->Snapshot.IsValid())
+    {
+    this->Snapshot.Keep();
+    }
+}
+
+cmListFileBacktrace::~cmListFileBacktrace()
+{
+}
+
 void cmListFileBacktrace::PrintTitle(std::ostream& out) const
 {
   if (!this->Snapshot.IsValid())

Source/cmListFileCache.h

@@ -90,10 +90,8 @@ class cmListFileBacktrace
 {
   public:
     cmListFileBacktrace(cmState::Snapshot snapshot = cmState::Snapshot(),
-                        cmCommandContext const& cc = cmCommandContext())
-      : Context(cc), Snapshot(snapshot)
-    {
-    }
+                        cmCommandContext const& cc = cmCommandContext());
+    ~cmListFileBacktrace();
 
     void PrintTitle(std::ostream& out) const;
     void PrintCallStack(std::ostream& out) const;

Source/cmLocalGenerator.cxx

@@ -3121,7 +3121,7 @@ void cmLocalGenerator::GenerateAppleInfoPList(cmGeneratorTarget* target,
   // override user make variables.  If not the configuration will fall
   // back to the directory-level values set by the user.
   cmMakefile* mf = this->Makefile;
-  mf->PushScope();
+  cmMakefile::ScopePushPop varScope(mf);
   mf->AddDefinition("MACOSX_BUNDLE_EXECUTABLE_NAME", targetName.c_str());
   cmLGInfoProp(mf, target, "MACOSX_BUNDLE_INFO_STRING");
   cmLGInfoProp(mf, target, "MACOSX_BUNDLE_ICON_FILE");
@@ -3132,7 +3132,6 @@ void cmLocalGenerator::GenerateAppleInfoPList(cmGeneratorTarget* target,
   cmLGInfoProp(mf, target, "MACOSX_BUNDLE_BUNDLE_VERSION");
   cmLGInfoProp(mf, target, "MACOSX_BUNDLE_COPYRIGHT");
   mf->ConfigureFile(inFile.c_str(), fname, false, false, false);
-  mf->PopScope();
 }
 
 //----------------------------------------------------------------------------
@@ -3165,12 +3164,11 @@ void cmLocalGenerator::GenerateFrameworkInfoPList(cmGeneratorTarget* target,
   // override user make variables.  If not the configuration will fall
   // back to the directory-level values set by the user.
   cmMakefile* mf = this->Makefile;
-  mf->PushScope();
+  cmMakefile::ScopePushPop varScope(mf);
   mf->AddDefinition("MACOSX_FRAMEWORK_NAME", targetName.c_str());
   cmLGInfoProp(mf, target, "MACOSX_FRAMEWORK_ICON_FILE");
   cmLGInfoProp(mf, target, "MACOSX_FRAMEWORK_IDENTIFIER");
   cmLGInfoProp(mf, target, "MACOSX_FRAMEWORK_SHORT_VERSION_STRING");
   cmLGInfoProp(mf, target, "MACOSX_FRAMEWORK_BUNDLE_VERSION");
   mf->ConfigureFile(inFile.c_str(), fname, false, false, false);
-  mf->PopScope();
 }

Source/cmMakefile.cxx

@@ -391,7 +391,7 @@ cmMakefile::IncludeScope::~IncludeScope()
       this->EnforceCMP0011();
       }
     }
-  this->Makefile->PopPolicyBarrier(this->ReportError);
+  this->Makefile->PopSnapshot(this->ReportError);
 
   this->Makefile->PopFunctionBlockerBarrier(this->ReportError);
 }
@@ -505,7 +505,7 @@ public:
 
   ~ListFileScope()
   {
-    this->Makefile->PopPolicyBarrier(this->ReportError);
+    this->Makefile->PopSnapshot(this->ReportError);
     this->Makefile->PopFunctionBlockerBarrier(this->ReportError);
   }
 
@@ -1534,7 +1534,7 @@ void cmMakefile::PopFunctionScope(bool reportError)
 {
   this->PopPolicy();
 
-  this->PopPolicyBarrier(reportError);
+  this->PopSnapshot(reportError);
 
   this->PopFunctionBlockerBarrier(reportError);
 
@@ -1565,7 +1565,7 @@ void cmMakefile::PushMacroScope(std::string const& fileName,
 void cmMakefile::PopMacroScope(bool reportError)
 {
   this->PopPolicy();
-  this->PopPolicyBarrier(reportError);
+  this->PopSnapshot(reportError);
 
   this->PopFunctionBlockerBarrier(reportError);
 }
@@ -1602,7 +1602,7 @@ public:
   ~BuildsystemFileScope()
   {
     this->Makefile->PopFunctionBlockerBarrier(this->ReportError);
-    this->Makefile->PopPolicyBarrier(this->ReportError);
+    this->Makefile->PopSnapshot(this->ReportError);
 #if defined(CMAKE_BUILD_WITH_CMAKE)
     this->GG->GetFileLockPool().PopFileScope();
 #endif
@@ -4168,9 +4168,7 @@ void cmMakefile::PopScope()
 
   this->CheckForUnusedVariables();
 
-  this->StateSnapshot =
-      this->GetState()->Pop(this->StateSnapshot);
-  assert(this->StateSnapshot.IsValid());
+  this->PopSnapshot();
 }
 
 void cmMakefile::RaiseScope(const std::string& var, const char *varDef)
@@ -4521,20 +4519,15 @@ bool cmMakefile::SetPolicy(cmPolicies::PolicyID id,
 }
 
 //----------------------------------------------------------------------------
-cmMakefile::PolicyPushPop::PolicyPushPop(cmMakefile* m, bool weak,
-                                         cmPolicies::PolicyMap const& pm):
-  Makefile(m), ReportError(true)
+cmMakefile::PolicyPushPop::PolicyPushPop(cmMakefile* m): Makefile(m)
 {
-  this->Makefile->StateSnapshot = this->Makefile->StateSnapshot.GetState()
-      ->CreatePolicyScopeSnapshot(this->Makefile->StateSnapshot);
-  this->Makefile->PushPolicy(weak, pm);
+  this->Makefile->PushPolicy();
 }
 
 //----------------------------------------------------------------------------
 cmMakefile::PolicyPushPop::~PolicyPushPop()
 {
   this->Makefile->PopPolicy();
-  this->Makefile->PopPolicyBarrier(this->ReportError);
 }
 
 //----------------------------------------------------------------------------
@@ -4554,8 +4547,11 @@ void cmMakefile::PopPolicy()
 }
 
 //----------------------------------------------------------------------------
-void cmMakefile::PopPolicyBarrier(bool reportError)
+void cmMakefile::PopSnapshot(bool reportError)
 {
+  // cmState::Snapshot manages nested policy scopes within it.
+  // Since the scope corresponding to the snapshot is closing,
+  // reject any still-open nested policy scopes with an error.
   while (!this->StateSnapshot.CanPopPolicyScope())
     {
     if(reportError)

Source/cmMakefile.h

@@ -315,14 +315,10 @@ public:
   class PolicyPushPop
   {
   public:
-    PolicyPushPop(cmMakefile* m,
-                  bool weak = false,
-                  cmPolicies::PolicyMap const& pm = cmPolicies::PolicyMap());
+    PolicyPushPop(cmMakefile* m);
     ~PolicyPushPop();
-    void Quiet() { this->ReportError = false; }
   private:
     cmMakefile* Makefile;
-    bool ReportError;
   };
   friend class PolicyPushPop;
 
@@ -878,7 +874,7 @@ private:
   void PushPolicy(bool weak = false,
                   cmPolicies::PolicyMap const& pm = cmPolicies::PolicyMap());
   void PopPolicy();
-  void PopPolicyBarrier(bool reportError = true);
+  void PopSnapshot(bool reportError = true);
   friend class cmCMakePolicyCommand;
   class IncludeScope;
   friend class IncludeScope;

Source/cmState.cxx

@@ -28,6 +28,7 @@ struct cmState::SnapshotDataType
   cmLinkedTree<cmState::PolicyStackEntry>::iterator PolicyRoot;
   cmLinkedTree<cmState::PolicyStackEntry>::iterator PolicyScope;
   cmState::SnapshotType SnapshotType;
+  bool Keep;
   cmLinkedTree<std::string>::iterator ExecutionListFile;
   cmLinkedTree<cmState::BuildsystemDirectoryStateType>::iterator
                                                           BuildSystemDirectory;
@@ -341,7 +342,7 @@ cmState::Snapshot cmState::Reset()
   std::string binDir =
       cmDefinitions::Get("CMAKE_BINARY_DIR", pos->Vars, pos->Root);
   this->VarTree.Clear();
-  pos->Vars = this->VarTree.Extend(this->VarTree.Root());
+  pos->Vars = this->VarTree.Push(this->VarTree.Root());
   pos->Parent = this->VarTree.Root();
   pos->Root = this->VarTree.Root();
 
@@ -818,14 +819,15 @@ void cmState::Directory::ComputeRelativePathTopBinary()
 
 cmState::Snapshot cmState::CreateBaseSnapshot()
 {
-  PositionType pos = this->SnapshotData.Extend(this->SnapshotData.Root());
+  PositionType pos = this->SnapshotData.Push(this->SnapshotData.Root());
   pos->DirectoryParent = this->SnapshotData.Root();
   pos->ScopeParent = this->SnapshotData.Root();
   pos->SnapshotType = BaseType;
+  pos->Keep = true;
   pos->BuildSystemDirectory =
-      this->BuildsystemDirectory.Extend(this->BuildsystemDirectory.Root());
+      this->BuildsystemDirectory.Push(this->BuildsystemDirectory.Root());
   pos->ExecutionListFile =
-      this->ExecutionListFiles.Extend(this->ExecutionListFiles.Root());
+      this->ExecutionListFiles.Push(this->ExecutionListFiles.Root());
   pos->IncludeDirectoryPosition = 0;
   pos->CompileDefinitionsPosition = 0;
   pos->CompileOptionsPosition = 0;
@@ -835,7 +837,7 @@ cmState::Snapshot cmState::CreateBaseSnapshot()
   pos->PolicyScope = this->PolicyStack.Root();
   assert(pos->Policies.IsValid());
   assert(pos->PolicyRoot.IsValid());
-  pos->Vars = this->VarTree.Extend(this->VarTree.Root());
+  pos->Vars = this->VarTree.Push(this->VarTree.Root());
   assert(pos->Vars.IsValid());
   pos->Parent = this->VarTree.Root();
   pos->Root = this->VarTree.Root();
@@ -848,17 +850,18 @@ cmState::CreateBuildsystemDirectorySnapshot(Snapshot originSnapshot,
                                     long entryPointLine)
 {
   assert(originSnapshot.IsValid());
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position);
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->DirectoryParent = originSnapshot.Position;
   pos->ScopeParent = originSnapshot.Position;
   pos->SnapshotType = BuildsystemDirectoryType;
+  pos->Keep = true;
   pos->BuildSystemDirectory =
-      this->BuildsystemDirectory.Extend(
+      this->BuildsystemDirectory.Push(
         originSnapshot.Position->BuildSystemDirectory);
   pos->ExecutionListFile =
-      this->ExecutionListFiles.Extend(
+      this->ExecutionListFiles.Push(
         originSnapshot.Position->ExecutionListFile);
   pos->BuildSystemDirectory->DirectoryEnd = pos;
   pos->Policies = originSnapshot.Position->Policies;
@@ -871,7 +874,7 @@ cmState::CreateBuildsystemDirectorySnapshot(Snapshot originSnapshot,
       originSnapshot.Position->Vars;
   pos->Parent = origin;
   pos->Root = origin;
-  pos->Vars = this->VarTree.Extend(origin);
+  pos->Vars = this->VarTree.Push(origin);
 
   cmState::Snapshot snapshot = cmState::Snapshot(this, pos);
   originSnapshot.Position->BuildSystemDirectory->Children.push_back(snapshot);
@@ -887,13 +890,14 @@ cmState::CreateFunctionCallSnapshot(cmState::Snapshot originSnapshot,
                                     long entryPointLine,
                                     std::string const& fileName)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->ScopeParent = originSnapshot.Position;
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->SnapshotType = FunctionCallType;
-  pos->ExecutionListFile = this->ExecutionListFiles.Extend(
+  pos->Keep = false;
+  pos->ExecutionListFile = this->ExecutionListFiles.Push(
         originSnapshot.Position->ExecutionListFile, fileName);
   pos->BuildSystemDirectory->DirectoryEnd = pos;
   pos->PolicyScope = originSnapshot.Position->Policies;
@@ -901,7 +905,7 @@ cmState::CreateFunctionCallSnapshot(cmState::Snapshot originSnapshot,
   cmLinkedTree<cmDefinitions>::iterator origin =
       originSnapshot.Position->Vars;
   pos->Parent = origin;
-  pos->Vars = this->VarTree.Extend(origin);
+  pos->Vars = this->VarTree.Push(origin);
   return cmState::Snapshot(this, pos);
 }
 
@@ -912,12 +916,13 @@ cmState::CreateMacroCallSnapshot(cmState::Snapshot originSnapshot,
                                     long entryPointLine,
                                     std::string const& fileName)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->SnapshotType = MacroCallType;
-  pos->ExecutionListFile = this->ExecutionListFiles.Extend(
+  pos->Keep = false;
+  pos->ExecutionListFile = this->ExecutionListFiles.Push(
         originSnapshot.Position->ExecutionListFile, fileName);
   assert(originSnapshot.Position->Vars.IsValid());
   pos->BuildSystemDirectory->DirectoryEnd = pos;
@@ -931,12 +936,13 @@ cmState::CreateCallStackSnapshot(cmState::Snapshot originSnapshot,
                                  long entryPointLine,
                                  const std::string& fileName)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->SnapshotType = CallStackType;
-  pos->ExecutionListFile = this->ExecutionListFiles.Extend(
+  pos->Keep = true;
+  pos->ExecutionListFile = this->ExecutionListFiles.Push(
         originSnapshot.Position->ExecutionListFile, fileName);
   assert(originSnapshot.Position->Vars.IsValid());
   pos->BuildSystemDirectory->DirectoryEnd = pos;
@@ -949,18 +955,20 @@ cmState::CreateVariableScopeSnapshot(cmState::Snapshot originSnapshot,
                                      std::string const& entryPointCommand,
                                      long entryPointLine)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->ScopeParent = originSnapshot.Position;
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->SnapshotType = VariableScopeType;
+  pos->Keep = false;
+  pos->PolicyScope = originSnapshot.Position->Policies;
   assert(originSnapshot.Position->Vars.IsValid());
 
   cmLinkedTree<cmDefinitions>::iterator origin =
       originSnapshot.Position->Vars;
   pos->Parent = origin;
-  pos->Vars = this->VarTree.Extend(origin);
+  pos->Vars = this->VarTree.Push(origin);
   assert(pos->Vars.IsValid());
   return cmState::Snapshot(this, pos);
 }
@@ -971,12 +979,13 @@ cmState::CreateInlineListFileSnapshot(cmState::Snapshot originSnapshot,
                                       long entryPointLine,
                                       const std::string& fileName)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->EntryPointLine = entryPointLine;
   pos->EntryPointCommand = entryPointCommand;
   pos->SnapshotType = InlineListFileType;
-  pos->ExecutionListFile = this->ExecutionListFiles.Extend(
+  pos->Keep = true;
+  pos->ExecutionListFile = this->ExecutionListFiles.Push(
         originSnapshot.Position->ExecutionListFile, fileName);
   pos->BuildSystemDirectory->DirectoryEnd = pos;
   pos->PolicyScope = originSnapshot.Position->Policies;
@@ -986,9 +995,10 @@ cmState::CreateInlineListFileSnapshot(cmState::Snapshot originSnapshot,
 cmState::Snapshot
 cmState::CreatePolicyScopeSnapshot(cmState::Snapshot originSnapshot)
 {
-  PositionType pos = this->SnapshotData.Extend(originSnapshot.Position,
-                                               *originSnapshot.Position);
+  PositionType pos = this->SnapshotData.Push(originSnapshot.Position,
+                                             *originSnapshot.Position);
   pos->SnapshotType = PolicyScopeType;
+  pos->Keep = false;
   pos->BuildSystemDirectory->DirectoryEnd = pos;
   pos->PolicyScope = originSnapshot.Position->Policies;
   return cmState::Snapshot(this, pos);
@@ -1007,6 +1017,21 @@ cmState::Snapshot cmState::Pop(cmState::Snapshot originSnapshot)
       prevPos->BuildSystemDirectory->CompileOptions.size();
   prevPos->BuildSystemDirectory->DirectoryEnd = prevPos;
 
+  if (!pos->Keep && this->SnapshotData.IsLast(pos))
+    {
+    if (pos->Vars != prevPos->Vars)
+      {
+      assert(this->VarTree.IsLast(pos->Vars));
+      this->VarTree.Pop(pos->Vars);
+      }
+    if (pos->ExecutionListFile != prevPos->ExecutionListFile)
+      {
+      assert(this->ExecutionListFiles.IsLast(pos->ExecutionListFile));
+      this->ExecutionListFiles.Pop(pos->ExecutionListFile);
+      }
+    this->SnapshotData.Pop(pos);
+    }
+
   return Snapshot(this, prevPos);
 }
 
@@ -1073,6 +1098,11 @@ void cmState::Directory::SetCurrentBinary(std::string const& dir)
   this->Snapshot_.SetDefinition("CMAKE_CURRENT_BINARY_DIR", loc.c_str());
 }
 
+void cmState::Snapshot::Keep()
+{
+  this->Position->Keep = true;
+}
+
 void cmState::Snapshot::SetListFile(const std::string& listfile)
 {
   *this->Position->ExecutionListFile = listfile;
@@ -1187,8 +1217,8 @@ void cmState::Snapshot::PushPolicy(cmPolicies::PolicyMap entry, bool weak)
 {
   PositionType pos = this->Position;
   pos->Policies =
-      this->State->PolicyStack.Extend(pos->Policies,
-                                      PolicyStackEntry(entry, weak));
+    this->State->PolicyStack.Push(pos->Policies,
+                                  PolicyStackEntry(entry, weak));
 }
 
 bool cmState::Snapshot::PopPolicy()
@@ -1198,7 +1228,7 @@ bool cmState::Snapshot::PopPolicy()
     {
     return false;
     }
-  ++pos->Policies;
+  pos->Policies = this->State->PolicyStack.Pop(pos->Policies);
   return true;
 }
 

Source/cmState.h

@@ -63,6 +63,7 @@ public:
     std::vector<std::string> ClosureKeys() const;
     bool RaiseScope(std::string const& var, const char* varDef);
 
+    void Keep();
     void SetListFile(std::string const& listfile);
 
     std::string GetExecutionListFile() const;