Front page | perl.perl5.porters |
Postings from February 2017
Re: more process_optree
Thread Previous
|
Thread Next
From:
Jim Cromie
Date:
February 15, 2017 00:12
Subject:
Re: more process_optree
Message ID:
CAJfuBxw10UZSTCWbYn_C3MXzXwXE49SO90CZU-UYD3Fw9trapQ@mail.gmail.com
hi all,
Id like to clarify & revise these patches, 2 new ones attached.
it wasnt ready for prime-time, hence no RT submission.
Now it passes make test, for 1 config.
Im trying others now...
other comments inline..
On Sun, Feb 12, 2017 at 2:40 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
> Subject: [PATCH 1/2] call S_process_optree from S_gen_constant_list
>
> S_gen_constant_list was recently changed by commit b369834, which
> wrapped a CALL_PEEP with an OP_NULL -> OP_CUSTOM temporary swap.
>
> At near the same time, commit 2790f0ae4da added S_process_optree() to
> package up almost all the uses of S_finalize_optree, CALL_PEEP,
> S_prune_chain_head, and various other tricky details. It noted in
> logmsg that a few other candidate cleanups were skipped for
> simplicity.
the commit-ID was actually 01f9673fe8388317fdf7fca63774e3bc0dfd58d3
the wrong id was for an earlier version of the patch,
from davem's op_signature* branch
> This patch attempts to address one of those cleanups, calling
> S_process_optree from S_gen_constant_list. To do this, we have to
> move the new wrapper added by 1st commit into S_process_optree, and
> add a boolean parameter to do that wrapping for the new user, but not
> for any other caller.
>
> no-bisect: new call to finalize_optree is breaking an assertion in
> Perl_op_free.
this could be a convention, even though `git bisect` doesnt understand it.
Id guess that Porting/*bisect* could be adjusted to deal with it, but ..
I understand that such patches shouldnt get into origin/blead,
and im not proposing to relax that rule now.
But this might allow relaxing later.
>
> miniperl: op.c:792:
> Perl_op_free: Assertion `!(o->op_private & ~PL_op_private_valid[type])' failed.
>
> Since this is a totally new callsite, the existing asserts may be
> correct, or too strict; TBD.
to clarify, whats actually new here is that finalize_optree is called
where it wasnt previously.
IOW, finalize_optree does not achieve full coverage of all OPs run.
IMO this is a latent bug, present since the function was added.
Specifically, S_fold_constants, gen_constant_list, and perhaps others,
do not finalize their optrees.
> ---
> op.c | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
...
> @@ -4532,6 +4541,7 @@ S_fold_constants(pTHX_ OP *const o)
> curop = LINKLIST(o);
> old_next = o->op_next;
> o->op_next = 0;
> + S_process_optree(aTHX_ NULL, o, curop, 0);
> PL_op = curop;
>
This chunk was inadvertently included.
removing it gets us a little further in make.
That said, it ISTM this chunk comports with
the basic pattern established by 01f9673fe83883,
ie:
start = linklist(root);
start->op_next = 0;
process_optree(...)
$ make test
./miniperl -Ilib make_ext.pl cpan/Archive-Tar/pm_to_blib MAKE="make"
LIBPERL_A=libperl.a
miniperl: op.c:2701: S_finalize_op: Assertion `has_last || family ==
OA_UNOP || family == OA_UNOP_AUX || family == OA_LOGOP || family ==
OA_BASEOP_OR_UNOP || family == OA_FILESTATOP || family == OA_LOOPEXOP
|| family == OA_METHOP || type == OP_CUSTOM || type == OP_NULL'
failed.
makefile:586: recipe for target 'cpan/Archive-Tar/pm_to_blib' failed
make: *** [cpan/Archive-Tar/pm_to_blib] Aborted (core dumped)
adding a little debug helps, a little:
has_last=0 family=3840 optype=152 name=multideref o=1e13050
has_last=1 family=1024 optype=159 name=anonlist o=1e0c448
has_last=0 family=256 optype=16 name=null o=1e0cf40
EXECUTING...
miniperl: op.c:2706: S_finalize_op: Assertion `has_last || family ==
OA_UNOP || family == OA_UNOP_AUX || family == OA_LOGOP || family ==
OA_BASEOP_OR_UNOP || family == OA_FILESTATOP || family == OA_LOOPEXOP
|| family == OA_METHOP || type == OP_CUSTOM || type == OP_NULL'
failed.
the new patch-2 adjusts the assert to pass,
but is basically a monkey-patch, so needs other eyeballs.
Thread Previous
|
Thread Next