This is my proposal for fixing several issues with the handling of compiled macro programs.
The first is a memory leak in source/macro.c::readCheckMacroString(), if we the caller wants to parse only the macro, i.e. passing runWindow == NULL. This was reported as SF BUG#2078867. The second is more subtile: if a macro function definition overwrites a previously one we unconditionaly free the old program. But this old program is may be currently in use. To solve this I have introduced a reference count for the program and FreeProgram() really releases the program only if this drops to zero. To track references to the program it is needed to put the program pointer into the stack frame and increment the reference count. There are currently 3 places were we build up a stacke frame, each needs different handling of the program pointer: 1) ExecuteMacro(): The caller is already responsible for freeing the prog (its runMacro()). Therefore we put a NULL pointer for the prog into the frame. 2) RunMacroAsSubrCall(): Here the caller passes the reference count to this function, so we don't need to increment the reference. The callers are runMacro() and readCheckMacroString(). 3) callSubroutine(): This is the usual usage: increment the reference and put it into the stack frame. All three places are now consolidated into the function setupFrame(). In case of an error, all frames need to be rewinded. This is handled with the new function rewindFrame(). Which is also used in returnValOrNone(), therefore consolidates this too. Before we can rewind the whole stack we need to update the execution context, therefore these new calls to saveContext(). This was reported as SF BUG#2113904. Tony reported a second case where we hit the 'free while in use' case, but I haven't done a test case for this yet. I assume that this is also catched by this patch. --- Changes in v3: * fix pushing of old frame pointer If there are no objections, I commit this by the end of the week, plus my proposal B for SF: BUG#2074318, from here: http://www.nedit.org/pipermail/develop/2008-August/014628.html source/interpret.c | 197 +++++++++++++++++++++++++++++------------------------ source/interpret.h | 1 source/macro.c | 6 + 3 files changed, 117 insertions(+), 87 deletions(-) diff --quilt old/source/macro.c new/source/macro.c --- old/source/macro.c +++ new/source/macro.c @@ -921,10 +921,11 @@ static int readCheckMacroString(Widget d } if (runWindow != NULL) { XEvent nextEvent; if (runWindow->macroCmdData == NULL) { + /* runMacro() is responsable for freeing prog */ runMacro(runWindow, prog); while (runWindow->macroCmdData != NULL) { XtAppNextEvent(XtWidgetToApplicationContext( runWindow->shell), &nextEvent); ServerDispatchEvent(&nextEvent); @@ -943,10 +944,15 @@ static int readCheckMacroString(Widget d Push(progStack, (void*) prog); } } inPtr = stoppedAt; } + + /* we are in 'parse only' mode, therefore release the prog */ + if (runWindow == NULL) { + FreeProgram(prog); + } } /* Unroll reversal stack for macros loaded from macros. */ while (NULL != (prog = (Program*) Pop(progStack))) { RunMacroAsSubrCall(prog); diff --quilt old/source/interpret.c new/source/interpret.c --- old/source/interpret.c +++ new/source/interpret.c @@ -213,16 +213,18 @@ static int (*OpFns[N_OPS])() = {returnNo /* Stack-> symN-sym0(FP), argArray, nArgs, oldFP, retPC, argN-arg1, next, ... */ #define FP_ARG_ARRAY_CACHE_INDEX (-1) #define FP_ARG_COUNT_INDEX (-2) #define FP_OLD_FP_INDEX (-3) #define FP_RET_PC_INDEX (-4) -#define FP_TO_ARGS_DIST (4) /* should be 0 - (above index) */ +#define FP_PROG_INDEX (-5) +#define FP_TO_ARGS_DIST (5) /* should be 0 - (above index) */ #define FP_GET_ITEM(xFrameP,xIndex) (*(xFrameP + xIndex)) #define FP_GET_ARG_ARRAY_CACHE(xFrameP) (FP_GET_ITEM(xFrameP, FP_ARG_ARRAY_CACHE_INDEX)) #define FP_GET_ARG_COUNT(xFrameP) (FP_GET_ITEM(xFrameP, FP_ARG_COUNT_INDEX).val.n) #define FP_GET_OLD_FP(xFrameP) ((FP_GET_ITEM(xFrameP, FP_OLD_FP_INDEX)).val.dataval) #define FP_GET_RET_PC(xFrameP) ((FP_GET_ITEM(xFrameP, FP_RET_PC_INDEX)).val.inst) +#define FP_GET_PROG(xFrameP) ((FP_GET_ITEM(xFrameP, FP_PROG_INDEX)).val.prog) #define FP_ARG_START_INDEX(xFrameP) (-(FP_GET_ARG_COUNT(xFrameP) + FP_TO_ARGS_DIST)) #define FP_GET_ARG_N(xFrameP,xN) (FP_GET_ITEM(xFrameP, xN + FP_ARG_START_INDEX(xFrameP))) #define FP_GET_SYM_N(xFrameP,xN) (FP_GET_ITEM(xFrameP, xN)) #define FP_GET_SYM_VAL(xFrameP,xSym) (FP_GET_SYM_N(xFrameP, xSym->value.val.n)) @@ -295,10 +297,11 @@ Program *FinishCreatingProgram(void) newProg = (Program *)XtMalloc(sizeof(Program)); progLen = ((char *)ProgP) - ((char *)Prog); newProg->code = (Inst *)XtMalloc(progLen); memcpy(newProg->code, Prog, progLen); newProg->localSymList = LocalSymList; + newProg->refcount = 1; LocalSymList = NULL; /* Local variables' values are stored on the stack. Here we assign frame pointer offsets to them. */ for (s = newProg->localSymList; s != NULL; s = s->next) @@ -309,13 +312,15 @@ Program *FinishCreatingProgram(void) return newProg; } void FreeProgram(Program *prog) { - freeSymbolTable(prog->localSymList); - XtFree((char *)prog->code); - XtFree((char *)prog); + if (--prog->refcount == 0) { + freeSymbolTable(prog->localSymList); + XtFree((char *)prog->code); + XtFree((char *)prog); + } } /* ** Add an operator (instruction) to the end of the current program */ @@ -457,10 +462,96 @@ void FillLoopAddrs(Inst *breakAddr, Inst fprintf(stderr, "NEdit: internal error (uat) in macro parser\n"); } } /* +** helper function to setup the next frame +*/ +static void setupFrame(DataValue **frameP, DataValue **stackP, + Inst **pc, Program *prog, int nArgs, DataValue *args, int passProg) +{ + static DataValue noValue = {NO_TAG, {0}}; + int i; + Symbol *s; + + /* Push arguments and caller information onto the stack */ + if (nArgs >= 0) { + for (i = 0; i < nArgs; i++) { + *((*stackP)++) = args[i]; + } + } + else { + /* a negative nArgs means, that the arguments are on the stack + ** already + */ + nArgs = -nArgs; + } + + /* prog to free, if requested */ + (*stackP)->tag = NO_TAG; + (*stackP)->val.prog = passProg ? prog : NULL; + (*stackP)++; + + /* return PC */ + (*stackP)->tag = NO_TAG; + (*stackP)->val.inst = *pc; + (*stackP)++; + + /* old FrameP */ + (*stackP)->tag = NO_TAG; + (*stackP)->val.dataval = *frameP; + (*stackP)++; + + /* nArgs */ + (*stackP)->tag = NO_TAG; + (*stackP)->val.n = nArgs; + (*stackP)++; + + /* cached arg array */ + *((*stackP)++) = noValue; + + *frameP = *stackP; + + /* Initialize and make room on the stack for local variables */ + for (s = prog->localSymList; s != NULL; s = s->next) { + FP_GET_SYM_VAL(*frameP, s) = noValue; + (*stackP)++; + } + + *pc = prog->code; +} + +static Inst *rewindFrame(DataValue **frameP, DataValue **stackP) +{ + /* get stored return information */ + int nArgs = FP_GET_ARG_COUNT(*frameP); + DataValue *newFrameP = FP_GET_OLD_FP(*frameP); + Inst *newPC = FP_GET_RET_PC(*frameP); + Program *prog = FP_GET_PROG(*frameP); + + /* pop past local variables */ + *stackP = *frameP; + /* pop past arguments */ + *stackP -= (FP_TO_ARGS_DIST + nArgs); + + *frameP = newFrameP; + + if (prog) { + FreeProgram(prog); + } + + return newPC; +} + +static void rewindStack(Inst *pc, DataValue *frameP, DataValue *stackP) +{ + while (pc) { + pc = rewindFrame(&frameP, &stackP); + } +} + +/* ** Execute a compiled macro, "prog", using the arguments in the array ** "args". Returns one of MACRO_DONE, MACRO_PREEMPT, or MACRO_ERROR. ** if MACRO_DONE is returned, the macro completed, and the returned value ** (if any) can be read from "result". If MACRO_PREEMPT is returned, the ** macro exceeded its alotted time-slice and scheduled... @@ -478,37 +569,17 @@ int ExecuteMacro(WindowInfo *window, Pro preemption and resumption of execution */ context = (RestartData *)XtMalloc(sizeof(RestartData)); context->stack = (DataValue *)XtMalloc(sizeof(DataValue) * STACK_SIZE); *continuation = context; context->stackP = context->stack; - context->pc = prog->code; context->runWindow = window; context->focusWindow = window; - /* Push arguments and call information onto the stack */ - for (i=0; i<nArgs; i++) - *(context->stackP++) = args[i]; - - context->stackP->val.subr = NULL; /* return PC */ - context->stackP->tag = NO_TAG; - context->stackP++; - - *(context->stackP++) = noValue; /* old FrameP */ - - context->stackP->tag = NO_TAG; /* nArgs */ - context->stackP->val.n = nArgs; - context->stackP++; - - *(context->stackP++) = noValue; /* cached arg array */ - - context->frameP = context->stackP; - - /* Initialize and make room on the stack for local variables */ - for (s = prog->localSymList; s != NULL; s = s->next) { - FP_GET_SYM_VAL(context->frameP, s) = noValue; - context->stackP++; - } + /* pre-set the return PC to NULL */ + context->pc = NULL; + setupFrame(&context->frameP, &context->stackP, &context->pc, + prog, nArgs, args, False); /* Begin execution, return on error or preemption */ return ContinueMacro(context, result, msg); } @@ -545,16 +616,18 @@ int ContinueMacro(RestartData *continuat saveContext(continuation); restoreContext(&oldContext); return MACRO_PREEMPT; } else if (status == STAT_ERROR) { *msg = ErrMsg; + saveContext(continuation); FreeRestartData(continuation); restoreContext(&oldContext); return MACRO_ERROR; } else if (status == STAT_DONE) { *msg = ""; *result = *--StackP; + saveContext(continuation); FreeRestartData(continuation); restoreContext(&oldContext); return MACRO_DONE; } } @@ -578,39 +651,16 @@ int ContinueMacro(RestartData *continuat ** separate contexts, and serializes processing of the two macros without ** additional work. */ void RunMacroAsSubrCall(Program *prog) { - Symbol *s; - static DataValue noValue = {NO_TAG, {0}}; - - /* See subroutine "callSubroutine" for a description of the stack frame - for a subroutine call */ - StackP->tag = NO_TAG; - StackP->val.inst = PC; /* return PC */ - StackP++; - - StackP->tag = NO_TAG; - StackP->val.dataval = FrameP; /* old FrameP */ - StackP++; - - StackP->tag = NO_TAG; /* nArgs */ - StackP->val.n = 0; - StackP++; - - *(StackP++) = noValue; /* cached arg array */ - - FrameP = StackP; - PC = prog->code; - for (s = prog->localSymList; s != NULL; s = s->next) { - FP_GET_SYM_VAL(FrameP, s) = noValue; - StackP++; - } + setupFrame(&FrameP, &StackP, &PC, prog, 0, NULL, True); } void FreeRestartData(RestartData *context) { + rewindStack(context->pc, context->frameP, context->stackP); XtFree((char *)context->stack); XtFree((char *)context); } /* @@ -1947,31 +1997,14 @@ static int callSubroutine(void) ** Push all of the required information to resume, and make space on the ** stack for local variables (and initialize them), on top of the argument ** values which are already there. */ if (sym->type == MACRO_FUNCTION_SYM) { - StackP->tag = NO_TAG; /* return PC */ - StackP->val.inst = PC; - StackP++; - - StackP->tag = NO_TAG; /* old FrameP */ - StackP->val.dataval = FrameP; - StackP++; - - StackP->tag = NO_TAG; /* nArgs */ - StackP->val.n = nArgs; - StackP++; - - *(StackP++) = noValue; /* cached arg array */ - - FrameP = StackP; - prog = sym->value.val.prog; - PC = prog->code; - for (s = prog->localSymList; s != NULL; s = s->next) { - FP_GET_SYM_VAL(FrameP, s) = noValue; - StackP++; - } + prog = sym->value.val.prog; + prog->refcount++; + /* -nArgs means 'arguments are on stack' */ + setupFrame(&FrameP, &StackP, &PC, prog, -nArgs, NULL, True); return STAT_OK; } /* ** Call an action routine @@ -2049,32 +2082,21 @@ static int returnVal(void) */ static int returnValOrNone(int valOnStack) { DataValue retVal; static DataValue noValue = {NO_TAG, {0}}; - DataValue *newFrameP; - int nArgs; DISASM_RT(PC-1, 1); STACKDUMP(StackP - FrameP + FP_GET_ARG_COUNT(FrameP) + FP_TO_ARGS_DIST, 3); /* return value is on the stack */ if (valOnStack) { POP(retVal); } - /* get stored return information */ - nArgs = FP_GET_ARG_COUNT(FrameP); - newFrameP = FP_GET_OLD_FP(FrameP); - PC = FP_GET_RET_PC(FrameP); - - /* pop past local variables */ - StackP = FrameP; - /* pop past function arguments */ - StackP -= (FP_TO_ARGS_DIST + nArgs); - FrameP = newFrameP; - + PC = rewindFrame(&FrameP, &StackP); + /* push returned value, if requsted */ if (PC == NULL) { if (valOnStack) { PUSH(retVal); } else { @@ -3049,10 +3071,11 @@ static void stackdump(int n, int extra) case 0: pos = "FrameP"; break; /* first local symbol value */ case FP_ARG_ARRAY_CACHE_INDEX: pos = "args"; break; /* arguments array */ case FP_ARG_COUNT_INDEX: pos = "NArgs"; break; /* number of arguments */ case FP_OLD_FP_INDEX: pos = "OldFP"; break; case FP_RET_PC_INDEX: pos = "RetPC"; break; + case FP_PROG_INDEX: pos = "Prog"; break; default: if (offset < -FP_TO_ARGS_DIST && offset >= -FP_TO_ARGS_DIST - nArgs) { sprintf(pos = buffer, STACK_DUMP_ARG_PREFIX "%d", offset + FP_TO_ARGS_DIST + nArgs + 1); } diff --quilt old/source/interpret.h new/source/interpret.h --- old/source/interpret.h +++ new/source/interpret.h @@ -103,10 +103,11 @@ typedef struct SymbolRec { } Symbol; typedef struct ProgramTag { Symbol *localSymList; Inst *code; + unsigned refcount; } Program; /* Information needed to re-start a preempted macro */ typedef struct { DataValue *stack; -- NEdit Develop mailing list - [email protected] http://www.nedit.org/mailman/listinfo/develop
