Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

base.pm broken by CORE::GLOBAL::require #13585

Closed
p5pRT opened this issue Feb 6, 2014 · 6 comments
Closed

base.pm broken by CORE::GLOBAL::require #13585

p5pRT opened this issue Feb 6, 2014 · 6 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 6, 2014

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

Searchable as RT121196$

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2014

From zefram@fysh.org

Created by zefram@fysh.org

If a CORE​::GLOBAL​::require is set up to wrap the standard require, if
it's done in the straightforward way, it breaks base.pm's error message
checking that tries to detect missing modules.

$ perl -lwe 'package Foo​::Bar { sub zzz {} } use base "Foo​::Bar"; print "OK"'
OK
$ perl -lwe 'BEGIN { *CORE​::GLOBAL​::require = sub { require $_[0] }; } package Foo​::Bar { sub zzz {} } use base "Foo​::Bar"; print "OK"'
Can't locate Foo/Bar.pm in @​INC (you may need to install the Foo​::Bar module) (@​INC contains​: /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/site_perl/5.19.8/x86_64-linux-thread-multi /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/site_perl/5.19.8 /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8/x86_64-linux-thread-multi /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8 .) at -e line 1.
  ...propagated at /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8/base.pm line 110.
BEGIN failed--compilation aborted at -e line 1.

I'd call that require override well-behaved, in the relevant respects.
It seems excessive to demand that overrides fix up error locations.

This problem can be avoided by base.pm using "CORE​::require" to suppress
the override. But that raises other problems. On some older perl
versions, Lexical​::SealRequireHints will use a CORE​::GLOBAL​::require to
work around a hint leakage bug, if its XS version can't be used.

Module​::Runtime suffered the same bug. I fixed it there by using
"CORE​::require". In that case I'm not concerned about breaking
Lexical​::SealRequireHints, because M​:R incorporates the fix that L​:SRH
would perform via the override.

base.pm could perhaps incorporate the same fix, but M​:R and base.pm
already duplicate a bunch of logic. Perhaps base.pm should use M​:R's
use_package_optimistically(), though that would require dual-lifing M​:R.
It would be the better factoring, as use_package_optimistically() was
specifically designed to provide this subpart of base.pm's operation
in isolation.

Another option is to drop the part of the error message matching that
is specific about location. The other parts probably suffice.

Perl Info

Flags:
    category=library
    severity=low
    module=base

Site configuration information for perl 5.19.8:

Configured by zefram at Tue Jan 21 23:12:46 GMT 2014.

Summary of my perl5 (revision 5 version 19 subversion 8) configuration:
   
  Platform:
    osname=linux, osvers=3.2.0-4-amd64, archname=x86_64-linux-thread-multi
    uname='linux barba.rous.org 3.2.0-4-amd64 #1 smp debian 3.2.46-1 x86_64 gnulinux '
    config_args='-des -Dprefix=/home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52 -Duselargefiles -Dusethreads -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dusedevel -Uversiononly -Ui_db'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2', 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 /usr/lib/gcc/x86_64-linux-gnu/4.7/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.17'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8/x86_64-linux-thread-multi/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'



@INC for perl 5.19.8:
    /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/site_perl/5.19.8/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/site_perl/5.19.8
    /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8/x86_64-linux-thread-multi
    /home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/lib/5.19.8
    .


Environment for perl 5.19.8:
    HOME=/home/zefram
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/zefram/usr/perl/perl_install/perl-5.19.8-i64-f52/bin:/home/zefram/usr/perl/util:/home/zefram/pub/x86_64-unknown-linux-gnu/bin:/home/zefram/pub/common/bin:/usr/bin:/bin:/usr/local/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh

@p5pRT
Copy link
Author

p5pRT commented Feb 17, 2014

From @tonycoz

On Thu Feb 06 15​:12​:28 2014, zefram@​fysh.org wrote​:

I'd call that require override well-behaved, in the relevant respects.
It seems excessive to demand that overrides fix up error locations.

This problem can be avoided by base.pm using "CORE​::require" to
suppress
the override. But that raises other problems. On some older perl
versions, Lexical​::SealRequireHints will use a CORE​::GLOBAL​::require
to
work around a hint leakage bug, if its XS version can't be used.

Module​::Runtime suffered the same bug. I fixed it there by using
"CORE​::require". In that case I'm not concerned about breaking
Lexical​::SealRequireHints, because M​:R incorporates the fix that L​:SRH
would perform via the override.

base.pm could perhaps incorporate the same fix, but M​:R and base.pm
already duplicate a bunch of logic. Perhaps base.pm should use M​:R's
use_package_optimistically(), though that would require dual-lifing
M​:R.
It would be the better factoring, as use_package_optimistically() was
specifically designed to provide this subpart of base.pm's operation
in isolation.

M​::R's functionality would be useful in core, but I'm not sure I'd want to do it now, with 5.20 coming close.

Another option is to drop the part of the error message matching that
is specific about location. The other parts probably suffice.

Attached is a patch to do just that. (I have a separate one to bump $base​::VERSION)

I'll apply this soonish, unless there's an objection.

Tony

@p5pRT
Copy link
Author

p5pRT commented Feb 17, 2014

From @tonycoz

0001-perl-121196-only-examine-the-name-being-included.patch
From b7426923f255f834dcf35cc83de2eb4c983e3ac0 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 17 Feb 2014 15:19:34 +1100
Subject: [PATCH 1/2] [perl #121196] only examine the name being included

Checking the location called from broke require overrides.
---
 MANIFEST                  |    1 +
 dist/base/lib/base.pm     |    6 ++----
 dist/base/t/core-global.t |   20 ++++++++++++++++++++
 3 files changed, 23 insertions(+), 4 deletions(-)
 create mode 100644 dist/base/t/core-global.t

diff --git a/MANIFEST b/MANIFEST
index e5dab21..8cee6d3 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -2894,6 +2894,7 @@ dist/base/t/base-open-chunk.t	See if base works
 dist/base/t/base-open-line.t	See if base works
 dist/base/t/base.t		See if base works
 dist/base/t/compile-time.t	See if base works
+dist/base/t/core-global.t	See if base works around CORE::GLOBAL::require
 dist/base/t/fields-5_6_0.t	See if fields work
 dist/base/t/fields-5_8_0.t	See if fields work
 dist/base/t/fields-base.t	See if fields work
diff --git a/dist/base/lib/base.pm b/dist/base/lib/base.pm
index d7ef70a..55d3b47 100644
--- a/dist/base/lib/base.pm
+++ b/dist/base/lib/base.pm
@@ -96,8 +96,6 @@ sub import {
             {
                 local $SIG{__DIE__};
                 my $fn = _module_to_filename($base);
-                my $file = __FILE__;
-                my $line = __LINE__ + 1;
                 eval { require $fn };
                 # Only ignore "Can't locate" errors from our eval require.
                 # Other fatal errors (syntax etc) must be reported.
@@ -107,8 +105,8 @@ sub import {
                 # probably be using parent.pm, which doesn't try to
                 # guess whether require is needed or failed,
                 # see [perl #118561]
-                die if $@ && $@ !~ /^Can't locate \Q$fn\E .*? at \Q$file\E line \Q$line\E(?:, <[^>]*> (?:line|chunk) [0-9]+)?\.\n\z/s
-                          || $@ =~ /Compilation failed in require at \Q$file\E line \Q$line\E(?:, <[^>]*> (?:line|chunk) [0-9]+)?\.\n\z/;
+                die if $@ && $@ !~ /^Can't locate \Q$fn\E .*? at .* line [0-9]+(?:, <[^>]*> (?:line|chunk) [0-9]+)?\.\n\z/s
+                          || $@ =~ /Compilation failed in require at .* line [0-9]+(?:, <[^>]*> (?:line|chunk) [0-9]+)?\.\n\z/;
                 unless (%{"$base\::"}) {
                     require Carp;
                     local $" = " ";
diff --git a/dist/base/t/core-global.t b/dist/base/t/core-global.t
new file mode 100644
index 0000000..a912166
--- /dev/null
+++ b/dist/base/t/core-global.t
@@ -0,0 +1,20 @@
+#!/usr/bin/perl -w
+
+use strict;
+use Test::More tests => 1;
+
+BEGIN { *CORE::GLOBAL::require = sub { require $_[0] }; }
+
+{
+    # [perl #121196]
+    {
+        package RequireOverride;
+        sub zzz {}
+    }
+    ok(eval <<'EOS', "handle require overrides")
+package RequireOverrideB;
+use base 'RequireOverride';
+1
+EOS
+        or diag $@;
+}
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented Feb 17, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2014

From @tonycoz

On Sun Feb 16 20​:36​:13 2014, tonyc wrote​:

On Thu Feb 06 15​:12​:28 2014, zefram@​fysh.org wrote​:

I'd call that require override well-behaved, in the relevant
respects.
It seems excessive to demand that overrides fix up error locations.

This problem can be avoided by base.pm using "CORE​::require" to
suppress
the override. But that raises other problems. On some older perl
versions, Lexical​::SealRequireHints will use a CORE​::GLOBAL​::require
to
work around a hint leakage bug, if its XS version can't be used.

Module​::Runtime suffered the same bug. I fixed it there by using
"CORE​::require". In that case I'm not concerned about breaking
Lexical​::SealRequireHints, because M​:R incorporates the fix that
L​:SRH
would perform via the override.

base.pm could perhaps incorporate the same fix, but M​:R and base.pm
already duplicate a bunch of logic. Perhaps base.pm should use M​:R's
use_package_optimistically(), though that would require dual-lifing
M​:R.
It would be the better factoring, as use_package_optimistically() was
specifically designed to provide this subpart of base.pm's operation
in isolation.

M​::R's functionality would be useful in core, but I'm not sure I'd
want to do it now, with 5.20 coming close.

Another option is to drop the part of the error message matching that
is specific about location. The other parts probably suffice.

Attached is a patch to do just that. (I have a separate one to bump
$base​::VERSION)

I'll apply this soonish, unless there's an objection.

Applied as 257518b with a version bump in 6d879f8.

Tony

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant