Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH 0/4] RFC op-type patchset #14203

Open
p5pRT opened this issue Nov 2, 2014 · 30 comments
Open

[PATCH 0/4] RFC op-type patchset #14203

p5pRT opened this issue Nov 2, 2014 · 30 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Nov 2, 2014

Migrated from rt.perl.org#123105 (status was 'open')

Searchable as RT123105$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, thus "promoting" its use as the "normal" way to do
so. One of its goals is that explicitly setting o->op_ppaddr or
o->op_type should indicate something special.

Patch 1 uses CHANGE_TYPE in 49 callsites, ie "normal" init.
Patch 2 removes pp_mapstart trickery, with 1 line in op.c
Patch 3 reduces variation in OP_AELEMFAST setup.
  the de-optimization is infinitesimal, and fixed by compilers.
Patch 4 folds pp_opaddr setup into S_alloc_LOGOP.
Patch 5 adds single CHANGE_TYPE where inits were separated by code.

TODO​: CHANGE_TYPE would properly apply in a few more spots;

perl.c could use it once, if it were hoisted into op.h.

op.c could use it 2x, but for a macro/sequence-point issue with ++(o->op_type).
Id lean toward a temp var in callers, rather than in macro.

pp.c could use it after patching OP_I_MODULO, but its a kinda special
case anyway (aside, its not clear why bugtest is in runtime).

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, added 4 years ago. In doing so it "promotes" its
use as the "normal" way to do so, one of its goals is that setting
o->op_ppaddr should indicate something special.

Patch 1 uses CHANGE_TYPE in 49 callsites.
Patch 2 removes pp_mapstart trickery, with 1 line in op.c
Patch 3 reduces variation in OP_AELEMFAST setup
Patch 4 folds pp_opaddr setup into S_alloc_LOGOP

TODO​: CHANGE_TYPE would properly apply in a few more spots;
perl.c could use it once, if it were hoisted out of op.c (into op.h)
op.c could use it, but for a macro/sequence-point issue with ++(o->op_type).

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

CHANGE_TYPE has been around for 4 years, since 23a8159b759. Use it to
encapsulate "standard" opcode setting, where the postcondition​:
o->op_ppaddr == PL_ppaddr[o->op_type] holds. This leaves only a
handful of op_type, op_ppaddr refs where the invariant is not true.


op.c | 147 +++++++++++++++++++++++--------------------------------------------
1 file changed, 49 insertions(+), 98 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index 397e3f1..dbb2886 100644
--- a/op.c
+++ b/op.c
@@ -1011,8 +1011,7 @@ Perl_op_null(pTHX_ OP *o)
 	return;
     op_clear(o);
     o->op_targ = o->op_type;
-    o->op_type = OP_NULL;
-    o->op_ppaddr = PL_ppaddr[OP_NULL];
+    CHANGE_TYPE(o, OP_NULL);
 }
 
 void
@@ -1771,23 +1770,19 @@ Perl_scalarvoid(pTHX_ OP *o)
 	break;
 
     case OP_POSTINC:
-	o->op_type = OP_PREINC;		/* pre-increment is faster */
-	o->op_ppaddr = PL_ppaddr[OP_PREINC];
+        CHANGE_TYPE(o, OP_PREINC);	/* pre-increment is faster */
 	break;
 
     case OP_POSTDEC:
-	o->op_type = OP_PREDEC;		/* pre-decrement is faster */
-	o->op_ppaddr = PL_ppaddr[OP_PREDEC];
+        CHANGE_TYPE(o, OP_PREDEC);	/* pre-decrement is faster */
 	break;
 
     case OP_I_POSTINC:
-	o->op_type = OP_I_PREINC;	/* pre-increment is faster */
-	o->op_ppaddr = PL_ppaddr[OP_I_PREINC];
+        CHANGE_TYPE(o, OP_I_PREINC);	/* pre-increment is faster */
 	break;
 
     case OP_I_POSTDEC:
-	o->op_type = OP_I_PREDEC;	/* pre-decrement is faster */
-	o->op_ppaddr = PL_ppaddr[OP_I_PREDEC];
+        CHANGE_TYPE(o, OP_I_PREDEC);	/* pre-decrement is faster */
 	break;
 
     case OP_SASSIGN: {
@@ -1844,11 +1839,9 @@ Perl_scalarvoid(pTHX_ OP *o)
 	if (kid->op_type == OP_NOT
 	    && (kid->op_flags & OPf_KIDS)) {
 	    if (o->op_type == OP_AND) {
-		o->op_type = OP_OR;
-		o->op_ppaddr = PL_ppaddr[OP_OR];
+                CHANGE_TYPE(o, OP_OR);
 	    } else {
-		o->op_type = OP_AND;
-		o->op_ppaddr = PL_ppaddr[OP_AND];
+                CHANGE_TYPE(o, OP_AND);
 	    }
 	    op_null(kid);
 	}
@@ -2401,8 +2394,7 @@ S_lvref(pTHX_ OP *o, I32 type)
 		return;
 	    }
 	  slurpy:
-	    o->op_type = OP_LVAVREF;
-	    o->op_ppaddr = PL_ppaddr[OP_LVAVREF];
+            CHANGE_TYPE(o, OP_LVAVREF);
 	    o->op_private &= OPpLVAL_INTRO|OPpPAD_STATE;
 	    o->op_flags |= OPf_MOD|OPf_REF;
 	    return;
@@ -2459,8 +2451,7 @@ S_lvref(pTHX_ OP *o, I32 type)
 	break;
     case OP_ASLICE:
     case OP_HSLICE:
-	o->op_type = OP_LVREFSLICE;
-	o->op_ppaddr = PL_ppaddr[OP_LVREFSLICE];
+        CHANGE_TYPE(o, OP_LVREFSLICE);
 	o->op_private &= OPpLVAL_INTRO|OPpLVREF_ELEM;
 	return;
     case OP_NULL:
@@ -2493,8 +2484,7 @@ S_lvref(pTHX_ OP *o, I32 type)
 		     PL_op_desc[type]));
 	return;
     }
-    o->op_type = OP_LVREF;
-    o->op_ppaddr = PL_ppaddr[OP_LVREF];
+    CHANGE_TYPE(o, OP_LVREF);
     o->op_private &=
 	OPpLVAL_INTRO|OPpLVREF_ELEM|OPpLVREF_TYPE|OPpPAD_STATE;
     if (type == OP_ENTERLOOP)
@@ -2533,8 +2523,7 @@ Perl_op_lvalue_flags(pTHX_ OP *o, I32 type, U32 flags)
     case OP_ENTERSUB:
 	if ((type == OP_UNDEF || type == OP_REFGEN || type == OP_LOCK) &&
 	    !(o->op_flags & OPf_STACKED)) {
-	    o->op_type = OP_RV2CV;		/* entersub => rv2cv */
-	    o->op_ppaddr = PL_ppaddr[OP_RV2CV];
+            CHANGE_TYPE(o, OP_RV2CV);		/* entersub => rv2cv */
 	    assert(cUNOPo->op_first->op_type == OP_NULL);
 	    op_null(((LISTOP*)cUNOPo->op_first)->op_first);/* disable pushmark */
 	    break;
@@ -2996,9 +2985,8 @@ Perl_doref(pTHX_ OP *o, I32 type, bool set_op_ref)
     case OP_ENTERSUB:
 	if ((type == OP_EXISTS || type == OP_DEFINED) &&
 	    !(o->op_flags & OPf_STACKED)) {
-	    o->op_type = OP_RV2CV;             /* entersub => rv2cv */
-	    o->op_ppaddr = PL_ppaddr[OP_RV2CV];
-	    assert(cUNOPo->op_first->op_type == OP_NULL);
+            CHANGE_TYPE(o, OP_RV2CV);             /* entersub => rv2cv */
+            assert(cUNOPo->op_first->op_type == OP_NULL);
 	    op_null(((LISTOP*)cUNOPo->op_first)->op_first);	/* disable pushmark */
 	    o->op_flags |= OPf_SPECIAL;
 	}
@@ -3575,13 +3563,11 @@ Perl_op_scope(pTHX_ OP *o)
     if (o) {
 	if (o->op_flags & OPf_PARENS || PERLDB_NOOPT || TAINTING_get) {
 	    o = op_prepend_elem(OP_LINESEQ, newOP(OP_ENTER, 0), o);
-	    o->op_type = OP_LEAVE;
-	    o->op_ppaddr = PL_ppaddr[OP_LEAVE];
+            CHANGE_TYPE(o, OP_LEAVE);
 	}
 	else if (o->op_type == OP_LINESEQ) {
 	    OP *kid;
-	    o->op_type = OP_SCOPE;
-	    o->op_ppaddr = PL_ppaddr[OP_SCOPE];
+            CHANGE_TYPE(o, OP_SCOPE);
 	    kid = ((LISTOP*)o)->op_first;
 	    if (kid->op_type == OP_NEXTSTATE || kid->op_type == OP_DBSTATE) {
 		op_null(kid);
@@ -4156,8 +4142,7 @@ S_gen_constant_list(pTHX_ OP *o)
     Perl_pp_anonlist(aTHX);
     PL_tmps_floor = oldtmps_floor;
 
-    o->op_type = OP_RV2AV;
-    o->op_ppaddr = PL_ppaddr[OP_RV2AV];
+    CHANGE_TYPE(o, OP_RV2AV);
     o->op_flags &= ~OPf_REF;	/* treat \(1..2) like an ordinary list */
     o->op_flags |= OPf_PARENS;	/* and flatten \(1..2,3) */
     o->op_opt = 0;		/* needs to be revisited in rpeep() */
@@ -4332,8 +4317,7 @@ Perl_op_convert_list(pTHX_ I32 type, I32 flags, OP *o)
 	}
     }
 
-    o->op_type = (OPCODE)type;
-    o->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(o, type);
     o->op_flags |= flags;
 
     o = CHECKOP(type, o);
@@ -4418,8 +4402,7 @@ Perl_newLISTOP(pTHX_ I32 type, I32 flags, OP *first, OP *last)
 
     NewOp(1101, listop, 1, LISTOP);
 
-    listop->op_type = (OPCODE)type;
-    listop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(listop, type);
     if (first || last)
 	flags |= OPf_KIDS;
     listop->op_flags = (U8)flags;
@@ -4481,8 +4464,7 @@ Perl_newOP(pTHX_ I32 type, I32 flags)
 	|| (PL_opargs[type] & OA_CLASS_MASK) == OA_LOOPEXOP);
 
     NewOp(1101, o, 1, OP);
-    o->op_type = (OPCODE)type;
-    o->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(o, type);
     o->op_flags = (U8)flags;
 
     o->op_next = o;
@@ -4533,8 +4515,7 @@ Perl_newUNOP(pTHX_ I32 type, I32 flags, OP *first)
 	first = force_list(first, 1);
 
     NewOp(1101, unop, 1, UNOP);
-    unop->op_type = (OPCODE)type;
-    unop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(unop, type);
     unop->op_first = first;
     unop->op_flags = (U8)(flags | OPf_KIDS);
     unop->op_private = (U8)(1 | (flags >> 8));
@@ -4588,8 +4569,7 @@ S_newMETHOP_internal(pTHX_ I32 type, I32 flags, OP* dynamic_meth, SV* const_meth
         methop->op_next = (OP*)methop;
     }
 
-    methop->op_type = (OPCODE)type;
-    methop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(methop, type);
     methop = (METHOP*) CHECKOP(type, methop);
 
     if (methop->op_next) return (OP*)methop;
@@ -4650,8 +4630,7 @@ Perl_newBINOP(pTHX_ I32 type, I32 flags, OP *first, OP *last)
     if (!first)
 	first = newOP(OP_NULL, 0);
 
-    binop->op_type = (OPCODE)type;
-    binop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(binop, type);
     binop->op_first = first;
     binop->op_flags = (U8)(flags | OPf_KIDS);
     if (!last) {
@@ -5045,8 +5024,7 @@ Perl_newPMOP(pTHX_ I32 type, I32 flags)
     assert((PL_opargs[type] & OA_CLASS_MASK) == OA_PMOP);
 
     NewOp(1101, pmop, 1, PMOP);
-    pmop->op_type = (OPCODE)type;
-    pmop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(pmop, type);
     pmop->op_flags = (U8)flags;
     pmop->op_private = (U8)(0 | (flags >> 8));
 
@@ -5493,8 +5471,7 @@ Perl_newSVOP(pTHX_ I32 type, I32 flags, SV *sv)
 	|| (PL_opargs[type] & OA_CLASS_MASK) == OA_FILESTATOP);
 
     NewOp(1101, svop, 1, SVOP);
-    svop->op_type = (OPCODE)type;
-    svop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(svop, type);
     svop->op_sv = sv;
     svop->op_next = (OP*)svop;
     svop->op_flags = (U8)flags;
@@ -5559,8 +5536,7 @@ Perl_newPADOP(pTHX_ I32 type, I32 flags, SV *sv)
 	|| (PL_opargs[type] & OA_CLASS_MASK) == OA_FILESTATOP);
 
     NewOp(1101, padop, 1, PADOP);
-    padop->op_type = (OPCODE)type;
-    padop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(padop, type);
     padop->op_padix =
 	pad_alloc(type, isGV(sv) ? SVf_READONLY : SVs_PADTMP);
     SvREFCNT_dec(PAD_SVl(padop->op_padix));
@@ -5627,8 +5603,7 @@ Perl_newPVOP(pTHX_ I32 type, I32 flags, char *pv)
 	|| (PL_opargs[type] & OA_CLASS_MASK) == OA_LOOPEXOP);
 
     NewOp(1101, pvop, 1, PVOP);
-    pvop->op_type = (OPCODE)type;
-    pvop->op_ppaddr = PL_ppaddr[type];
+    CHANGE_TYPE(pvop, type);
     pvop->op_pv = pv;
     pvop->op_next = (OP*)pvop;
     pvop->op_flags = (U8)flags;
@@ -6388,12 +6363,10 @@ Perl_newSTATEOP(pTHX_ I32 flags, char *label, OP *o)
 
     NewOp(1101, cop, 1, COP);
     if (PERLDB_LINE && CopLINE(PL_curcop) && PL_curstash != PL_debstash) {
-	cop->op_type = OP_DBSTATE;
-	cop->op_ppaddr = PL_ppaddr[ OP_DBSTATE ];
+        CHANGE_TYPE(cop, OP_DBSTATE);
     }
     else {
-	cop->op_type = OP_NEXTSTATE;
-	cop->op_ppaddr = PL_ppaddr[ OP_NEXTSTATE ];
+        CHANGE_TYPE(cop, OP_NEXTSTATE);
     }
     cop->op_flags = (U8)flags;
     CopHINTS_set(cop, PL_hints);
@@ -7060,8 +7033,7 @@ Perl_newWHILEOP(pTHX_ I32 flags, I32 debuggable, LOOP *loop,
 
     if (!loop) {
 	NewOp(1101,loop,1,LOOP);
-	loop->op_type = OP_ENTERLOOP;
-	loop->op_ppaddr = PL_ppaddr[OP_ENTERLOOP];
+        CHANGE_TYPE(loop, OP_ENTERLOOP);
 	loop->op_private = 0;
 	loop->op_next = (OP*)loop;
     }
@@ -7120,8 +7092,7 @@ Perl_newFOROP(pTHX_ I32 flags, OP *sv, OP *expr, OP *block, OP *cont)
     if (sv) {
 	if (sv->op_type == OP_RV2SV) {	/* symbol table variable */
 	    iterpflags = sv->op_private & OPpOUR_INTRO; /* for our $x () */
-	    sv->op_type = OP_RV2GV;
-	    sv->op_ppaddr = PL_ppaddr[OP_RV2GV];
+            CHANGE_TYPE(sv, OP_RV2GV);
 
 	    /* The op_type check is needed to prevent a possible segfault
 	     * if the loop variable is undeclared and 'strict vars' is in
@@ -8957,14 +8928,12 @@ Perl_oopsAV(pTHX_ OP *o)
     switch (o->op_type) {
     case OP_PADSV:
     case OP_PADHV:
-	o->op_type = OP_PADAV;
-	o->op_ppaddr = PL_ppaddr[OP_PADAV];
+        CHANGE_TYPE(o, OP_PADAV);
 	return ref(o, OP_RV2AV);
 
     case OP_RV2SV:
     case OP_RV2HV:
-	o->op_type = OP_RV2AV;
-	o->op_ppaddr = PL_ppaddr[OP_RV2AV];
+        CHANGE_TYPE(o, OP_RV2AV);
 	ref(o, OP_RV2AV);
 	break;
 
@@ -8985,14 +8954,12 @@ Perl_oopsHV(pTHX_ OP *o)
     switch (o->op_type) {
     case OP_PADSV:
     case OP_PADAV:
-	o->op_type = OP_PADHV;
-	o->op_ppaddr = PL_ppaddr[OP_PADHV];
+        CHANGE_TYPE(o, OP_PADHV);
 	return ref(o, OP_RV2HV);
 
     case OP_RV2SV:
     case OP_RV2AV:
-	o->op_type = OP_RV2HV;
-	o->op_ppaddr = PL_ppaddr[OP_RV2HV];
+        CHANGE_TYPE(o, OP_RV2HV);
 	ref(o, OP_RV2HV);
 	break;
 
@@ -9011,8 +8978,7 @@ Perl_newAVREF(pTHX_ OP *o)
     PERL_ARGS_ASSERT_NEWAVREF;
 
     if (o->op_type == OP_PADANY) {
-	o->op_type = OP_PADAV;
-	o->op_ppaddr = PL_ppaddr[OP_PADAV];
+        CHANGE_TYPE(o, OP_PADAV);
 	return o;
     }
     else if ((o->op_type == OP_RV2AV || o->op_type == OP_PADAV)) {
@@ -9037,8 +9003,7 @@ Perl_newHVREF(pTHX_ OP *o)
     PERL_ARGS_ASSERT_NEWHVREF;
 
     if (o->op_type == OP_PADANY) {
-	o->op_type = OP_PADHV;
-	o->op_ppaddr = PL_ppaddr[OP_PADHV];
+        CHANGE_TYPE(o, OP_PADHV);
 	return o;
     }
     else if ((o->op_type == OP_RV2HV || o->op_type == OP_PADHV)) {
@@ -9052,8 +9017,7 @@ Perl_newCVREF(pTHX_ I32 flags, OP *o)
 {
     if (o->op_type == OP_PADANY) {
 	dVAR;
-	o->op_type = OP_PADCV;
-	o->op_ppaddr = PL_ppaddr[OP_PADCV];
+        CHANGE_TYPE(o, OP_PADCV);
     }
     return newUNOP(OP_RV2CV, flags, scalar(o));
 }
@@ -9066,8 +9030,7 @@ Perl_newSVREF(pTHX_ OP *o)
     PERL_ARGS_ASSERT_NEWSVREF;
 
     if (o->op_type == OP_PADANY) {
-	o->op_type = OP_PADSV;
-	o->op_ppaddr = PL_ppaddr[OP_PADSV];
+        CHANGE_TYPE(o, OP_PADSV);
 	return o;
     }
     return newUNOP(OP_RV2SV, 0, scalar(o));
@@ -9352,8 +9315,7 @@ Perl_ck_eval(pTHX_ OP *o)
 	    enter->op_next = (OP*)enter;
 
 	    o = op_prepend_elem(OP_LINESEQ, (OP*)enter, (OP*)kid);
-	    o->op_type = OP_LEAVETRY;
-	    o->op_ppaddr = PL_ppaddr[OP_LEAVETRY];
+            CHANGE_TYPE(o, OP_LEAVETRY);
 	    enter->op_other = o;
 	    return o;
 	}
@@ -10079,12 +10041,10 @@ Perl_ck_smartmatch(pTHX_ OP *o)
 	
 	/* Implicitly take a reference to a regular expression */
 	if (first->op_type == OP_MATCH) {
-	    first->op_type = OP_QR;
-	    first->op_ppaddr = PL_ppaddr[OP_QR];
+            CHANGE_TYPE(first, OP_QR);
 	}
 	if (second->op_type == OP_MATCH) {
-	    second->op_type = OP_QR;
-	    second->op_ppaddr = PL_ppaddr[OP_QR];
+            CHANGE_TYPE(second, OP_QR);
         }
     }
     
@@ -10145,8 +10105,7 @@ Perl_ck_sassign(pTHX_ OP *o)
 	    OP *const nullop = newCONDOP(0, first, o, other);
 	    OP *const condop = first->op_next;
 
-	    condop->op_type = OP_ONCE;
-	    condop->op_ppaddr = PL_ppaddr[OP_ONCE];
+            CHANGE_TYPE(condop, OP_ONCE);
 	    other->op_targ = target;
 
 	    /* Store the initializedness of state vars in a separate
@@ -10460,8 +10419,7 @@ Perl_ck_select(pTHX_ OP *o)
     if (o->op_flags & OPf_KIDS) {
 	kid = OP_SIBLING(cLISTOPo->op_first);	/* get past pushmark */
 	if (kid && OP_HAS_SIBLING(kid)) {
-	    o->op_type = OP_SSELECT;
-	    o->op_ppaddr = PL_ppaddr[OP_SSELECT];
+            CHANGE_TYPE(o, OP_SSELECT);
 	    o = ck_fun(o);
 	    return fold_constants(op_integerize(op_std_init(o)));
 	}
@@ -10715,9 +10673,7 @@ Perl_ck_split(pTHX_ OP *o)
         kid = pmruntime( newPMOP(OP_MATCH, OPf_SPECIAL), kid, 0, 0);
         op_sibling_splice(o, NULL, 0, kid);
     }
-
-    kid->op_type = OP_PUSHRE;
-    kid->op_ppaddr = PL_ppaddr[OP_PUSHRE];
+    CHANGE_TYPE(kid, OP_PUSHRE);
     scalar(kid);
     if (((PMOP *)kid)->op_pmflags & PMf_GLOBAL) {
       Perl_ck_warner(aTHX_ packWARN(WARN_REGEXP),
@@ -12055,8 +12011,7 @@ Perl_rpeep(pTHX_ OP *o)
                     o->op_flags &=~ OPf_KIDS;
                     /* stub is a baseop; repeat is a binop */
                     assert(sizeof(OP) <= sizeof(BINOP));
-                    o->op_type = OP_STUB;
-                    o->op_ppaddr = PL_ppaddr[OP_STUB];
+                    CHANGE_TYPE(o, OP_STUB);
                     o->op_private = 0;
                     break;
                 }
@@ -12284,8 +12239,7 @@ Perl_rpeep(pTHX_ OP *o)
                  * *always* formerly a pushmark */
                 assert(o->op_type == OP_PUSHMARK);
                 o->op_next = followop;
-                o->op_type = OP_PADRANGE;
-                o->op_ppaddr = PL_ppaddr[OP_PADRANGE];
+                CHANGE_TYPE(o, OP_PADRANGE);
                 o->op_targ = base;
                 /* bit 7: INTRO; bit 6..0: count */
                 o->op_private = (intro | count);
@@ -12370,8 +12324,7 @@ Perl_rpeep(pTHX_ OP *o)
 		    o->op_private |= o->op_next->op_private & (OPpLVAL_INTRO
 							       | OPpOUR_INTRO);
 		    o->op_next = o->op_next->op_next;
-		    o->op_type = OP_GVSV;
-		    o->op_ppaddr = PL_ppaddr[OP_GVSV];
+                    CHANGE_TYPE(o, OP_GVSV);
 		}
 	    }
 	    else if (o->op_next->op_type == OP_READLINE
@@ -12379,9 +12332,8 @@ Perl_rpeep(pTHX_ OP *o)
 		    && (o->op_next->op_next->op_flags & OPf_STACKED))
 	    {
 		/* Turn "$a .= <FH>" into an OP_RCATLINE. AMS 20010917 */
-		o->op_type   = OP_RCATLINE;
+                CHANGE_TYPE(o, OP_RCATLINE);
 		o->op_flags |= OPf_STACKED;
-		o->op_ppaddr = PL_ppaddr[OP_RCATLINE];
 		op_null(o->op_next->op_next);
 		op_null(o->op_next);
 	    }
@@ -12682,8 +12634,7 @@ Perl_rpeep(pTHX_ OP *o)
 		    sv_rvweaken(sv);
 		    SvREADONLY_on(sv);
 		}
-		o->op_type = OP_CONST;
-		o->op_ppaddr = PL_ppaddr[OP_CONST];
+                CHANGE_TYPE(o, OP_CONST);
 		o->op_flags |= OPf_SPECIAL;
 		cSVOPo->op_sv = sv;
 	    }
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

Current codebase wires Perl_pp_mapstart to Perl_unimplemented_op (by
regen/opcode.pl) [1], but then avoids runtime panics by Perl_ck_grep
changing all OP_MAPSTART nodes to use PL_ppaddr[OP_GREPSTART] [2].

This is all too clever by half, so this patch undoes the trickery, and
treats these 2 OPS like 93bad3f did for OP_AELEMFAST and \1_LEX.

I cant glean a reason for this historical arrangement​:
Looking at regen/opcode.pl blamelog..

65bca31 added Perl_unimplemented_op() used by 3 'unreachable' ops,
and replaced a 'panic​: mapstart' diag with a common one, so the trick
goes further back.

c78ff97 moved a minimal/DIEing pp_mapstart implementation to
mathoms.c from pp_ctl.c. Perl_ck_grep also did the GREPSTART patching
back then.

f54cb97 did minor tweaks to a full pp_mapstart implementation. I
couldnt find the commit between it and c78ff that changed pp_mapstart
to a DIEing one, or I fat-fingered it, or I got distracted.

looking at ck-grep(), the code doing [2] is from 22c35a8 in 1998.

So anyway, I tried the following, it worked, it seems the historical
reason is no longer relevant.

[1] change regen/opcode.pl generated mapping
  -#define Perl_pp_mapstart Perl_unimplemented_op
  +#define Perl_pp_mapstart Perl_pp_grepstart

  this sets PL_ppaddr[OP_MAPSTART] = PL_ppaddr[OP_GREPSTART] during
  init, which makes the optype trickery in ck_grep[2] unneeded.

[2] Drop re-type-ing of MAPSTARTs as GREPSTARTs by Perl_ck_grep(OP* o)
  Given 1, mapstart & grepstart share code, so just leave optype alone.

requires make regen


op.c | 1 -
opcode.h | 4 ++--
regen/opcode.pl | 3 ++-
3 files changed, 4 insertions(+), 4 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index dbb2886..f259e68 100644
--- a/op.c
+++ b/op.c
@@ -9850,7 +9850,6 @@ Perl_ck_grep(pTHX_ OP *o)
 
     PERL_ARGS_ASSERT_CK_GREP;
 
-    o->op_ppaddr = PL_ppaddr[OP_GREPSTART];
     /* don't allocate gwop here, as we may leak it if PL_parser->error_count > 0 */
 
     if (o->op_flags & OPf_STACKED) {
diff --git a/opcode.h b/opcode.h
index 40cdc81..c367c09 100644
--- a/opcode.h
+++ b/opcode.h
@@ -44,7 +44,7 @@
 #define Perl_pp_keys Perl_do_kv
 #define Perl_pp_rv2hv Perl_pp_rv2av
 #define Perl_pp_pop Perl_pp_shift
-#define Perl_pp_mapstart Perl_unimplemented_op
+#define Perl_pp_mapstart Perl_pp_grepstart
 #define Perl_pp_dor Perl_pp_defined
 #define Perl_pp_andassign Perl_pp_and
 #define Perl_pp_orassign Perl_pp_or
@@ -1106,7 +1106,7 @@ EXT Perl_ppaddr_t PL_ppaddr[] /* or perlvars.h */
 	Perl_pp_reverse,
 	Perl_pp_grepstart,
 	Perl_pp_grepwhile,
-	Perl_pp_mapstart,	/* implemented by Perl_unimplemented_op */
+	Perl_pp_mapstart,	/* implemented by Perl_pp_grepstart */
 	Perl_pp_mapwhile,
 	Perl_pp_range,
 	Perl_pp_flip,
diff --git a/regen/opcode.pl b/regen/opcode.pl
index b9d7042..df0fbf4 100755
--- a/regen/opcode.pl
+++ b/regen/opcode.pl
@@ -78,7 +78,7 @@ my %alias;
 # Format is "this function" => "does these op names"
 my @raw_alias = (
 		 Perl_do_kv => [qw( keys values )],
-		 Perl_unimplemented_op => [qw(padany mapstart custom)],
+		 Perl_unimplemented_op => [qw(padany custom)],
 		 # All the ops with a body of { return NORMAL; }
 		 Perl_pp_null => [qw(scalar regcmaybe lineseq scope)],
 
@@ -135,6 +135,7 @@ my @raw_alias = (
 					 spwent epwent sgrent egrent)],
 		 Perl_pp_shostent => [qw(snetent sprotoent sservent)],
 		 Perl_pp_aelemfast => ['aelemfast_lex'],
+		 Perl_pp_grepstart => ['mapstart'],
 		);
 
 while (my ($func, $names) = splice @raw_alias, 0, 2) {
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

A previous patch promoted CHANGE_TYPE as the "normal" way to
initialize op_type and op_ppaddr fields. This suggests that
Perl_rpeep()s tweaking of OP_AELEMFAST and OP_AELEMFAST_LEX is
something of a special case.

It turns out that these are pretty normal too; since regen/opcode.pl
insures that PL_ppaddr[OP_AELEMFAST_LEX] == PL_ppaddr[OP_AELEMFAST],
the invariant​: o->op_ppaddr == PL_ppaddr[o->op_type] holds after both
the OP_AELEMFAST and OP_AELEMFAST_LEX branches, and the stronger
semantics of CHANGE_TYPE() can be used to communicate this normality.


op.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index f259e68..de5f93d 100644
--- a/op.c
+++ b/op.c
@@ -12282,15 +12282,14 @@ Perl_rpeep(pTHX_ OP *o)
 		    op_null(pop);
 		    o->op_flags |= pop->op_next->op_flags & OPf_MOD;
 		    o->op_next = pop->op_next->op_next;
-		    o->op_ppaddr = PL_ppaddr[OP_AELEMFAST];
 		    o->op_private = (U8)i;
 		    if (o->op_type == OP_GV) {
 			gv = cGVOPo_gv;
 			GvAVn(gv);
-			o->op_type = OP_AELEMFAST;
+                        CHANGE_TYPE(o, OP_AELEMFAST);
 		    }
 		    else
-			o->op_type = OP_AELEMFAST_LEX;
+                        CHANGE_TYPE(o, OP_AELEMFAST_LEX);
 		}
 		if (o->op_type != OP_GV)
 		    break;
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

in Perl_rpeep, remove special handling of OP_AELEMFAST_LEX vs OP_AELEMFAST.

Because PL_ppaddr[OP_AELEMFAST_LEX] == PL_ppaddr[OP_AELEMFAST] due to
regen/opcode.pl @​raw_alias, both op_type branches result in same
postcondition​: o->op_ppaddr == PL_ppaddr[o->op_type]
So its clearer to use the code construct that implies it.


op.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index f259e68..de5f93d 100644
--- a/op.c
+++ b/op.c
@@ -12282,15 +12282,14 @@ Perl_rpeep(pTHX_ OP *o)
 		    op_null(pop);
 		    o->op_flags |= pop->op_next->op_flags & OPf_MOD;
 		    o->op_next = pop->op_next->op_next;
-		    o->op_ppaddr = PL_ppaddr[OP_AELEMFAST];
 		    o->op_private = (U8)i;
 		    if (o->op_type == OP_GV) {
 			gv = cGVOPo_gv;
 			GvAVn(gv);
-			o->op_type = OP_AELEMFAST;
+                        CHANGE_TYPE(o, OP_AELEMFAST);
 		    }
 		    else
-			o->op_type = OP_AELEMFAST_LEX;
+                        CHANGE_TYPE(o, OP_AELEMFAST_LEX);
 		}
 		if (o->op_type != OP_GV)
 		    break;
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

S_alloc_LOGOP inits the op_type field, but not op_ppaddr.
It has 8 uses, all of which are immediately followed by
o->op_ppaddr = PL_ppaddr[type].

So CHANGE_TYPE()s postcondition pertains, lets say so.


op.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index de5f93d..79307df 100644
--- a/op.c
+++ b/op.c
@@ -1226,7 +1226,7 @@ S_alloc_LOGOP(pTHX_ I32 type, OP *first, OP* other)
     LOGOP *logop;
     OP *kid = first;
     NewOp(1101, logop, 1, LOGOP);
-    logop->op_type = (OPCODE)type;
+    CHANGE_TYPE(logop, type);
     logop->op_first = first;
     logop->op_other = other;
     logop->op_flags = OPf_KIDS;
@@ -5366,7 +5366,6 @@ Perl_pmruntime(pTHX_ OP *o, OP *expr, bool isreg, I32 floor)
 	}
 
         rcop = S_alloc_LOGOP(aTHX_ OP_REGCOMP, scalar(expr), o);
-	rcop->op_ppaddr = PL_ppaddr[OP_REGCOMP];
 	rcop->op_flags |=  ((PL_hints & HINT_RE_EVAL) ? OPf_SPECIAL : 0)
 			   | (reglist ? OPf_STACKED : 0);
 	rcop->op_targ = cv_targ;
@@ -5430,7 +5429,6 @@ Perl_pmruntime(pTHX_ OP *o, OP *expr, bool isreg, I32 floor)
 	}
 	else {
             rcop = S_alloc_LOGOP(aTHX_ OP_SUBSTCONT, scalar(repl), o);
-	    rcop->op_ppaddr = PL_ppaddr[OP_SUBSTCONT];
 	    rcop->op_private = 1;
 
 	    /* establish postfix order */
@@ -6668,7 +6666,6 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
 	other->op_private |= OPpASSIGN_BACKWARDS;  /* other is an OP_SASSIGN */
 
     logop = S_alloc_LOGOP(aTHX_ type, first, LINKLIST(other));
-    logop->op_ppaddr = PL_ppaddr[type];
     logop->op_flags |= (U8)flags;
     logop->op_private = (U8)(1 | (flags >> 8));
 
@@ -6738,7 +6735,6 @@ Perl_newCONDOP(pTHX_ I32 flags, OP *first, OP *trueop, OP *falseop)
 	return live;
     }
     logop = S_alloc_LOGOP(aTHX_ OP_COND_EXPR, first, LINKLIST(trueop));
-    logop->op_ppaddr = PL_ppaddr[OP_COND_EXPR];
     logop->op_flags |= (U8)flags;
     logop->op_private = (U8)(1 | (flags >> 8));
     logop->op_next = LINKLIST(falseop);
@@ -6789,7 +6785,6 @@ Perl_newRANGE(pTHX_ I32 flags, OP *left, OP *right)
     PERL_ARGS_ASSERT_NEWRANGE;
 
     range = S_alloc_LOGOP(aTHX_ OP_RANGE, left, LINKLIST(right));
-    range->op_ppaddr = PL_ppaddr[OP_RANGE];
     range->op_flags = OPf_KIDS;
     leftstart = LINKLIST(left);
     range->op_private = (U8)(1 | (flags >> 8));
@@ -7309,7 +7304,6 @@ S_newGIVWHENOP(pTHX_ OP *cond, OP *block,
     PERL_ARGS_ASSERT_NEWGIVWHENOP;
 
     enterop = S_alloc_LOGOP(aTHX_ enter_opcode, block, NULL);
-    enterop->op_ppaddr = PL_ppaddr[enter_opcode];
     enterop->op_targ = ((entertarg == NOT_IN_PAD) ? 0 : entertarg);
     enterop->op_private = 0;
 
@@ -9309,7 +9303,6 @@ Perl_ck_eval(pTHX_ OP *o)
 	    op_free(o);
 
             enter = S_alloc_LOGOP(aTHX_ OP_ENTERTRY, NULL, NULL);
-	    enter->op_ppaddr = PL_ppaddr[OP_ENTERTRY];
 
 	    /* establish postfix order */
 	    enter->op_next = (OP*)enter;
@@ -9872,7 +9865,6 @@ Perl_ck_grep(pTHX_ OP *o)
     kid = kUNOP->op_first;
 
     gwop = S_alloc_LOGOP(aTHX_ type, o, LINKLIST(kid));
-    gwop->op_ppaddr = PL_ppaddr[type];
     kid->op_next = (OP*)gwop;
     offset = pad_findmy_pvs("$_", 0);
     if (offset == NOT_IN_PAD || PAD_COMPNAME_FLAGS_isOUR(offset)) {
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @jimc

Here, op_type and op_ppaddr inits were separated by many lines.
Move them together, and replace with CHANGE_TYPE callsite.


op.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Inline Patch
diff --git a/op.c b/op.c
index 79307df..2bb5637 100644
--- a/op.c
+++ b/op.c
@@ -9459,7 +9459,8 @@ Perl_ck_rvconst(pTHX_ OP *o)
 		  && SvTYPE(SvRV(gv)) != SVt_PVCV)
 		    gv_fetchsv(kidsv, GV_ADDMULTI, SVt_PVCV);
 	    }
-	    kid->op_type = OP_GV;
+            CHANGE_TYPE(kid, OP_GV);
+	    kid->op_ppaddr = PL_ppaddr[OP_GV];
 	    SvREFCNT_dec(kid->op_sv);
 #ifdef USE_ITHREADS
 	    /* XXX hack: dependence on sizeof(PADOP) <= sizeof(SVOP) */
@@ -9471,7 +9472,6 @@ Perl_ck_rvconst(pTHX_ OP *o)
 	    kid->op_sv = SvREFCNT_inc_simple_NN(gv);
 #endif
 	    kid->op_private = 0;
-	    kid->op_ppaddr = PL_ppaddr[OP_GV];
 	    /* FAKE globs in the symbol table cause weird bugs (#77810) */
 	    SvFAKE_off(gv);
 	}
-- 
1.8.3.1
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @cpansprout

On Sun Nov 02 13​:01​:58 2014, yoduh wrote​:

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, thus "promoting" its use as the "normal" way to do
so. One of its goals is that explicitly setting o->op_ppaddr or
o->op_type should indicate something special.

Patch 1 uses CHANGE_TYPE in 49 callsites, ie "normal" init.
Patch 2 removes pp_mapstart trickery, with 1 line in op.c
Patch 3 reduces variation in OP_AELEMFAST setup.
the de-optimization is infinitesimal, and fixed by compilers.
Patch 4 folds pp_opaddr setup into S_alloc_LOGOP.
Patch 5 adds single CHANGE_TYPE where inits were separated by code.

Please don’t use git send-email. While I *could* apply your patches, it would involve a lot of copying and pasting to put the headers back in the patch. If you could run​:

  git format-patch --stdout -M blead.. > topic-branch-changes.patch

and attach the file, it would save committers a lot of work. Thank you.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @tonycoz

On Sun Nov 02 13​:57​:20 2014, sprout wrote​:

Please don’t use git send-email. While I *could* apply your patches,
it would involve a lot of copying and pasting to put the headers back
in the patch. If you could run​:

git format-patch --stdout -M blead.. > topic-branch-changes.patch

and attach the file, it would save committers a lot of work. Thank
you.

I thought the new RT was meant to preserve the headers well enough to allow patches to be applied, but I guess I was mistaken, git am couldn't recognize the patch format.

I've merged all eight tickets created into 123105.

I'm not sure this was created directly with git send-email, there's 2 header emails, and the patch numbering is strange too, with some numbered /3 and the others /5, and both header emails as /4.

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @cpansprout

On Sun Nov 02 15​:08​:14 2014, tonyc wrote​:

On Sun Nov 02 13​:57​:20 2014, sprout wrote​:

Please don’t use git send-email. While I *could* apply your patches,
it would involve a lot of copying and pasting to put the headers back
in the patch. If you could run​:

git format-patch --stdout -M blead.. > topic-branch-changes.patch

and attach the file, it would save committers a lot of work. Thank
you.

I thought the new RT was meant to preserve the headers well enough to
allow patches to be applied,

I didn’t realise that.

but I guess I was mistaken, git am
couldn't recognize the patch format.

On the contrary, I just tried applying one after following the ‘with headers’ link, and it worked for me.

But, still, it is easier to apply patch sequences that come in one file.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @cpansprout

On Sun Nov 02 13​:01​:58 2014, yoduh wrote​:

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, thus "promoting" its use as the "normal" way to do
so. One of its goals is that explicitly setting o->op_ppaddr or
o->op_type should indicate something special.

Patch 1 uses CHANGE_TYPE in 49 callsites, ie "normal" init.
Patch 2 removes pp_mapstart trickery, with 1 line in op.c
Patch 3 reduces variation in OP_AELEMFAST setup.
the de-optimization is infinitesimal, and fixed by compilers.
Patch 4 folds pp_opaddr setup into S_alloc_LOGOP.
Patch 5 adds single CHANGE_TYPE where inits were separated by code.

Thank you. I have applied three of your patches (1,2,4) as 10884df, 21b66d1 and ddacf10, respectively.

Patch 3 increases the size of op.o. Patch 5 leaves a stray ppaddr assignment.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @jimc

On Sun, Nov 2, 2014 at 7​:07 PM, Father Chrysostomos via RT <
perlbug-followup@​perl.org> wrote​:

On Sun Nov 02 13​:01​:58 2014, yoduh wrote​:

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, thus "promoting" its use as the "normal" way to do
so. One of its goals is that explicitly setting o->op_ppaddr or

Thank you. I have applied three of your patches (1,2,4) as 10884df,
21b66d1 and ddacf10, respectively.

Patch 3 increases the size of op.o. Patch 5 leaves a stray ppaddr
assignment.

thank you.

Ive fixed patch 5, and juggled the order (so it applies 1st).
next is old patch 3...
3rd replaces o->op_ppaddr = PL_ppaddr[++o->op_type] with more obvious
CHANGE_TYPE

new spin of patchset created and attached, as per your request.

Re old patch 3, the increase, while only 16 bytes, is mildly surprising.

==> op.report.before <==
  text data bss dec hex filename
242719 0 0 242719 3b41f op.o

==> op.report.after <==
  text data bss dec hex filename
242735 0 0 242735 3b42f op.o

I had expected default -O2 settings on linux to detect and hoist
the extra ppaddr assignment out of the branches.
I did objdump -DS op.o before and after, and sdiffd -lw200 the files,

the output suggests that the code is identical,
and that the difference is in the debug info tables.

Heres what Im seeing in the diff to conclude as above.
(please forgive the apparent space-mangling here, its pretty clean on gterm)

And if you'll indulge me a bit here,
1 - is this a fair, valid use of tools to examine the situation ?
2- what other binutils-fu might I use to show the presence of this
supposedly added debug info,
rather than trying to show the absence of difference in the assembly ?

  break;
  (
  /* FALLTHROUGH */
  (
  case OP_GV​:
  (
  if (o->op_type == OP_PADAV || o->op_next->op_type == OP_RV2AV)
{ (
  OP* const pop = (o->op_type == OP_PADAV) ?
  (
  o->op_next : o->op_next->op_next;
  (
  1392c​: 48 8b 2b mov (%rbx),%rbp
  (
  IV i;
  (
  if (pop && pop->op_type == OP_CONST &&
  (
  1392f​: 48 85 ed test %rbp,%rbp
  (
  13932​: 74 12 je 13946 <Perl_rpeep+0x626>
  (
  13934​: 0f b7 45 20 movzwl 0x20(%rbp),%eax
  (
  13938​: 66 25 ff 01 and $0x1ff,%ax
  (
  1393c​: 66 83 f8 05 cmp $0x5,%ax
  (
  13940​: 0f 84 bd 15 00 00 je 14f03 <Perl_rpeep+0x1be3>
  (
  o->op_type = OP_AELEMFAST;
  | CHANGE_TYPE(o,
OP_AELEMFAST);
  }
  (
  else
  (
  o->op_type = OP_AELEMFAST_LEX;
  | CHANGE_TYPE(o,
OP_AELEMFAST_LEX);
  }
  (
  if (o->op_type != OP_GV)
  (
  13946​: 89 c8 mov %ecx,%eax
  (
  13948​: 66 25 ff 01 and $0x1ff,%ax
  (

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @jimc

optype-cleanups.patch
From 432528294c62250ed17a490dc41946ce83bf0a2e Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 2 Nov 2014 13:33:48 -0700
Subject: [PATCH 1/3] op.c: CHANGE_TYPE in Perl_ck_rvconst

Here, op_type and op_ppaddr inits were separated by many lines.
Move them together, then replace with CHANGE_TYPE call.

--
v2- actually remove the explicit op_ppaddr assignment.
---
 op.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/op.c b/op.c
index 7a795e3..28811c7 100644
--- a/op.c
+++ b/op.c
@@ -9459,7 +9459,7 @@ Perl_ck_rvconst(pTHX_ OP *o)
 		  && SvTYPE(SvRV(gv)) != SVt_PVCV)
 		    gv_fetchsv(kidsv, GV_ADDMULTI, SVt_PVCV);
 	    }
-	    kid->op_type = OP_GV;
+            CHANGE_TYPE(kid, OP_GV);
 	    SvREFCNT_dec(kid->op_sv);
 #ifdef USE_ITHREADS
 	    /* XXX hack: dependence on sizeof(PADOP) <= sizeof(SVOP) */
@@ -9471,7 +9471,6 @@ Perl_ck_rvconst(pTHX_ OP *o)
 	    kid->op_sv = SvREFCNT_inc_simple_NN(gv);
 #endif
 	    kid->op_private = 0;
-	    kid->op_ppaddr = PL_ppaddr[OP_GV];
 	    /* FAKE globs in the symbol table cause weird bugs (#77810) */
 	    SvFAKE_off(gv);
 	}
-- 
1.8.3.1


From e39deb66d953f66a2e3e5ce17c96bb9e89a7f83a Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sat, 1 Nov 2014 17:24:50 -0600
Subject: [PATCH 2/3] op.c: regularize OP_AELEMFAST_LEX vs OP_AELEMFAST setup.

commit 10884df96723d4 promoted CHANGE_TYPE as the "normal" way to
initialize op_type and op_ppaddr fields.  This suggests that
Perl_rpeep()s tweaking of OP_AELEMFAST and OP_AELEMFAST_LEX is
something of a special case.

It turns out that these are pretty normal too; since regen/opcode.pl
insures that PL_ppaddr[OP_AELEMFAST_LEX] == PL_ppaddr[OP_AELEMFAST],
the invariant: o->op_ppaddr == PL_ppaddr[o->op_type] holds after both
the OP_AELEMFAST and OP_AELEMFAST_LEX branches, and the stronger
semantics of CHANGE_TYPE() can be used to communicate this normality.

On a gcc x86-64 -DEBUGGING build, this enlarges 'size op.o' by 16
bytes, but before and after 'objdump -DS op.o' output suggests that
the code itself is identical.
---
 op.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/op.c b/op.c
index 28811c7..c702813 100644
--- a/op.c
+++ b/op.c
@@ -12273,15 +12273,14 @@ Perl_rpeep(pTHX_ OP *o)
 		    op_null(pop);
 		    o->op_flags |= pop->op_next->op_flags & OPf_MOD;
 		    o->op_next = pop->op_next->op_next;
-		    o->op_ppaddr = PL_ppaddr[OP_AELEMFAST];
 		    o->op_private = (U8)i;
 		    if (o->op_type == OP_GV) {
 			gv = cGVOPo_gv;
 			GvAVn(gv);
-			o->op_type = OP_AELEMFAST;
+                        CHANGE_TYPE(o, OP_AELEMFAST);
 		    }
 		    else
-			o->op_type = OP_AELEMFAST_LEX;
+                        CHANGE_TYPE(o, OP_AELEMFAST_LEX);
 		}
 		if (o->op_type != OP_GV)
 		    break;
-- 
1.8.3.1


From 14952b14b341c1c227be80d9909b220e8e00861d Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 2 Nov 2014 21:22:03 -0700
Subject: [PATCH 3/3] op.c: CHANGE_TYPE for op_integerize and ck_spair

These 2 callsites alter an OP's type and ppaddr to a closely related
op, by incrementing the op_type, then setting ppaddr, exactly what
CHANGE_TYPE does.

Since CHANGE_TYPE(o, ++type) expands type 2x, it has a problem
with the ++, avoid this using auto vars.
---
 op.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/op.c b/op.c
index c702813..f2e5eef 100644
--- a/op.c
+++ b/op.c
@@ -3927,7 +3927,8 @@ S_op_integerize(pTHX_ OP *o)
     if ((PL_opargs[type] & OA_OTHERINT) && (PL_hints & HINT_INTEGER))
     {
 	dVAR;
-	o->op_ppaddr = PL_ppaddr[++(o->op_type)];
+        type++; /* avoid ++ in macro arg */
+        CHANGE_TYPE(o, type);
     }
 
     if (type == OP_NEGATE)
@@ -9190,14 +9191,15 @@ OP *
 Perl_ck_spair(pTHX_ OP *o)
 {
     dVAR;
+    OPCODE type;
 
     PERL_ARGS_ASSERT_CK_SPAIR;
 
+    type = o->op_type;
     if (o->op_flags & OPf_KIDS) {
 	OP* newop;
 	OP* kid;
         OP* kidkid;
-	const OPCODE type = o->op_type;
 	o = modkids(ck_fun(o), type);
 	kid    = cUNOPo->op_first;
 	kidkid = kUNOP->op_first;
@@ -9219,8 +9221,9 @@ Perl_ck_spair(pTHX_ OP *o)
 	op_free(kidkid);
     }
     /* transforms OP_REFGEN into OP_SREFGEN, OP_CHOP into OP_SCHOP,
-     * and OP_CHOMP into OP_SCHOMP */
-    o->op_ppaddr = PL_ppaddr[++o->op_type];
+     * and OP_CHOMP into OP_SCHOMP, avoiding ++ in macro arg */
+    type++;
+    CHANGE_TYPE(o, type);
     return ck_fun(o);
 }
 
-- 
1.8.3.1

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @jimc

On Sun, Nov 2, 2014 at 4​:08 PM, Tony Cook via RT <perlbug-followup@​perl.org>
wrote​:

I've merged all eight tickets created into 123105.

I'm not sure this was created directly with git send-email, there's 2
header emails, and the patch numbering is strange too, with some numbered
/3 and the others /5, and both header emails as /4.

it was git send-email. set started as 3 patches, then added more, then I
copied 0001- to 0000-intro and modified,
then reworded a patch subject, which then didnt overwrite the old 3/3, and
I inadvertently sent it.

if you were curious...

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @craigberry

On Mon, Nov 3, 2014 at 1​:59 AM, Jim Cromie <jim.cromie@​gmail.com> wrote​:

On Sun, Nov 2, 2014 at 4​:08 PM, Tony Cook via RT <perlbug-followup@​perl.org>
wrote​:

I've merged all eight tickets created into 123105.

I'm not sure this was created directly with git send-email, there's 2
header emails, and the patch numbering is strange too, with some numbered /3
and the others /5, and both header emails as /4.

it was git send-email. set started as 3 patches, then added more, then I
copied 0001- to 0000-intro and modified,
then reworded a patch subject, which then didnt overwrite the old 3/3, and I
inadvertently sent it.

if you were curious...

perlbug -p is available and was specifically intended for sending
patches to RT in case anyone's interested.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

On Mon Nov 03 09​:38​:21 2014, craig.a.berry@​gmail.com wrote​:

perlbug -p is available and was specifically intended for sending
patches to RT in case anyone's interested.

Yes, of course. I’d forgotten about that.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

On Sun Nov 02 23​:54​:23 2014, yoduh wrote​:

Ive fixed patch 5, and juggled the order (so it applies 1st).

Thank you. Applied as 72e8a95.

next is old patch 3...
3rd replaces o->op_ppaddr = PL_ppaddr[++o->op_type] with more obvious
CHANGE_TYPE

In this case, I’m not sure it makes the code clearer. To me it just seems more complex. But that is just my opinion. What do others think?

new spin of patchset created and attached, as per your request.

Re old patch 3, the increase, while only 16 bytes, is mildly surprising.

==> op.report.before <==
text data bss dec hex filename
242719 0 0 242719 3b41f op.o

==> op.report.after <==
text data bss dec hex filename
242735 0 0 242735 3b42f op.o

I had expected default -O2 settings on linux to detect and hoist
the extra ppaddr assignment out of the branches.
I did objdump -DS op.o before and after, and sdiffd -lw200 the files,

the output suggests that the code is identical,
and that the difference is in the debug info tables.

Heres what Im seeing in the diff to conclude as above.
(please forgive the apparent space-mangling here, its pretty clean on gterm)

And if you'll indulge me a bit here,
1 - is this a fair, valid use of tools to examine the situation ?
2- what other binutils-fu might I use to show the presence of this
supposedly added debug info,
rather than trying to show the absence of difference in the assembly ?

I don’t know the answer to that.

However, I am seeing a 20-byte increase with g++ and debugging disabled. Do you see the same thing with debugging off?

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 11, 2015

From @iabyn

On Sun, Nov 02, 2014 at 01​:02​:02PM -0800, Jim Cromie wrote​:

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, added 4 years ago. In doing so it "promotes" its
use as the "normal" way to do so, one of its goals is that setting
o->op_ppaddr should indicate something special.

Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?

Because​:

* this macro is used to initialise the type/ppaddr of an op, in addition to
  changing it;
* CHANGE_TYPE() is a very generic name giving no indication of what it
  operates on.

--
Lear​: Dost thou call me fool, boy?
Fool​: All thy other titles thou hast given away; that thou wast born with.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 11, 2015

From @tonycoz

On Wed, Mar 11, 2015 at 05​:19​:08PM +0000, Dave Mitchell wrote​:

On Sun, Nov 02, 2014 at 01​:02​:02PM -0800, Jim Cromie wrote​:

This patchset cleans up op_type and op_ppaddr initialization, using
CHANGE_TYPE macro, added 4 years ago. In doing so it "promotes" its
use as the "normal" way to do so, one of its goals is that setting
o->op_ppaddr should indicate something special.

Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?

Because​:

* this macro is used to initialise the type/ppaddr of an op, in addition to
changing it;
* CHANGE_TYPE() is a very generic name giving no indication of what it
operates on.

+1

Tony

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 12, 2015

From perl5-porters@perl.org

Dave Mitchell asked​:

Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?

OpTYPE_set.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 12, 2015

From @iabyn

On Thu, Mar 12, 2015 at 04​:21​:32AM -0000, Father Chrysostomos wrote​:

Dave Mitchell asked​:

Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?

OpTYPE_set.

works for me.

--
"Emacs isn't a bad OS once you get used to it.
It just lacks a decent editor."

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 19, 2015

From @iabyn

On Thu, Mar 12, 2015 at 03​:37​:21PM +0000, Dave Mitchell wrote​:

On Thu, Mar 12, 2015 at 04​:21​:32AM -0000, Father Chrysostomos wrote​:

Dave Mitchell asked​:

Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?

OpTYPE_set.

works for me.

Now done as b9a0709.

--
Music lesson​: a symbiotic relationship whereby a pupil's embellishments
concerning the amount of practice performed since the last lesson are
rewarded with embellishments from the teacher concerning the pupil's
progress over the corresponding period.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

Im attaching a revised patchset for this ticket

0001-op.c-fix-latent-bug-in-OP_AELEMFAST_LEX-OP_AELEMFAST.patch
0002-op.c-use-OpTYPE_set-for-op_integerize-and-ck_spair.patch
0003-use-OpTYPE_set-in-Perl_newUNOP_AUX.patch
0004-hoist-OpTYPE_set-to-op.h-from-op.c-use-in-perl.c.patch
0005-pp.c-use-OpTYPE_set-in-PP-pp_i_modulo-plus.patch

p1 - its a bit of a stretch to call this a BUG,
but its a tiny micro-optimization in not-hot code,
and is reliant upon regen/opcode (action at a distance,
unwarranted chumminess with the implementation)

Father C noted this patch caused op.o to grow by ~20 bytes,
I now think this is because optimizing compilers dont know that
PL_ppaddr[OP_AELEMFAST_LEX] == PL_ppaddr[OP_AELEMFAST]
Perhaps adding an assert would provide a hint ?

p2 - these callsites use the (++o->op_type) idiom to modify an OP to its
optimized doppelganger, but are otherwize just like the others.
the macro is tweaked with an auto-var to avoid double-expansions of the ++

p3 - callsite added since 10884df was applied.
its normal wrt op_ppaddr, op_type fields, should use normal init

p4 - hoist OpTYPE_set macro to op.h, use in perl.c
the API legislature might consider this as a timid transgression,
or not valuable enough, esp as p5 is now obsoleted

p5 - obsoleted by patch in [perl #128112]

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

0001-op.c-fix-latent-bug-in-OP_AELEMFAST_LEX-OP_AELEMFAST.patch
From f6257db99b53be401b3924614ecc67f8aa2d739b Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sat, 1 Nov 2014 17:24:50 -0600
Subject: [PATCH 1/5] op.c: fix latent bug in OP_AELEMFAST_LEX, OP_AELEMFAST
 setup.

OpTYPE_set() is the "normal" way to initialize an OP.
Commit 10884df967 left a number of unusual use cases, we pick up one here.

In Perl_rpeep(): case OP_GV: code open-codes OpTYPE_set(), but
micro-optimizes by hoisting the op->pp_addr = PL_ppaddr[OP_AELEMFAST]
assignment out of the if-else.

This works, cuz:
  PL_ppaddr[OP_AELEMFAST_LEX] == PL_ppaddr[OP_AELEMFAST] [*]

While this is guaranteed currently by regen/opcode.pl, it seems
rather cryptic and magical, and is certainly action at a distance.

On a gcc x86-64 -DEBUGGING build, this patch enlarges 'size op.o' by
24 bytes (last I checked).  gcc evidently doesnt know that [*] holds.

This isnt a runtime hot-path, so doesnt warrant micro-optimiation, and
the OP setup is otherwise normal.  OpTYPE_set()s postcondition,
o->op_ppaddr == PL_ppaddr[o->op_type], also holds, which is worth
saying clearly.
---
 op.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/op.c b/op.c
index 93205fe..d4c5783 100644
--- a/op.c
+++ b/op.c
@@ -13878,15 +13878,14 @@ Perl_rpeep(pTHX_ OP *o)
 		    op_null(pop);
 		    o->op_flags |= pop->op_next->op_flags & OPf_MOD;
 		    o->op_next = pop->op_next->op_next;
-		    o->op_ppaddr = PL_ppaddr[OP_AELEMFAST];
 		    o->op_private = (U8)i;
 		    if (o->op_type == OP_GV) {
 			gv = cGVOPo_gv;
 			GvAVn(gv);
-			o->op_type = OP_AELEMFAST;
+                        OpTYPE_set(o, OP_AELEMFAST);
 		    }
 		    else
-			o->op_type = OP_AELEMFAST_LEX;
+                        OpTYPE_set(o, OP_AELEMFAST_LEX);
 		}
 		if (o->op_type != OP_GV)
 		    break;
-- 
2.5.5

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

0002-op.c-use-OpTYPE_set-for-op_integerize-and-ck_spair.patch
From e62d857d9fb68ee406daecd7309f200ac6b82d46 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sun, 2 Nov 2014 21:22:03 -0700
Subject: [PATCH 2/5] op.c: use OpTYPE_set() for op_integerize and ck_spair

These 2 users employ the (++o->op_type) trick to modify an OP to its
optimized doppelganger.  This is tricky, but wrt OP setup its
otherwize normal, in particular OpTYPE_set()s postcondition holds
true, ie (o->op_ppaddr == PL_ppaddr[o->op_type]), so using OpTYPE_set
conveys this "normality".

Also fix OpTYPE_set(o, ++type) expansion problem with an autovar
in the macro body.
---
 op.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/op.c b/op.c
index d4c5783..86a8bf5 100644
--- a/op.c
+++ b/op.c
@@ -514,8 +514,9 @@ Perl_op_refcnt_dec(pTHX_ OP *o)
 
 #define OpTYPE_set(o,type) \
     STMT_START {				\
-	o->op_type = (OPCODE)type;		\
-	o->op_ppaddr = PL_ppaddr[type];		\
+	OPCODE tval = type;			\
+	o->op_type = tval;			\
+	o->op_ppaddr = PL_ppaddr[tval];		\
     } STMT_END
 
 STATIC OP *
@@ -4252,7 +4253,7 @@ S_op_integerize(pTHX_ OP *o)
     if ((PL_opargs[type] & OA_OTHERINT) && (PL_hints & HINT_INTEGER))
     {
 	dVAR;
-	o->op_ppaddr = PL_ppaddr[++(o->op_type)];
+        OpTYPE_set(o, ++(o->op_type));
     }
 
     if (type == OP_NEGATE)
@@ -9430,14 +9431,15 @@ OP *
 Perl_ck_spair(pTHX_ OP *o)
 {
     dVAR;
+    OPCODE type;
 
     PERL_ARGS_ASSERT_CK_SPAIR;
 
+    type = o->op_type;
     if (o->op_flags & OPf_KIDS) {
 	OP* newop;
 	OP* kid;
         OP* kidkid;
-	const OPCODE type = o->op_type;
 	o = modkids(ck_fun(o), type);
 	kid    = cUNOPo->op_first;
 	kidkid = kUNOP->op_first;
@@ -9461,7 +9463,7 @@ Perl_ck_spair(pTHX_ OP *o)
     }
     /* transforms OP_REFGEN into OP_SREFGEN, OP_CHOP into OP_SCHOP,
      * and OP_CHOMP into OP_SCHOMP */
-    o->op_ppaddr = PL_ppaddr[++o->op_type];
+    OpTYPE_set(o, ++type);
     return ck_fun(o);
 }
 
-- 
2.5.5

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

0003-use-OpTYPE_set-in-Perl_newUNOP_AUX.patch
From 01f8b2262154cf4325eaa6a61becd19b32a38813 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Tue, 25 Aug 2015 18:06:59 -0600
Subject: [PATCH 3/5] use OpTYPE_set in Perl_newUNOP_AUX

Replace open-code with macro-call, dropping the (OPCODE) cast, as was
done numerous times in 10884df.
---
 op.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/op.c b/op.c
index 86a8bf5..c3b9bd4 100644
--- a/op.c
+++ b/op.c
@@ -4876,8 +4876,7 @@ Perl_newUNOP_AUX(pTHX_ I32 type, I32 flags, OP *first, UNOP_AUX_item *aux)
         || type == OP_CUSTOM);
 
     NewOp(1101, unop, 1, UNOP_AUX);
-    unop->op_type = (OPCODE)type;
-    unop->op_ppaddr = PL_ppaddr[type];
+    OpTYPE_set(unop, type);
     unop->op_first = first;
     unop->op_flags = (U8)(flags | (first ? OPf_KIDS : 0));
     unop->op_private = (U8)((first ? 1 : 0) | (flags >> 8));
-- 
2.5.5

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

0004-hoist-OpTYPE_set-to-op.h-from-op.c-use-in-perl.c.patch
From c48f5fca2fd590f7d5a2805cfa4bd3eb5585ecdc Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Sat, 15 Nov 2014 15:06:01 -0700
Subject: [PATCH 4/5] hoist OpTYPE_set() to op.h from op.c, use in perl.c

hoist OpTYPE_set() into op.h so its available to perl.c and others,
and use it in perl.c.  Add parens around (o) so its robust with args
like &myop.
---
 op.c   | 7 -------
 op.h   | 6 ++++++
 perl.c | 9 +++------
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/op.c b/op.c
index c3b9bd4..2a823a3 100644
--- a/op.c
+++ b/op.c
@@ -512,13 +512,6 @@ Perl_op_refcnt_dec(pTHX_ OP *o)
 
 #define RETURN_UNLIMITED_NUMBER (PERL_INT_MAX / 2)
 
-#define OpTYPE_set(o,type) \
-    STMT_START {				\
-	OPCODE tval = type;			\
-	o->op_type = tval;			\
-	o->op_ppaddr = PL_ppaddr[tval];		\
-    } STMT_END
-
 STATIC OP *
 S_no_fh_allowed(pTHX_ OP *o)
 {
diff --git a/op.h b/op.h
index 3ded4bb..364b44f 100644
--- a/op.h
+++ b/op.h
@@ -993,6 +993,12 @@ C<sib> is non-null. For a higher-level interface, see C<L</op_sibling_splice>>.
 #define OP_TYPE_ISNT_AND_WASNT(o, type) \
     ( (o) && OP_TYPE_ISNT_AND_WASNT_NN(o, type) )
 
+#define OpTYPE_set(o,type) \
+    STMT_START {				\
+	OPCODE tval = type;			\
+	(o)->op_type = tval;			\
+	(o)->op_ppaddr = PL_ppaddr[tval];       \
+    } STMT_END
 
 #ifdef PERL_OP_PARENT
 #  define OpHAS_SIBLING(o)	(cBOOL((o)->op_moresib))
diff --git a/perl.c b/perl.c
index 1d8876b..76ef0e6 100644
--- a/perl.c
+++ b/perl.c
@@ -2783,15 +2783,12 @@ Perl_call_sv(pTHX_ SV *sv, VOL I32 flags)
         method_op.op_next = (OP*)&myop;
         PL_op = (OP*)&method_op;
         if ( flags & G_METHOD_NAMED ) {
-            method_op.op_ppaddr = PL_ppaddr[OP_METHOD_NAMED];
-            method_op.op_type = OP_METHOD_NAMED;
+            OpTYPE_set(&method_op, OP_METHOD_NAMED);
             method_op.op_u.op_meth_sv = sv;
         } else {
-            method_op.op_ppaddr = PL_ppaddr[OP_METHOD];
-            method_op.op_type = OP_METHOD;
+            OpTYPE_set(&method_op, OP_METHOD);
         }
-        myop.op_ppaddr = PL_ppaddr[OP_ENTERSUB];
-        myop.op_type = OP_ENTERSUB;
+        OpTYPE_set(&myop, OP_ENTERSUB);
     }
 
     if (!(flags & G_EVAL)) {
-- 
2.5.5

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 10, 2016

From @jimc

0005-pp.c-use-OpTYPE_set-in-PP-pp_i_modulo-plus.patch
From 9ea525e1c7b0566d4ede91bb99dd83aacf3f5aa1 Mon Sep 17 00:00:00 2001
From: Jim Cromie <jim.cromie@gmail.com>
Date: Mon, 17 Nov 2014 03:19:27 -0700
Subject: [PATCH 5/5] pp.c: use OpTYPE_set in PP(pp_i_modulo) plus..

PP(pp_i_modulo) is unusual code on several levels, and warrants some
explication:

For glibc version < 2.8, the PP-code detects a glibc _mod3i bug at
runtime (pp-code execution time), and updates both op->ppaddr and
PL_ppaddr[OP_I_MODULO] to work around it.  This seems tricky:

- many ops may have been built prior to bug-detection, without correction.
- detector converts each different I_MODULO op at 1st execution.
- PL_ppaddr[OP_I_MODULO] updated repeatedly.

While the bug detection seems better done once in perl_construct(),
before code is parsed and optrees are built, but thats not the focus now.

This patch replaces the op_ppaddr assigment with OpTYPE_set,
tolerating the redundant assignment of the optype, effectively undoing
a micro-optimization which depends upon the optype not changing.

OpTYPE_set(op, type) has stronger postconditions:
  (op->pp_addr == PL_ppaddr[op->optype])

Since the postcondition holds in this case even without the patch, its
best to communicate this by using macro.

Caveat: the OP-tweaking code is #idefd, and since commit bf3d06a
largely dormant on porters' boxes, so maybe leave it alone.  Does
eliminating an ad-hoc o->op_ppaddr assignment, and reducing arbitrary
usage differences (code-entropy) outweigh the churn ?
---
 pp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/pp.c b/pp.c
index 4a2cde0..ae48c8c 100644
--- a/pp.c
+++ b/pp.c
@@ -2842,9 +2842,9 @@ PP(pp_i_modulo)
 	  if (!right)
 	       DIE(aTHX_ "Illegal modulus zero");
 	  /* The assumption is to use hereafter the old vanilla version... */
-	  PL_op->op_ppaddr =
-	       PL_ppaddr[OP_I_MODULO] =
-	           Perl_pp_i_modulo_0;
+          PL_ppaddr[OP_I_MODULO] = &Perl_pp_i_modulo_0;
+          OpTYPE_set(PL_op, OP_I_MODULO);
+
 	  /* .. but if we have glibc, we might have a buggy _moddi3
 	   * (at least glibc 2.2.5 is known to have this bug), in other
 	   * words our integer modulus with negative quad as the second
@@ -2860,9 +2860,9 @@ PP(pp_i_modulo)
 	       if (l % r == -3) {
 		    /* Yikes, we have the bug.
 		     * Patch in the workaround version. */
-		    PL_op->op_ppaddr =
-			 PL_ppaddr[OP_I_MODULO] =
-			     &Perl_pp_i_modulo_1;
+                   PL_ppaddr[OP_I_MODULO] = &Perl_pp_i_modulo_1;
+                   OpTYPE_set(PL_op, OP_I_MODULO);
+
 		    /* Make certain we work right this time, too. */
 		    right = PERL_ABS(right);
 	       }
-- 
2.5.5

@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.