Skip Menu |
Report information
Id: 123105
Status: open
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: yoduh <jim.cromie [at] gmail.com>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)

Attachments


CC: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 0/4] RFC op-type patchset
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Date: Sun, 2 Nov 2014 14:00:44 -0700
Download (untitled) / with headers
text/plain 992b
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).
Subject: [PATCH 0/4] RFC op-type patchset
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Date: Sun, 2 Nov 2014 14:00:45 -0700
CC: Jim Cromie <jim.cromie [...] gmail.com>
Download (untitled) / with headers
text/plain 658b
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).
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 1/5] op.c: use CHANGE_TYPE everywhere its obvious
CC: Jim Cromie <jim.cromie [...] gmail.com>
Date: Sun, 2 Nov 2014 14:00:46 -0700
Download (untitled) / with headers
text/plain 15.1k

Message body is not shown because it is too large.

Subject: [PATCH 2/5] end the pp_mapstart trickery
From: Jim Cromie <jim.cromie [...] gmail.com>
To: perlbug [...] perl.org
Date: Sun, 2 Nov 2014 14:00:47 -0700
CC: Jim Cromie <jim.cromie [...] gmail.com>
Download (untitled) / with headers
text/plain 3.4k
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(-) 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
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 3/5] op.c: regularize OP_AELEMFAST_LEX vs OP_AELEMFAST setup.
CC: Jim Cromie <jim.cromie [...] gmail.com>
Date: Sun, 2 Nov 2014 14:00:48 -0700
Download (untitled) / with headers
text/plain 1.2k
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(-) 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
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 3/3] op.c: simplfy OP_AELEMFAST_LEX vs OP_AELEMFAST
CC: Jim Cromie <jim.cromie [...] gmail.com>
Date: Sun, 2 Nov 2014 14:00:49 -0700
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(-) 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
Date: Sun, 2 Nov 2014 14:00:50 -0700
CC: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 4/5] op.c: use CHANGE_TYPE in S_alloc_LOGOP
From: Jim Cromie <jim.cromie [...] gmail.com>
To: perlbug [...] perl.org
Download (untitled) / with headers
text/plain 3.1k
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(-) 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
To: perlbug [...] perl.org
From: Jim Cromie <jim.cromie [...] gmail.com>
Subject: [PATCH 5/5] op.c: one more CHANGE_TYPE callsite
Date: Sun, 2 Nov 2014 14:00:51 -0700
CC: Jim Cromie <jim.cromie [...] gmail.com>
Download (untitled) / with headers
text/plain 923b
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(-) 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1010b
On Sun Nov 02 13:01:58 2014, yoduh wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 801b
On Sun Nov 02 13:57:20 2014, sprout wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 854b
On Sun Nov 02 15:08:14 2014, tonyc wrote: Show quoted text
> 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. Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 883b
On Sun Nov 02 13:01:58 2014, yoduh wrote: Show quoted text
> 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
Date: Mon, 3 Nov 2014 00:53:22 -0700
Subject: Re: [perl #123105] [PATCH 0/4] RFC op-type patchset
From: Jim Cromie <jim.cromie [...] gmail.com>
To: Father Chrysostomos via RT <perlbug-followup [...] perl.org>
Download (untitled) / with headers
text/plain 3.5k


On Sun, Nov 2, 2014 at 7:07 PM, Father Chrysostomos via RT <perlbug-followup@perl.org> wrote:
Show quoted text
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
 
Show quoted text
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                                          (







Show quoted text
--

Father Chrysostomos




 

Message body is not shown because sender requested not to inline it.

Date: Mon, 3 Nov 2014 00:59:28 -0700
From: Jim Cromie <jim.cromie [...] gmail.com>
To: Father Chrysostomos via RT <perlbug-followup [...] perl.org>
Subject: Re: [perl #123105] [PATCH 0/4] RFC op-type patchset
Download (untitled) / with headers
text/plain 599b


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

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...
 
Show quoted text
Tony
CC: Father Chrysostomos via RT <perlbug-followup [...] perl.org>
Date: Mon, 3 Nov 2014 11:37:53 -0600
From: "Craig A. Berry" <craig.a.berry [...] gmail.com>
To: Jim Cromie <jim.cromie [...] gmail.com>
Subject: Re: [perl #123105] [PATCH 0/4] RFC op-type patchset
Download (untitled) / with headers
text/plain 801b
On Mon, Nov 3, 2014 at 1:59 AM, Jim Cromie <jim.cromie@gmail.com> wrote: Show quoted text
> > > 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.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 245b
On Mon Nov 03 09:38:21 2014, craig.a.berry@gmail.com wrote: Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.6k
On Sun Nov 02 23:54:23 2014, yoduh wrote: Show quoted text
> Ive fixed patch 5, and juggled the order (so it applies 1st).
Thank you. Applied as 72e8a9531. Show quoted text
> 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? Show quoted text
> 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
From: Dave Mitchell <davem [...] iabyn.com>
Date: Wed, 11 Mar 2015 17:19:08 +0000
To: perl5-porters [...] perl.org
Subject: Re: [perl #123106] [PATCH 0/4] RFC op-type patchset
Download (untitled) / with headers
text/plain 690b
On Sun, Nov 02, 2014 at 01:02:02PM -0800, Jim Cromie wrote: Show quoted text
> 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.
From: Tony Cook <tony [...] develop-help.com>
Date: Thu, 12 Mar 2015 08:42:17 +1100
To: perl5-porters [...] perl.org
Subject: Re: [perl #123106] [PATCH 0/4] RFC op-type patchset
Download (untitled) / with headers
text/plain 676b
On Wed, Mar 11, 2015 at 05:19:08PM +0000, Dave Mitchell wrote: Show quoted text
> 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
From: Father Chrysostomos <sprout [...] cpan.org>
Date: 12 Mar 2015 04:21:32 -0000
To: perl5-porters [...] perl.org
Subject: Re: [perl #123106] [PATCH 0/4] RFC op-type patchset
Download (untitled) / with headers
text/plain 102b
Dave Mitchell asked: Show quoted text
> Would anyone object if I renamed CHANGE_TYPE() to Op_SET_TYPE()?
OpTYPE_set.
Subject: Re: [perl #123106] [PATCH 0/4] RFC op-type patchset
To: perl5-porters [...] perl.org
Date: Thu, 12 Mar 2015 15:37:21 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 278b
On Thu, Mar 12, 2015 at 04:21:32AM -0000, Father Chrysostomos wrote: Show quoted text
> 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."
Subject: Re: [perl #123106] [PATCH 0/4] RFC op-type patchset
To: perl5-porters [...] perl.org
Date: Thu, 19 Mar 2015 11:20:44 +0000
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 582b
On Thu, Mar 12, 2015 at 03:37:21PM +0000, Dave Mitchell wrote: Show quoted text
> 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.
Date: Tue, 10 May 2016 09:57:48 -0600
Subject: [perl #123106] bugfix / cleanup - use more OpTYPE_set
From: Jim Cromie <jim.cromie [...] gmail.com>
To: perlbug [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
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]

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.



This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org