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

IO::Seekable + POSIX -> constant subroutines redefined #9330

Closed
p5pRT opened this issue May 14, 2008 · 16 comments
Closed

IO::Seekable + POSIX -> constant subroutines redefined #9330

p5pRT opened this issue May 14, 2008 · 16 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented May 14, 2008

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

Searchable as RT54186$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 14, 2008

From @ntyni

This is a bug report for perl from ntyni@​debian.org,
generated with the help of perlbug 1.36 running under perl 5.10.0.


As reported in http​://bugs.debian.org/479957 :

% perl -w -e 'use IO​::Seekable; use POSIX'
Constant subroutine main​::SEEK_SET redefined at -e line 1
Constant subroutine main​::SEEK_END redefined at -e line 1
Constant subroutine main​::SEEK_CUR redefined at -e line 1



Flags​:
  category=library
  severity=low


Site configuration information for perl 5.10.0​:

Configured by Debian Project at Thu May 8 11​:57​:24 UTC 2008.

Summary of my perl5 (revision 5 version 10 subversion 0) configuration​:
  Platform​:
  osname=linux, osvers=2.6.18-6-xen-amd64, archname=x86_64-linux-gnu-thread-multi
  uname='linux sid 2.6.18-6-xen-amd64 #1 smp thu apr 24 05​:10​:26 utc 2008 x86_64 gnulinux '
  config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.10 -Darchlib=/usr/lib/perl/5.10 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.10.0 -Dsitearch=/usr/local/lib/perl/5.10.0 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -DDEBUGGING=-g -Doptimize=-O2 -Duseshrplib -Dlibperl=libperl.so.5.10.0 -Dd_dosuid -des'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2 -g',
  cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -I/usr/local/include'
  ccversion='', gccversion='4.2.3 (Debian 4.2.3-5)', 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 =' -L/usr/local/lib'
  libpth=/usr/local/lib /lib /usr/lib /lib64 /usr/lib64
  libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
  perllibs=-ldl -lm -lpthread -lc -lcrypt
  libc=/lib/libc-2.7.so, so=so, useshrplib=true, libperl=libperl.so.5.10.0
  gnulibc_version='2.7'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib'

Locally applied patches​:
 


@​INC for perl 5.10.0​:
  /etc/perl
  /usr/local/lib/perl/5.10.0
  /usr/local/share/perl/5.10.0
  /usr/lib/perl5
  /usr/share/perl5
  /usr/lib/perl/5.10
  /usr/share/perl/5.10
  /usr/local/lib/site_perl
  .


Environment for perl 5.10.0​:
  HOME=/home/niko
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LC_CTYPE=fi_FI.UTF-8
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/niko/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/bin/X11​:/usr/games​:/sbin​:/usr/sbin
  PERL_BADLANG (unset)
  SHELL=/bin/zsh

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

From @rgs

2008/5/14 via RT Niko Tyni <perlbug-followup@​perl.org>​:

As reported in http​://bugs.debian.org/479957 :

% perl -w -e 'use IO​::Seekable; use POSIX'
Constant subroutine main​::SEEK_SET redefined at -e line 1
Constant subroutine main​::SEEK_END redefined at -e line 1
Constant subroutine main​::SEEK_CUR redefined at -e line 1

Or :

perl -w -e 'use Fcntl "SEEK_SET"; use POSIX'
Constant subroutine main​::SEEK_SET redefined at -e line 1

Probably SEEK_* constants should be moved out of POSIX's Makefile.PL
and put in POSIX.pm with other constants imported from Fcntl.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

From @nwc10

On Thu, May 15, 2008 at 10​:42​:04AM +0200, Rafael Garcia-Suarez wrote​:

2008/5/14 via RT Niko Tyni <perlbug-followup@​perl.org>​:

As reported in http​://bugs.debian.org/479957 :

% perl -w -e 'use IO​::Seekable; use POSIX'
Constant subroutine main​::SEEK_SET redefined at -e line 1
Constant subroutine main​::SEEK_END redefined at -e line 1
Constant subroutine main​::SEEK_CUR redefined at -e line 1

Or :

perl -w -e 'use Fcntl "SEEK_SET"; use POSIX'
Constant subroutine main​::SEEK_SET redefined at -e line 1

Probably SEEK_* constants should be moved out of POSIX's Makefile.PL
and put in POSIX.pm with other constants imported from Fcntl.

It's actually worse than those three​:

$ ./perl -Ilib -w -e 'use Fcntl; BEGIN {Fcntl->import(@​Fcntl​::EXPORT_OK)} use POSIX;'Constant subroutine main​::SEEK_SET redefined at -e line 1Constant subroutine main​::S_IXOTH redefined at -e line 1Subroutine main​::S_ISCHR redefined at -e line 1
Constant subroutine main​::S_IRWXO redefined at -e line 1
Constant subroutine main​::S_IRUSR redefined at -e line 1
Subroutine main​::S_ISBLK redefined at -e line 1
Constant subroutine main​::S_IWOTH redefined at -e line 1
Constant subroutine main​::S_IWGRP redefined at -e line 1
Constant subroutine main​::S_IRWXG redefined at -e line 1
Constant subroutine main​::S_IXGRP redefined at -e line 1
Constant subroutine main​::SEEK_END redefined at -e line 1
Constant subroutine main​::S_IRGRP redefined at -e line 1
Constant subroutine main​::S_IXUSR redefined at -e line 1
Constant subroutine main​::SEEK_CUR redefined at -e line 1
Subroutine main​::S_ISREG redefined at -e line 1
Subroutine main​::S_ISDIR redefined at -e line 1
Subroutine main​::S_ISFIFO redefined at -e line 1
Constant subroutine main​::S_ISUID redefined at -e line 1
Constant subroutine main​::S_IWUSR redefined at -e line 1
Constant subroutine main​::S_IROTH redefined at -e line 1
Constant subroutine main​::S_IRWXU redefined at -e line 1
Constant subroutine main​::S_ISGID redefined at -e line 1

The "Constant subroutine" warnings were new with 5.10, and I've fixed them
with change 33825 (which inevitably became more complex because there was a
regression test for something else that was assuming that POSIX​::SEEK_SET was
defined in package POSIX, rather than being imported from somewhere else)

The other 5 appear to be have been around rather longer than 5.10 - this
doesn't warn with 5.005_03, but does with 5.6.2 (I don't have 5.6.0 or 5.6.1
compiled anywhere)​:

$ ~/Reference/5.6.2-g/bin/perl -w -e 'use Fcntl; BEGIN {Fcntl->import(@​Fcntl​::EXPORT_OK)} use POSIX;'
Subroutine S_ISBLK redefined at /export/home/nwc10/Reference/5.6.2-g/lib/perl5/5.6.2/Exporter.pm line 57.
at -e line 1
Subroutine S_ISCHR redefined at /export/home/nwc10/Reference/5.6.2-g/lib/perl5/5.6.2/Exporter.pm line 57.
at -e line 1
Subroutine S_ISDIR redefined at /export/home/nwc10/Reference/5.6.2-g/lib/perl5/5.6.2/Exporter.pm line 57.
at -e line 1
Subroutine S_ISFIFO redefined at /export/home/nwc10/Reference/5.6.2-g/lib/perl5/5.6.2/Exporter.pm line 57.
at -e line 1
Subroutine S_ISREG redefined at /export/home/nwc10/Reference/5.6.2-g/lib/perl5/5.6.2/Exporter.pm line 57.
at -e line 1

With change 33826 POSIX.pm now imports them from Fcntl.pm, rather than having
its own definition. POSIX.pm was actually doing it in XS, using the real C
macro, whereas Fcntl.pm does the bitmask in pure Perl. I assume that the POSIX
standard is nailed down sufficiently that the macros have to be implemented in
system C headers the way that Fcntl.pm does them​:

sub S_IFMT { @​_ ? ( $_[0] & _S_IFMT() ) : _S_IFMT() }
sub S_ISREG { ( $_[0] & _S_IFMT() ) == S_IFREG() }

(where _S_IFMT and S_IFREF are constants defined by XS from the system
headers)

POSIX.xs had this code in AUTOLOAD, to call into POSIX​::int_macro_int

  if ($NON_CONSTS{$constname}) {
  my ($val, $error) = &int_macro_int($constname, $_[0]);
  croak $error if $error;
  *$AUTOLOAD = sub { &int_macro_int($constname, $_[0]) };
  } else {

It's probably actually slower than the Fcntl approach. Fcntl generates these
ops (as imported into package POSIX)

$ ./perl -Ilib -MPOSIX -MO=Concise,POSIX​::S_ISREG -e0POSIX​::S_ISREG​:
7 <1> leavesub[1 ref] K/REFC,1 ->(end)
- <@​> lineseq KP ->7
1 <;> nextstate(Fcntl -850 Fcntl.pm​:221) v​:*,&,$ ->2
6 <2> eq sK/2 ->7
4 <2> bit_and[t2] sKP ->5
- <1> ex-aelem sK/2 ->3
- <1> ex-rv2av sKR/3 ->-
2 <#> aelemfast[*_] s ->3
- <0> ex-const s ->-
3 <$> const[IV 61440] s ->4
5 <$> const[IV 32768] s ->6
-e syntax OK

whereas the closure POSIX used to create generates these ops​:

$ ./perl -Ilib -MPOSIX -e 'BEGIN{POSIX​::S_ISREG();} use O qw(Concise POSIX​::S_ISREG)'
POSIX​::S_ISREG​:
a <1> leavesub[2 refs] K/REFC,1 ->(end)
- <@​> lineseq KP ->a
1 <;> nextstate(POSIX -776 POSIX.pm​:55) v ->2
9 <1> entersub[t4] KS/TARG,AMPER,1 ->a
- <1> ex-list K ->9
2 <0> pushmark s ->3
3 <0> padsv[$constname​:FAKE​:m​:8] lM ->4
7 <2> aelem sKM/LVDEFER,2 ->8
5 <1> rv2av sKR/1 ->6
4 <#> gv[*_] s ->5
6 <$> const[IV 0] s ->7
- <1> ex-rv2cv sK/8 ->-
8 <#> gv[*POSIX​::int_macro_int] s ->9
-e syntax OK

Two entersubs for the price of one! (Not the cheapest of ops)

Assuming I didn't do anything stupid here, this will be fixed in 5.10.1

Nicholas Clark

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

From @nwc10

Change 33825 by nicholas@​nicholas-bouvard on 2008/05/15 10​:10​:27

  It transpires that POSIX.xs also duplicated several constants defined
  by Fcntl but only conditionally exported by Fcntl. The most obvious
  were SEEK_CUR, SEEK_END and SEEK_SET, as reported in bug #54186.
  So add them to the list of constants that POSIX imports from Fcntl.

Affected files ...

... //depot/perl/ext/POSIX/Makefile.PL#32 edit
... //depot/perl/ext/POSIX/POSIX.pm#54 edit
... //depot/perl/t/lib/proxy_constant_subs.t#2 edit

Differences ...

==== //depot/perl/ext/POSIX/Makefile.PL#32 (text) ====

@​@​ -48,13 +48,11 @​@​
  MAX_INPUT MB_LEN_MAX MSG_CTRUNC MSG_DONTROUTE MSG_EOR MSG_OOB MSG_PEEK
  MSG_TRUNC MSG_WAITALL NAME_MAX NCCS NGROUPS_MAX NOFLSH OPEN_MAX OPOST
  PARENB PARMRK PARODD PATH_MAX PIPE_BUF RAND_MAX R_OK SCHAR_MAX
- SCHAR_MIN SEEK_CUR SEEK_END SEEK_SET SHRT_MAX SHRT_MIN SIGABRT SIGALRM
+ SCHAR_MIN SHRT_MAX SHRT_MIN SIGABRT SIGALRM
  SIGCHLD SIGCONT SIGFPE SIGHUP SIGILL SIGINT SIGKILL SIGPIPE SIGQUIT
  SIGSEGV SIGSTOP SIGTERM SIGTSTP SIGTTIN SIGTTOU
  SIGUSR1 SIGUSR2 SIG_BLOCK SIG_SETMASK SIG_UNBLOCK SSIZE_MAX
- STDERR_FILENO STDIN_FILENO STDOUT_FILENO STREAM_MAX
- S_IRGRP S_IROTH S_IRUSR S_IRWXG S_IRWXO S_IRWXU S_ISGID S_ISUID
- S_IWGRP S_IWOTH S_IWUSR S_IXGRP S_IXOTH S_IXUSR TCIFLUSH TCIOFF
+ STDERR_FILENO STDIN_FILENO STDOUT_FILENO STREAM_MAX TCIFLUSH TCIOFF
  TCIOFLUSH TCION TCOFLUSH TCOOFF TCOON TCSADRAIN TCSAFLUSH TCSANOW
  TMP_MAX TOSTOP TZNAME_MAX VEOF VEOL VERASE VINTR VKILL VMIN VQUIT
  VSTART VSTOP VSUSP VTIME WNOHANG WUNTRACED W_OK X_OK

==== //depot/perl/ext/POSIX/POSIX.pm#54 (text) ====

@​@​ -4,7 +4,7 @​@​

our(@​ISA, %EXPORT_TAGS, @​EXPORT_OK, @​EXPORT, $AUTOLOAD, %SIGRT) = ();

-our $VERSION = "1.14";
+our $VERSION = "1.15";

use AutoLoader;

@​@​ -13,7 +13,9 @​@​
use Fcntl qw(FD_CLOEXEC F_DUPFD F_GETFD F_GETFL F_GETLK F_RDLCK F_SETFD
  F_SETFL F_SETLK F_SETLKW F_UNLCK F_WRLCK O_ACCMODE O_APPEND
  O_CREAT O_EXCL O_NOCTTY O_NONBLOCK O_RDONLY O_RDWR O_TRUNC
- O_WRONLY);
+ O_WRONLY SEEK_CUR SEEK_END SEEK_SET
+ S_IRGRP S_IROTH S_IRUSR S_IRWXG S_IRWXO S_IRWXU S_ISGID S_ISUID
+ S_IWGRP S_IWOTH S_IWUSR S_IXGRP S_IXOTH S_IXUSR);

# Grandfather old foo_h form to new :foo_h form
my $loaded;

==== //depot/perl/t/lib/proxy_constant_subs.t#2 (text) ====

@​@​ -7,20 +7,20 @​@​
  print "1..0 # Skip -- Perl configured without B module\n";
  exit 0;
  }
- if ($Config​::Config{'extensions'} !~ /\bPOSIX\b/) {
- print "1..0 # Skip -- Perl configured without POSIX\n";
+ if ($Config​::Config{'extensions'} !~ /\bFcntl\b/) {
+ print "1..0 # Skip -- Perl configured without Fcntl\n";
  exit 0;
  }
- # errno is a real subroutine, and acts as control
+ # S_IFMT is a real subroutine, and acts as control
  # SEEK_SET is a proxy constant subroutine.
- @​symbols = qw(errno SEEK_SET);
+ @​symbols = qw(S_IFMT SEEK_SET);
}

use strict;
use warnings;
use Test​::More tests => 4 * @​symbols;
use B qw(svref_2object GVf_IMPORTED_CV);
-use POSIX @​symbols;
+use Fcntl @​symbols;

# GVf_IMPORTED_CV should not be set on the original, but should be set on the
# imported GV.
@​@​ -29,7 +29,7 @​@​
  my ($ps, $ms);
  {
  no strict 'refs';
- $ps = svref_2object(\*{"POSIX​::$symbol"});
+ $ps = svref_2object(\*{"Fcntl​::$symbol"});
  $ms = svref_2object(\*{"​::$symbol"});
  }
  isa_ok($ps, 'B​::GV');

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

From @nwc10

Change 33826 by nicholas@​mouse-mill on 2008/05/15 11​:24​:43

  Remove POSIX's internal implementation of S_ISBLK, S_ISCHR, S_ISDIR,
  S_ISFIFO and S_ISREG, and pull them in from Fcntl. Spotted as a result
  of bug #54186, but there has been a redefined subroutine warning for
  ages if you elected to import all of POSIX and Fcntl's exports.

Affected files ...

... //depot/perl/ext/B/t/concise-xs.t#41 edit
... //depot/perl/ext/POSIX/POSIX.pm#55 edit
... //depot/perl/ext/POSIX/POSIX.xs#151 edit

Differences ...

==== //depot/perl/ext/B/t/concise-xs.t#41 (text) ====

@​@​ -177,7 +177,10 @​@​
  },

  POSIX => { dflt => 'constant', # all but 252/589
- skip => [qw/ _POSIX_JOB_CONTROL /], # platform varying
+ skip => [qw/ _POSIX_JOB_CONTROL /, # platform varying
+ # Might be XS or imported from Fctnl, depending on your
+ # perl version​:
+ qw / S_ISBLK S_ISCHR S_ISDIR S_ISFIFO S_ISREG /],
  perl => [qw/ import croak AUTOLOAD /],

  XS => [qw/ write wctomb wcstombs uname tzset tzname

==== //depot/perl/ext/POSIX/POSIX.pm#55 (text) ====

@​@​ -14,6 +14,7 @​@​
  F_SETFL F_SETLK F_SETLKW F_UNLCK F_WRLCK O_ACCMODE O_APPEND
  O_CREAT O_EXCL O_NOCTTY O_NONBLOCK O_RDONLY O_RDWR O_TRUNC
  O_WRONLY SEEK_CUR SEEK_END SEEK_SET
+ S_ISBLK S_ISCHR S_ISDIR S_ISFIFO S_ISREG
  S_IRGRP S_IROTH S_IRUSR S_IRWXG S_IRWXO S_IRWXU S_ISGID S_ISUID
  S_IWGRP S_IWOTH S_IWUSR S_IXGRP S_IXOTH S_IXUSR);

@​@​ -34,9 +35,9 @​@​

XSLoader​::load 'POSIX', $VERSION;

-my %NON_CONSTS = (map {($_,1)}
- qw(S_ISBLK S_ISCHR S_ISDIR S_ISFIFO S_ISREG WEXITSTATUS
- WIFEXITED WIFSIGNALED WIFSTOPPED WSTOPSIG WTERMSIG));
+my %NON_CONSTS
+ = (map {($_,1)} qw(WEXITSTATUS WIFEXITED WIFSIGNALED WIFSTOPPED WSTOPSIG
+ WTERMSIG));

sub AUTOLOAD {
  no strict;

==== //depot/perl/ext/POSIX/POSIX.xs#151 (text) ====

@​@​ -404,7 +404,7 @​@​
use ExtUtils​::Constant qw (constant_types C_constant XS_constant);

my $types = {map {($_, 1)} qw(IV)};
-my @​names = (qw(S_ISBLK S_ISCHR S_ISDIR S_ISFIFO S_ISREG WEXITSTATUS WIFEXITED
+my @​names = (qw(WEXITSTATUS WIFEXITED
  WIFSIGNALED WIFSTOPPED WSTOPSIG WTERMSIG));

print constant_types(); # macro defs
@​@​ -416,65 +416,14 @​@​
  */

  switch (len) {
- case 7​:
- /* Names all of length 7. */
- /* S_ISBLK S_ISCHR S_ISDIR S_ISREG */
- /* Offset 5 gives the best switch position. */
- switch (name[5]) {
- case 'E'​:
- if (memEQ(name, "S_ISREG", 7)) {
- /* ^ */
-#ifdef S_ISREG
- *arg_result = S_ISREG(*arg_result);
- return PERL_constant_ISIV;
-#else
- return PERL_constant_NOTDEF;
-#endif
- }
- break;
- case 'H'​:
- if (memEQ(name, "S_ISCHR", 7)) {
- /* ^ */
-#ifdef S_ISCHR
- *arg_result = S_ISCHR(*arg_result);
- return PERL_constant_ISIV;
-#else
- return PERL_constant_NOTDEF;
-#endif
- }
- break;
- case 'I'​:
- if (memEQ(name, "S_ISDIR", 7)) {
- /* ^ */
-#ifdef S_ISDIR
- *arg_result = S_ISDIR(*arg_result);
- return PERL_constant_ISIV;
-#else
- return PERL_constant_NOTDEF;
-#endif
- }
- break;
- case 'L'​:
- if (memEQ(name, "S_ISBLK", 7)) {
- /* ^ */
-#ifdef S_ISBLK
- *arg_result = S_ISBLK(*arg_result);
- return PERL_constant_ISIV;
-#else
- return PERL_constant_NOTDEF;
-#endif
- }
- break;
- }
- break;
  case 8​:
  /* Names all of length 8. */
- /* S_ISFIFO WSTOPSIG WTERMSIG */
- /* Offset 3 gives the best switch position. */
- switch (name[3]) {
- case 'O'​:
+ /* WSTOPSIG WTERMSIG */
+ /* Offset 1 gives the best switch position. */
+ switch (name[1]) {
+ case 'S'​:
  if (memEQ(name, "WSTOPSIG", 8)) {
- /* ^ */
+ /* ^ */
#ifdef WSTOPSIG
  int i = *arg_result;
  *arg_result = WSTOPSIG(WMUNGE(i));
@​@​ -484,9 +433,9 @​@​
#endif
  }
  break;
- case 'R'​:
+ case 'T'​:
  if (memEQ(name, "WTERMSIG", 8)) {
- /* ^ */
+ /* ^ */
#ifdef WTERMSIG
  int i = *arg_result;
  *arg_result = WTERMSIG(WMUNGE(i));
@​@​ -496,17 +445,6 @​@​
#endif
  }
  break;
- case 'S'​:
- if (memEQ(name, "S_ISFIFO", 8)) {
- /* ^ */
-#ifdef S_ISFIFO
- *arg_result = S_ISFIFO(*arg_result);
- return PERL_constant_ISIV;
-#else
- return PERL_constant_NOTDEF;
-#endif
- }
- break;
  }
  break;
  case 9​:

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 15, 2008

From @Abigail

On Thu, May 15, 2008 at 12​:40​:36PM +0100, Nicholas Clark wrote​:

The other 5 appear to be have been around rather longer than 5.10 - this
doesn't warn with 5.005_03, but does with 5.6.2 (I don't have 5.6.0 or 5.6.1
compiled anywhere)​:
$ ~/Reference/5.6.2-g/bin/perl -w -e 'use Fcntl; BEGIN {Fcntl->import(@​Fcntl​::EXPORT_OK)} use POSIX;'

It warns with 5.6.0 and 5.6.1 as well, and doesn't warn with 5.005_04.

Abigail

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 16, 2008

From maddingue@free.fr

Nicholas Clark wrote​:

POSIX.xs had this code in AUTOLOAD, to call into POSIX​::int_macro_int

if \($NON\_CONSTS\{$constname\}\) \{
    my \($val\, $error\) = &int\_macro\_int\($constname\, $\_\[0\]\);
    croak $error if $error;
    \*$AUTOLOAD = sub \{ &int\_macro\_int\($constname\, $\_\[0\]\) \};
\} else \{

Isn't this the standard way constant are handled by ExtUtils​::Constant?
For example, there's this AUTOLOAD function in Sys​::Syslog​:

sub AUTOLOAD {
  # This AUTOLOAD is used to 'autoload' constants from the constant()
  # XS function.
  no strict 'vars';
  my $constname;
  ($constname = $AUTOLOAD) =~ s/.*​:://;
  croak "Sys​::Syslog​::constant() not defined" if $constname eq
'constant';
  my ($error, $val) = constant($constname);
  croak $error if $error;
  no strict 'refs';
  *$AUTOLOAD = sub { $val };
  goto &$AUTOLOAD;
}

and I have a similar one in Net​::Pcap.

Given Perl constants are pretty fast, wouldn't it be both faster and
less complex wrt the generated code to create a pure Perl file with
all the declarations, and then require it from the main module?

--
Sébastien Aperghis-Tramoni

Close the world, txEn eht nepO.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 16, 2008

From @nwc10

On Sat, May 17, 2008 at 01​:01​:23AM +0200, Sbastien Aperghis-Tramoni wrote​:

Nicholas Clark wrote​:

POSIX.xs had this code in AUTOLOAD, to call into POSIX​::int_macro_int

if ($NON_CONSTS{$constname}) {
my ($val, $error) = &int_macro_int($constname, $_[0]);
croak $error if $error;
*$AUTOLOAD = sub { &int_macro_int($constname, $_[0]) };
} else {

Isn't this the standard way constant are handled by ExtUtils​::Constant?

No. It's not a constant function. It's 5 macros that process arguments,
wrapped in a single C function.

For example, there's this AUTOLOAD function in Sys​::Syslog​:

sub AUTOLOAD {
# This AUTOLOAD is used to 'autoload' constants from the constant()
# XS function.
no strict 'vars';
my $constname;
($constname = $AUTOLOAD) =~ s/.*​:://;
croak "Sys​::Syslog​::constant() not defined" if $constname eq
'constant';
my ($error, $val) = constant($constname);
croak $error if $error;
no strict 'refs';
*$AUTOLOAD = sub { $val };
goto &$AUTOLOAD;
}

and I have a similar one in Net​::Pcap.

There's similar code in POSIX.pm just after the C<} else {> above for dealing
with constants.

All this originated because POSIX.pm pre 5.8.0 had some really really sick
code that (from memory) set errno to EAGAIN, and then POSIX.pm re-evaluate
the code every time. Mmm, memory correct - here's the code before I re-wrote
it​: http​://public.activestate.com/cgi-bin/perlbrowse/b;p=55,53/ext/POSIX/POSIX.pm@​9272#L

The change that got rid of it was 10541. The diff is really hard to follow
because it's wholesale removal of the old constant() function, and insertion
of one generated by an early version of ExtUtils​::Constant

Given Perl constants are pretty fast, wouldn't it be both faster and
less complex wrt the generated code to create a pure Perl file with
all the declarations, and then require it from the main module?

They're not constants, so I don't think that this is valid.

Nicholas Clark

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

From @nwc10

On Sat, May 17, 2008 at 12​:10​:28AM +0100, Nicholas Clark wrote​:

On Sat, May 17, 2008 at 01​:01​:23AM +0200, Sbastien Aperghis-Tramoni wrote​:

Nicholas Clark wrote​:

POSIX.xs had this code in AUTOLOAD, to call into POSIX​::int_macro_int

if ($NON_CONSTS{$constname}) {
my ($val, $error) = &int_macro_int($constname, $_[0]);
croak $error if $error;
*$AUTOLOAD = sub { &int_macro_int($constname, $_[0]) };
} else {

Isn't this the standard way constant are handled by ExtUtils​::Constant?

No. It's not a constant function. It's 5 macros that process arguments,
wrapped in a single C function.

I can't count. It's 6, not 5. Anyway, I removed int_macro_int with change
33896, and improved the XS with change 33897

Change 33896 by nicholas@​mouse-mill on 2008/05/21 09​:18​:00

  Eliminate POSIX​::int_macro_int, and all the complex AUTOLOAD fandango
  that creates closures round it. Instead, wrap WEXITSTATUS, WIFEXITED,
  WIFSIGNALED, WIFSTOPPED, WSTOPSIG and WTERMSIG directly with XS.
  The shared library is slightly larger, but dynamic memory usage savings
  beat this, even within one thread of one process. Simpler code too.

Change 33897 by nicholas@​mouse-mill on 2008/05/21 10​:31​:32

  Replaced the WEXITSTATUS, WIFEXITED, WIFSIGNALED, WIFSTOPPED, WSTOPSIG
  and WTERMSIG wrappers with one wrapper using the XS "ALIAS" feature.
  This gets the shared object size back below the size before the removal
  of int_macro_int. It looks like there are other space savings to be
  made this way.

I think I spent a bit more time on my code to measure the memory usage of the
old and new (attached)approaches. (attached as posix.pl, along with before and
after output.) By swapping from 6 XS wrappers to 1 using "ALIAS", change 33897
made the shared library shrink from 105944 to 105784 bytes. There are other
similar savings to be made in POSIX.xs, and probably in other core XS code.
This could make an "interesting" "cage-cleaner" type TODO task, if anyone's
game.

Bottom line of the two changes is that comparable memory usage from use POSIX;
drops from 153606 to 152563 bytes, and if one actually uses all 6 functions,
from 153641 to 152563 bytes. There's no AUTOLOAD now, so there's no change in
memory on running them. Oh, and they run fewer ops, so will be faster too.
See the script for the explaination of the games needed to cope with AUTOLOADed
stubs causing Devel​::Size to see things in other packages.

Only user visible change is that if you call the 6 with too many arguments you
now actually get an error​:

Was​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
$

Now​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
Usage​: WEXITSTATUS(status) at -e line 1.

Is this fair game to inflict on buggy users of 5.10.0?

Nicholas Clark

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

From @nwc10

Exporter​::Heavy​::heavy_export 3104
Exporter​::import 3402
POSIX​::AUTOLOAD 24549
POSIX​::WEXITSTATUS 23439
POSIX​::WIFEXITED 23433
POSIX​::WIFSIGNALED 23439
POSIX​::WIFSTOPPED 23436
POSIX​::WSTOPSIG 23430
POSIX​::WTERMSIG 23430
POSIX​::assert 23424
POSIX​::int_macro_int 387
POSIX.bundle 105788
  ======
  301261
  ======
105788 + 47818 153606
....................
Exporter​::Heavy​::heavy_export 3104
Exporter​::import 3402
POSIX​::AUTOLOAD 24549
POSIX​::WEXITSTATUS 25208
POSIX​::WIFEXITED 25204
POSIX​::WIFSIGNALED 25208
POSIX​::WIFSTOPPED 25206
POSIX​::WSTOPSIG 25202
POSIX​::WTERMSIG 25202
POSIX​::assert 23424
POSIX​::int_macro_int 387
POSIX.bundle 105788
  ======
  311884
  ======
105788 + 47853 153641

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

From @nwc10

Exporter​::Heavy​::heavy_export 3104
Exporter​::import 3402
POSIX​::AUTOLOAD 24374
POSIX​::WEXITSTATUS 384
POSIX​::WIFEXITED 378
POSIX​::WIFSIGNALED 384
POSIX​::WIFSTOPPED 381
POSIX​::WSTOPSIG 375
POSIX​::WTERMSIG 375
POSIX​::assert 23424
POSIX.bundle 105784
  ======
  162365
  ======
105784 + 46779 152563
....................
Exporter​::Heavy​::heavy_export 3104
Exporter​::import 3402
POSIX​::AUTOLOAD 24374
POSIX​::WEXITSTATUS 384
POSIX​::WIFEXITED 378
POSIX​::WIFSIGNALED 384
POSIX​::WIFSTOPPED 381
POSIX​::WSTOPSIG 375
POSIX​::WTERMSIG 375
POSIX​::assert 23424
POSIX.bundle 105784
  ======
  162365
  ======
105784 + 46779 152563

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

From @rgs

2008/5/21 Nicholas Clark <nick@​ccl4.org>​:

Only user visible change is that if you call the 6 with too many arguments you
now actually get an error​:

Was​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
$

Now​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
Usage​: WEXITSTATUS(status) at -e line 1.

Is this fair game to inflict on buggy users of 5.10.0?

I tend to think so. Assuming the buggy call is not widely used on CPAN ?

@p5pRT
Copy link
Author

@p5pRT p5pRT commented May 21, 2008

From maddingue@free.fr

Rafael Garcia-Suarez <rgarciasuarez@​gmail.wrote​:

2008/5/21 Nicholas Clark <nick@​ccl4.org>​:

Only user visible change is that if you call the 6 with too many arguments
you
now actually get an error​:

Was​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
$

Now​:
$ ./perl -Ilib -MPOSIX -we 'WEXITSTATUS(3,2,3)'
Usage​: WEXITSTATUS(status) at -e line 1.

Is this fair game to inflict on buggy users of 5.10.0?

I tend to think so. Assuming the buggy call is not widely used on CPAN ?

A quick search with Google Code Search seems to indicate that nobody
wrote code with such buggy calls.

--
Sébastien Aperghis-Tramoni

Close the world, txEn eht nepO.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Aug 11, 2008

p5p@spam.wizbit.be - 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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant