Refactor ugly nested stack datastructure into something simpler

src/debug/decoding/general.ts

@@ -49,7 +49,7 @@ function decodeValInt(
     }
 
     if (loc.kind === DataLocationKind.CallData) {
-        const lastExtFrame = topExtFrame(state.stack);
+        const lastExtFrame = topExtFrame(state);
 
         let abiType: TypeNode;
 
@@ -112,7 +112,7 @@ export function decodeValue(
         let pointedToLoc: MemoryLocation;
 
         if (isCalldataType2Slots(typ)) {
-            const lastExtFrame = topExtFrame(state.stack);
+            const lastExtFrame = topExtFrame(state);
 
             let abiType: TypeNode;
 

src/debug/tracers/sol_debugger.ts

@@ -11,7 +11,7 @@ import {
 } from "./transformers";
 import { addEventInfo } from "./transformers/events";
 import { addExternalFrame } from "./transformers/ext_stack";
-import { addInternalFrame } from "./transformers/int_stack";
+import { addInternalFrameInfo } from "./transformers/int_stack";
 import { addSource } from "./transformers/source";
 
 export class SolTxDebugger extends MapOnlyTracer<StepState> {
@@ -32,7 +32,7 @@ export class SolTxDebugger extends MapOnlyTracer<StepState> {
             tx
         );
         const source = await addSource(vm, step, extFrameInfo);
-        const intStack = await addInternalFrame(
+        const intStack = await addInternalFrameInfo(
             vm,
             step,
             source,

src/debug/tracers/transformers/contract_lifetime.ts

@@ -77,7 +77,7 @@ export function addContractLifetimeInfo<T extends object & BasicStepInfo & Exter
 
     // Case 2: We return from a
     const lastStep = trace[trace.length - 1];
-    const lastStepFrame = topExtFrame(lastStep.stack);
+    const lastStepFrame = topExtFrame(lastStep);
 
     // Successful return from a creation frame
     if (

src/debug/tracers/transformers/ext_stack.ts

@@ -19,27 +19,17 @@ import {
     DataLocationKind,
     DataView,
     ExternalFrame,
-    Frame,
-    FrameKind,
-    isFrame
+    FrameKind
 } from "../../types";
 import { BasicStepInfo } from "./basic_info";
 
 export interface ExternalFrameInfo {
     stack: ExternalFrame[];
 }
 
-export function topExtFrame(arg: ExternalFrame[] | ExternalFrameInfo | Frame): ExternalFrame {
-    if (isFrame(arg)) {
-        return arg.kind === FrameKind.InternalCall ? arg.nearestExtFrame : arg;
-    }
-
-    if (!(arg instanceof Array)) {
-        arg = arg.stack;
-    }
-
-    assert(arg.length > 0, `Empty stack!`);
-    return arg[arg.length - 1];
+export function topExtFrame(arg: ExternalFrameInfo): ExternalFrame {
+    assert(arg.stack.length > 0, `Empty stack!`);
+    return arg.stack[arg.stack.length - 1];
 }
 
 export function getContractInfo(step: ExternalFrameInfo): ContractInfo | undefined {
@@ -126,8 +116,7 @@ function makeCallFrame(
         arguments: args,
         codeMdHash: codeHash,
         codeAddress,
-        internalFrames: [],
-        internalFramesBroken: false
+        internalFramesSus: false
     };
 }
 
@@ -159,8 +148,7 @@ function makeCreationFrame(
         startStep: step,
         arguments: args,
         codeMdHash: getCreationCodeHash(data),
-        internalFrames: [],
-        internalFramesBroken: false
+        internalFramesSus: false
     };
 }
 
@@ -296,24 +284,24 @@ export async function addExternalFrame<T extends object & BasicStepInfo>(
             ...state
         };
     } else {
-        const stack = [...lastStep.stack];
+        const newStack = [...lastStep.stack];
         // External return or exception
         let nFramesPopped = lastStep.depth - state.depth;
 
         // Pop as many external frames as neccessary to match the decrease in
         // depth reported by web3. We need the loop since we don't count the internal frames as decreasing depth
-        while (nFramesPopped > 0 && stack.length > 0) {
-            const topFrame = stack[stack.length - 1];
+        while (nFramesPopped > 0 && newStack.length > 0) {
+            const topFrame = newStack[newStack.length - 1];
 
             if (topFrame.kind === FrameKind.Creation || topFrame.kind === FrameKind.Call) {
                 nFramesPopped--;
             }
 
-            stack.pop();
+            newStack.pop();
         }
 
         return {
-            stack: stack,
+            stack: newStack,
             ...state
         };
     }

src/debug/tracers/transformers/int_stack.ts

@@ -7,7 +7,6 @@ import { OPCODES } from "../../opcodes";
 import {
     DataLocationKind,
     DataView,
-    ExternalFrame,
     Frame,
     FrameKind,
     InternalCallFrame,
@@ -17,18 +16,22 @@ import { BasicStepInfo } from "./basic_info";
 import { ExternalFrameInfo, topExtFrame } from "./ext_stack";
 import { SourceInfo } from "./source";
 
-export function topFrame(stack: ExternalFrame[] | ExternalFrameInfo): Frame {
-    const topExt = topExtFrame(stack);
+export interface InternalFrameInfo {
+    intStack: InternalCallFrame[];
+}
 
-    if (topExt.internalFrames === undefined || topExt.internalFrames.length === 0) {
-        return topExt;
+function topFrame(step: InternalFrameInfo & ExternalFrameInfo & BasicStepInfo): Frame {
+    if (step.intStack.length > 0) {
+        return step.intStack[step.intStack.length - 1];
     }
 
-    return topExt.internalFrames[topExt.internalFrames.length - 1];
+    assert(step.stack.length > 0, `Unexpected empty stack in step at pc {0}`, step.pc);
+    return step.stack[step.stack.length - 1];
 }
 
 /**
- * WIP: TODO document
+ * Given a callable (function definition or public state variable) try to build
+ * `DataView`s for all the callable arguments. On failure return undefined.
  */
 function buildFunArgViews(
     callee: FunctionDefinition | VariableDeclaration,
@@ -87,43 +90,56 @@ function buildFunArgViews(
 /**
  * Adds external frame info for each step
  */
-export async function addInternalFrame<
+export async function addInternalFrameInfo<
     T extends object & BasicStepInfo & ExternalFrameInfo & SourceInfo
 >(
     vm: VM,
     step: InterpreterStep,
     state: T,
-    trace: T[],
+    trace: Array<T & InternalFrameInfo>,
     artifactManager: IArtifactManager,
     strict: boolean
-): Promise<T> {
-    // No internal stack frame on first step of trace
+): Promise<T & InternalFrameInfo> {
+    // No internal stack frame on first step of trace. We are in the contract preamble
     if (trace.length === 0) {
-        return state;
+        return {
+            ...state,
+            intStack: []
+        };
     }
 
     const lastStep = trace[trace.length - 1];
 
-    // If we are ending execution, and still have leftover internal frames, then internal frame decoding
-    // is probably broken.
-    if (state.op.opcode === OPCODES.STOP) {
-        const curExtFrame = topExtFrame(state.stack);
-
-        curExtFrame.internalFramesBroken =
-            curExtFrame.internalFramesBroken || curExtFrame.internalFrames.length > 1;
-    }
-
-    // External call/return - no change to internal stack
-    if (lastStep.depth !== state.depth) {
+    // Return/exception
+    if (state.depth < lastStep.depth) {
+        // Check if upon normal return there were leftover internal stack frames in previous context
         if (lastStep.op.opcode === OPCODES.RETURN) {
-            const lastExtFrame = topExtFrame(lastStep.stack);
+            const lastExtFrame = topExtFrame(lastStep);
+
             // If we had a normal return with multiple stack frames left in the last frame, then it was probably broken
-            lastExtFrame.internalFramesBroken =
-                lastExtFrame.internalFramesBroken || lastExtFrame.internalFrames.length > 1;
+            lastExtFrame.internalFramesSus =
+                lastExtFrame.internalFramesSus || lastStep.intStack.length > 1;
         }
-        return state;
+
+        // Assume we return in the same internal stack as right before we made the call
+        const lastStepBeforeCall = trace[lastStep.stack[state.stack.length].startStep - 1];
+
+        return {
+            ...state,
+            intStack: lastStepBeforeCall.intStack
+        };
+    }
+
+    // Call/creation - initially empty internal stack as we start in the contract preamble
+    if (state.depth === lastStep.depth + 1) {
+        return {
+            ...state,
+            intStack: []
+        };
     }
 
+    assert(state.depth === lastStep.depth, ``);
+
     // There are 2 ways to enter an internal function:
     let enteringInternalFun = false;
 
@@ -138,7 +154,7 @@ export async function addInternalFrame<
     }
 
     const ast = state.astNode;
-    const curExtFrame = topExtFrame(state.stack);
+    const curExtFrame = topExtFrame(state);
 
     //  2. Fall-through (the previous instruction is literally the pervious instruction in the contract body,
     //      AND the current JUMPDEST corresponds to a whole function, AND the pervious instructions' callee is different
@@ -148,7 +164,7 @@ export async function addInternalFrame<
         state.op.mnemonic === "JUMPDEST" &&
         (ast instanceof FunctionDefinition ||
             (ast instanceof VariableDeclaration && ast.stateVariable)) &&
-        topFrame(lastStep.stack).callee !== ast
+        topFrame(lastStep).callee !== ast
     ) {
         enteringInternalFun = true;
     }
@@ -173,21 +189,15 @@ export async function addInternalFrame<
             arguments: args
         };
 
-        const newInternalFrames = [...curExtFrame.internalFrames, newFrame];
-
         return {
             ...state,
-            stack: [
-                ...state.stack.slice(0, -1),
-                { ...curExtFrame, internalFrames: newInternalFrames }
-            ]
+            intStack: [...lastStep.intStack, newFrame]
         };
     }
 
     // Returning from an internal function call
     if (state.op.mnemonic === "JUMP" && state.src && state.src.jump === "o") {
-        const curFrame = topFrame(state.stack);
-        const newInternalFrames = curExtFrame.internalFrames.slice(0, -1);
+        const curFrame = topFrame(lastStep);
 
         if (strict) {
             assert(
@@ -197,18 +207,18 @@ export async function addInternalFrame<
             );
         } else {
             if (curFrame.kind !== FrameKind.InternalCall) {
-                curExtFrame.internalFramesBroken = true;
+                curExtFrame.internalFramesSus = true;
             }
         }
 
         return {
             ...state,
-            stack: [
-                ...state.stack.slice(0, -1),
-                { ...curExtFrame, internalFrames: newInternalFrames }
-            ]
+            intStack: lastStep.intStack.slice(0, -1)
         };
     }
 
-    return state;
+    return {
+        ...state,
+        intStack: lastStep.intStack
+    };
 }

src/debug/tracers/transformers/source.ts

@@ -47,7 +47,7 @@ export async function addSource<T extends object & BasicStepInfo & ExternalFrame
     step: InterpreterStep,
     state: T
 ): Promise<T & SourceInfo> {
-    const [src, astNode] = decodeSourceLoc(state.pc, topExtFrame(state.stack));
+    const [src, astNode] = decodeSourceLoc(state.pc, topExtFrame(state));
 
     return {
         src,

src/debug/types.ts

@@ -72,10 +72,8 @@ export interface BaseExternalFrame extends BaseFrame {
     readonly info?: ContractInfo;
     readonly code: Uint8Array;
     readonly codeMdHash: HexString | undefined;
-    internalFrames: InternalCallFrame[];
-    // Set if the internal frame decoding detects broken traces due to
-    // invalid source maps in the presence of optimizations
-    internalFramesBroken: boolean;
+    // Set if the internal call/returns in a contract dont match up.
+    internalFramesSus: boolean;
 }
 
 /**
@@ -203,6 +201,7 @@ export interface StepVMState {
  */
 export interface StepState extends StepVMState {
     stack: ExternalFrame[];
+    intStack: InternalCallFrame[];
     src: sol.DecodedBytecodeSourceMapEntry | undefined;
     astNode: sol.ASTNode | undefined;
     emittedEvent: EventDesc | undefined;