perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

$SIG{__DIE__} does not get reliably triggered by compilation errors.

Open demerphq opened this issue 3 years ago • 7 comments

Description During compilation Perl "saves up errors", so that it can show as many errors as possible to the user as it can. When it reaches a threshold (in theory 1 for syntax errors and 10 for other errors but in practice sometimes 11 depending on the error), it will trigger a Perl_croak and stop compilation, this in turn will feed the list of errors encountered to the $SIG{__DIE__} handler, however if it runs off the end of the code without reaching the maximum it will not call Perl_croak when inside of an eval, and instead will stuff the error directly into $@, bypassing $SIG{__DIE__}. Thus $SIG{__DIE__} is inconsistently called by compilation errors.

Steps to Reproduce This demonstrates code with two errors, neither of them syntax errors, which fail the compilation but do not trigger $SIG{__DIE__}:

$ perl -le'use strict; print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval q($z+$y); print "\$@=",$@;'
5.037004
$@=Global symbol "$z" requires explicit package name (did you forget to declare "my $z"?) at (eval 1) line 1.
Global symbol "$y" requires explicit package name (did you forget to declare "my $y"?) at (eval 1) line 1.

This demonstrates code with one error, a syntax error, which fails compilation and does trigger $SIG{__DIE__}:

norole:yorton@oncidium:master:~/git_tree/HTML-Mason-1.59$ perl -le'use strict; print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval q(1=); print "\$@=",$@;'
5.037004
in __DIE__
$@=syntax error at (eval 1) line 1, at EOF

And curiously die inside of a BEGIN triggers $SIG{__DIE__} twice:

perl -le'print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval "BEGIN{die}"; print "\$@=",$@;'
5.037004
in __DIE__
in __DIE__
$@=Died at (eval 1) line 1.
BEGIN failed--compilation aborted at (eval 1) line 1.

Expected behavior I think the ideal would be that each error triggers $SIG{__DIE__} once, regardless as to whether it stops compilation or not. Whether that means saving up a list of errors and calling it once per, or interleaving compilation after error and the call to $SIG{__DIE__} would be an implementation detail. It certainly should call $SIG{__DIE__} at least once before compilation terminates if it encounters an error, it shouldn't be random based on how many preceding errors there were.

Perl configuration

$ perl -V
Summary of my perl5 (revision 5 version 37 subversion 4) configuration:
  Commit id: 1b6686b25071fafd79b9bae431ef8190f3e29d0f
  Platform:
    osname=linux
    osvers=5.14.0-1051-oem
    archname=x86_64-linux-thread-multi
    uname='linux oncidium 5.14.0-1051-oem #58-ubuntu smp fri aug 26 05:50:00 utc 2022 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de -Dcc=ccache gcc -Dld=gcc -Dprefix=/home/yorton/perl5/perlbrew/perls/stop_first_error -Dusethreads -Dusemultiplicity -DDEBUGGING -Dusedevel -Aeval:scriptdir=/home/yorton/perl5/perlbrew/perls/stop_first_error/bin'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-O2 -g'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='9.4.0'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='gcc'
    ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /usr/lib64
    libs=-lpthread -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.31.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.31'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'


Characteristics of this binary (from libperl): 
  Compile-time options:
    DEBUGGING
    HAS_TIMES
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_HASH_FUNC_SIPHASH13
    PERL_HASH_USE_SBOX32
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    PERL_TRACK_MEMPOOL
    PERL_USE_DEVEL
    PERL_USE_SAFE_PUTENV
    USE_64_BIT_ALL
    USE_64_BIT_INT
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
    USE_REENTRANT_API
    USE_THREAD_SAFE_LOCALE
  Built under linux
  Compiled at Sep 13 2022 09:49:21
  %ENV:
    PERLBREW_CONFIGURE_FLAGS="-de -Dcc=ccache\ gcc -Dld=gcc"
    PERLBREW_HOME="/home/yorton/.perlbrew"
    PERLBREW_MANPATH="/home/yorton/perl5/perlbrew/perls/stop_first_error/man"
    PERLBREW_PATH="/home/yorton/perl5/perlbrew/bin:/home/yorton/perl5/perlbrew/perls/stop_first_error/bin"
    PERLBREW_PERL="stop_first_error"
    PERLBREW_ROOT="/home/yorton/perl5/perlbrew"
    PERLBREW_SHELLRC_VERSION="0.88"
    PERLBREW_VERSION="0.88"
  @INC:
    /home/yorton/perl5/perlbrew/perls/stop_first_error/lib/site_perl/5.37.4/x86_64-linux-thread-multi
    /home/yorton/perl5/perlbrew/perls/stop_first_error/lib/site_perl/5.37.4
    /home/yorton/perl5/perlbrew/perls/stop_first_error/lib/5.37.4/x86_64-linux-thread-multi
    /home/yorton/perl5/perlbrew/perls/stop_first_error/lib/5.37.4

demerphq avatar Sep 13 '22 10:09 demerphq

Description During compilation Perl "saves up errors", so that it can show as many errors as possible to the user as it can. When it reaches a threshold (in theory 1 for syntax errors and 10 for other errors but in practice sometimes 11 depending on the error), it will trigger a Perl_croak and stop compilation, this in turn will feed the list of errors encountered to the $SIG{__DIE__} handler, however if it runs off the end of the code without reaching the maximum it will not call Perl_croak when inside of an eval, and instead will stuff the error directly into $@, bypassing $SIG{__DIE__}. Thus $SIG{__DIE__} is inconsistently called by compilation errors.

Steps to Reproduce This demonstrates code with two errors, neither of them syntax errors, which fail the compilation but do not trigger $SIG{__DIE__}:

I have confirmed the 3 anomalies for which you gave examples on threaded builds on FreeBSD with both perl-5.32.1 and blead built at 5d56ab3b35.

jkeenan avatar Sep 13 '22 12:09 jkeenan

*Description During compilation Perl "saves up errors", so that it can show as many errors as possible to the user as it can. When it reaches a threshold (in theory 1 for syntax errors and 10 for other errors but in practice sometimes 11 depending on the error),

What qualifies as 'other' error? I tried with something like;

./perl -Ilib -e '$SIG{__DIE__} = sub { print "in DIE handler: @_"; }; use strict; eval q#$c1;$c2;$c3;$c4;$c5;$c6;$c7;$c8;$c9;$c10;$c11;$c12;$c13;$c14;$c15;$c16;$c17;$c18;#;print "\$@=$@";

but that does not end up in the die-handler despite outputting 18 'errors'.

Steps to Reproduce This demonstrates code with two errors, neither of them syntax errors, which fail the compilation but do not trigger $SIG{__DIE__}:

$ perl -le'use strict; print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval q($z+$y); print "\$@=",$@;'
5.037004
$@=Global symbol "$z" requires explicit package name (did you forget to declare "my $z"?) at (eval 1) line 1.
Global symbol "$y" requires explicit package name (did you forget to declare "my $y"?) at (eval 1) line 1.

Just a note: this goes back to at least perl v5.6.0

This demonstrates code with one error, a syntax error, which fails compilation and does trigger $SIG{__DIE__}:

norole:yorton@oncidium:master:~/git_tree/HTML-Mason-1.59$ perl -le'use strict; print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval q(1=); print "\$@=",$@;'
5.037004
in __DIE__
$@=syntax error at (eval 1) line 1, at EOF

It's important to note that this is a recent change which is the result of commit eb54d46f7264ff7af62c409d8a6ab984a5a34f57. On perl v5.36.0 (and all perls before that) it did not end up in the die-handler when there was only one syntax error. (only when there were 10+ syntax(?) errors it ended up in the die-handler)

And curiously die inside of a BEGIN triggers $SIG{__DIE__} twice:

perl -le'print $]; $SIG{__DIE__}= sub { print STDERR "in __DIE__"; $_[0] }; eval "BEGIN{die}"; print "\$@=",$@;'
5.037004
in __DIE__
in __DIE__
$@=Died at (eval 1) line 1.
BEGIN failed--compilation aborted at (eval 1) line 1.

It does end up twice in the die-handler but with a different message:

$ ./perl -le'$SIG{__DIE__}= sub { print STDERR "in __DIE__: $_[0]"; $_[0] }; eval "BEGIN{die}"; print "\$@=",$@;'
in __DIE__: Died at (eval 1) line 1.

in __DIE__: Died at (eval 1) line 1.
BEGIN failed--compilation aborted at (eval 1) line 1.

$@=Died at (eval 1) line 1.
BEGIN failed--compilation aborted at (eval 1) line 1.

This is due to the code in perl.c: Perl_call_list: it catches the error and then re-throws it by calling Perl_croak (That behavior is present since at least perl v5.6.0)

bram-perl avatar Sep 13 '22 21:09 bram-perl

I'm not sure what should happen with this..

There are two concerns that I have:

  • blead currently already shows a different behavior then v5.36.0 (and all previous versions) when there is only one syntax error in eval (before: die-handler not called, now: die-handler called)
  • you want to call the die-handler in even more situations

These changes could be unanticipated and surprising for the users.. It's changing behavior that existed since - at least - v5.6.0.. I consider the current change (calling die-handler for evals with one syntax error) to be a backwards-incompatible change...

The 'stopping on first syntax error' change is OK in itself since that shouldn't affect legal/working code. Calling die-handler for evals with only one syntax error could alter the behavior of legal/working code.. (obviously depending on what the die-handler does, a simple example: with: $SIG{__DIE__} = sub { exit; } users might be in for a surprise when upgrading)

I think this should be raised to a bigger audience and opinions gathered (and a decision made)... I fear that this may have a big fallout..

(There - obviously - are work-arounds for it but it requires people to adapt the code when upgrading perl)


Regardless of the above, what I think should happen:

  • test should be added (that is: test when the die-handler is called for eval, the behavior changed and "we" weren't aware of it)
  • (assuming the change is kept:) backwards incompatibility section updated in perldelta (to explicitly mention $SIG{__DIE__} and eval; i.e. that it's now called in more cases)

bram-perl avatar Sep 13 '22 21:09 bram-perl

I'm not sure what should happen with this..

Options to get more information about possible fallout include:

  • push it to a smoke-me branch
  • merge it for a development release

It seems clear to me that die handlers should get called on death (and only once!), and that this is therefore a bug which we would ideally fix. If it turns out to cause a lot of fallout, we might need to defer the fix or provide other mitigation (including, in extremis, not fixing it at all). But the best way to get the information is to make code available that fixes it by some route that will get it some automated CPAN testing.

I think I remember that smoke-me branches tend to get some CPAN testing, but I don't remember how much; @jkeenan would probably have a better idea.

FWIW I've used die handlers a lot, in both work and personal code, and have never knowingly encountered this bug (though I may occasionally have had unexpected behaviour that I was unable to diagnose). For the ways I have used it there is no situation in which I wouldn't want the die handler called on death, nor one in which I'd want it called twice.

hvds avatar Sep 14 '22 00:09 hvds

There are two concerns that I have:

* blead currently already shows a different behavior then v5.36.0 (and all previous versions) when there is only one syntax error in `eval` (before: die-handler not called, now: die-handler called)

* you want to call the die-handler in even more situations

These changes could be unanticipated and surprising for the users.. It's changing behavior that existed since - at least - v5.6.0..

I share these concerns.

I consider the current change (calling die-handler for evals with one syntax error) to be a backwards-incompatible change...

[snip]

I think this should be raised to a bigger audience and opinions gathered (and a decision made)... I fear that this may have a big fallout..

(There - obviously - are work-arounds for it but it requires people to adapt the code when upgrading perl)

I agree.

jkeenan avatar Sep 14 '22 00:09 jkeenan

I think I remember that smoke-me branches tend to get some CPAN testing, but I don't remember how much; @jkeenan would probably have a better idea.

Unfortunately, we have no systematic way of getting CPANtesters-style feedback on non-blead branches in the core distribution.

smoke-me branches can be very useful along two dimensions: (1) getting feedback on platforms other than Linux or Windows; (2) multiplying the number of different build configurations. But to be useful there must be humans who have an interest in, and access to, the platforms mentioned in (1). And they only exercise the core distribution -- not CPAN modules built on top of the core distro.

In principle, you get install a non-blead branch (e.g., a smoke-me branch), use cpan or cpanm to install a list of 500 to 2000 CPAN distros against that perl, and then examine logs to see what FAILed. But to have meaningful results, you'd first have to install blead and install all those CPAN distros against it. And you need a human to look over the results and distinguish between failures caused by the changes in the smoke-me branch and failures because a particular distro has been failing its tests for the past twelve years.

This is, in essence, what I was doing in my "test CPAN River against dev releases" work in the period 2017-2020, but it was a lot of work, especially for one person, and the amount of attention it garnered was limited.

jkeenan avatar Sep 14 '22 01:09 jkeenan

On Wed, 14 Sept 2022 at 02:57, James E Keenan @.***> wrote:

There are two concerns that I have:

  • blead currently already shows a different behavior then v5.36.0 (and all previous versions) when there is only one syntax error in eval (before: die-handler not called, now: die-handler called)

  • you want to call the die-handler in even more situations

These changes could be unanticipated and surprising for the users.. It's changing behavior that existed since - at least - v5.6.0..

I share these concerns.

I consider the current change (calling die-handler for evals with one syntax error) to be a backwards-incompatible change...

[snip]

I think this should be raised to a bigger audience and opinions gathered (and a decision made)... I fear that this may have a big fallout..

(There - obviously - are work-arounds for it but it requires people to adapt the code when upgrading perl)

I agree.

I strongly disagree. This is a long standing bug, and we should just fix it. It has always been present. It is surprising, and if it had come to our attention any other way we would consider it a bug.

It's a bug, lets fix it and move on.

Yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

demerphq avatar Sep 14 '22 05:09 demerphq

Yes, https://github.com/Perl/perl5/pull/20357 is the patch to fix this.

demerphq avatar Oct 18 '22 16:10 demerphq

Hi, somehow this didnt get posted days ago when it should have been. My apologies for the long response, I was sick for a few weeks and lost track of things.

Regarding the issue of the count of syntax errors triggering a Perl_croak. Personally I consider this a red-herring. Even if we hadn't applied that patch the inconsistency from the point of view of a given error would exist, it just wouldn't be as obvious.

The final behavior (does $SIG{__DIE__} get called) is sensitive to the content of the string and the exact error involved:

  1. Certain compile time errors always triggered Perl_croak. I do not know what they all are yet, but one of the them is unterminated s/// operators. Another was 10 or more syntax errors in a piece of code. There are a few more I believe.
  2. Certain others don't count towards the syntax error threshold at all. For instance 'use strict' violations are not "syntax errors", and do not count towards the threshold at all (whether this is a bug or not is debatable, I consider it at the very least problematic as it means a carefully constructed string can become O(N*2) in terms of compile time).
  3. The underlying nature of this is that anyone writing code that uses $SIG{__DIE__} and eval STRING has to account for the fact that code might die (eg the eval fail) but $SIG{__DIE__} won't have been called, but given the subtleties involved it is very easy to imagine someone never noticing that their code isn't handling a case that /could/ happen if perchance the stars align just right. That isn't the basis for a sane mental model of how $SIG{__DIE__} works.
  4. People could get the idea that $SIG{__DIE__} is only called if the code actually compiled and started running, but we know that isn't true, the code s/ will trigger $SIG{__DIE__} in any perl, the code 1+; will trigger it in new perls, the code 1+;1+;1+;1+;1+;1+;1+;1+;1+;1+;1+; will trigger it in any Perl. People could get the idea that $SIG{__DIE__} is called always, but we know that isn't true, the code use strict; $undeclared_var=1; will not trigger it.
  5. Currently we only have some general idea about which compile issues will trigger $SIG{__DIE__} and which wont. We don't document it, and what happens in different cases seems to be somewhat haphazard. I think changing this to be consistent is worth the pain.

What this means is that any test that fails due to the new syntax error change would have failed if their test case contained an unterminated s/// operator, or if 10 syntax errors had been in their test case. If their test fails then it says to me they are simply not handling valid cases and their test is incomplete. Same for this change in behavior with $SIG{__DIE__}. Likewise for the opposite, anybody only testing things like s/ or eval "BEGIN{die}" might not notice that sometimes compile time errors dont trigger $SIG{__DIE__}.

So I personally view this is a bug. However I admit the language of the docs leaves some room for debate, for instance the docs do not specifically say that compile errors will be handled by $SIG{__DIE__}, but clearly some already are, and personally I think when the docs are ambiguous it is prudent to choose the interpretation that leads to consistency. So either we change compilation so none of the failed compile routes call $SIG{__DIE__} or we change it so that they all do. Since we would expect die hooks to be called in eval "BEGIN { die 'whatever' } print 'x'" which is executed during the compilation phase of the code involved, it seems that at least some errors during compilation must call the die hooks, so it seems reasonable to assume they all should.

demerphq avatar Oct 21 '22 12:10 demerphq