More fixes on optimizer (#911)

* fix try if

* safer dumpnef

* no ABORT in try with catch & catch with finally

* safer debuginfo changes; nef & manifest UNHANDLED

* format

* cover catch before any return of branch type

---------

Co-authored-by: Jimmy <jimmy@r3e.network>
Co-authored-by: Shargon <shargon@gmail.com>

Author: 3 people

src/Neo.Compiler.CSharp/Optimizer/DumpNef.cs

@@ -183,7 +183,7 @@ public static string GenerateDumpNef(NefFile nef, JToken debugInfo)
                 addressToInstruction.Add(a, i);
             Dictionary<int, string> methodStartAddrToName = new();
             Dictionary<int, string> methodEndAddrToName = new();
-            Dictionary<int, (int docId, int startLine, int startCol, int endLine, int endCol)> newAddrToSequencePoint = new();
+            Dictionary<int, List<(int docId, int startLine, int startCol, int endLine, int endCol)>> newAddrToSequencePoint = new();
 
             foreach (JToken? method in (JArray)debugInfo["methods"]!)
             {
@@ -196,7 +196,10 @@ public static string GenerateDumpNef(NefFile nef, JToken debugInfo)
                 {
                     GroupCollection sequencePointGroups = SequencePointRegex.Match(sequencePoint!.AsString()).Groups;
                     GroupCollection documentGroups = DocumentRegex.Match(sequencePointGroups[2].ToString()).Groups;
-                    newAddrToSequencePoint.Add(int.Parse(sequencePointGroups[1].Value), (
+                    int addr = int.Parse(sequencePointGroups[1].Value);
+                    if (!newAddrToSequencePoint.ContainsKey(addr))
+                        newAddrToSequencePoint.Add(addr, new());
+                    newAddrToSequencePoint[addr].Add((
                         int.Parse(documentGroups[1].ToString()),
                         int.Parse(documentGroups[2].ToString()),
                         int.Parse(documentGroups[3].ToString()),
@@ -216,26 +219,28 @@ public static string GenerateDumpNef(NefFile nef, JToken debugInfo)
                     dumpnef += $"# Method End {methodEndAddrToName[a]}\n";
                 if (newAddrToSequencePoint.ContainsKey(a))
                 {
-                    var (docId, startLine, startCol, endLine, endCol) = newAddrToSequencePoint[a];
-                    string docPath = debugInfo["documents"]![docId]!.AsString();
-                    if (debugInfo["document-root"] != null)
-                        docPath = Path.Combine(debugInfo["document-root"]!.AsString(), docPath);
-                    if (!docPathToContent.ContainsKey(docPath))
-                        docPathToContent.Add(docPath, File.ReadAllLines(docPath).ToArray());
-                    if (startLine == endLine)
-                        dumpnef += $"# Code {Path.GetFileName(docPath)} line {startLine}: \"{docPathToContent[docPath][startLine - 1][(startCol - 1)..(endCol - 1)]}\"\n";
-                    else
-                        for (int lineIndex = startLine; lineIndex <= endLine; lineIndex++)
-                        {
-                            string src;
-                            if (lineIndex == startLine)
-                                src = docPathToContent[docPath][lineIndex - 1][(startCol - 1)..].Trim();
-                            else if (lineIndex == endLine)
-                                src = docPathToContent[docPath][lineIndex - 1][..(endCol - 1)].Trim();
-                            else
-                                src = docPathToContent[docPath][lineIndex - 1].Trim();
-                            dumpnef += $"# Code {Path.GetFileName(docPath)} line {startLine}: \"{src}\"\n";
-                        }
+                    foreach ((int docId, int startLine, int startCol, int endLine, int endCol) in newAddrToSequencePoint[a])
+                    {
+                        string docPath = debugInfo["documents"]![docId]!.AsString();
+                        if (debugInfo["document-root"] != null)
+                            docPath = Path.Combine(debugInfo["document-root"]!.AsString(), docPath);
+                        if (!docPathToContent.ContainsKey(docPath))
+                            docPathToContent.Add(docPath, File.ReadAllLines(docPath).ToArray());
+                        if (startLine == endLine)
+                            dumpnef += $"# Code {Path.GetFileName(docPath)} line {startLine}: \"{docPathToContent[docPath][startLine - 1][(startCol - 1)..(endCol - 1)]}\"\n";
+                        else
+                            for (int lineIndex = startLine; lineIndex <= endLine; lineIndex++)
+                            {
+                                string src;
+                                if (lineIndex == startLine)
+                                    src = docPathToContent[docPath][lineIndex - 1][(startCol - 1)..].Trim();
+                                else if (lineIndex == endLine)
+                                    src = docPathToContent[docPath][lineIndex - 1][..(endCol - 1)].Trim();
+                                else
+                                    src = docPathToContent[docPath][lineIndex - 1].Trim();
+                                dumpnef += $"# Code {Path.GetFileName(docPath)} line {startLine}: \"{src}\"\n";
+                            }
+                    }
                 }
                 if (a < script.Length)
                     dumpnef += $"{WriteInstruction(a, script.GetInstruction(a), script.GetInstructionAddressPadding(), nef.Tokens)}\n";

src/Neo.Compiler.CSharp/Optimizer/Strategies/Reachability.cs

@@ -106,9 +106,7 @@ public static (NefFile, ContractManifest, JToken) RemoveUncoveredInstructions(Ne
             nef.Compiler = AppDomain.CurrentDomain.FriendlyName;
             nef.CheckSum = NefFile.ComputeChecksum(nef);
 
-            Dictionary<int, (int docId, int startLine, int startCol, int endLine, int endCol)> newAddrToSequencePoint = new();
-            Dictionary<int, string> newMethodStart = new();
-            Dictionary<int, string> newMethodEnd = new();
+            //Dictionary<int, (int docId, int startLine, int startCol, int endLine, int endCol)> newAddrToSequencePoint = new();
             HashSet<JToken> methodsToRemove = new();
             foreach (JToken? method in (JArray)debugInfo["methods"]!)
             {
@@ -121,8 +119,6 @@ public static (NefFile, ContractManifest, JToken) RemoveUncoveredInstructions(Ne
                 }
                 int methodStart = (int)simplifiedInstructionsToAddress[oldAddressToInstruction[oldMethodStart]]!;
                 int methodEnd = (int)simplifiedInstructionsToAddress[oldAddressToInstruction[oldMethodEnd]]!;
-                newMethodStart.Add(methodStart, method["id"]!.AsString());  // TODO: same format of method name as dumpnef
-                newMethodEnd.Add(methodEnd, method["id"]!.AsString());
                 method["range"] = $"{methodStart}-{methodEnd}";
 
                 int previousSequencePoint = methodStart;
@@ -140,14 +136,6 @@ public static (NefFile, ContractManifest, JToken) RemoveUncoveredInstructions(Ne
                     }
                     else
                         newSequencePoints.Add(new JString($"{previousSequencePoint}{sequencePointGroups[2]}"));
-                    GroupCollection documentGroups = DocumentRegex.Match(sequencePointGroups[2].ToString()).Groups;
-                    newAddrToSequencePoint.Add(previousSequencePoint, (
-                        int.Parse(documentGroups[1].ToString()),
-                        int.Parse(documentGroups[2].ToString()),
-                        int.Parse(documentGroups[3].ToString()),
-                        int.Parse(documentGroups[4].ToString()),
-                        int.Parse(documentGroups[5].ToString())
-                        ));
                 }
                 method["sequence-points"] = newSequencePoints;
             }
@@ -173,6 +161,8 @@ public static Dictionary<int, bool>
             Parallel.ForEach(manifest.Abi.Methods, method =>
                 CoverInstruction(method.Offset, script, coveredMap)
             );
+            //foreach (ContractMethodDescriptor method in manifest.Abi.Methods)
+            //    CoverInstruction(method.Offset, script, coveredMap);
             // start from _deploy method
             foreach (JToken? method in (JArray)debugInfo["methods"]!)
             {
@@ -249,7 +239,17 @@ public static BranchType CoverInstruction(int addr, Script script, Dictionary<in
 
                 // TODO: ABORTMSG may THROW instead of ABORT. Just throw new NotImplementedException for ABORTMSG?
                 if (instruction.OpCode == OpCode.ABORT || instruction.OpCode == OpCode.ABORTMSG)
+                {
+                    // See if we are in a try. There may still be runtime exceptions
+                    ((catchAddr, finallyAddr), stackType) = stack.Peek();
+                    if (stackType == TryStack.TRY && catchAddr != -1)
+                        // Visit catchAddr because there may still be exceptions at runtime
+                        return CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                    if (stackType == TryStack.CATCH && finallyAddr != -1)
+                        // Visit finallyAddr because there may still be exceptions at runtime
+                        return CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
                     return BranchType.ABORT;
+                }
                 if (callWithJump.Contains(instruction.OpCode))
                 {
                     int callTarget = ComputeJumpTarget(addr, instruction);
@@ -260,12 +260,32 @@ public static BranchType CoverInstruction(int addr, Script script, Dictionary<in
                         continue;
                     }
                     if (returnedType == BranchType.ABORT)
+                    {
+                        // See if we are in a try. There may still be runtime exceptions
+                        ((catchAddr, finallyAddr), stackType) = stack.Peek();
+                        if (stackType == TryStack.TRY && catchAddr != -1)
+                            // Visit catchAddr because there may still be exceptions at runtime
+                            return CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                        if (stackType == TryStack.CATCH && finallyAddr != -1)
+                            // Visit finallyAddr because there may still be exceptions at runtime
+                            return CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
                         return BranchType.ABORT;
+                    }
                     if (returnedType == BranchType.THROW)
                         goto HANDLE_THROW;
                 }
                 if (instruction.OpCode == OpCode.RET)
+                {
+                    // See if we are in a try. There may still be runtime exceptions
+                    ((catchAddr, finallyAddr), stackType) = stack.Peek();
+                    if (stackType == TryStack.TRY && catchAddr != -1)
+                        // Visit catchAddr because there may still be exceptions at runtime
+                        CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                    if (stackType == TryStack.CATCH && finallyAddr != -1)
+                        // Visit finallyAddr because there may still be exceptions at runtime
+                        CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
                     return BranchType.OK;
+                }
                 if (tryThrowFinally.Contains(instruction.OpCode))
                 {
                     if (instruction.OpCode == OpCode.TRY || instruction.OpCode == OpCode.TRY_L)
@@ -275,14 +295,15 @@ public static BranchType CoverInstruction(int addr, Script script, Dictionary<in
                     if (instruction.OpCode == OpCode.ENDTRY)
                     {
                         ((catchAddr, finallyAddr), stackType) = stack.Peek();
-                        if (stackType != TryStack.TRY && stackType != TryStack.CATCH) throw new BadScriptException("No try stack on ENDTRY");
+                        if (stackType != TryStack.TRY && stackType != TryStack.CATCH)
+                            throw new BadScriptException("No try stack on ENDTRY");
 
-                        if (stackType == TryStack.TRY)
-                        {
+                        if (stackType == TryStack.TRY && catchAddr != -1)
                             // Visit catchAddr because there may still be exceptions at runtime
-                            Stack<((int returnAddr, int finallyAddr), TryStack stackType)> newStack = new(stack);
-                            CoverInstruction(catchAddr, script, coveredMap, stack: newStack, throwed: true);
-                        }
+                            CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                        if (stackType == TryStack.CATCH && finallyAddr != -1)
+                            // Visit finallyAddr because there may still be exceptions at runtime
+                            CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
 
                         stack.Pop();
                         int endPointer = addr + instruction.TokenI8;
@@ -325,12 +346,32 @@ public static BranchType CoverInstruction(int addr, Script script, Dictionary<in
                 if (conditionalJump.Contains(instruction.OpCode) || conditionalJump_L.Contains(instruction.OpCode))
                 {
                     int targetAddress = ComputeJumpTarget(addr, instruction);
-                    BranchType noJump = CoverInstruction(addr + instruction.Size, script, coveredMap);
-                    BranchType jump = CoverInstruction(targetAddress, script, coveredMap);
+                    BranchType noJump = CoverInstruction(addr + instruction.Size, script, coveredMap, stack: new(stack.Reverse()));
+                    BranchType jump = CoverInstruction(targetAddress, script, coveredMap, stack: new(stack.Reverse()));
                     if (noJump == BranchType.OK || jump == BranchType.OK)
+                    {
+                        // See if we are in a try. There may still be runtime exceptions
+                        ((catchAddr, finallyAddr), stackType) = stack.Peek();
+                        if (stackType == TryStack.TRY && catchAddr != -1)
+                            // Visit catchAddr because there may still be exceptions at runtime
+                            CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                        if (stackType == TryStack.CATCH && finallyAddr != -1)
+                            // Visit finallyAddr because there may still be exceptions at runtime
+                            CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
                         return BranchType.OK;
+                    }
                     if (noJump == BranchType.ABORT && jump == BranchType.ABORT)
+                    {
+                        // See if we are in a try. There may still be runtime exceptions
+                        ((catchAddr, finallyAddr), stackType) = stack.Peek();
+                        if (stackType == TryStack.TRY && catchAddr != -1)
+                            // Visit catchAddr because there may still be exceptions at runtime
+                            return CoverInstruction(catchAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
+                        if (stackType == TryStack.CATCH && finallyAddr != -1)
+                            // Visit finallyAddr because there may still be exceptions at runtime
+                            return CoverInstruction(finallyAddr, script, coveredMap, stack: new(stack.Reverse()), throwed: true);
                         return BranchType.ABORT;
+                    }
                     if (noJump == BranchType.THROW || jump == BranchType.THROW)  // THROW, ABORT => THROW
                         goto HANDLE_THROW;
                     throw new Exception($"Unknown {nameof(BranchType)} {noJump} {jump}");

src/Neo.Compiler.CSharp/Program.cs

@@ -157,7 +157,7 @@ private static int ProcessOutputs(Options options, string folder, CompilationCon
                 {
                     try
                     {
-                        (nef, manifest, debugInfo) = Reachability.RemoveUncoveredInstructions(nef, manifest, debugInfo);
+                        (nef, manifest, debugInfo) = Reachability.RemoveUncoveredInstructions(nef, manifest, debugInfo.Clone());
                     }
                     catch (Exception ex)
                     {
@@ -272,9 +272,16 @@ private static int ProcessOutputs(Options options, string folder, CompilationCon
                     path = Path.Combine(outputFolder, $"{baseName}.asm");
                     File.WriteAllText(path, context.CreateAssembly());
                     Console.WriteLine($"Created {path}");
-                    path = Path.Combine(outputFolder, $"{baseName}.nef.txt");
-                    File.WriteAllText(path, DumpNef.GenerateDumpNef(nef, debugInfo));
-                    Console.WriteLine($"Created {path}");
+                    try
+                    {
+                        path = Path.Combine(outputFolder, $"{baseName}.nef.txt");
+                        File.WriteAllText(path, DumpNef.GenerateDumpNef(nef, debugInfo));
+                        Console.WriteLine($"Created {path}");
+                    }
+                    catch (Exception ex)
+                    {
+                        Console.Error.WriteLine($"Failed to dumpnef: {ex}");
+                    }
                 }
                 Console.WriteLine("Compilation completed successfully.");
                 return 0;