[perl #24821] enhancement patch for B::Concise
Jim Cromie
I picked this back up recently, I think it now incorporates all comments
from this thread started 1/6/04, plus a few off-list suggestions.
In the interest of full disclosure, theres (at least) a couple of maybes
in here:
1. some regexp match failures under debugger (only).
these may expose a bug, and also suggests that
stringifying a structure (an optree in this case) then
string-matching it against something else
is not robust generally. In this case, I hope its enough.
$> ./perl -d -Ilib ext/B/t/concise.t
...
not ok 43 - 2stringout vs hardcoded reference data
# Failed at ext/B/t/concise.t line 394
# got 'B::Concise::compile(CODE(0x85c98ac))
# 7 <1> leavesub[2 refs] K/REFC,1 ->(end)
# - <@> lineseq KP ->7
# 1 <;> dbstate(main -803 concise.t:389) v ->2
# 6 <2> sassign sKS/2 ->7
# 4 <2> add[t3] sK/2 ->5
# - <1> ex-rv2sv sK/1 ->3
# 2 <#> gvsv[*b] s ->3
# 3 <$> const[IV 42] s ->4
# - <1> ex-rv2sv sKRM*/1 ->6
# 5 <#> gvsv[*a] s ->6
# '
# expected /(?-xism:^B::Concise::compile\(CODE\(0x[0-9a-fA-F]+\)\)
# 7 <1> leavesub\[1 ref\] K/REFC,1 ->\(end\)
# - <\@> lineseq KP ->7
# 1 <;> nextstate\(main -\d+ concise\.t:\d+\) v ->2
# 6 <2> sassign sKS/2 ->7
# 4 <2> add\[t3\] sK/2 ->5
# - <1> ex-rv2sv sK/1 ->3
# 2 <\#> gvsv\[\*b\] s ->3
# 3 <\$> const\[IV 42\] s ->4
# - <1> ex-rv2sv sKRM\*/1 ->6
# 5 <\#> gvsv\[\*a\] s ->6
# $)/
2. Above output shows how my 'announcement' line displays an anonymous sub.
its close to this analogous form (from bleadperl, unmodified)
[jimc@harpo bleadperl]$ perl -MO=Concise,foo -e 'sub foo {1}; foo'
main::foo:
3 <1> leavesub[1 ref] K/REFC,1 ->(end)
- <@> lineseq KP ->3
1 <;> nextstate(main 1 -e:1) v ->2
2 <$> const[IV 1] s ->3
-e syntax OK
whereas bleadperl doesnt produce an announcement line for an anonymous sub.
[jimc@harpo bleadperl]$ perl -MO=Concise -e 'sub {1}'
6 <@> leave[1 ref] vKP/REFC ->(end)
1 <0> enter ->2
2 <;> nextstate(main 2 -e:1) v ->3
5 <1> refgen vK/1 ->6
- <1> ex-list lKRM ->5
3 <0> pushmark sRM ->4
4 <$> anoncode[CV ] lRM ->5
-e syntax OK
the question here is - whether I should revert the asub announce,
and if not, should I simplify it -
get rid of () parens. A space would be visually less cluttered.
and/or get rid of the stringified "$coderef". I put it there to
disambiguate between 2 reports.
3. 'extra' arg
if invoked with extra arg, last test prints output directly.
I stuffed this in cuz I didnt grok why the 'goto -' lines were
printed in -exec mode
and I wanted to flag it to y'all.
I think its cuz of a flawed set of style-formats, or cuz of a prob with
my callback,
and *not* cuz of a bug I added to B::Concise.
That said, I did add a 5th arg to the callback, namely the current
$stylename.
This allows any callback to do style-specific stuff, albeit in a
slightly hackish way.
It seemed overkill/incompatible to rework the callback dispatch itself
to pay heed to the
stylename. So I chose to 'give em the rope', maybe theyll pull a car
out of a ditch.
$> ./perl -Ilib ext/B/t/concise.t Foo
ok 46 - suppressed all but 3 lines of output
con-scope: B::Concise::compile(CODE(0x81b5088))
7 <1> leavesub[1 ref] K/REFC,1 ->(end)
1 <;> nextstate(main -464 concise.t:455) v ->2
exec-scope: B::Concise::compile(CODE(0x8275d84))
goto -
1 <;> nextstate(main -463 concise.t:464) v
7 <1> leavesub[1 ref] K/REFC,1
B::Concise::compile(CODE(0x8283fac))
goto -
8 <;> nextstate(main -462 concise.t:465) v
9 <#> gvsv[*b] s
a <$> const[IV 42] s
b <2> add[t3] sK/2
c <#> gvsv[*a] s
d <2> sassign sKS/2
e <1> leavesub[1 ref] K/REFC,1
Rafael Garcia-Suarez
> I picked this back up recently, I think it now incorporates all comments
> from this thread started 1/6/04, plus a few off-list suggestions.
Thanks, I applied the Concise.pm part of your patch as change #22539.
I didn't apply the test patch, because :
1. you probably missed the fact that Concise returns different output
with threaded and unthreaded perls
2. you gave me an idea for a Grand Plan.
The other day I added an optimisation that basically transforms
my $x = undef;
into
my $x;
but I didn't add a test for it, because I didn't knew how to do it.
Thus, if we then modify the optree building in a way that breaks this
optimisation, this will go unnoticed, and this is not a good thing.
But your patch to B::Concise gives a tool to test such things.
So what I'd like is a new test file, let's say ext/B/t/optrees.t, that
lists code snippets and expected optrees in a way perhaps inspired by
the things under t/lib/warnings/*, in a way that it's easy to add new
stuff ; that takes into account perl configuration differences
(useithreads for example) ; and in which in the future we would add
regression tests for optree optimisations. (@x = sort @x in place comes
to mind as well.) What do you think about this ?
Jim Cromie
>jim cromie wrote:
>
>
>>I picked this back up recently, I think it now incorporates all comments
>>from this thread started 1/6/04, plus a few off-list suggestions.
>>
>>
>
>Thanks, I applied the Concise.pm part of your patch as change #22539.
>
>
>
:-)
>I didn't apply the test patch, because :
>1. you probably missed the fact that Concise returns different output
>with threaded and unthreaded perls
>
>
true - I never thought to test that. I can repackage tests 1-20, which
dont
actually run compile, and so wouldnt see any threading diffs.
I could also try to rewrite 21-46 to be thread-independent, it may be as
simple
as having 2 versions of ref-text.
BTW - if you still have the broken non-threaded test output, pls send to me.
Ill have to build a non-threaded blead anyway, but it will help dispell my
fear that concise-nothreads.t & concise-threads.t might be needed for 21-46
>2. you gave me an idea for a Grand Plan.
>
>The other day I added an optimisation that basically transforms
> my $x = undef;
>into
> my $x;
>but I didn't add a test for it, because I didn't knew how to do it.
>Thus, if we then modify the optree building in a way that breaks this
>optimisation, this will go unnoticed, and this is not a good thing.
>But your patch to B::Concise gives a tool to test such things.
>
>
>
Hey look - brittleness IS a feature ;-)
For this, I suppose that its better to have false positives than false
negatives.
But actually, this test is actually more robust than comparing to a
ref-string.
is ($s1, $s2, "assign properly optimized away")
BTW - theres another optimization yet to do.
I'll see if I can follow your optimization patch.
[jimc@harpo bleadperl]$ ./perl -MO=Concise -e 'my $foo=()';
6 <@> leave[1 ref] vKP/REFC ->(end)
1 <0> enter ->2
2 <;> nextstate(main 1 -e:1) v ->3
5 <2> sassign vKS/2 ->6
3 <0> stub sP ->4
4 <0> padsv[$foo:1,2] sRM*/LVINTRO ->5
-e syntax OK
our $foo=undef is also optimizable, though probly not worth it.
(approx once per script, highly unlikely inside a subroutine
-might even warrant a warning, under use warnings 'paranoid')
>So what I'd like is a new test file, let's say ext/B/t/optrees.t, that
>lists code snippets and expected optrees in a way perhaps inspired by
>the things under t/lib/warnings/*, in a way that it's easy to add new
>stuff ; that takes into account perl configuration differences
>(useithreads for example) ; and in which in the future we would add
>regression tests for optree optimisations. (@x = sort @x in place comes
>to mind as well.) What do you think about this ?
>
>
>
hmm - while t/lib/warnings/* looks somewhat daunting,
is this a crude but reasonable starting point ?
ext/B/t/optweaks/{assign,sort,map-scalar,grep-boolean}.t
thx,
jimc
Yitzchak Scott-Thoennes
If you look at op/split.t, there's a test there that "my ($a,$b) = split;"
uses a default limit of 3. It might be better to add additional capability
to test.pl to make that kind of testing easier.
Jim Cromie
>jim cromie wrote:
>
>
>>I picked this back up recently, I think it now incorporates all comments
>>from this thread started 1/6/04, plus a few off-list suggestions.
>>
>>
>
>Thanks, I applied the Concise.pm part of your patch as change #22539.
>
>I didn't apply the test patch, because :
>1. you probably missed the fact that Concise returns different output
>with threaded and unthreaded perls
>2. you gave me an idea for a Grand Plan.
>
>
>
Attached patch pbc2.3 reworks the tests:
ext/B/t/concise.t tests B::Concise API - based on prev tests 1-20
ext/B/t/optree.t
defines test function & helper
uses heredocs to give args to the test-function
its: cleaner, clearer, shorter, easier to use.
most tests commented out now - easily added back
will redo previous regex tests here RSN ..
wrt threaded vs non-threaded;
mostly, the differences are in [] vs () chars used in formatting.
however, some opcodes dont do the bracketting switch.
my regex based fixup is a HACK.
are threaded/non-threaded differences considered to be a feature ?
is eliminating the diff a wrong-headed fix ?
But more basically - even under threaded - the regexes dont match what
Im expecting.
Ive gone *blind* looking at it, Im hoping that someone who's
handy with use re 'debug' can *identify* the probs with the Regexs.
Ive taken liberty of pasting in some output, in hopes that it might
entice you to apply the patch and try it..
$> ./perl -Ilib ext/B/t/optree.t
ok 1 - require B::Concise;
not ok 2 - my-var no-init baseline
# Failed test (ext/B/t/optree.t at line 28)
# 'B::Concise::compile(CODE(0x82b5b30))
# 1 <;> nextstate(main -439 optree.t:73) v
# 2 <0> padsv[$a:-439,-438] M/LVINTRO
# 3 <1> leavesub[1 ref] K/REFC,1
# '
# doesn't match '(?ms-xi:^B::Concise::compile\(CODE\(0x[0-9a-f]+\)\)
# 1 <;> (?:db|next)state\(.*?:d+\) v
# 2 <0> padsv\[\$a:-\d+,-\d+\] M/LVINTRO
# 3 <1> leavesub\[1 ref\] K/REFC,1
# $)'
not ok 3 - my-var init w undef
# Failed test (ext/B/t/optree.t at line 28)
# 'B::Concise::compile(CODE(0x82b5c08))
# goto -
# 1 <;> nextstate(main -437 optree.t:82) v
# 2 <0> padsv[$a:-437,-436] sRM*/LVINTRO
# 3 <1> leavesub[1 ref] K/REFC,1
# '
# doesn't match '(?ms-xi:^B::Concise::compile\(CODE\(0x[0-9a-f]+\)\)
# 1 <;> (?:db|next)state\(.*?:d+\) v
# 2 <0> padsv\[\$a:-\d+,-\d+\] sRM\*/LVINTRO
# 3 <1> leavesub\[1 ref\] K/REFC,1
# $)'
In my attempt to figure this out, I added a VERBOSE feature to the test,
This causes the script to write another test program usable for better
isolation.
./perl -Ilib ext/B/t/optree.t VERBOSE | perl -pi -e 's/((not )?ok )/#
$1/g' >junk.pl
single-stepping thru junk.pl might be helpful - though I couldnt
decipher the
clues sufficiently to figure out whats going wrong.
[jimc@harpo bc3]$ more junk.pl
use re 'debug';
# ok 1 - require B::Concise;
# not ok 2 - my-var no-init baseline
$str = q{B::Concise::compile(CODE(0x82b5a6c))
1 <;> nextstate(main -439 optree.t:72) v
2 <0> padsv[$a:-439,-438] M/LVINTRO
3 <1> leavesub[1 ref] K/REFC,1
};
$str =~ m{B::Concise::compile\(CODE\(0x[0-9a-f]+\)\)
1 <;> (?:db|next)state\(.*?:d+\) v
2 <0> padsv\[\$a:-\d+,-\d+\] M/LVINTRO
3 <1> leavesub\[1 ref\] K/REFC,1
}ms or print "doh\n";
$> ./perl -d junk.pl
....
Guessing start of match, REx `B::Concise::compile\(CODE\(0x[0-9a-f]+\)\)
1 <;> (?:db|next...' against `B::Concise::compile(CODE(0x82b5a6c))
1 <;> nextstate(main -...'...
Found floating substr `] M/LVINTRO
3 <1> leavesub[1 ref] K/REFC,1
' at offset 104...
Found anchored substr `B::Concise::compile(CODE(0x' at offset 0...
Guessed: match at offset 0
Matching REx `B::Concise::compile\(CODE\(0x[0-9a-f]+\)\)
1 <;> (?:db|next)state\(.*?:d+\) v
2 <0> padsv\[\$a:-\d+,-\d+\] M/LVINTRO
3 <1> leavesub\[1 ref\] K/REFC,1
...' against `B::Concise::compile(CODE(0x82b5a6c))
1 <;> nextstate(main -...'
Setting an EVAL scope, savestack=6
0 <> <B::Concise::> | 1: EXACT <B::Concise::compile(CODE(0x>
27 <DE(0x> <82b5a6c> | 9: PLUS
ANYOF[0-9a-f] can match 7 times out of
2147483647...
Setting an EVAL scope, savestack=6
34 <b5a6c> <))
1 <> | 21: EXACT <))\n1 <;> >
44 < <;> > <nextsta> | 25: BRANCH
Setting an EVAL scope, savestack=16
44 < <;> > <nextsta> | 26: EXACT <db>
failed...
44 < <;> > <nextsta> | 29: EXACT <next>
48 < next> <state(m> | 32: EXACT <state(>
54 <tate(> <main -4> | 35: MINMOD
54 <tate(> <main -4> | 36: STAR
Setting an EVAL scope, savestack=16
SANY can match 18 times out of 18...
72 <ree.t> <:72) v
> | 38: EXACT <:>
73 <ee.t:> <72) v
2> | 40: PLUS
EXACT <d> can match 0 times out of 2147483647...
Setting an EVAL scope, savestack=16
failed...
SANY can match 22 times out of 22...
94 <sv[$a> <:-439,-> | 38: EXACT <:>
95 <v[$a:> <-439,-4> | 40: PLUS
EXACT <d> can match 0 times out of 2147483647...
Setting an EVAL scope, savestack=16
failed...
failed...
Clearing an EVAL scope, savestack=6..16
failed...
Match failed
doh
thanks in advance for any solutions offered,
jimc
Jim Cromie
>
> Ive gone *blind* looking at it, Im hoping that someone who's
> handy with use re 'debug' can *identify* the probs with the Regexs.
naturally, I saw straight shortly after returning to it.
If you care, the correct line is this.
# allow any file & line (step 2)
$str =~ s/MINWILDMATCH/.*?:\\d+/msg;
Sorry for the chatter
Stephen McCamant
JimC> wrt threaded vs non-threaded;
JimC> mostly, the differences are in [] vs () chars used in
JimC> formatting. however, some opcodes dont do the bracketting
JimC> switch. my regex based fixup is a HACK.
JimC> are threaded/non-threaded differences considered to be a
JimC> feature ?
I think I intended them as one, yes. The idea is that the arguments
should be in brackets exactly when they're stored in the pad (via the
op_targ field) rather than directly via a pointer in the OP. This
subtle difference in how the OP prints corresponds to a subtle
implementation difference, which is supposed to be mainly transparent,
but which you might also want to be aware of.
JimC> is eliminating the diff a wrong-headed fix ?
Concise was meant as a tool for people, and I'd like the default
output format to be optimized for conveying information to people
working on the core, rather than for parsing by programs. It may be
that an alternate output format would be better for automated
comparison (better to leave out the variable information, than
accommodate the variability in the matching).
-- Stephen
Rafael Garcia-Suarez
>
> Heres a 'working' version of my earlier proposal,
> patched against blead@22601
>
> I hope you find it useful for regression testing of the optimizer,
> and the opcode generation phases.
Thanks, applied to bleadperl as change #22664.