From 7a09d0c4e86c612dae287424ef1d169574a6d50c Mon Sep 17 00:00:00 2001 From: aparajit-pratap Date: Wed, 4 Nov 2020 10:07:43 -0500 Subject: [PATCH] crash when undoing deletion of function definition code blocks (#11220) * account for deletion of nodes Co-authored-by: pinzart --- src/Engine/ProtoCore/CodeGen.cs | 4 +- src/Engine/ProtoCore/Core.cs | 18 +++-- src/Engine/ProtoCore/DSASM/Executable.cs | 13 +++- src/Engine/ProtoCore/DSASM/Executive.cs | 26 +++++-- .../ProtoCore/DSASM/Mirror/ExecutionMirror.cs | 21 ++++-- src/Engine/ProtoCore/FunctionPointerTable.cs | 8 ++- .../ProtoCore/Properties/AssemblyInfo.cs | 3 +- src/Engine/ProtoCore/Utils/ClassUtils.cs | 15 ++-- src/Engine/ProtoCore/Utils/CoreUtils.cs | 2 - src/Engine/ProtoImperative/CodeGen.cs | 17 +++-- src/Engine/ProtoScript/Runners/LiveRunner.cs | 4 +- .../LiveRunnerTests/MicroFeatureTests.cs | 69 +++++++++++++++++++ test/Engine/ProtoTestFx/DebugTestFx.cs | 9 ++- 13 files changed, 162 insertions(+), 47 deletions(-) diff --git a/src/Engine/ProtoCore/CodeGen.cs b/src/Engine/ProtoCore/CodeGen.cs index e7e6a1e406d..08141eb5ccf 100644 --- a/src/Engine/ProtoCore/CodeGen.cs +++ b/src/Engine/ProtoCore/CodeGen.cs @@ -1022,7 +1022,7 @@ protected bool VerifyAllocation(string name, int classScope, int functionScope, bool hasThisSymbol; AddressType addressType; - symbolIndex = ClassUtils.GetSymbolIndex(thisClass, name, classScope, functionScope, currentCodeBlock.codeBlockId, core.CompleteCodeBlockList, out hasThisSymbol, out addressType); + symbolIndex = ClassUtils.GetSymbolIndex(thisClass, name, classScope, functionScope, currentCodeBlock.codeBlockId, core.CompleteCodeBlockDict, out hasThisSymbol, out addressType); if (Constants.kInvalidIndex != symbolIndex) { // It is static member, then get node from code block @@ -1080,7 +1080,7 @@ protected bool IsProperty(string name) ProtoCore.DSASM.AddressType addressType; ProtoCore.DSASM.ClassNode classnode = core.ClassTable.ClassNodes[globalClassIndex]; - int symbolIndex = ClassUtils.GetSymbolIndex(classnode, name, globalClassIndex, globalProcIndex, 0, core.CompleteCodeBlockList, out hasThisSymbol, out addressType); + int symbolIndex = ClassUtils.GetSymbolIndex(classnode, name, globalClassIndex, globalProcIndex, 0, core.CompleteCodeBlockDict, out hasThisSymbol, out addressType); if (symbolIndex == ProtoCore.DSASM.Constants.kInvalidIndex) { return false; diff --git a/src/Engine/ProtoCore/Core.cs b/src/Engine/ProtoCore/Core.cs index 2cb36a0255e..6a7e62f2e11 100644 --- a/src/Engine/ProtoCore/Core.cs +++ b/src/Engine/ProtoCore/Core.cs @@ -214,7 +214,13 @@ public struct ErrorEntry public List CodeBlockList { get; set; } // The Complete Code Block list contains all the code blocks // unlike the codeblocklist which only stores the outer most code blocks - public List CompleteCodeBlockList { get; set; } + [Obsolete("Property will be deprecated in Dynamo 3.0")] + public List CompleteCodeBlockList + { + get { return CompleteCodeBlockDict.Select(x => x.Value).ToList(); } + set { value.ForEach(x => CompleteCodeBlockDict.Add(x.codeBlockId, x)); } + } + internal SortedDictionary CompleteCodeBlockDict { get; set; } /// /// ForLoopBlockIndex tracks the current number of new for loop blocks created at compile time for every new compile phase @@ -347,7 +353,7 @@ public void SetFunctionInactive(FunctionDefinitionNode functionDef) // Remove codeblock defined in procNode from CodeBlockList and CompleteCodeBlockList foreach (int cbID in procNode.ChildCodeBlocks) { - CompleteCodeBlockList.RemoveAll(x => x.codeBlockId == cbID); + CompleteCodeBlockDict.Remove(cbID); foreach (CodeBlock cb in CodeBlockList) { @@ -438,7 +444,7 @@ private void ResetAll(Options options) CodeBlockIndex = 0; RuntimeTableIndex = 0; CodeBlockList = new List(); - CompleteCodeBlockList = new List(); + CompleteCodeBlockDict = new SortedDictionary(); CallsiteGuidMap = new Dictionary(); AssocNode = null; @@ -717,9 +723,7 @@ public void GenerateExecutable() // Create the code block list data DSExecutable.CodeBlocks = new List(); DSExecutable.CodeBlocks.AddRange(CodeBlockList); - DSExecutable.CompleteCodeBlocks = new List(); - DSExecutable.CompleteCodeBlocks.AddRange(CompleteCodeBlockList); - + DSExecutable.CompleteCodeBlockDict = new SortedDictionary(CompleteCodeBlockDict); // Retrieve the class table directly since it is a global table DSExecutable.classTable = ClassTable; @@ -770,7 +774,7 @@ public void GenerateExecutable() internal int GetRuntimeTableSize() { // Due to the way this list is constructed, the largest id is the one of the last block. - var lastBlock = CompleteCodeBlockList.LastOrDefault(); + var lastBlock = CompleteCodeBlockDict.LastOrDefault().Value; // If there are no code blocks yet, then the required size for tables is 0. // This happens when the first code block is being created and its id is being generated. if (lastBlock == null) diff --git a/src/Engine/ProtoCore/DSASM/Executable.cs b/src/Engine/ProtoCore/DSASM/Executable.cs index d82c693bd12..8946bd5168c 100644 --- a/src/Engine/ProtoCore/DSASM/Executable.cs +++ b/src/Engine/ProtoCore/DSASM/Executable.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using ProtoCore.AssociativeGraph; using ProtoCore.Lang; using ProtoFFI; @@ -46,7 +47,13 @@ public class Executable public TypeSystem TypeSystem { get; set; } public List CodeBlocks { get; set; } - public List CompleteCodeBlocks { get; set; } + + [Obsolete("Property will be deprecated in Dynamo 3.0")] + public List CompleteCodeBlocks { + get { return CompleteCodeBlockDict.Select(x => x.Value).ToList(); } + set { value.ForEach(x => CompleteCodeBlockDict.Add(x.codeBlockId, x)); } + } + internal SortedDictionary CompleteCodeBlockDict { get; set; } public InstructionStream[] instrStreamList { get; set; } public InstructionStream iStreamCanvas { get; set; } @@ -94,7 +101,7 @@ public void Reset() instrStreamList = null; iStreamCanvas = null; CodeBlocks = null; - CompleteCodeBlocks = null; + CompleteCodeBlockDict = null; ContextDataMngr = null; CodeToLocation = null; CurrentDSFileName = string.Empty; @@ -156,7 +163,7 @@ public CodeBlock(Guid guid, CodeBlockType type, ProtoCore.Language langId, int c isBreakable = isBreakableBlock; codeBlockId = core.GetRuntimeTableSize(); - core.CompleteCodeBlockList.Add(this); + core.CompleteCodeBlockDict.Add(codeBlockId, this); symbols.RuntimeIndex = codeBlockId; diff --git a/src/Engine/ProtoCore/DSASM/Executive.cs b/src/Engine/ProtoCore/DSASM/Executive.cs index 6230a86526e..1b9f1012c91 100644 --- a/src/Engine/ProtoCore/DSASM/Executive.cs +++ b/src/Engine/ProtoCore/DSASM/Executive.cs @@ -1513,8 +1513,15 @@ private void XLangUpdateDependencyGraph(int currentLangBlock) // happens. if (graphNode.isLanguageBlock && currentLangBlock != Constants.kInvalidIndex) { - if (graphNode.languageBlockId == currentLangBlock - || exe.CompleteCodeBlocks[currentLangBlock].IsMyAncestorBlock(graphNode.languageBlockId)) + if (graphNode.languageBlockId == currentLangBlock) + { + continue; + } + + bool found = exe.CompleteCodeBlockDict.TryGetValue(currentLangBlock, out CodeBlock cb); + Validity.Assert(found, $"Could not find code block with codeBlockId {currentLangBlock}"); + + if (cb.IsMyAncestorBlock(graphNode.languageBlockId)) { continue; } @@ -2708,7 +2715,7 @@ private bool ResolveDynamicFunction(Instruction instr, out bool isMemberFunction SymbolNode node = null; bool isStatic = false; ClassNode classNode = exe.classTable.ClassNodes[type]; - int symbolIndex = ClassUtils.GetSymbolIndex(classNode, procName, type, Constants.kGlobalScope, runtimeCore.RunningBlock, exe.CompleteCodeBlocks, out hasThisSymbol, out addressType); + int symbolIndex = ClassUtils.GetSymbolIndex(classNode, procName, type, Constants.kGlobalScope, runtimeCore.RunningBlock, exe.CompleteCodeBlockDict, out hasThisSymbol, out addressType); if (Constants.kInvalidIndex != symbolIndex) { @@ -2904,7 +2911,10 @@ public void GCCodeBlock(int blockId, int functionIndex = Constants.kGlobalScope, public void ReturnSiteGC(int blockId, int classIndex, int functionIndex) { - foreach (CodeBlock cb in exe.CompleteCodeBlocks[blockId].children) + bool found = exe.CompleteCodeBlockDict.TryGetValue(blockId, out CodeBlock codeBlock); + Validity.Assert(found, $"Could find code block with codeBlockId {blockId}"); + + foreach (CodeBlock cb in codeBlock.children) { if (cb.blockType == CodeBlockType.Construct) GCCodeBlock(cb.codeBlockId, functionIndex, classIndex); @@ -2927,6 +2937,10 @@ public AssociativeGraph.GraphNode GetFirstGraphNode(string varName, out int bloc for (int n = 0; n < exe.instrStreamList.Length; ++n) { InstructionStream stream = exe.instrStreamList[n]; + if (stream == null) + { + continue; + } for (int i = 0; i < stream.dependencyGraph.GraphList.Count; ++i) { AssociativeGraph.GraphNode node = stream.dependencyGraph.GraphList[i]; @@ -4409,7 +4423,9 @@ private void RETCN_Handler(Instruction instruction) StackValue op1 = instruction.op1; int blockId = op1.BlockIndex; - CodeBlock codeBlock = exe.CompleteCodeBlocks[blockId]; + bool found = exe.CompleteCodeBlockDict.TryGetValue(blockId, out CodeBlock codeBlock); + Validity.Assert(found, $"Could find code block with codeBlockId {blockId}"); + runtimeVerify(codeBlock.blockType == CodeBlockType.Construct); GCCodeBlock(blockId); pc++; diff --git a/src/Engine/ProtoCore/DSASM/Mirror/ExecutionMirror.cs b/src/Engine/ProtoCore/DSASM/Mirror/ExecutionMirror.cs index 0d3d69569a6..488569740bd 100644 --- a/src/Engine/ProtoCore/DSASM/Mirror/ExecutionMirror.cs +++ b/src/Engine/ProtoCore/DSASM/Mirror/ExecutionMirror.cs @@ -332,7 +332,8 @@ private int GetSymbolIndex(string name, out int ci, ref int block, out SymbolNod } else { - CodeBlock searchBlock = runtimeCore.DSExecutable.CompleteCodeBlocks[block]; + bool found = runtimeCore.DSExecutable.CompleteCodeBlockDict.TryGetValue(block, out CodeBlock searchBlock); + Validity.Assert(found, $"Could not find code block with codeBlockId {block}"); // To detal with the case that a language block defined in a function // @@ -718,10 +719,15 @@ public StackValue GetValue(string name, int startBlock = 0) for (int block = startBlock; block < exe.runtimeSymbols.Length; block++) { - int index = exe.runtimeSymbols[block].IndexOf(name, Constants.kInvalidIndex, Constants.kGlobalScope); + var symbolTable = exe.runtimeSymbols[block]; + if (symbolTable == null) + { + continue; + } + int index = symbolTable.IndexOf(name, Constants.kInvalidIndex, Constants.kGlobalScope); if (Constants.kInvalidIndex != index) { - SymbolNode symNode = exe.runtimeSymbols[block].symbolList[index]; + SymbolNode symNode = symbolTable.symbolList[index]; if (symNode.absoluteFunctionIndex == Constants.kGlobalScope) { return MirrorTarget.rmem.GetAtRelative(symNode.index); @@ -738,11 +744,16 @@ public StackValue GetRawFirstValue(string name, int startBlock = 0, int classcop for (int block = startBlock; block < exe.runtimeSymbols.Length; block++) { - int index = exe.runtimeSymbols[block].IndexOf(name, classcope, Constants.kGlobalScope); + var symbolTable = exe.runtimeSymbols[block]; + if (symbolTable == null) + { + continue; + } + int index = symbolTable.IndexOf(name, classcope, Constants.kGlobalScope); if (Constants.kInvalidIndex != index) { //Q(Luke): This seems to imply that the LHS is an array index? - var symbol = exe.runtimeSymbols[block].symbolList[index]; + var symbol = symbolTable.symbolList[index]; return MirrorTarget.rmem.GetSymbolValue(symbol); } } diff --git a/src/Engine/ProtoCore/FunctionPointerTable.cs b/src/Engine/ProtoCore/FunctionPointerTable.cs index 01990b87454..6e349fa12cf 100644 --- a/src/Engine/ProtoCore/FunctionPointerTable.cs +++ b/src/Engine/ProtoCore/FunctionPointerTable.cs @@ -1,4 +1,5 @@ -using System; +using ProtoCore.Utils; +using System; using System.Collections.Generic; namespace ProtoCore.DSASM @@ -38,7 +39,10 @@ public bool TryGetFunction(StackValue functionPointer, RuntimeCore runtimeCore, } else { - procNode = runtimeCore.DSExecutable.CompleteCodeBlocks[blockId].procedureTable.Procedures[functionIndex]; + bool found = runtimeCore.DSExecutable.CompleteCodeBlockDict.TryGetValue(blockId, out CodeBlock codeBlock); + Validity.Assert(found, $"Could find code block with codeBlockId {blockId}"); + + procNode = codeBlock.procedureTable.Procedures[functionIndex]; } return true; diff --git a/src/Engine/ProtoCore/Properties/AssemblyInfo.cs b/src/Engine/ProtoCore/Properties/AssemblyInfo.cs index 5601535cb3c..68c68330b89 100644 --- a/src/Engine/ProtoCore/Properties/AssemblyInfo.cs +++ b/src/Engine/ProtoCore/Properties/AssemblyInfo.cs @@ -14,4 +14,5 @@ [assembly:InternalsVisibleTo("DynamoCore")] [assembly: InternalsVisibleTo("DynamoCoreTests")] [assembly: InternalsVisibleTo("DynamoCoreWpfTests")] - +[assembly: InternalsVisibleTo("ProtoImperative")] +[assembly: InternalsVisibleTo("ProtoScript")] diff --git a/src/Engine/ProtoCore/Utils/ClassUtils.cs b/src/Engine/ProtoCore/Utils/ClassUtils.cs index 2e1cb79081f..5559dab27da 100644 --- a/src/Engine/ProtoCore/Utils/ClassUtils.cs +++ b/src/Engine/ProtoCore/Utils/ClassUtils.cs @@ -69,7 +69,7 @@ public static int GetUpcastCountTo(ClassNode from, ClassNode to, RuntimeCore run // // 2.3 Otherwise, classScope == kInvalidIndex && functionScope == kInvalidIndex // Return public member in derived class, or public member in base classes - public static int GetSymbolIndex(ClassNode classNode, string name, int classScope, int functionScope, int blockId, List codeblockList, out bool hasThisSymbol, out ProtoCore.DSASM.AddressType addressType) + public static int GetSymbolIndex(ClassNode classNode, string name, int classScope, int functionScope, int blockId, SortedDictionary codeblocks, out bool hasThisSymbol, out ProtoCore.DSASM.AddressType addressType) { hasThisSymbol = false; addressType = ProtoCore.DSASM.AddressType.Invalid; @@ -92,7 +92,7 @@ public static int GetSymbolIndex(ClassNode classNode, string name, int classScop classNode.ProcTable.Procedures[functionScope].IsStatic; // Try for member function variables - var blocks = GetAncestorBlockIdsOfBlock(blockId, codeblockList); + var blocks = GetAncestorBlockIdsOfBlock(blockId, codeblocks); blocks.Insert(0, blockId); Dictionary symbolOfBlockScope = new Dictionary(); @@ -154,15 +154,14 @@ public static int GetSymbolIndex(ClassNode classNode, string name, int classScop return Constants.kInvalidIndex; } - public static List GetAncestorBlockIdsOfBlock(int blockId, List codeblockList) + public static List GetAncestorBlockIdsOfBlock(int blockId, SortedDictionary codeblocks) { - if (blockId >= codeblockList.Count || blockId < 0) + var ancestors = new List(); + if (!codeblocks.TryGetValue(blockId, out CodeBlock thisBlock)) { - return new List(); + return ancestors; } - CodeBlock thisBlock = codeblockList[blockId]; - - var ancestors = new List(); + CodeBlock codeBlock = thisBlock.parent; while (codeBlock != null) { diff --git a/src/Engine/ProtoCore/Utils/CoreUtils.cs b/src/Engine/ProtoCore/Utils/CoreUtils.cs index 4872eceb4e7..d343dc0aa4a 100644 --- a/src/Engine/ProtoCore/Utils/CoreUtils.cs +++ b/src/Engine/ProtoCore/Utils/CoreUtils.cs @@ -7,8 +7,6 @@ namespace ProtoCore.Utils { public static class CoreUtils { - - public static void InsertPredefinedAndBuiltinMethods(Core core, CodeBlockNode root) { if (DSASM.InterpreterMode.Normal == core.Options.RunMode) diff --git a/src/Engine/ProtoImperative/CodeGen.cs b/src/Engine/ProtoImperative/CodeGen.cs index 62b54514c2c..145b1ddc412 100644 --- a/src/Engine/ProtoImperative/CodeGen.cs +++ b/src/Engine/ProtoImperative/CodeGen.cs @@ -881,25 +881,24 @@ private void EmitLanguageBlockNode(ImperativeNode node, ref ProtoCore.Type infer if (propogateUpdateGraphNode != null) { propogateUpdateGraphNode.languageBlockId = blockId; - CodeBlock childBlock = core.CompleteCodeBlockList[blockId]; + bool foundChild = core.CompleteCodeBlockDict.TryGetValue(blockId, out CodeBlock childBlock); + Validity.Assert(foundChild, $"Could find code block with codeBlockId {blockId}"); + foreach (var subGraphNode in childBlock.instrStream.dependencyGraph.GraphList) { foreach (var depentNode in subGraphNode.dependentList) { - if (depentNode.updateNodeRefList != null - && depentNode.updateNodeRefList.Count > 0 + if (depentNode.updateNodeRefList != null + && depentNode.updateNodeRefList.Count > 0 && depentNode.updateNodeRefList[0].nodeList != null && depentNode.updateNodeRefList[0].nodeList.Count > 0) { SymbolNode dependentSymbol = depentNode.updateNodeRefList[0].nodeList[0].symbol; int symbolBlockId = dependentSymbol.codeBlockId; - if (symbolBlockId != Constants.kInvalidIndex) + if (core.CompleteCodeBlockDict.TryGetValue(symbolBlockId, out CodeBlock symbolBlock) && + !symbolBlock.IsMyAncestorBlock(codeBlock.codeBlockId)) { - CodeBlock symbolBlock = core.CompleteCodeBlockList[symbolBlockId]; - if (!symbolBlock.IsMyAncestorBlock(codeBlock.codeBlockId)) - { - propogateUpdateGraphNode.PushDependent(depentNode); - } + propogateUpdateGraphNode.PushDependent(depentNode); } } } diff --git a/src/Engine/ProtoScript/Runners/LiveRunner.cs b/src/Engine/ProtoScript/Runners/LiveRunner.cs index 41b89332e97..d337fdeb9da 100644 --- a/src/Engine/ProtoScript/Runners/LiveRunner.cs +++ b/src/Engine/ProtoScript/Runners/LiveRunner.cs @@ -322,8 +322,10 @@ private void ClearModifiedNestedBlocks(List astNodes) // Remove from the global codeblocks core.CodeBlockList.RemoveAll(x => x.guid == bnode.guid);// && x.AstID == bnode.OriginalAstID); + // Remove from the runtime codeblocks - core.CompleteCodeBlockList.RemoveAll(x => x.guid == bnode.guid);// && x.AstID == bnode.OriginalAstID); + var keysToRemove = core.CompleteCodeBlockDict.Where(x => x.Value.guid == bnode.guid).Select(x => x.Key).ToList(); + keysToRemove.ForEach(key => core.CompleteCodeBlockDict.Remove(key)); } } } diff --git a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs index 09c51206570..31c5895e93b 100644 --- a/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs +++ b/test/Engine/ProtoTest/LiveRunnerTests/MicroFeatureTests.cs @@ -5930,6 +5930,75 @@ public void TestMAGN5477() AssertValue("k", 3); } + [Test] + public void CrashOnUndoDelFuncDef() + { + List codes = new List() + { + @" + def foo(a, b) + { + return [Imperative] + { + list = a..b; + x = []; + count = 0; + for(i in list) + { + x[count] = i; + count = count+1; + } + return x; + } + }; + ", + @" + x = false; + l1 = foo(1, 10); + l2 = foo(10, 20); + [Imperative] + { + if(x==true) + { + return l1; + } + else + { + return l2; + } + }; + " + }; + + Guid guidFuncDef = Guid.NewGuid(); + Guid guidOther = Guid.NewGuid(); + + List added = new List(); + added.Add(TestFrameWork.CreateSubTreeFromCode(guidFuncDef, codes[0])); + added.Add(TestFrameWork.CreateSubTreeFromCode(guidOther, codes[1])); + + var syncData = new GraphSyncData(null, added, null); + liveRunner.UpdateGraph(syncData); + + List removed = new List(); + removed.Add(TestFrameWork.CreateSubTreeFromCode(guidFuncDef, codes[0])); + + syncData = new GraphSyncData(removed, null, null); + Assert.DoesNotThrow(() => + { + liveRunner.UpdateGraph(syncData); + }); + + List undoRemove = new List(); + undoRemove.Add(TestFrameWork.CreateSubTreeFromCode(guidFuncDef, codes[0])); + + syncData = new GraphSyncData(null, undoRemove, null); + Assert.DoesNotThrow(() => + { + liveRunner.UpdateGraph(syncData); + }); + } + [Test] public void RegressMAGN7759() { diff --git a/test/Engine/ProtoTestFx/DebugTestFx.cs b/test/Engine/ProtoTestFx/DebugTestFx.cs index 04ae0f002cb..ad1762980e5 100644 --- a/test/Engine/ProtoTestFx/DebugTestFx.cs +++ b/test/Engine/ProtoTestFx/DebugTestFx.cs @@ -190,10 +190,15 @@ internal static void CompareCores(RuntimeCore rtcore1, RuntimeCore rtcore2, stri for (int symTableIndex = 0; symTableIndex < rtcore1.DSExecutable.runtimeSymbols.Length; symTableIndex++) { - foreach (SymbolNode symNode in rtcore1.DSExecutable.runtimeSymbols[symTableIndex].symbolList.Values) + var rtc1Symbols = rtcore1.DSExecutable.runtimeSymbols[symTableIndex]; + if (rtc1Symbols == null) + { + continue; + } + foreach (SymbolNode symNode in rtc1Symbols.symbolList.Values) { - ExecutionMirror runExecMirror = new ExecutionMirror(rtcore1.CurrentExecutive.CurrentDSASMExec,rtcore1); + ExecutionMirror runExecMirror = new ExecutionMirror(rtcore1.CurrentExecutive.CurrentDSASMExec, rtcore1); ExecutionMirror debugExecMirror = new ExecutionMirror(rtcore2.CurrentExecutive.CurrentDSASMExec, rtcore2); bool lookupOk = false;