From 20d917a307de86e4f9d01e8bac71ca63f48b2ef7 Mon Sep 17 00:00:00 2001 From: Artem Golubikhin Date: Fri, 2 Nov 2018 16:15:31 +0300 Subject: [PATCH] Port bugfix for incorrect heap deallocation on conditional operator (#627) * Revert 4f8917ec (experimental bugfix for heap in conditional) * Port bugfix for incorrect heap deallocation on conditional operator (ported from compuphase upstream) * Fix the upstream bugfix Fixed the wrong order of heaplist nodes and the incorrect calculation of the max. heap usage. * Add an additional pass for functions that return array if they are used before definition (inside definition (recursion) is a "before definition" situation too) --- compiler/libpc300/sc.h | 9 ++++++ compiler/libpc300/sc1.c | 12 ++++++++ compiler/libpc300/sc3.c | 59 ++++++++++++++++++++++++++++---------- compiler/libpc300/sclist.c | 46 +++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 15 deletions(-) mode change 100755 => 100644 compiler/libpc300/sc3.c diff --git a/compiler/libpc300/sc.h b/compiler/libpc300/sc.h index 46107475..fd3d269a 100755 --- a/compiler/libpc300/sc.h +++ b/compiler/libpc300/sc.h @@ -280,6 +280,12 @@ typedef struct s_stringpair { char *documentation; } stringpair; +typedef struct s_valuepair { + struct s_valuepair *next; + long first; + long second; +} valuepair; + /* macros for code generation */ #define opcodes(n) ((n)*sizeof(cell)) /* opcode size */ #define opargs(n) ((n)*sizeof(cell)) /* size of typical argument */ @@ -700,6 +706,9 @@ SC_FUNC void delete_docstringtable(void); SC_FUNC stringlist *insert_autolist(char *string); SC_FUNC char *get_autolist(int index); SC_FUNC void delete_autolisttable(void); +SC_FUNC valuepair *push_heaplist(long first, long second); +SC_FUNC int popfront_heaplist(long *first, long *second); +SC_FUNC void delete_heaplisttable(void); SC_FUNC stringlist *insert_dbgfile(const char *filename); SC_FUNC stringlist *insert_dbgline(int linenr); SC_FUNC stringlist *insert_dbgsymbol(symbol *sym); diff --git a/compiler/libpc300/sc1.c b/compiler/libpc300/sc1.c index 03d261e3..66af30d4 100755 --- a/compiler/libpc300/sc1.c +++ b/compiler/libpc300/sc1.c @@ -628,6 +628,7 @@ int pc_compile(int argc, char *argv[]) /* reset "defined" flag of all functions and global variables */ reduce_referrers(&glbtab); delete_symbols(&glbtab,0,TRUE,FALSE); + delete_heaplisttable(); #if !defined NO_DEFINE delete_substtable(); inst_datetime_defines(); @@ -805,6 +806,7 @@ cleanup: free(sc_documentation); #endif delete_autolisttable(); + delete_heaplisttable(); if (errnum!=0) { if (strlen(errfname)==0) pc_printf("\n%d Error%s.\n",errnum,(errnum>1) ? "s" : ""); @@ -5450,6 +5452,16 @@ static void doreturn(void) /* nothing */; sub=addvariable(curfunc->name,(argcount+3)*sizeof(cell),iREFARRAY,sGLOBAL,curfunc->tag,dim,numdim,idxtag); sub->parent=curfunc; + /* Function that returns array can be used before it is defined, so at + * the call point (if it is before definition) we may not know if this + * function returns array and what is its size (for example inside the + * conditional operator), so we don't know how many cells on the heap + * we need. Calculating heap consumption is required for the fix of + * incorrect heap deallocation on conditional operator. That's why we + * need an additional pass. + */ + if ((curfunc->usage & uREAD)!=0) + sc_reparse=TRUE; } /* if */ /* get the hidden parameter, copy the array (the array is on the heap; * it stays on the heap for the moment, and it is removed -usually- at diff --git a/compiler/libpc300/sc3.c b/compiler/libpc300/sc3.c old mode 100755 new mode 100644 index da6f8ed9..f54bdb85 --- a/compiler/libpc300/sc3.c +++ b/compiler/libpc300/sc3.c @@ -1010,38 +1010,60 @@ static int hier13(value *lval) { int lvalue=plnge1(hier12,lval); if (matchtoken('?')) { + int locheap=decl_heap; /* save current heap delta */ + long heap1,heap2; /* max. heap delta either branch */ + valuepair *heaplist_node; int flab1=getlabel(); int flab2=getlabel(); value lval2 = {0}; int array1,array2; - int orig_heap=decl_heap; - int diff1=0,diff2=0; if (lvalue) { rvalue(lval); } else if (lval->ident==iCONSTEXPR) { ldconst(lval->constval,sPRI); error(lval->constval ? 206 : 205); /* redundant test */ } /* if */ + if (sc_status==statFIRST) { + /* We should push a new node right now otherwise we will pop it in the + * wrong order on the write stage. + */ + heaplist_node=push_heaplist(0,0); /* save the pointer to write the actual data later */ + } else if (sc_status==statWRITE || sc_status==statSKIP) { + #if !defined NDEBUG + int result= + #endif + popfront_heaplist(&heap1,&heap2); + assert(result); /* pop off equally many items than were pushed */ + } /* if */ jmp_eq0(flab1); /* go to second expression if primary register==0 */ PUSHSTK_I(sc_allowtags); sc_allowtags=FALSE; /* do not allow tagnames here (colon is a special token) */ + if (sc_status==statWRITE) { + modheap(heap1*sizeof(cell)); + decl_heap+=heap1; /* equilibrate the heap (see comment below) */ + } /* if */ if (hier13(lval)) rvalue(lval); if (lval->ident==iCONSTEXPR) /* load constant here */ ldconst(lval->constval,sPRI); sc_allowtags=(short)POPSTK_I(); /* restore */ + heap1=decl_heap-locheap; /* save heap space used in "true" branch */ + assert(heap1>=0); + decl_heap=locheap; /* restore heap delta */ jumplabel(flab2); setlabel(flab1); - if (orig_heap!=decl_heap) { - diff1=abs(decl_heap-orig_heap); - decl_heap=orig_heap; - } needtoken(':'); + if (sc_status==statWRITE) { + modheap(heap2*sizeof(cell)); + decl_heap+=heap2; /* equilibrate the heap (see comment below) */ + } /* if */ if (hier13(&lval2)) rvalue(&lval2); if (lval2.ident==iCONSTEXPR) /* load constant here */ ldconst(lval2.constval,sPRI); + heap2=decl_heap-locheap; /* save heap space used in "false" branch */ + assert(heap2>=0); array1= (lval->ident==iARRAY || lval->ident==iREFARRAY); array2= (lval2.ident==iARRAY || lval2.ident==iREFARRAY); if (array1 && !array2) { @@ -1055,19 +1077,26 @@ static int hier13(value *lval) if (!matchtag(lval->tag,lval2.tag,FALSE)) error(213); /* tagname mismatch ('true' and 'false' expressions) */ setlabel(flab2); + if (sc_status==statFIRST) { + /* Calculate the max. heap space used by either branch and save values of + * max - heap1 and max - heap2. On the second pass, we use these values + * to equilibrate the heap space used by either branch. This is needed + * because we don't know (at compile time) which branch will be taken, + * but the heap cannot be restored inside each branch because the result + * on the heap may needed by the remaining expression. + */ + int max=(heap1>heap2) ? heap1 : heap2; + heaplist_node->first=max-heap1; + heaplist_node->second=max-heap2; + decl_heap=locheap+max; /* otherwise it will contain locheap+heap2 and the + * max. heap usage will be wrong for the upper + * expression */ + } /* if */ + assert(sc_status!=statWRITE || heap1==heap2); if (lval->ident==iARRAY) lval->ident=iREFARRAY; /* iARRAY becomes iREFARRAY */ else if (lval->ident!=iREFARRAY) lval->ident=iEXPRESSION; /* iREFARRAY stays iREFARRAY, rest becomes iEXPRESSION */ - if (orig_heap!=decl_heap) { - diff2=abs(decl_heap-orig_heap); - decl_heap=orig_heap; - } - if (diff1==diff2) { - decl_heap+=(diff1/2); - } else { - decl_heap+=(diff1+diff2); - } return FALSE; /* conditional expression is no lvalue */ } else { return lvalue; diff --git a/compiler/libpc300/sclist.c b/compiler/libpc300/sclist.c index 7d60226c..e4d36060 100755 --- a/compiler/libpc300/sclist.c +++ b/compiler/libpc300/sclist.c @@ -443,6 +443,52 @@ SC_FUNC void delete_autolisttable(void) } +/* ----- value pair list ----------------------------------------- */ +static valuepair heaplist = {NULL, 0, 0}; + +SC_FUNC valuepair *push_heaplist(long first, long second) +{ + valuepair *cur, *last; + if ((cur=malloc(sizeof(valuepair)))==NULL) + error(103); /* insufficient memory (fatal error) */ + + cur->first=first; + cur->second=second; + cur->next=NULL; + + for (last=&heaplist; last->next!=NULL; last=last->next) + /* nothing */; + last->next=cur; + return cur; +} + +SC_FUNC int popfront_heaplist(long *first, long *second) +{ + valuepair *front=heaplist.next; + if (front==NULL) + return 0; + + /* copy fields */ + *first=front->first; + *second=front->second; + + /* unlink and free */ + heaplist.next=front->next; + free(front); + return 1; +} + +SC_FUNC void delete_heaplisttable(void) +{ + valuepair *cur; + while (heaplist.next!=NULL) { + cur=heaplist.next; + heaplist.next=cur->next; + free(cur); + } /* while */ +} + + /* ----- debug information --------------------------------------- */ static stringlist dbgstrings = {NULL, NULL};