Handle scope changes between function calls properly

renderdoc/driver/shaders/spirv/spirv_debug.cpp

@@ -97,6 +97,21 @@ void ThreadState::EnterFunction(ShaderDebugState *state, const rdcarray<Id> &arg
   OpFunction func(it);
   StackFrame *frame = new StackFrame();
   frame->function = func.result;
+
+  // if there's a previous stack frame, save its live list
+  if(!callstack.empty())
+  {
+    callstack.back()->live = live;
+    callstack.back()->sourceVars = sourceVars;
+  }
+
+  // start with just globals
+  live = debugger.GetLiveGlobals();
+  sourceVars = debugger.GetGlobalSourceVars();
+  // process the outgoing scope
+  if(state)
+    ProcessScopeChange(*state, frame->live, live);
+
   callstack.push_back(frame);
 
   it++;
@@ -156,9 +171,6 @@ void ThreadState::EnterFunction(ShaderDebugState *state, const rdcarray<Id> &arg
     if(decl.HasInitializer())
       AssignValue(stackvar, ids[decl.initializer]);
 
-    // TODO we need to handle re-entry into functions (ID lifetime)
-    live.removeOne(decl.result);
-
     SetDst(state, decl.result, debugger.MakePointerVariable(decl.result, &stackvar));
 
     it++;
@@ -176,102 +188,47 @@ const ShaderVariable &ThreadState::GetSrc(Id id)
 
 void ThreadState::SetDst(ShaderDebugState *state, Id id, const ShaderVariable &val)
 {
-  // if we don't have a state to track, we can take a much faster pa We just update the value and
-  // return. We don't have to propagate ID changes between aliased pointers because *internally* we
-  // always look up pointers and don't care about aliasing. That's only needed for *external* facing
-  // things because we have to update the changes for all copied pointer values.
-  if(!state)
-  {
-    if(ids[id].name.empty())
-    {
-      // for uninitialised values, init by copying
-      ids[id] = val;
-      ids[id].name = debugger.GetRawName(id);
-      live.push_back(id);
-
-      debugger.AddSourceVars(id);
-    }
-    else
-    {
-      RDCASSERT(ids[id].isPointer);
-      // otherwise just update the pointed-to value (only pointers should be existing before being
-      // assigned)
-      debugger.WriteThroughPointer(ids[id], val);
-    }
-
-    return;
-  }
-
-  // otherwise when we're tracking changes, take a slower pa
-
-  if(ContainsNaNInf(val))
+  if(state && ContainsNaNInf(val))
     state->flags |= ShaderEvents::GeneratedNanOrInf;
 
-  ShaderVariable &var = ids[id];
+  ids[id] = val;
+  ids[id].name = debugger.GetRawName(id);
 
-  if(!var.name.empty())
+  auto it = std::lower_bound(live.begin(), live.end(), id);
+  live.insert(it - live.begin(), id);
+
+  if(state)
   {
-    // if this ID was already initialised, we must have a change of a pointer. Otherwise we're SSA
-    // form so no other ID should change.
-
-    RDCASSERT(var.isPointer);
-
-    // if var is a pointer we update the underlying storage and generate at least one change, plus
-    // any additional ones for other pointers.
-    Id ptrid = debugger.GetPointerBaseId(var);
-
-    rdcarray<ShaderVariableChange> changes;
-    ShaderVariableChange basechange;
-    basechange.before = debugger.EvaluatePointerVariable(ids[ptrid]);
-
-    rdcarray<Id> &pointers = pointersForId[ptrid];
-
-    changes.resize(pointers.size());
-
-    // for every other pointer, evaluate its value now before
-    for(size_t i = 0; i < pointers.size(); i++)
-      changes[i].before = debugger.EvaluatePointerVariable(ids[pointers[i]]);
-
-    debugger.WriteThroughPointer(var, val);
-
-    // now evaluate the value after
-    for(size_t i = 0; i < pointers.size(); i++)
-      changes[i].after = debugger.EvaluatePointerVariable(ids[pointers[i]]);
-
-    // if the pointer we're writing is one of the aliased pointers, be sure we add it even if it's a
-    // no-op change
-    int ptrIdx = pointers.indexOf(id);
-
-    if(ptrIdx >= 0)
-    {
-      state->changes.push_back(changes[ptrIdx]);
-      changes.erase(ptrIdx);
-    }
-
-    // remove any no-op changes. Some pointers might point to the same ID but a child that wasn't
-    // written to. Note that this might not actually mean nothing was changed (if e.g. we're
-    // assigning the same value) but that false negative is not a concern.
-    changes.removeIf([](const ShaderVariableChange &c) { return c.before == c.after; });
-
-    state->changes.append(changes);
-
-    // always add a change for the base storage variable written itself, even if that's a no-op.
-    // This one is not included in any of the pointers lists above
-    basechange.after = debugger.EvaluatePointerVariable(ids[ptrid]);
-    state->changes.push_back(basechange);
-  }
-  else
-  {
-    // otherwise it's a new SSA variable, record a change-from-nothing
-    ids[id] = val;
-    ids[id].name = debugger.GetRawName(id);
-    live.push_back(id);
-
     ShaderVariableChange change;
     change.after = debugger.EvaluatePointerVariable(ids[id]);
     state->changes.push_back(change);
 
-    debugger.AddSourceVars(id);
+    debugger.AddSourceVars(sourceVars, id);
+  }
+}
+
+void ThreadState::ProcessScopeChange(ShaderDebugState &state, const rdcarray<Id> &oldLive,
+                                     const rdcarray<Id> &newLive)
+{
+  // all oldLive (except globals) are going out of scope. all newLive (except globals) are coming
+  // into scope
+
+  const rdcarray<Id> &liveGlobals = debugger.GetLiveGlobals();
+
+  for(const Id id : oldLive)
+  {
+    if(liveGlobals.contains(id))
+      continue;
+
+    state.changes.push_back({debugger.EvaluatePointerVariable(ids[id])});
+  }
+
+  for(const Id id : newLive)
+  {
+    if(liveGlobals.contains(id))
+      continue;
+
+    state.changes.push_back({ShaderVariable(), debugger.EvaluatePointerVariable(ids[id])});
   }
 }
 
@@ -315,7 +272,67 @@ void ThreadState::StepNext(ShaderDebugState *state,
 
       RDCASSERT(ids[store.pointer].isPointer);
 
-      SetDst(state, store.pointer, GetSrc(store.object));
+      // this is the only place we don't use SetDst because it's the only place that "violates" SSA
+      // i.e. changes an existing value. That way SetDst can always unconditionally assign values,
+      // and only here do we write through pointers
+
+      ShaderVariable val = GetSrc(store.object);
+
+      if(!state)
+      {
+        debugger.WriteThroughPointer(ids[store.pointer], val);
+      }
+      else
+      {
+        ShaderVariable &var = ids[store.pointer];
+
+        if(ContainsNaNInf(val))
+          state->flags |= ShaderEvents::GeneratedNanOrInf;
+
+        // if var is a pointer we update the underlying storage and generate at least one change,
+        // plus any additional ones for other pointers.
+        Id ptrid = debugger.GetPointerBaseId(var);
+
+        rdcarray<ShaderVariableChange> changes;
+        ShaderVariableChange basechange;
+        basechange.before = debugger.EvaluatePointerVariable(ids[ptrid]);
+
+        rdcarray<Id> &pointers = pointersForId[ptrid];
+
+        changes.resize(pointers.size());
+
+        // for every other pointer, evaluate its value now before
+        for(size_t i = 0; i < pointers.size(); i++)
+          changes[i].before = debugger.EvaluatePointerVariable(ids[pointers[i]]);
+
+        debugger.WriteThroughPointer(var, val);
+
+        // now evaluate the value after
+        for(size_t i = 0; i < pointers.size(); i++)
+          changes[i].after = debugger.EvaluatePointerVariable(ids[pointers[i]]);
+
+        // if the pointer we're writing is one of the aliased pointers, be sure we add it even if
+        // it's a no-op change
+        int ptrIdx = pointers.indexOf(store.pointer);
+
+        if(ptrIdx >= 0)
+        {
+          state->changes.push_back(changes[ptrIdx]);
+          changes.erase(ptrIdx);
+        }
+
+        // remove any no-op changes. Some pointers might point to the same ID but a child that
+        // wasn't written to. Note that this might not actually mean nothing was changed (if e.g.
+        // we're assigning the same value) but that false negative is not a concern.
+        changes.removeIf([](const ShaderVariableChange &c) { return c.before == c.after; });
+
+        state->changes.append(changes);
+
+        // always add a change for the base storage variable written itself, even if that's a no-op.
+        // This one is not included in any of the pointers lists above
+        basechange.after = debugger.EvaluatePointerVariable(ids[ptrid]);
+        state->changes.push_back(basechange);
+      }
 
       break;
     }
@@ -483,6 +500,14 @@ void ThreadState::StepNext(ShaderDebugState *state,
         }
 
         nextInstruction = exitingFrame->funcCallInstruction;
+
+        // process the outgoing and incoming scopes
+        if(state)
+          ProcessScopeChange(*state, live, callstack.back()->live);
+
+        // restore the live list from the calling frame
+        live = callstack.back()->live;
+        sourceVars = callstack.back()->sourceVars;
       }
 
       delete exitingFrame;

renderdoc/driver/shaders/spirv/spirv_debug.h

@@ -60,6 +60,10 @@ struct StackFrame
   // allocated storage for locals
   rdcarray<ShaderVariable> locals;
 
+  // the thread's live list before the function was entered
+  rdcarray<Id> live;
+  rdcarray<SourceVariableMapping> sourceVars;
+
 private:
   // disallow copying to ensure the locals we allocate never move around
   StackFrame(const StackFrame &o) = delete;
@@ -105,6 +109,8 @@ struct ThreadState
   // the list of IDs that are currently valid and live
   rdcarray<Id> live;
 
+  rdcarray<SourceVariableMapping> sourceVars;
+
   // index in the pixel quad
   int workgroupIndex;
   bool done;
@@ -112,6 +118,8 @@ struct ThreadState
 private:
   const ShaderVariable &GetSrc(Id id);
   void SetDst(ShaderDebugState *state, Id id, const ShaderVariable &val);
+  void ProcessScopeChange(ShaderDebugState &state, const rdcarray<Id> &oldLive,
+                          const rdcarray<Id> &newLive);
 };
 
 class Debugger : public Processor, public ShaderDebugger
@@ -133,7 +141,7 @@ public:
   const DataType &GetType(Id typeId);
   rdcstr GetRawName(Id id) const;
   rdcstr GetHumanName(Id id);
-  void AddSourceVars(Id id);
+  void AddSourceVars(rdcarray<SourceVariableMapping> &sourceVars, Id id);
   void AllocateVariable(Id id, Id typeId, DebugVariableType sourceVarType, const rdcstr &sourceName,
                         ShaderVariable &outVar);
 
@@ -146,6 +154,8 @@ public:
 
   uint32_t GetNumInstructions() { return (uint32_t)instructionOffsets.size(); }
   GlobalState GetGlobal() { return global; }
+  const rdcarray<Id> &GetLiveGlobals() { return liveGlobals; }
+  const rdcarray<SourceVariableMapping> &GetGlobalSourceVars() { return globalSourceVars; }
   ThreadState &GetActiveLane() { return workgroup[activeLaneIndex]; }
 private:
   virtual void PreParse(uint32_t maxId);
@@ -155,8 +165,8 @@ private:
   void AllocateVariable(const Decorations &varDecorations, const Decorations &curDecorations,
                         DebugVariableType sourceVarType, const rdcstr &sourceName, uint32_t offset,
                         const DataType &inType, ShaderVariable &outVar);
-  void AddSourceVars(const DataType &inType, const rdcstr &sourceName, const rdcstr &varName,
-                     uint32_t &offset);
+  void AddSourceVars(rdcarray<SourceVariableMapping> &sourceVars, const DataType &inType,
+                     const rdcstr &sourceName, const rdcstr &varName, uint32_t &offset);
 
   /////////////////////////////////////////////////////////
   // debug data
@@ -166,8 +176,6 @@ private:
   GlobalState global;
   rdcarray<ThreadState> workgroup;
 
-  rdcarray<SourceVariableMapping> sourceVars;
-
   uint32_t activeLaneIndex = 0;
   ShaderStage stage;
 
@@ -187,9 +195,17 @@ private:
   rdcarray<MemberName> memberNames;
   std::map<rdcstr, Id> entryLookup;
 
+  SparseIdMap<size_t> idDeathOffset;
+
+  // the live mutable global variables, to initialise a stack frame's live list
+  rdcarray<Id> liveGlobals;
+  rdcarray<SourceVariableMapping> globalSourceVars;
+
   struct Function
   {
     size_t begin = 0;
+    rdcarray<Id> parameters;
+    rdcarray<Id> variables;
   };
 
   SparseIdMap<Function> functions;

renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp

@@ -127,7 +127,7 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader
 
       rdcstr sourceName = GetHumanName(v.id);
 
-      size_t oldSize = sourceVars.size();
+      size_t oldSize = globalSourceVars.size();
 
       const DataType &type = dataTypes[v.type];
 
@@ -141,8 +141,8 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader
 
       // I/O variable structs don't have offsets, so give them fake offsets to ensure they sort as
       // we want. Since FillVariable is depth-first the source vars are already in order.
-      for(size_t i = oldSize; i < sourceVars.size(); i++)
-        sourceVars[i].offset = uint32_t(i - oldSize);
+      for(size_t i = oldSize; i < globalSourceVars.size(); i++)
+        globalSourceVars[i].offset = uint32_t(i - oldSize);
 
       if(isInput)
       {
@@ -197,7 +197,7 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader
         sourceVar.offset = 0;
         sourceVar.variables.push_back(DebugVariableReference(DebugVariableType::Constant, var.name));
 
-        sourceVars.push_back(sourceVar);
+        globalSourceVars.push_back(sourceVar);
       }
     }
   }
@@ -210,17 +210,19 @@ ShaderDebugTrace *Debugger::BeginDebug(DebugAPIWrapper *apiWrapper, const Shader
   for(size_t i = 0; i < global.constantBlocks.size(); i++)
     active.ids[cbufferIDs[i]] = MakePointerVariable(cbufferIDs[i], &global.constantBlocks[i]);
 
-  // only outputs are considered mutable
-  active.live.append(outputIDs);
+  std::sort(outputIDs.begin(), outputIDs.end());
 
-  for(size_t i = 0; i < sourceVars.size();)
+  // only outputs are considered mutable
+  liveGlobals.append(outputIDs);
+
+  for(size_t i = 0; i < globalSourceVars.size();)
   {
-    if(!sourceVars[i].variables.empty() &&
-       (sourceVars[i].variables[0].type == DebugVariableType::Input ||
-        sourceVars[i].variables[0].type == DebugVariableType::Constant))
+    if(!globalSourceVars[i].variables.empty() &&
+       (globalSourceVars[i].variables[0].type == DebugVariableType::Input ||
+        globalSourceVars[i].variables[0].type == DebugVariableType::Constant))
     {
-      ret->sourceVars.push_back(sourceVars[i]);
-      sourceVars.erase(i);
+      ret->sourceVars.push_back(globalSourceVars[i]);
+      globalSourceVars.erase(i);
       continue;
     }
     i++;
@@ -262,7 +264,7 @@ rdcarray<ShaderDebugState> Debugger::ContinueDebug()
     for(const Id &v : active.live)
       initial.changes.push_back({ShaderVariable(), EvaluatePointerVariable(active.ids[v])});
 
-    initial.sourceVars = sourceVars;
+    initial.sourceVars = active.sourceVars;
 
     initial.stepIndex = steps;
 
@@ -299,30 +301,56 @@ rdcarray<ShaderDebugState> Debugger::ContinueDebug()
     CalcActiveMask(activeMask);
 
     // step all active members of the workgroup
-    for(size_t i = 0; i < workgroup.size(); i++)
+    for(size_t lane = 0; lane < workgroup.size(); lane++)
     {
-      if(activeMask[i])
+      ThreadState &thread = workgroup[lane];
+
+      if(activeMask[lane])
       {
-        if(workgroup[i].nextInstruction >= instructionOffsets.size())
+        if(thread.nextInstruction >= instructionOffsets.size())
         {
-          if(i == activeLaneIndex)
+          if(lane == activeLaneIndex)
             ret.push_back(ShaderDebugState());
 
           continue;
         }
 
-        if(i == activeLaneIndex)
+        if(lane == activeLaneIndex)
         {
           ShaderDebugState state;
-          workgroup[i].StepNext(&state, oldworkgroup);
+
+          // see if we're retiring any IDs at this state
+          for(size_t l = 0; l < thread.live.size();)
+          {
+            Id id = thread.live[l];
+            if(idDeathOffset[id] < instructionOffsets[thread.nextInstruction])
+            {
+              thread.live.erase(l);
+              ShaderVariableChange change;
+              change.before = EvaluatePointerVariable(thread.ids[id]);
+              state.changes.push_back(change);
+
+              rdcstr name = GetRawName(id);
+
+              thread.sourceVars.removeIf([name](const SourceVariableMapping &var) {
+                return var.variables[0].name.beginsWith(name);
+              });
+
+              continue;
+            }
+
+            l++;
+          }
+
+          thread.StepNext(&state, oldworkgroup);
           state.stepIndex = steps;
-          state.sourceVars = sourceVars;
-          workgroup[i].FillCallstack(state);
+          state.sourceVars = thread.sourceVars;
+          thread.FillCallstack(state);
           ret.push_back(state);
         }
         else
         {
-          workgroup[i].StepNext(NULL, oldworkgroup);
+          thread.StepNext(NULL, oldworkgroup);
         }
       }
     }
@@ -552,7 +580,7 @@ rdcstr Debugger::GetHumanName(Id id)
   return name;
 }
 
-void Debugger::AddSourceVars(Id id)
+void Debugger::AddSourceVars(rdcarray<SourceVariableMapping> &sourceVars, Id id)
 {
   rdcstr name;
 
@@ -567,12 +595,12 @@ void Debugger::AddSourceVars(Id id)
     Id type = idTypes[id];
 
     uint32_t offset = 0;
-    AddSourceVars(dataTypes[type], name, GetRawName(id), offset);
+    AddSourceVars(sourceVars, dataTypes[type], name, GetRawName(id), offset);
   }
 }
 
-void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName,
-                             const rdcstr &varName, uint32_t &offset)
+void Debugger::AddSourceVars(rdcarray<SourceVariableMapping> &sourceVars, const DataType &inType,
+                             const rdcstr &sourceName, const rdcstr &varName, uint32_t &offset)
 {
   SourceVariableMapping sourceVar;
 
@@ -585,7 +613,7 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName,
     case DataType::PointerType:
     {
       // step silently into pointers
-      AddSourceVars(dataTypes[inType.InnerType()], sourceName, varName, offset);
+      AddSourceVars(sourceVars, dataTypes[inType.InnerType()], sourceName, varName, offset);
       return;
     }
     case DataType::ScalarType:
@@ -621,7 +649,8 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName,
         else
           childSourceName = sourceName + "." + inType.children[i].name;
 
-        AddSourceVars(dataTypes[inType.children[i].type], childSourceName, childVarName, offset);
+        AddSourceVars(sourceVars, dataTypes[inType.children[i].type], childSourceName, childVarName,
+                      offset);
       }
       return;
     }
@@ -632,7 +661,8 @@ void Debugger::AddSourceVars(const DataType &inType, const rdcstr &sourceName,
       {
         rdcstr idx = StringFormat::Fmt("[%u]", i);
 
-        AddSourceVars(dataTypes[inType.InnerType()], sourceName + idx, varName + idx, offset);
+        AddSourceVars(sourceVars, dataTypes[inType.InnerType()], sourceName + idx, varName + idx,
+                      offset);
       }
       return;
     }
@@ -783,7 +813,7 @@ void Debugger::AllocateVariable(const Decorations &varDecorations, const Decorat
   if(curDecorations.flags & Decorations::HasBuiltIn)
     sourceVar.builtin = MakeShaderBuiltin(stage, curDecorations.builtIn);
 
-  sourceVars.push_back(sourceVar);
+  globalSourceVars.push_back(sourceVar);
 
   if(sourceVarType == DebugVariableType::Input)
   {
@@ -862,6 +892,10 @@ void Debugger::PostParse()
   for(const MemberName &mem : memberNames)
     dataTypes[mem.id].children[mem.member].name = mem.name;
 
+  // global IDs never hit a death point
+  for(const Variable &v : globals)
+    idDeathOffset[v.id] = ~0U;
+
   memberNames.clear();
 }
 
@@ -871,6 +905,13 @@ void Debugger::RegisterOp(Iter it)
 
   OpDecoder opdata(it);
 
+  // we add +1 so that we don't remove the ID on its last use, but the next subsequent instruction
+  // since blocks always end with a terminator that doesn't consume IDs we're interested in
+  // (variables) we'll always have one extra instruction to step to
+  OpDecoder::ForEachID(it, [this, &it](Id id, bool result) {
+    idDeathOffset[id] = RDCMAX(it.offs() + 1, idDeathOffset[id]);
+  });
+
   if(opdata.op == Op::Line || opdata.op == Op::NoLine)
   {
     // ignore OpLine/OpNoLine
@@ -909,6 +950,19 @@ void Debugger::RegisterOp(Iter it)
 
     curFunction->begin = it.offs();
   }
+  else if(opdata.op == Op::FunctionParameter)
+  {
+    OpFunctionParameter param(it);
+
+    curFunction->parameters.push_back(param.result);
+  }
+  else if(opdata.op == Op::Variable)
+  {
+    OpVariable var(it);
+
+    if(var.storageClass == StorageClass::Function && curFunction)
+      curFunction->variables.push_back(var.result);
+  }
 
   // everything else inside a function becomes an instruction, including the OpFunction and
   // OpFunctionEnd. We won't actually execute these instructions
@@ -917,6 +971,13 @@ void Debugger::RegisterOp(Iter it)
 
   if(opdata.op == Op::FunctionEnd)
   {
+    // don't automatically kill function parameters and variables. They will be manually killed when
+    // returning from a function's scope
+    for(const Id id : curFunction->parameters)
+      idDeathOffset[id] = ~0U;
+    for(const Id id : curFunction->variables)
+      idDeathOffset[id] = ~0U;
+
     curFunction = NULL;
   }
 }