search

Perl / perl5

šŸŖ The Perl programming language
https://dev.perl.org/perl5/
Other
1.93k stars 552 forks source link

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

Open p5pRT opened 9 years ago

p5pRT commented 9 years ago

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

Searchable as RT123105$

p5pRT commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 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 .= " 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 commented 9 years ago

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 93bad3fd5548 did for OP_AELEMFAST and \1_LEX.

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

65bca31a68 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.

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

f54cb97a39f 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 22c35a8c23 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 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 commented 9 years ago

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 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 commented 9 years ago

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 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 commented 9 years ago

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 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 commented 9 years ago

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 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 commented 9 years ago

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 commented 9 years ago

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

p5pRT commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 10884df967\, 21b66d1ce5 and ddacf10992\, respectively.

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

--

Father Chrysostomos

p5pRT commented 9 years ago

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 10884df967\, 21b66d1ce5 and ddacf10992\, 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 commented 9 years ago

From @jimc

optype-cleanups.patch ```diff From 432528294c62250ed17a490dc41946ce83bf0a2e Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 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 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 commented 9 years ago

From @jimc

On Sun\, Nov 2\, 2014 at 4​:08 PM\, Tony Cook via RT \perlbug\-followup@&#8203;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 commented 9 years ago

From @craigberry

On Mon\, Nov 3\, 2014 at 1​:59 AM\, Jim Cromie \jim\.cromie@&#8203;gmail\.com wrote​:

On Sun\, Nov 2\, 2014 at 4​:08 PM\, Tony Cook via RT \perlbug\-followup@&#8203;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 commented 9 years ago

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 commented 9 years ago

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 72e8a9531.

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

From perl5-porters@perl.org

Dave Mitchell asked​:

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

OpTYPE_set.

p5pRT commented 9 years ago

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 commented 9 years ago

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

-- 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 commented 8 years ago

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 10884df967 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 commented 8 years ago

From @jimc

0001-op.c-fix-latent-bug-in-OP_AELEMFAST_LEX-OP_AELEMFAST.patch ```diff From f6257db99b53be401b3924614ecc67f8aa2d739b Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 commented 8 years ago

From @jimc

0002-op.c-use-OpTYPE_set-for-op_integerize-and-ck_spair.patch ```diff From e62d857d9fb68ee406daecd7309f200ac6b82d46 Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 commented 8 years ago

From @jimc

0003-use-OpTYPE_set-in-Perl_newUNOP_AUX.patch ```diff From 01f8b2262154cf4325eaa6a61becd19b32a38813 Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 commented 8 years ago

From @jimc

0004-hoist-OpTYPE_set-to-op.h-from-op.c-use-in-perl.c.patch ```diff From c48f5fca2fd590f7d5a2805cfa4bd3eb5585ecdc Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 is non-null. For a higher-level interface, see C>. #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 commented 8 years ago

From @jimc

0005-pp.c-use-OpTYPE_set-in-PP-pp_i_modulo-plus.patch ```diff From 9ea525e1c7b0566d4ede91bb99dd83aacf3f5aa1 Mon Sep 17 00:00:00 2001 From: Jim Cromie 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 ```