Skip to content

Commit

Permalink
RC_STACK asserts without DEBUG_LEAKING_SCALARS
Browse files Browse the repository at this point in the history
An earlier commit added some asserts to the old-style and new-style
stack push/pop functions such as PUSHs() or rpp_popfree_1().  These
assert that the stack is reference-counted or not as appropriate.

These asserts were enabled only if perl was built with all of

    DEBUGGING && PERL_RC_STACK && DEBUG_LEAKING_SCALARS.

This commit makes the asserts available under just

    DEBUGGING && PERL_RC_STACK

Initially  I was worried about the performance implications (you expect
DEBUGGING builds to be slow, but not *too* slow), but they don't seem
too bad. So by making them more likely to be enabled, they're more
likely to help people spot code that needs fixing (e.g. code still doing
POPs when the stack is reference counted and the function hasn't been
wrapped).
  • Loading branch information
iabyn committed Aug 4, 2023
1 parent e96dd01 commit 14148aa
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 27 deletions.
22 changes: 2 additions & 20 deletions inline.h
Original file line number Diff line number Diff line change
Expand Up @@ -439,9 +439,7 @@ Perl_rpp_popfree_to(pTHX_ SV **sp)

assert(sp <= PL_stack_sp);
#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
while (PL_stack_sp > sp) {
SV *sv = *PL_stack_sp--;
SvREFCNT_dec(sv);
Expand All @@ -466,9 +464,7 @@ Perl_rpp_popfree_1(pTHX)
PERL_ARGS_ASSERT_RPP_POPFREE_1;

#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
SV *sv = *PL_stack_sp--;
SvREFCNT_dec(sv);
#else
Expand All @@ -493,9 +489,7 @@ Perl_rpp_popfree_2(pTHX)
PERL_ARGS_ASSERT_RPP_POPFREE_2;

#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
for (int i = 0; i < 2; i++) {
SV *sv = *PL_stack_sp--;
SvREFCNT_dec(sv);
Expand Down Expand Up @@ -534,9 +528,7 @@ Perl_rpp_pop_1_norc(pTHX)
#ifndef PERL_RC_STACK
SvREFCNT_inc(sv);
#else
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
#endif
return sv;
}
Expand All @@ -563,9 +555,7 @@ Perl_rpp_push_1(pTHX_ SV *sv)

*++PL_stack_sp = sv;
#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
SvREFCNT_inc_simple_void_NN(sv);
#endif
}
Expand All @@ -578,9 +568,7 @@ Perl_rpp_push_2(pTHX_ SV *sv1, SV *sv2)
*++PL_stack_sp = sv1;
*++PL_stack_sp = sv2;
#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
SvREFCNT_inc_simple_void_NN(sv1);
SvREFCNT_inc_simple_void_NN(sv2);
#endif
Expand Down Expand Up @@ -624,9 +612,7 @@ Perl_rpp_push_1_norc(pTHX_ SV *sv)

*++PL_stack_sp = sv;
#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
#else
sv_2mortal(sv);
#endif
Expand All @@ -649,9 +635,7 @@ Perl_rpp_replace_1_1(pTHX_ SV *sv)
PERL_ARGS_ASSERT_RPP_REPLACE_1_1;

#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
SV *oldsv = *PL_stack_sp;
*PL_stack_sp = sv;
SvREFCNT_inc_simple_void_NN(sv);
Expand All @@ -678,9 +662,7 @@ Perl_rpp_replace_2_1(pTHX_ SV *sv)
PERL_ARGS_ASSERT_RPP_REPLACE_2_1;

#ifdef PERL_RC_STACK
# ifdef DEBUG_LEAKING_SCALARS
assert(rpp_stack_is_rc());
# endif
/* replace PL_stack_sp[-1] first; leave PL_stack_sp[0] in place while
* we free [-1], so if an exception occurs, [0] will still be freed.
*/
Expand Down Expand Up @@ -767,14 +749,14 @@ context.
PERL_STATIC_INLINE bool
Perl_rpp_is_lone(pTHX_ SV *sv)
{
#if defined(PERL_RC_STACK) && defined(DEBUG_LEAKING_SCALARS)
#ifdef PERL_RC_STACK
/* note that rpp_is_lone() can be used in wrapped pp functions,
* where technically the stack is no longer ref-counted; but because
* the args are non-RC copies of RC args further down the stack, we
* can't be in a *completely* non-ref stack.
*/
assert(AvREAL(PL_curstack));
# endif
#endif

return SvREFCNT(sv) <= cBOOL(SvTEMP(sv))
#ifdef PERL_RC_STACK
Expand Down
9 changes: 4 additions & 5 deletions pod/perlguts.pod
Original file line number Diff line number Diff line change
Expand Up @@ -4477,8 +4477,7 @@ API.
A reference-counted perl can be built using the PERL_RC_STACK define.
For development and debugging purposes, it is best to enable leaking
scalar debugging too, as that displays extra information about scalars
that have leaked or been prematurely freed, and it also enables extra
assertions in the macros and functions which manipulate the stack:
that have leaked or been prematurely freed.

Configure -DDEBUGGING \
-Accflags='-DPERL_RC_STACK -DDEBUG_LEAKING_SCALARS'
Expand Down Expand Up @@ -4896,9 +4895,9 @@ call_sv() is called from a standard PP function (rpp_stack_is_rc() is
true) or from a wrapped PP or XS function (rpp_stack_is_rc() is false).
Note that you're unlikely to need to use this function, as in most places,
such as PP or XS functions, it is always RC or non-RC respectively. In
fact, under C<DEBUG_LEAKING_SCALARS>, PUSHs() and similar macros include
an C<assert(!rpp_stack_is_rc())>, while rpp_push_1() and similar functions
have C<assert(rpp_stack_is_rc())>.
fact on debugging builds under C<PERL_RC_STACK>, PUSHs() and similar
macros include an C<assert(!rpp_stack_is_rc())>, while rpp_push_1() and
similar functions have C<assert(rpp_stack_is_rc())>.

The macros for pushing new stackinfos have been replaced with inline
functions which don't rely on C<dSP> being in scope, and which have less
Expand Down
4 changes: 2 additions & 2 deletions pp.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ Pops an unsigned long off the stack.
#define RETURNOP(o) return (PUTBACK, o)
#define RETURNX(x) return (x, PUTBACK, NORMAL)

#if defined(PERL_RC_STACK) && defined(DEBUG_LEAKING_SCALARS)
#ifdef PERL_RC_STACK
# define POPs (assert(!rpp_stack_is_rc()), *sp--)
#else
# define POPs (*sp--)
Expand Down Expand Up @@ -543,7 +543,7 @@ Does not use C<TARG>. See also C<L</XPUSHu>>, C<L</mPUSHu>> and C<L</PUSHu>>.
sv_setnv_mg(targ, TARGn_nv); \
} STMT_END

#if defined(PERL_RC_STACK) && defined(DEBUG_LEAKING_SCALARS)
#ifdef PERL_RC_STACK
# define PUSHs(s) (assert(!rpp_stack_is_rc()), *++sp = (s))
#else
# define PUSHs(s) (*++sp = (s))
Expand Down

0 comments on commit 14148aa

Please sign in to comment.