Front page | perl.perl5.porters |
Postings from May 2016
Re: revert MG consting (Coro breakage) for 5.24?
From:
Nicholas Clark
Date:
May 4, 2016 10:44
Subject:
Re: revert MG consting (Coro breakage) for 5.24?
Message ID:
20160504104345.GP2048@ceres.etla.org
On Wed, May 04, 2016 at 10:08:20AM +0100, Nicholas Clark wrote:
> 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*
It both is and it isn't. As best I can figure out, it's working around a
deficiency in how it would *like* the core to behave.
It would like the core to work such that it's viable to localise
$SIG{__WARN__} by writing to PL_warnhook (and similar to die) directly.
ie not to have to go through the more complex APIs to localise the key
'__WARN__' of the HV %SIG, and have the various magic callbacks fire and
keep the relevant interpreter pointers intact.
So, to do *that*, it overrides the core magic on %SIG to behave differently,
to facilitate the way that it would like the world to be.
For example, the changelog for 4.744 is this:
4.744 Tue Jul 8 22:06:35 CEST 2008
- correctly provide default DIE/WARN handlers as documented.
- also overwrite PL_vtbl_sigelem.svt_clear, even though current
implementations inside perl work fine for us.
1) Why override something that is already working?
2) Look at this change, that seems to be needed to achieve the above.
Any code review would flag this as "relying on implementation details":
diff --git a/Coro/State.pm b/Coro/State.pm
index 28247a6..8e83bed 100644
--- a/Coro/State.pm
+++ b/Coro/State.pm
@@ -90,16 +90,23 @@ sub warnhook { &$WARNHOOK }
use XSLoader;
BEGIN {
- our $VERSION = 4.742;
+ our $VERSION = 4.744;
# must be done here because the xs part expects it to exist
# it might exist already because Coro::Specific created it.
$Coro::current ||= { };
- $SIG{__DIE__} = \&diehook;
- $SIG{__WARN__} = \&warnhook;
+ {
+ # save/restore the handlers before/after overwriting %SIG magic
+ local $SIG{__DIE__};
+ local $SIG{__WARN__};
- XSLoader::load __PACKAGE__, $VERSION;
+ XSLoader::load __PACKAGE__, $VERSION;
+ }
+
+ # need to do it after overwriting the %SIG magic
+ $SIG{__DIE__} ||= \&diehook;
+ $SIG{__WARN__} ||= \&warnhook;
}
use Exporter;
If you're relying on precise sequencing of scopes, hash keys existing, and
localising them to undefined values, *so that your monkey patched signal
handler changes* work, then you can't *with a clear conscience* say that your
code is sticking to the intent of an API.
It's utterly relying on implementation details.
[This code has subsequently been removed with the change from 5.x to 6.0]
The reason that it can get away with sv_setsv(sv, &PL_sv_undef); in its
injected signal handler is because *its* subroutine set for PL_warnhook
was never set via assignment to %SIG, so doesn't have sigelem magic on it.
So sv_setsv() on *that* SV doesn't trigger side effects.
Is that playing by the rules?
> 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.
So if you take Leon's patch at that URL, and amend it like this:
diff --git a/mg.c b/mg.c
index 048e324..d0f11af 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])
@@ -1638,7 +1642,7 @@ Perl_magic_setsig(pTHX_ SV *sv, MAGIC *mg)
(void)rsignal(i, PL_csighandlerp);
}
else
- *svp = SvREFCNT_inc_simple_NN(sv);
+ *svp = newSVsv(sv);
} else {
if (sv && SvOK(sv)) {
s = SvPV_force(sv, len);
@@ -1677,7 +1681,7 @@ Perl_magic_setsig(pTHX_ SV *sv, MAGIC *mg)
if (i)
(void)rsignal(i, PL_csighandlerp);
else
- *svp = SvREFCNT_inc_simple_NN(sv);
+ *svp = newSVsv(sv);
}
}
so that it
1) *does* return undef when the hook is NULL
2) but also copies values to the hook (instead of aliasing them), so that
hook values *don't* have magic
then
1) All core tests pass
2) All Coro's tests pass *even with all the hookery disabled*
3) The internal state of PL_warnhook and PL_diehook is consistent with how
Coro would like them to be (magic free, so no side effects on reading)
> 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
So I would suggest that the least worst thing to do is to
1) take Leon's patch, ammend it as I have suggested clean it up
(I have duplicate code in there, and bulk88 made a comment on RT too about
a tweak),
2) review it carefully
3) Manually test a chunk of code on CPAN
(All 6 things that modify PL_diehook, and all 4 that modify PL_warnhook
http://grep.cpan.me/?q=PL_diehook+-file%3Appport.h
http://grep.cpan.me/?q=PL_warnhook
)
4) Commit it to *blead*
5) Wait for the BBC, and hope that it doesn't call
(Where would we be without Andreas? Thanks, Andreas, for keeping us honest)
6) If so, backport it (to maint-5.24 and to maint-5.22)
7) Offer patches, (to Marc, and directly to downstream maintainers. eg
"Hi Dom")
On the assumption that this fixes the actual problems that Coro hit,
short of test cases to demonstrate otherwise.
Nicholas Clark