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.
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.
Regards
Bert
---
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
*/
@@ -486,10 +491,14 @@ int ExecuteMacro(WindowInfo *window, Pro
/* Push arguments and call information onto the stack */
for (i=0; i<nArgs; i++)
*(context->stackP++) = args[i];
+ context->stackP->val.prog = NULL; /* prog, nothing to be free */
+ context->stackP->tag = NO_TAG;
+ context->stackP++;
+
context->stackP->val.subr = NULL; /* return PC */
context->stackP->tag = NO_TAG;
context->stackP++;
*(context->stackP++) = noValue; /* old FrameP */
@@ -584,10 +593,14 @@ void RunMacroAsSubrCall(Program *prog)
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.prog = prog; /* prog */
+ StackP++;
+
+ StackP->tag = NO_TAG;
StackP->val.inst = PC; /* return PC */
StackP++;
StackP->tag = NO_TAG;
StackP->val.dataval = FrameP; /* old FrameP */
@@ -1947,10 +1960,17 @@ 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) {
+ prog = sym->value.val.prog;
+
+ prog->refcount++;
+ StackP->tag = NO_TAG; /* prog */
+ StackP->val.prog = prog;
+ StackP++;
+
StackP->tag = NO_TAG; /* return PC */
StackP->val.inst = PC;
StackP++;
StackP->tag = NO_TAG; /* old FrameP */
@@ -1962,11 +1982,10 @@ static int callSubroutine(void)
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++;
}
@@ -2050,10 +2069,11 @@ static int returnVal(void)
static int returnValOrNone(int valOnStack)
{
DataValue retVal;
static DataValue noValue = {NO_TAG, {0}};
DataValue *newFrameP;
+ Program *prog;
int nArgs;
DISASM_RT(PC-1, 1);
STACKDUMP(StackP - FrameP + FP_GET_ARG_COUNT(FrameP) + FP_TO_ARGS_DIST, 3);
@@ -2064,16 +2084,21 @@ static int returnValOrNone(int valOnStac
/* get stored return information */
nArgs = FP_GET_ARG_COUNT(FrameP);
newFrameP = FP_GET_OLD_FP(FrameP);
PC = FP_GET_RET_PC(FrameP);
+ prog = FP_GET_PROG(FrameP);
/* pop past local variables */
StackP = FrameP;
/* pop past function arguments */
StackP -= (FP_TO_ARGS_DIST + nArgs);
FrameP = newFrameP;
+
+ if (prog) {
+ FreeProgram(prog);
+ }
/* push returned value, if requsted */
if (PC == NULL) {
if (valOnStack) {
PUSH(retVal);
@@ -3049,10 +3074,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
