"in cleanup" warnings generated from destructor errors are no longer enabled by "use warnings" #12214

p5pRT · 2012-06-22T21:44:42Z

Migrated from rt.perl.org#113794 (status was 'resolved')

Searchable as RT113794$

p5pRT · 2012-06-22T21:44:42Z

From @doy

Created by @doy

../bisect.pl --start=v5.12.4 --end=v5.14.2 --expect-fail -e \
'BEGIN { $SIG{ __WARN__} = sub { exit 1 if $_[0] =~ /in cleanup/ }; }
use strict; use warnings;
package A;
sub new { bless {}, "A"}; sub DESTROY { warn 1; die 2; }
{ my $a = A ->new }'

returns this:

96d9b9c is the first bad commit
commit 96d9b9c
Author: Zefram <zefram@fysh.org>
Date: Tue Apr 20 21:32:53 2010 +0100

make die reliably hand error to post-eval code

Put the exception into $@ last thing before longjmping to the op following
the eval block, where previously it went into $@ before unwinding the
stack. This change means that the exception is not liable to be lost
by $@ being clobbered by destructors, cleanup code, or restoration after
"local $@". The code running immediately after eval can now rely on $@
accurately indicating the exception status of the eval.

:100644 100644 5ca4f13aa00796d3864bcc7258510dd7de56986d 6ae162687a665f15ba1c38080a58c8cc310b65a4 M MANIFEST
:100644 100644 d62d58ada367650c283a1219cf573905d85cbecb 921688d656fcc00f7705487138e247f13d7d0127 M pp_ctl.c
:040000 040000 14e682d7e45cbebfcef4e4a0b14861aa6ad7651c facef45a345cb919ad8120eaabd0711469c9b564 M t
bisect run success
That took 1839 seconds

But this still warns:

$ perl -we 'package A; sub new { bless {}, "A" } sub DESTROY { die "foo" }
{ my $a = A->new }'
(in cleanup) foo at -e line 1.

This seems unintentional. The code seems to still be using the 'misc'
warnings category, so I'm not sure why "use warnings" no longer enables
it.

Perl Info


Flags:
    category=core
    severity=low

Site configuration information for perl 5.16.0:

Configured by doy at Sat Jun  2 13:46:21 CDT 2012.

Summary of my perl5 (revision 5 version 16 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=3.3.5-1-arch, archname=x86_64-linux
    uname='linux xtahua 3.3.5-1-arch #1 smp preempt mon may 7 19:57:51 cest 2012 x86_64 gnulinux '
    config_args='-de -Dprefix=/home/doy/perl5/perlbrew/perls/perl-5.16.0'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.0 20120505 (prerelease)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.15.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.15'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.16.0:
    /home/doy/perl5/local/
    /home/doy/perl5/perlbrew/perls/perl-5.16.0/lib/site_perl/5.16.0/x86_64-linux
    /home/doy/perl5/perlbrew/perls/perl-5.16.0/lib/site_perl/5.16.0
    /home/doy/perl5/perlbrew/perls/perl-5.16.0/lib/5.16.0/x86_64-linux
    /home/doy/perl5/perlbrew/perls/perl-5.16.0/lib/5.16.0
    .


Environment for perl 5.16.0:
    HOME=/home/doy
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/doy/perl5/perlbrew/bin:/home/doy/perl5/perlbrew/perls/perl-5.16.0/bin:/home/doy/.bin/marathon:/home/doy/.bin/nethack:/home/doy/.bin/ghc:/home/doy/.bin:/usr/local/sbin:/usr/local/bin:/usr/lib/ccache/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/bin/vendor_perl:/usr/bin/core_perl
    PERL5LIB=/home/doy/perl5/local/
    PERLBREW_BASHRC_VERSION=0.33
    PERLBREW_HOME=/home/doy/.perlbrew
    PERLBREW_MANPATH=/home/doy/perl5/perlbrew/perls/perl-5.16.0/man
    PERLBREW_PATH=/home/doy/perl5/perlbrew/bin:/home/doy/perl5/perlbrew/perls/perl-5.16.0/bin
    PERLBREW_PERL=perl-5.16.0
    PERLBREW_ROOT=/home/doy/perl5/perlbrew
    PERLBREW_VERSION=0.43
    PERL_BADLANG (unset)
    PERL_CPANM_OPT=-q --mirror file:///home/doy/perl5/minicpan/ --mirror http://mirrors.kernel.org/cpan/ --mirror http://search.cpan.org/CPAN --prompt
    SHELL=/bin/zsh

p5pRT · 2013-03-28T11:09:20Z

From zefram@fysh.org

Jesse Luehrs wrote:

This seems unintentional.

Indeed. The Perl_ck_warner() call in die_unwind() used to happen before
unwinding, so would be affected by the lexical warning state at the die()
site. Now it happens after unwinding, so takes the lexical warning state
at the catching site. I don't have a clear idea of which behaviour is
more correct. t/op/die_keeperr.t, which was introduced as part of my
exception handling changes, is actually testing for the catching-site
criterion, but that's not asserting that the criterion should be that.
The documentation speaks of "no warnings 'misc'", but doesn't say which
lexical scope matters.

Assuming we want to revert this change, the easy fix is to move
the conditional Perl_ck_warner() back to before unwinding. A more
difficult way would be to determine the disposition of the warning
before unwinding and then warn in the required manner after unwinding.
I see no compelling reason to warn after unwinding rather than before,
so just moving the warning code should be fine.

Patch attached.

-zefram

p5pRT · 2013-03-28T11:09:20Z

From zefram@fysh.org

Inline Patch

diff --git a/pp_ctl.c b/pp_ctl.c
index c9d833f..b5daf63 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -1638,6 +1638,11 @@ Perl_die_unwind(pTHX_ SV *msv)
 	    sv_setsv(ERRSV, exceptsv);
 	}
 
+	if (in_eval & EVAL_KEEPERR) {
+	    Perl_ck_warner(aTHX_ packWARN(WARN_MISC), "\t(in cleanup) %"SVf,
+			   SVfARG(exceptsv));
+	}
+
 	while ((cxix = dopoptoeval(cxstack_ix)) < 0
 	       && PL_curstackinfo->si_prev)
 	{
@@ -1696,13 +1701,8 @@ Perl_die_unwind(pTHX_ SV *msv)
 			   SVfARG(exceptsv ? exceptsv : newSVpvs_flags("Unknown error\n",
                                                                     SVs_TEMP)));
 	    }
-	    if (in_eval & EVAL_KEEPERR) {
-		Perl_ck_warner(aTHX_ packWARN(WARN_MISC), "\t(in cleanup) %"SVf,
-			       SVfARG(exceptsv));
-	    }
-	    else {
+	    if (!(in_eval & EVAL_KEEPERR))
 		sv_setsv(ERRSV, exceptsv);
-	    }
 	    PL_restartjmpenv = restartjmpenv;
 	    PL_restartop = restartop;
 	    JMPENV_JUMP(3);
diff --git a/t/op/die_keeperr.t b/t/op/die_keeperr.t
index 9b41cb5..083bd5d 100644
--- a/t/op/die_keeperr.t
+++ b/t/op/die_keeperr.t
@@ -3,7 +3,7 @@
 BEGIN {
     chdir 't' if -d 't';
     require 'test.pl';
-    plan(20);
+    plan(24);
 }
 
 sub End::DESTROY { $_[0]->() }
@@ -31,14 +31,45 @@ foreach my $inx ("", "aabbcc\n", [qw(aa bb cc)]) {
     no warnings "misc";
     my $warn = "";
     local $SIG{__WARN__} = sub { $warn .= $_[0] };
-    { my $e = end { die "aa\n"; }; }
+    { my $e = end { no warnings "misc"; die "aa\n"; }; }
     is $warn, "";
 }
 
 {
+    no warnings "misc";
+    my $warn = "";
+    local $SIG{__WARN__} = sub { $warn .= $_[0] };
+    { my $e = end { use warnings "misc"; die "aa\n"; }; }
+    is $warn, "\t(in cleanup) aa\n";
+}
+
+{
     my $warn = "";
     local $SIG{__WARN__} = sub { $warn .= $_[0] };
     { my $e = end { no warnings "misc"; die "aa\n"; }; }
+    is $warn, "";
+}
+
+{
+    my $warn = "";
+    local $SIG{__WARN__} = sub { $warn .= $_[0] };
+    { my $e = end { use warnings "misc"; die "aa\n"; }; }
+    is $warn, "\t(in cleanup) aa\n";
+}
+
+{
+    use warnings "misc";
+    my $warn = "";
+    local $SIG{__WARN__} = sub { $warn .= $_[0] };
+    { my $e = end { no warnings "misc"; die "aa\n"; }; }
+    is $warn, "";
+}
+
+{
+    use warnings "misc";
+    my $warn = "";
+    local $SIG{__WARN__} = sub { $warn .= $_[0] };
+    { my $e = end { use warnings "misc"; die "aa\n"; }; }
     is $warn, "\t(in cleanup) aa\n";
 }

p5pRT · 2013-03-28T11:09:21Z

The RT System itself - Status changed from 'new' to 'open'

p5pRT · 2013-03-28T11:14:26Z

From zefram@fysh.org

I wrote:

Patch attached.

Bah, breaks an XS-APItest test. That needs updating, similarly to
t/op/die_keeperr.t.

-zefram

p5pRT · 2013-04-23T14:59:37Z

From @rjbs

On Thu Mar 28 04:14:26 2013, zefram@fysh.org wrote:

I wrote:

Patch attached.

Bah, breaks an XS-APItest test. That needs updating, similarly to
t/op/die_keeperr.t.

I have made the update and applied it experimentally in the branch rjbs/rt-113794

I would appreciate some eyes on this before I pull it into blead.

--
rjbs

p5pRT · 2013-04-23T15:22:27Z

From @rjbs

On Thu Mar 28 04:09:20 2013, zefram@fysh.org wrote:

Jesse Luehrs wrote:

This seems unintentional.

Indeed. The Perl_ck_warner() call in die_unwind() used to happen before
unwinding, so would be affected by the lexical warning state at the die()
site. Now it happens after unwinding, so takes the lexical warning state
at the catching site. I don't have a clear idea of which behaviour is
more correct.

I wanted to get on record saying that my attempt to get this patch applied was not (mere)
desperation for closing blockers, but also an endorsement that it restores the more correct
behavior.

--
rjbs

p5pRT · 2013-04-24T16:20:14Z

From @rjbs

I landed this change this morning.

--
rjbs

p5pRT · 2013-04-24T16:20:14Z

From [Unknown Contact. See original ticket]

I landed this change this morning.

--
rjbs

p5pRT · 2013-04-24T16:20:14Z

@rjbs - Status changed from 'open' to 'resolved'

p5pRT closed this as completed Apr 24, 2013

p5pRT added Severity Low distro-Linux type-core labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"in cleanup" warnings generated from destructor errors are no longer enabled by "use warnings" #12214

"in cleanup" warnings generated from destructor errors are no longer enabled by "use warnings" #12214

p5pRT commented Jun 22, 2012

p5pRT commented Jun 22, 2012

p5pRT commented Mar 28, 2013

p5pRT commented Mar 28, 2013

p5pRT commented Mar 28, 2013

p5pRT commented Mar 28, 2013

p5pRT commented Apr 23, 2013

p5pRT commented Apr 23, 2013

p5pRT commented Apr 24, 2013

p5pRT commented Apr 24, 2013

p5pRT commented Apr 24, 2013

"in cleanup" warnings generated from destructor errors are no longer enabled by "use warnings" #12214

"in cleanup" warnings generated from destructor errors are no longer enabled by "use warnings" #12214

Comments

p5pRT commented Jun 22, 2012

p5pRT commented Jun 22, 2012

From @doy

Created by @doy

p5pRT commented Mar 28, 2013

From zefram@fysh.org

p5pRT commented Mar 28, 2013

From zefram@fysh.org

p5pRT commented Mar 28, 2013

p5pRT commented Mar 28, 2013

From zefram@fysh.org

p5pRT commented Apr 23, 2013

From @rjbs

p5pRT commented Apr 23, 2013

From @rjbs

p5pRT commented Apr 24, 2013

From @rjbs

p5pRT commented Apr 24, 2013

From [Unknown Contact. See original ticket]

p5pRT commented Apr 24, 2013