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

@@ -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);

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

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;

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};