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

Owner: Nobody
Requestors: corion [at] cpan.org
Cc:
AdminCc:

Operating System: mswin32
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.23.0
Fixed In: (no value)

Attachments
0001-Add-failing-test-for-setting-PL_warnhook-via-XS.patch
0001-Handle-warnhook-and-diehook-better-is-SIG-get.patch
0001-SIG-__WARN__-always-accesses-PL_warnhook-always-a-co.patch



Date: Fri, 19 Jun 2015 15:48:03 +0200
From: Max Maischein <corion [...] cpan.org>
To: perlbug [...] perl.org
Subject: $SIG{__WARN__} and PL_warnhook can have different values (but shouldn't)
Download (untitled) / with headers
text/plain 4.5k
This is a bug report for perl from corion@cpan.org, generated with the help of perlbug 1.40 running under perl 5.23.0. ----------------------------------------------------------------- [Please describe your issue here] After thinking a bit more about the problems of Coro and %SIG, I think that %SIG suffers from the same issue that $$ suffers from (and was patched by me). $SIG{__WARN__} and PL_warnhook are two different values that should always be the same, but in practice can deviate. For example, PerlIO_find_layer assigns directly to PL_warnhook without updating $SIG{__WARN__} , and buggy XS modules could do the same. I think Perl should take the code from Coro to make $SIG{__WARN__} always write+read PL_warnhook . Analogous for $SIG{__DIE__} . The code to do this can be found in Coro/State.xs , but if nobody comes up with a compelling reason to keep $SIG{__WARN__} and PL_warnhook separate, I will submit a patch to mg.c , which adds the appropriate code branch to Perl_magic_getsig() to return PL_warnhook / PL_diehook for __WARN__ and __DIE__ . With this change, Coro could eliminate its workaround of patching the magic vtable entries for sig magic and buggy XS modules can't make PL_warnhook and $SIG{__WARN__} different anymore . There is a price to pay in the CPU cost of the branch in Perl_magic_getsig() for handling __WARN__ and __DIE__ , so code reading very often from %SIG or $SIG{__WARN__} might experience a slowdown. -max [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- This perlbug was built using Perl 5.20.1 - Mon Sep 15 13:28:28 2014 It is being executed now by Perl 5.23.0 - Tue Jun 9 20:51:10 2015. Site configuration information for perl 5.23.0: Configured by Corion at Tue Jun 9 20:51:10 2015. Summary of my perl5 (revision 5 version 23 subversion 0) configuration: Derived from: 268da237d4c8792097559baa7a7581be61dc179e Platform: osname=MSWin32, osvers=6.1, archname=MSWin32-x64-multi-thread uname='' config_args='undef' hint=recommended, useposix=true, d_sigaction=undef useithreads=define, usemultiplicity=define use64bitint=define, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='gcc', ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fwrapv -fno-strict-aliasing -mms-bitfields', optimize='-s -O2', cppflags='-DWIN32' ccversion='', gccversion='4.8.3', gccosandvers='' intsize=4, longsize=4, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=8, longdblkind=3 ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='long long', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='g++', ldflags ='-s -L"c:\bleadperl\lib\CORE" -L"C:\MinGW\lib"' libpth=C:\MinGW\lib libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32 perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32 libc=, so=dll, useshrplib=true, libperl=libperl523.a gnulibc_version='' Dynamic Linking: dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' ' cccdlflags=' ', lddlflags='-mdll -s -L"c:\bleadperl\lib\CORE" -L"C:\MinGW\lib"' Locally applied patches: uncommitted-changes --- @INC for perl 5.23.0: c:/Users/Corion/Projekte/bleadperl/lib . --- Environment for perl 5.23.0: HOME (unset) LANG (unset) LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=c:\strawberry-perl-5.20.1.1-x64\perl\bin;c:\strawberry-perl-5.20.1.1-x64\c\bin;c:\strawberry-perl-5.20.1.1-x64\perl\site\bin;C:\Program Files\Microsoft IntelliType Pro\;C:\Program Files (x86)\NVIDIA Corporation\PhysX\Common;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Program Files (x86)\Git\cmd;C:\Program Files\PuTTY;C:\Program Files\MiKTeX 2.9\miktex\bin\x64\;C:\Program Files\nodejs\;C:\strawberry-perl-5.20.1.1-x64\c\bin;C:\strawberry-perl-5.20.1.1-x64\perl\site\bin;C:\strawberry-perl-5.20.1.1-x64\perl\bin;C:\Program Files (x86)\Skype\Phone\;C:\Users\Corion\AppData\Roaming\npm;C:\Program Files\Oracle\VirtualBox PERL_BADLANG (unset) SHELL (unset)
Date: Wed, 24 Jun 2015 13:35:05 +0100
From: Dave Mitchell <davem [...] iabyn.com>
To: perl5-porters [...] perl.org
Subject: Re: [perl #125439] $SIG{__WARN__} and PL_warnhook can have different values (but shouldn't)
Download (untitled) / with headers
text/plain 1.4k
On Fri, Jun 19, 2015 at 06:48:41AM -0700, Max Maischein wrote: Show quoted text
> After thinking a bit more about the problems of Coro and %SIG, I > think that %SIG suffers from the same issue that $$ suffers from > (and was patched by me). $SIG{__WARN__} and PL_warnhook are two > different values that should always be the same, but in practice > can deviate. > > For example, PerlIO_find_layer assigns directly to PL_warnhook > without updating $SIG{__WARN__} , and buggy XS modules could > do the same. > > I think Perl should take the code from Coro to make $SIG{__WARN__} > always write+read PL_warnhook . Analogous for $SIG{__DIE__} . > > The code to do this can be found in Coro/State.xs , but if nobody > comes up with a compelling reason to keep $SIG{__WARN__} > and PL_warnhook separate, I will submit a patch to mg.c , which > adds the appropriate code branch to Perl_magic_getsig() to return > PL_warnhook / PL_diehook for __WARN__ and __DIE__ . > > With this change, Coro could eliminate its workaround of patching > the magic vtable entries for sig magic and buggy XS modules can't > make PL_warnhook and $SIG{__WARN__} different anymore . There is > a price to pay in the CPU cost of the branch in Perl_magic_getsig() > for handling __WARN__ and __DIE__ , so code reading very > often from %SIG or $SIG{__WARN__} might experience a slowdown.
+1 -- "Do not dabble in paradox, Edward, it puts you in danger of fortuitous wit." -- Lady Croom, "Arcadia"
Subject: Re: [perl #125439] $SIG{__WARN__} and PL_warnhook can have different values (but shouldn't)
Date: Mon, 28 Sep 2015 21:30:01 +0200
From: Max Maischein <corion [...] cpan.org>
To: perlbug-followup [...] perl.org
Download (untitled) / with headers
text/plain 2.3k
Hi all, Am 24.06.2015 um 14:35 schrieb Dave Mitchell via RT: Show quoted text
> On Fri, Jun 19, 2015 at 06:48:41AM -0700, Max Maischein wrote:
>> After thinking a bit more about the problems of Coro and %SIG, I >> think that %SIG suffers from the same issue that $$ suffers from >> (and was patched by me). $SIG{__WARN__} and PL_warnhook are two >> different values that should always be the same, but in practice >> can deviate. >> >> For example, PerlIO_find_layer assigns directly to PL_warnhook >> without updating $SIG{__WARN__} , and buggy XS modules could >> do the same. >> >> I think Perl should take the code from Coro to make $SIG{__WARN__} >> always write+read PL_warnhook . Analogous for $SIG{__DIE__} . >>
> +1 >
I've attached a patch that adds a (failing) test for this to XS::APItest. Unfortunately, all my changes to mg.c trying to make Perl_magic_setsig write to PL_warnhook and Perl_magic_getsig to read from PL_warnhook made this test pass but various parts of the test suite fail in nonobvious ways. What I've tried is basically: int Perl_magic_getsig(pTHX_ SV *sv, MAGIC *mg) { STRLEN siglen; const char *s = MgPV_const(mg,siglen); /* Are we fetching a signal entry? */ int i = (I16)mg->mg_private; PERL_ARGS_ASSERT_MAGIC_GETSIG; if (!i) { STRLEN siglen; mg->mg_private = i = whichsig_pvn(s, siglen); } if (*s == '_') { SV **svp = 0; if (memEQs(s, siglen, "__DIE__")) svp = &PL_diehook; else if (memEQs(s, siglen, "__WARN__")) { svp = &PL_warnhook; printf("Get: Read PL_warnhook is %08x\n", PL_warnhook); }; if (svp) { SV *ssv; if (!*svp) ssv = &PL_sv_undef; else if (SvTYPE (*svp) == SVt_PVCV) // thanks, PerlIO ssv = sv_2mortal (newRV_inc (*svp)); else ssv = *svp; sv_setsv (sv, ssv); } } else if (i > 0) { I hope somebody sees what's obviously wrong with this, as PL_warnhook doesn't seem to keep its value and parts of PL_warnhook don't seem to get called. I've tried with the following oneliner: ..\perl.exe -e "BEGIN{$|=1;$SIG{__WARN__}=sub{eval {print$_[0]}; die qq(bar\n)}; warn qq(foo\n)}" The expected output is foo bar BEGIN failed--compilation aborted at -e line 1. The output I get is foo Frustrated, -max

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

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 426b
On Mon Sep 28 12:31:00 2015, corion@cpan.org wrote: Show quoted text
> Hi all, >
[snip] Show quoted text
> >
> I've attached a patch that adds a (failing) test for this to > XS::APItest.
A non-essential question: Would it be possible to place these tests in an existing file underneath ext/XS-APItest/t/ ? One less test file to run on systems that have a startup penalty for each such file. Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 606b
On Wed Jun 24 05:35:29 2015, davem wrote: Show quoted text
> +1
+1 I cleaned up Coro to work on 5.22 in https://github.com/rurban/Coro/tree/5.22 by removing all the crazy workarounds schmorp had to do to support this arguable core bug. His problem is to override the global mg vtbl, but perl5 now only allows attaching magic to objects, not changing the global default behavior at all anymore. That change is questionable. But: But the underlying problem is what Max says. A quirks. With my 5.22 Coro most localized warning and die handler work fine, just not all. I rather urge core to fix that also. -- Reini Urban
From: Max Maischein <corion [...] cpan.org>
To: perlbug-followup [...] perl.org
Subject: Re: [perl #125439] $SIG{__WARN__} and PL_warnhook can have different values (but shouldn't)
Date: Fri, 16 Oct 2015 21:45:06 +0200
Hello all, Show quoted text
> His problem is to override the global mg vtbl, but perl5 now only
allows attaching magic to objects, not changing the global default behavior at all anymore. That change is questionable. But: Show quoted text
> But the underlying problem is what Max says. A quirks. > > With my 5.22 Coro most localized warning and die handler work fine, just not all. > I rather urge core to fix that also.
I have attached my work in progress. Moving the getsig() access to always use PL_warnhook is more convoluted than I first thought, because S_invoke_exception() sets PL_warnhook to NULL temporarily and _then_ invokes get-magic on $SIG{__WARN__}. I've currently hacked around this by assuming that PL_warnhook being NULL implies to leave the passed in SV unchanged as it presumably already has the appropriate value. The attached changes still fail a lot of the test suite, most likely because somewhere I'm going horribly wrong with the memory management. If somebody more familiar with XS spots some of the more egregious bad things I do and corrects them, I'd be more than happy! -max

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

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Fri Oct 16 12:45:50 2015, corion@cpan.org wrote: Show quoted text
> Hello all, >
> > His problem is to override the global mg vtbl, but perl5 now only
> allows attaching magic to objects, not changing the global default > behavior at all anymore. That change is questionable. But:
> > But the underlying problem is what Max says. A quirks. > > > > With my 5.22 Coro most localized warning and die handler work fine, > > just not all. > > I rather urge core to fix that also.
> > I have attached my work in progress. Moving the getsig() access to > always use PL_warnhook is more convoluted than I first thought, > because > S_invoke_exception() sets PL_warnhook to NULL temporarily and _then_ > invokes get-magic on $SIG{__WARN__}. I've currently hacked around this > by assuming that PL_warnhook being NULL implies to leave the passed in > SV unchanged as it presumably already has the appropriate value. > > The attached changes still fail a lot of the test suite, most likely > because somewhere I'm going horribly wrong with the memory management. > If somebody more familiar with XS spots some of the more egregious bad > things I do and corrects them, I'd be more than happy! > > -max
I based this patch on yours. It passes the tests, but doesn't quite fix Coro (one failing test). I've been looking into it, but something is still evading me (probably PL_warnhook == NULL related). Leon
Subject: 0001-Handle-warnhook-and-diehook-better-is-SIG-get.patch
From 4c4fd04d526aec1a032f55c550202ad80a51ef58 Mon Sep 17 00:00:00 2001 From: Leon Timmermans <fawaka@gmail.com> Date: Mon, 2 May 2016 19:23:19 +0200 Subject: [PATCH] Handle warnhook and diehook better is %SIG get --- mg.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/mg.c b/mg.c index 064a1ae..048e324 100644 --- a/mg.c +++ b/mg.c @@ -1335,15 +1335,32 @@ Perl_magic_getsig(pTHX_ SV *sv, MAGIC *mg) /* Are we fetching a signal entry? */ int i = (I16)mg->mg_private; + STRLEN siglen; + const char *s = MgPV_const(mg,siglen); + PERL_ARGS_ASSERT_MAGIC_GETSIG; if (!i) { - STRLEN siglen; - const char * sig = MgPV_const(mg, siglen); - mg->mg_private = i = whichsig_pvn(sig, siglen); + mg->mg_private = i = whichsig_pvn(s, siglen); } - if (i > 0) { + if (*s == '_') { + SV **svp = 0; + if (memEQs(s, siglen, "__DIE__")) + svp = &PL_diehook; + else if (memEQs(s, siglen, "__WARN__")) + svp = &PL_warnhook; + if (svp && *svp) { + SV *ssv; + if (SvTYPE (*svp) == SVt_PVCV) /* thanks, PerlIO*/ + ssv = sv_2mortal(newRV_inc (*svp)); + else + ssv = *svp; + sv_setsv(sv, ssv); + return 0; + } + } + else if (i > 0) { if(PL_psig_ptr[i]) sv_setsv(sv,PL_psig_ptr[i]); else { -- 2.8.2-433-g5ace313
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 375b
On Tue May 03 00:58:59 2016, LeonT wrote: Show quoted text
> I based this patch on yours. It passes the tests, but doesn't quite > fix Coro (one failing test). I've been looking into it, but something > is still evading me (probably PL_warnhook == NULL related). > > Leon
+ const char *s = MgPV_const(mg,siglen); Dont fetch/execute that unless i > 0. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 908b
On Tue May 03 00:58:59 2016, LeonT wrote: Show quoted text
> I based this patch on yours. It passes the tests, but doesn't quite > fix Coro (one failing test). I've been looking into it, but something > is still evading me (probably PL_warnhook == NULL related). > > Leon
Nicholas Clark posted a variation of the patch to <http://www.nntp.perl.org/group/perl.perl5.porters/2016/05/msg236187.html>, which gets all of Coro’s and all of core’s tests passing. Note also that he points out a real Perl bug (not including Coro) that has to do with the warnhook/__WARN__ discrepancy, at <http://www.nntp.perl.org/group/perl.perl5.porters/2016/05/msg236201.html>. I don’t know whether the patch fixes it, but if it does, it should be added to the test suite and the patch applied. (But note that the patch is not necessary for Coro to work. See <86840949-52FC-4F12-A3E2-628EEAAE0C66@cpan.org>.) -- Father Chrysostomos


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