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.)

This fixes GH Issue #20826

4 files changed, 42 insertions, 24 deletions

diff --git a/regexec.c b/regexec.c
index 3d9caa5104..09b6d2fdcd 100644
--- a/regexec.c
+++ b/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);
     assert((IV)PL_savestack_max > (IV)(total_elems + REGCP_FRAME_ELEMS));
 
     /* memcpy the offs inside the stack - it's faster than for loop */
diff --git a/scope.c b/scope.c
index 6cb1433d1c..ce6837ae3b 100644
--- a/scope.c
+++ b/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,44 @@ Perl_markstack_grow(pTHX)
 void
 Perl_savestack_grow(pTHX)
 {
-    IV new_max;
-#ifdef STRESS_REALLOC
-    new_max = PL_savestack_max + SS_MAXPUSH;
-#else
-    new_max = GROW(PL_savestack_max);
-#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;
+    const I32 by = PL_savestack_max - PL_savestack_ix;
+    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);
+    /* NOTE: PL_savestack_max and PL_savestack_ix are I32.
+     *
+     * This makes sense when you consider that having I32_MAX items on
+     * the stack would be quite large.
+     *
+     * However, we use IV here so that we can detect if the new requested
+     * amount is larger than I32_MAX.
+     */
+    const IV new_floor = PL_savestack_max + need; /* what we need */
+    /* the GROW() macro normally does scales by 1.5 but under
+     * STRESS_REALLOC it simply adds 1 */
+    IV new_max         = GROW(new_floor); /* and some extra */
+
+    /* the new_max < PL_savestack_max is for cases where IV is I32
+     * and we have rolled over from I32_MAX to a small value */
+    if (new_max > I32_MAX || new_max < PL_savestack_max) {
+        if (new_floor > I32_MAX || new_floor < PL_savestack_max) {
+            Perl_croak(aTHX_ "panic: savestack overflows I32_MAX");
+        }
+        new_max = new_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 +652,7 @@ Perl_save_iv(pTHX_ IV *ivp)
 {
     PERL_ARGS_ASSERT_SAVE_IV;
 
-    SSCHECK(3);
+    SSGROW(3);
     SSPUSHIV(*ivp);
     SSPUSHPTR(ivp);
     SSPUSHUV(SAVEt_IV);
diff --git a/scope.h b/scope.h
index 4acd9a9186..21ad7202ae 100644
--- a/scope.h
+++ b/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))
diff --git a/t/re/pat.t b/t/re/pat.t
index d950c30ec9..c494434675 100644
--- a/t/re/pat.t
+++ b/t/re/pat.t
@@ -2413,8 +2413,7 @@ SKIP:
         is($y,"b","Branch reset in list context check 11 (b)");
         is($z,"z","Branch reset in list context check 12 (z)");
     }
-    TODO:{
-        local $::TODO = "Will be fixed next commit";
+    {
         # Test for GH Issue #20826. Save stack overflow introduced in
         # 92373dea9d7bcc0a017f20cb37192c1d8400767f PR #20530.
         # Note this test depends on an assert so it will only fail