Front page | perl.perl5.porters |
Postings from May 2016
Re: revert MG consting (Coro breakage) for 5.24?
Thread Previous
|
Thread Next
From:
Nicholas Clark
Date:
May 4, 2016 09:08
Subject:
Re: revert MG consting (Coro breakage) for 5.24?
Message ID:
20160504090820.GO2048@ceres.etla.org
On Wed, May 04, 2016 at 08:56:47AM +0100, Dave Mitchell wrote:
> On Tue, May 03, 2016 at 06:09:52PM -0400, Ricardo Signes wrote:
> > * Dave Mitchell <davem@iabyn.com> [2016-05-03T11:56:31]
> > > I propose that we revert this commit for 5.24 (we need another RC anyway
> > > for the SvGROW) issue.
> >
> > I have reverted this in a local branch.
> >
> > It is not clear to me that there is anything much to gain, here. As per my
> > message of a few minutes ago, Coro doesn't seem to need this change, if code is
> > (conditionally?) removed from Coro. Why did no one see this earlier?
>
> I had always vaguely assumed that Coro had been patched up to work with
> 5.22 - I knew that there were efforts to so do - and it was only when
> someone pointed out to me Aristotle's blog entry from yesterday that
> I became aware that it (may) still be an issue.
>
> http://blogs.perl.org/users/aristotle/2016/05/coro-vs-5022.html
>
> I don't know enough about the technical issues with Coro to understand
> whether the vtable consting is actually a constraint on getting Coro to
> run under 5.24.
It is really unclear to me too. The comment in Coro/State.xs is this:
/*
* This overrides the default magic get method of %SIG elements.
* The original one doesn't provide for reading back of PL_diehook/PL_warnhook
* and instead of trying to save and restore the hash elements (extremely slow),
* we just provide our own readback here.
*/
It seems to be working round a bug or deficiency in the core. *but*
There are no tests that actually exercise the problem that it attempts to
fix.
If I disable the code that sets the custom handlers, all tests still pass.
(Tested on 5.8.3 and 5.8.8 too. So it's not a "fixed recently" bug)
leont attempted to merge the change into the core signal handler code
https://rt.perl.org/Public/Bug/Display.html?id=125439#txn-1398485
Core tests pass. But breaks 1 of Coro's t/13_diewarn.t tests.
However, effectively Leon's patch (linked above) misses out one part of
the alteration in Coro/State.xs, namely
if (!*svp)
ssv = &PL_sv_undef;
...
So if I add that, most easily done like this:
diff --git a/mg.c b/mg.c
index 048e324..1f2202a 100644
--- a/mg.c
+++ b/mg.c
@@ -1359,6 +1359,10 @@ Perl_magic_getsig(pTHX_ SV *sv, MAGIC *mg)
sv_setsv(sv, ssv);
return 0;
}
+ if (svp) {
+ sv_setsv(sv, &PL_sv_undef);
+ return 0;
+ }
}
else if (i > 0) {
if(PL_psig_ptr[i])
then core tests fail everywhere, and *so* do Coro's. So that's not correct.
The most minimal problem example is from t/base/lex.t (this is 5.22):
./perl -e 'local $SIG{__WARN__} = sub {}; eval q|my $_|'
Use of my $_ is experimental at (eval 1) line 1.
ie basically the localisation seems to fail. Reason being that the core *needs*
to cause PL_warnhook and $SIG{__WARN__} to diverge, and that code change above
means that (thanks to set magic) any *read* of $SIG{__WARN__} forcibly resets
it back to the current PL_warnhook value. (due to the insanely ugly way that
get magic works, with an API that relies on the called code copying whatever
value it needed into the SV to which magic is attached, instead of a seemingly
more sane "return a value and let my caller deal with it")
The above test case breaks with the change because when invoking the warn
hook, the first thing that happens when trying to invoke the warn hook:
S_invoke_exception_hook(pTHX_ SV *ex, bool warn)
{
HV *stash;
GV *gv;
CV *cv;
SV **const hook = warn ? &PL_warnhook : &PL_diehook;
/* sv_2cv might call Perl_croak() or Perl_warner() */
SV * const oldhook = *hook;
if (!oldhook)
return FALSE;
ENTER;
SAVESPTR(*hook);
*hook = NULL;
cv = sv_2cv(oldhook, &stash, &gv, 0);
LEAVE;
is that the (changed) read code resets the returned hook to undef whilst
trying to read it:
(gdb) where
#0 Perl_magic_getsig (sv=0x8ec358, mg=0x90d990) at mg.c:1363
#1 0x00000000004db8e9 in Perl_mg_get (sv=sv@entry=0x8ec358) at mg.c:197
#2 0x000000000050862c in Perl_sv_2cv (sv=sv@entry=0x8ec358,
st=st@entry=0x7fffffffd758, gvp=gvp@entry=0x7fffffffd760,
lref=lref@entry=0) at sv.c:9754
#3 0x00000000004cde95 in S_invoke_exception_hook (ex=ex@entry=0x900af0,
Perl_warn=Perl_warn@entry=true) at util.c:1525
#4 0x00000000004cfa36 in Perl_vwarn (pat=<optimized out>,
args=<optimized out>) at util.c:1842
#5 0x00000000004cfdd3 in Perl_ck_warner_d (err=err@entry=53,
pat=pat@entry=0x5e77c2 "Use of %s $_ is experimental") at util.c:1908
#6 0x0000000000423b65 in Perl_allocmy (name=0x90ae28 "$_", len=2,
flags=<optimized out>) at op.c:612
#7 0x000000000046d208 in S_pending_ident () at toke.c:8265
#8 Perl_yylex () at toke.c:4347
#9 0x0000000000486da3 in Perl_yyparse (gramtype=55) at perly.c:322
#10 0x0000000000559495 in S_doeval (gimme=1, outside=0x0, seq=8,
hh=0x5f5f4e5241575f5f) at pp_ctl.c:3491
#11 0x000000000056fdb2 in Perl_pp_entereval () at pp_ctl.c:4302
#12 0x00000000004cc82a in Perl_runops_debug () at dump.c:2234
#13 0x000000000044fe42 in S_run_body (oldscope=1) at perl.c:2448
#14 perl_run (my_perl=<optimized out>) at perl.c:2371
#15 0x00000000004209e5 in main (argc=3, argv=0x7fffffffe298,
env=0x7fffffffe2b8) at perlmain.c:116
So the thing is, this makes me think that it the changed signal handlers
Coro installs ought to exhibit this problem.
But while I can get the core to misbehave, I can't figure out how to translate
this to a testcase which passes normally, but fails with Coro loaded.
> > This problem has generated much heat, but little light, and now there's an 11th
> > hour rush to delay a release to make a possibly unneeded change that could've
> > been made months ago. Will we ever find out whether this change is really
> > needed? I have no idea. I imagine, though, that no matter what, there are
> > going to be hard feelings on *some* part for making *or* not making this
> > change.
> >
> > Of course, if that's lose-lose, what's to be lost by applying this patch?
> > Probably nothing.
>
> I proposed making the change at this very late stage because there is
> negligible risk of a downside, there's a probable upside, and a new RC has
> to be released anyway for the SvGROW stuff.
There's a possible upside. But given that
1) it's not actually clear whether these hooks are even needed
2) if they are, it's clear that they're monkeypatching a bug fix into place
which really ought to be *fixed* for everyone, not just Coro
3) other code changes are needed to make Coro work on 5.24.0 and Marc is
on the record as having no interest in supporting 5.22.x or later
then realistically, it's not going to happen.
(I didn't say "don't do it". I said "it's not actually going to fix anything")
I believe that the odds usually quoted for this sort of impossible event
are 1-5000. :-)
Nicholas Clark
Thread Previous
|
Thread Next