scope.c - rework SSGROW() and SSCHECK() macros and undelying functions

Prior to this patch SSCHECK() took a "needs" parameter, but did not
actually guarantee that the stack would be sufficiently large to
accomodate that many elements after the call. This was quite misleading.
Especially as SSGROW() would not do geometric preallocation, but
SSCHECK() would, so much of the time SSCHECK() would appear to be a
better choice, but not always.

This patch makes it so SSCHECK() is an alias for SSGROW(), and it makes
it so that SSGROW() also geometrically overallocates. The underlying
function that used to implement SSCHECK() savestack_grow() now calls the
savestack_grow_cnt() which has always implemented SSGROW(). Anything
in the internals that used to call SSCHECK() now calls SSGROW() instead.

At the same time the preallocation has been made a little bit more
aggressive, ensuring that we always allocate at least SS_MAXPUSH
elements on top of what was requested as part of the "known" size of the
stack, and additional SS_MAXPUSH elements which are not part of the
"known" size of the stack. This "hidden extra" is used to simply some of
the other macros which are used a lot internally. (I have beefed up the
comment explaining this as well.)

See #20530 where Dave M points out the
problem with this logic. Currently I don't think we have a good way to
test for the error that he showed so unfortunately this patch lacks a
test.

Author: demerphq

regexec.c

@@ -262,7 +262,7 @@ S_regcppush(pTHX_ const regexp *rex, I32 parenfloor, U32 maxopenparen _pDEPTH)
             );
     );
 
-    SSCHECK(total_elems + REGCP_FRAME_ELEMS);
+    SSGROW(total_elems + REGCP_FRAME_ELEMS);
 
     /* memcpy the offs inside the stack - it's faster than for loop */
     memcpy(&PL_savestack[PL_savestack_ix], rex->offs + parenfloor + 1, paren_bytes_to_push);

scope.c

@@ -63,10 +63,10 @@ Perl_stack_grow(pTHX_ SV **sp, SV **p, SSize_t n)
     return PL_stack_sp;
 }
 
-#ifndef STRESS_REALLOC
-#define GROW(old) ((old) * 3 / 2)
-#else
+#ifdef STRESS_REALLOC
 #define GROW(old) ((old) + 1)
+#else
+#define GROW(old) ((old) * 3 / 2)
 #endif
 
 PERL_SI *
@@ -166,25 +166,30 @@ Perl_markstack_grow(pTHX)
 void
 Perl_savestack_grow(pTHX)
 {
-    IV new_max;
+    IV by = PL_savestack_max - PL_savestack_ix;
 #ifdef STRESS_REALLOC
-    new_max = PL_savestack_max + SS_MAXPUSH;
-#else
-    new_max = GROW(PL_savestack_max);
+    by += SS_MAXPUSH;
 #endif
-    /* Note that we allocate SS_MAXPUSH slots higher than ss_max
-     * so that SS_ADD_END(), SSGROW() etc can do a simper check */
-    Renew(PL_savestack, new_max + SS_MAXPUSH, ANY);
-    PL_savestack_max = new_max;
+    Perl_savestack_grow_cnt(aTHX_ by);
 }
 
 void
 Perl_savestack_grow_cnt(pTHX_ I32 need)
 {
-    const IV new_max = PL_savestack_ix + need;
-    /* Note that we allocate SS_MAXPUSH slots higher than ss_max
-     * so that SS_ADD_END(), SSGROW() etc can do a simper check */
-    Renew(PL_savestack, new_max + SS_MAXPUSH, ANY);
+    /* Add what they ask for, plus at least SS_MAXPUSH (currently 4) */
+    const IV new_max_floor = PL_savestack_ix + need + SS_MAXPUSH;
+    /* and then multiply the whole thing by 1.5 */
+    const IV new_max       = GROW(new_max_floor);
+
+    /* Note that we add an additional SS_MAXPUSH slots on top of
+     * PL_savestack_max so that SS_ADD_END(), SSGROW() etc can do
+     * a simper check and if necessary realloc *after* apparently
+     * overwriting the current PL_savestack_max. See scope.h.
+     *
+     * The +1 is because new_max/PL_savestack_max is the highest
+     * index, by Renew needs the number of items, which is one
+     * larger than the highest index. */
+    Renew(PL_savestack, new_max + SS_MAXPUSH + 1, ANY);
     PL_savestack_max = new_max;
 }
 
@@ -633,7 +638,7 @@ Perl_save_iv(pTHX_ IV *ivp)
 {
     PERL_ARGS_ASSERT_SAVE_IV;
 
-    SSCHECK(3);
+    SSGROW(3);
     SSPUSHIV(*ivp);
     SSPUSHPTR(ivp);
     SSPUSHUV(SAVEt_IV);

scope.h

@@ -27,8 +27,8 @@
  * macros */
 #define SS_MAXPUSH 4
 
-#define SSCHECK(need) if (UNLIKELY(PL_savestack_ix + (I32)(need) > PL_savestack_max)) savestack_grow()
 #define SSGROW(need) if (UNLIKELY(PL_savestack_ix + (I32)(need) > PL_savestack_max)) savestack_grow_cnt(need)
+#define SSCHECK(need) SSGROW(need) /* legacy */
 #define SSPUSHINT(i) (PL_savestack[PL_savestack_ix++].any_i32 = (I32)(i))
 #define SSPUSHLONG(i) (PL_savestack[PL_savestack_ix++].any_long = (long)(i))
 #define SSPUSHBOOL(p) (PL_savestack[PL_savestack_ix++].any_bool = (p))
@@ -47,7 +47,7 @@
  * like save_pushptrptr() to half its former size.
  * Of course, doing the size check *after* pushing means we must always
  * ensure there are SS_MAXPUSH free slots on the savestack. This is ensured by
- * savestack_grow() and savestack_grow_cnt always allocating SS_MAXPUSH slots
+ * savestack_grow_cnt always allocating SS_MAXPUSH slots
  * more than asked for, or that it sets PL_savestack_max to
  *
  * These are for internal core use only and are subject to change */
@@ -61,7 +61,7 @@
     ix += (need);                                               \
     PL_savestack_ix = ix;                                       \
     assert(ix <= PL_savestack_max + SS_MAXPUSH);                \
-    if (UNLIKELY(ix > PL_savestack_max)) savestack_grow();      \
+    if (UNLIKELY(ix > PL_savestack_max)) savestack_grow_cnt(ix - PL_savestack_max);      \
     assert(PL_savestack_ix <= PL_savestack_max);
 
 #define SS_ADD_INT(i)   ((ssp++)->any_i32 = (I32)(i))