Rename HE_SVSLOT to HE_ARENA_ROOT_IX

The longer name more accurately reflects what the constant refers to.

Correct the comments describing how some arena roots are re-used.

5 files changed, 35 insertions, 17 deletions

diff --git a/ext/XS-APItest/APItest.pm b/ext/XS-APItest/APItest.pm
index 334d7ebf37..1abb45f685 100644
--- a/ext/XS-APItest/APItest.pm
+++ b/ext/XS-APItest/APItest.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use Carp;
 
-our $VERSION = '1.19';
+our $VERSION = '1.20';
 
 require XSLoader;
 
diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs
index 83244ce2d9..2b5aeee5fa 100644
--- a/ext/XS-APItest/APItest.xs
+++ b/ext/XS-APItest/APItest.xs
@@ -202,10 +202,10 @@ test_freeent(freeent_function *f) {
 
     /* We need to "inline" new_he here as it's static, and the functions we
        test expect to be able to call del_HE on the HE  */
-    if (!PL_body_roots[HE_SVSLOT])
+    if (!PL_body_roots[HE_ARENA_ROOT_IX])
         croak("PL_he_root is 0");
-    victim = (HE*) PL_body_roots[HE_SVSLOT];
-    PL_body_roots[HE_SVSLOT] = HeNEXT(victim);
+    victim = (HE*) PL_body_roots[HE_ARENA_ROOT_IX];
+    PL_body_roots[HE_ARENA_ROOT_IX] = HeNEXT(victim);
 #endif
 
     victim->hent_hek = Perl_share_hek(aTHX_ "", 0, 0);
diff --git a/hv.c b/hv.c
index bec737ebf6..954eb493d6 100644
--- a/hv.c
+++ b/hv.c
@@ -60,10 +60,10 @@ STATIC HE*
 S_new_he(pTHX)
 {
     HE* he;
-    void ** const root = &PL_body_roots[HE_SVSLOT];
+    void ** const root = &PL_body_roots[HE_ARENA_ROOT_IX];
 
     if (!*root)
-        Perl_more_bodies(aTHX_ HE_SVSLOT, sizeof(HE), PERL_ARENA_SIZE);
+        Perl_more_bodies(aTHX_ HE_ARENA_ROOT_IX, sizeof(HE), PERL_ARENA_SIZE);
     he = (HE*) *root;
     assert(he);
     *root = HeNEXT(he);
@@ -73,8 +73,8 @@ S_new_he(pTHX)
 #define new_HE() new_he()
 #define del_HE(p) \
     STMT_START { \
-        HeNEXT(p) = (HE*)(PL_body_roots[HE_SVSLOT]);	\
-        PL_body_roots[HE_SVSLOT] = p; \
+        HeNEXT(p) = (HE*)(PL_body_roots[HE_ARENA_ROOT_IX]);	\
+        PL_body_roots[HE_ARENA_ROOT_IX] = p; \
     } STMT_END
 
 
diff --git a/sv.c b/sv.c
index 15b05f0795..1e6f3cad68 100644
--- a/sv.c
+++ b/sv.c
@@ -865,7 +865,7 @@ are used for this, except for arena_size.
 For the sv-types that have no bodies, arenas are not used, so those
 PL_body_roots[sv_type] are unused, and can be overloaded.  In
 something of a special case, SVt_NULL is borrowed for HE arenas;
-PL_body_roots[HE_SVSLOT=SVt_NULL] is filled by S_more_he, but the
+PL_body_roots[HE_ARENA_ROOT_IX=SVt_NULL] is filled by S_more_he, but the
 bodies_by_type[SVt_NULL] slot is not used, as the table is not
 available in hv.c.
 
@@ -6654,9 +6654,29 @@ Perl_sv_clear(pTHX_ SV *const orig_sv)
         assert(SvTYPE(sv) != (svtype)SVTYPEMASK);
 
         if (type <= SVt_IV) {
-            /* See the comment in sv.h about the collusion between this
-             * early return and the overloading of the NULL slots in the
-             * size table.  */
+            /* Historically this check on type was needed so that the code to
+             * free bodies wasn't reached for these types, because the arena
+             * slots were re-used for HEs and pointer table entries. The
+             * metadata table `bodies_by_type` had the information for the sizes
+             * for HEs and PTEs, hence the code here had to have a special-case
+             * check to ensure that the "regular" body freeing code wasn't
+             * reached, and get confused by the "lies" in `bodies_by_type`.
+             *
+             * However, it hasn't actually been needed for that reason since
+             * Aug 2010 (commit 829cd18aa7f45221), because `bodies_by_type` was
+             * changed to always hold the accurate metadata for the SV types.
+             * This was possible because PTEs were no longer allocated from the
+             * "SVt_IV" arena, and the code to allocate HEs from the "SVt_NULL"
+             * arena is entirely in hv.c, so doesn't access the table.
+             *
+             * Some sort of check is still needed to handle SVt_IVs - pure RVs
+             * need to take one code path which is common with RVs stored in
+             * SVt_PV (or larger), but pure IVs mustn't take the "PV but not RV"
+             * path, as SvPVX() doesn't point to valid memory.
+             *
+             * Hence this code is still the most efficient way to handle this.
+             */
+
             if (SvROK(sv))
                 goto free_rv;
             SvFLAGS(sv) &= SVf_BREAK;
diff --git a/sv.h b/sv.h
index ef901f9da8..83fa92380b 100644
--- a/sv.h
+++ b/sv.h
@@ -173,12 +173,10 @@ typedef enum {
 #  define SVt_RV	SVt_IV
 #endif
 
-/* There is collusion here with sv_clear - sv_clear exits early for SVt_NULL
-   so never reaches the clause at the end that uses sv_type_details->body_size
-   to determine whether to call safefree(). Hence body_size can be set
-   non-zero to record the size of HEs, without fear of bogus frees.  */
+/* The array of arena roots for SV bodies is indexed by SvTYPE. SVt_NULL doesn't
+ * use a body, so that arena root is re-used for HEs. */
 #if defined(PERL_IN_HV_C) || defined(PERL_IN_XS_APITEST)
-#define HE_SVSLOT	SVt_NULL
+#  define HE_ARENA_ROOT_IX      SVt_NULL
 #endif
 #ifdef PERL_IN_SV_C
 #  define SVt_FIRST SVt_NULL	/* the type of SV that new_SV() in sv.c returns */