From @wolfsage

Created by wolfsage@gmail.com

The DynaLoader documentation for dl_install_xsub says​:

  This is simply a direct call to newXSUB().

It looks like newXSUB() was removed a long time ago. I've updated the doc
with​:

  This is simply a direct call to newXS()/newXS_flags().

Attached is the patch.

Cheers,

-- Matthew Horsfall (alh)

Flags:
    category=library
    severity=low
    module=DynaLoader

Site configuration information for perl 5.14.2:

Configured by Debian Project at Mon Mar 18 19:16:26 UTC 2013.

Summary of my perl5 (revision 5 version 14 subversion 2) configuration:

  Platform:
    osname=linux, osvers=2.6.42-37-generic,
archname=x86_64-linux-gnu-thread-multi
    uname='linux batsu 2.6.42-37-generic #58-ubuntu smp thu jan 24 15:28:10
utc 2013 x86_64 x86_64 x86_64 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN
-Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr
-Dprivlib=/usr/share/perl/5.14 -Darchlib=/usr/lib/perl/5.14
-Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5
-Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl/5.14.2
-Dsitearch=/usr/local/lib/perl/5.14.2 -Dman1dir=/usr/share/man/man1
-Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1
-Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1 -Dman3ext=3perl
-Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm
-Ui_libutil -DDEBUGGING=-g -Doptimize=-O2 -Duseshrplib
-Dlibperl=libperl.so.5.14.2 -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 -fstack-protector -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 -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.6.3', 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/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=, so=so, useshrplib=true, libperl=libperl.so.5.14.2
    gnulibc_version='2.15'
  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'

Locally applied patches:



@INC for perl 5.14.2:
    /home/mhorsfall/perl5/lib/perl5/x86_64-linux-gnu-thread-multi
    /home/mhorsfall/perl5/lib/perl5
    /etc/perl
    /usr/local/lib/perl/5.14.2
    /usr/local/share/perl/5.14.2
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.14
    /usr/share/perl/5.14
    /usr/local/lib/site_perl
    .


Environment for perl 5.14.2:
    HOME=/home/mhorsfall
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=/home/mhorsfall/bin:/home/mhorsfall/perl5/bin:/home/mhorsfall/bin:/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
    PERL5LIB=/home/mhorsfall/perl5/lib/perl5:
    PERL_BADLANG (unset)
    PERL_LOCAL_LIB_ROOT=:/home/mhorsfall/perl5
    PERL_MB_OPT=--install_base /home/mhorsfall/perl5
    PERL_MM_OPT=INSTALL_BASE=/home/mhorsfall/perl5
    SHELL=/bin/bash

From @wolfsage

From 8e0f3b59235138c0fa6f2220a0184c85135a3b2d Mon Sep 17 00:00:00 2001
From: "Matthew Horsfall (alh)" <wolfsage@gmail.com>
Date: Thu, 29 Aug 2013 08:41:06 -0400
Subject: [PATCH] Replace reference to newXSUB with newXS.

newXSUB hasn't been around for a long time
---
 ext/DynaLoader/DynaLoader_pm.PL |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/DynaLoader/DynaLoader_pm.PL b/ext/DynaLoader/DynaLoader_pm.PL
index c67b4ab..03d45d1 100644
--- a/ext/DynaLoader/DynaLoader_pm.PL
+++ b/ext/DynaLoader/DynaLoader_pm.PL
@@ -870,7 +870,7 @@ Syntax:
 
 Create a new Perl external subroutine named $perl_name using $symref as
 a pointer to the function which implements the routine.  This is simply
-a direct call to newXSUB().  Returns a reference to the installed
+a direct call to newXS()/newXS_flags().  Returns a reference to the installed
 function.
 
 The $filename parameter is used by Perl to identify the source file for
-- 
1.7.9.5

From @nwc10

On Thu, Aug 29, 2013 at 07​:15​:12AM -0700, Matthew Horsfall wrote​:

The DynaLoader documentation for dl_install_xsub says​:

This is simply a direct call to newXSUB().

It looks like newXSUB() was removed a long time ago. I've updated the doc
with​:

This is simply a direct call to newXS()/newXS_flags().

Attached is the patch.

Good catch.

Subject​: [PATCH] Replace reference to newXSUB with newXS.

newXSUB hasn't been around for a long time
---
ext/DynaLoader/DynaLoader_pm.PL | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/DynaLoader/DynaLoader_pm.PL b/ext/DynaLoader/DynaLoader_pm.PL
index c67b4ab..03d45d1 100644
--- a/ext/DynaLoader/DynaLoader_pm.PL
+++ b/ext/DynaLoader/DynaLoader_pm.PL
@​@​ -870,7 +870,7 @​@​ Syntax​:

Create a new Perl external subroutine named $perl_name using $symref as
a pointer to the function which implements the routine. This is simply
-a direct call to newXSUB(). Returns a reference to the installed
+a direct call to newXS()/newXS_flags(). Returns a reference to the installed
function.

The $filename parameter is used by Perl to identify the source file for

win32 seems to be the only code using newXS(). newXS() And newXS_flags()
are both (now) wrappers to newXS_len_flags(), and possibly some of the
callers of newXS_flags() might change to the latter. So is it better just
to word it as "newXS_len_flags() or one of its wrappers" ?

This history of all this seems to be rather stranger than you might first
think. newXSUB was eliminated ages ago​:

commit 5fec647
Author​: Malcolm Beattie <mbeattie@​sable.ox.ac.uk>
Date​: Thu Nov 6 14​:37​:37 1997 +0000

  Remove #ifdef DEPRECATED stuff​: newXSUB, pp_entersubr, FREE_TMPS().
 
  p4raw-id​: //depot/perl@​206

but it seems that nothing in DynaLoader ever used it. In the 5.000 docs say
that DynaLoader uses newXSUB(), but actually it uses newXS().

Most of DynaLoader's platform specific code switched over to newXS_flags()
with this commit​:

commit 77004de
Author​: Nicholas Clark <nick@​ccl4.org>
Date​: Tue May 2 15​:55​:25 2006 +0000

  Fix bug in DynaLoader, which has been passing a filename in dynamic
  storage to newXS() seemingly forever. This involves creating
  newXS_flags(), with the first flag being "arrange to copy the
  filename and free it at the right time".
 
  p4raw-id​: //depot/perl@​28063

I'm wary of documenting exactly the current state, as I see that
newXS_len_flags() would be preferable to what we have, as it would avoid a
needless strlen() call.

Nicholas Clark

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

From @wolfsage

On Fri, Aug 30, 2013 at 7​:43 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

I'm wary of documenting exactly the current state, as I see that
newXS_len_flags() would be preferable to what we have, as it would avoid a
needless strlen() call.

How would we avoid the strlen call?

It looks like newXS_len_flags() still needs the length of char * perl_name
passed to it.

Thanks,

-- Matthew Horsfall (alh)

From @nwc10

On Fri, Aug 30, 2013 at 08​:18​:59AM -0400, Matthew Horsfall (alh) wrote​:

On Fri, Aug 30, 2013 at 7​:43 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

I'm wary of documenting exactly the current state, as I see that
newXS_len_flags() would be preferable to what we have, as it would avoid a
needless strlen() call.

How would we avoid the strlen call?

It looks like newXS_len_flags() still needs the length of char * perl_name
passed to it.

Yes. The length is known, but being throw away by the XS typemap​:

void
dl_install_xsub(perl_name, symref, filename="$Package")
  char * perl_name
  void * symref
  const char * filename

Change char *perl_name to SV *perl name, and one can get the length.
(And, like every other filename interaction, start to wonder which character
set the OS was expecting. Rather than pretending to ignore it)

Nicholas Clark

From @wolfsage

On Fri, Aug 30, 2013 at 8​:38 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Fri, Aug 30, 2013 at 08​:18​:59AM -0400, Matthew Horsfall (alh) wrote​:

How would we avoid the strlen call?

It looks like newXS_len_flags() still needs the length of char *
perl_name
passed to it.

Yes. The length is known, but being throw away by the XS typemap​:

void
dl_install_xsub(perl_name, symref, filename="$Package")
char * perl_name
void * symref
const char * filename

Change char *perl_name to SV *perl name, and one can get the length.
(And, like every other filename interaction, start to wonder which
character
set the OS was expecting. Rather than pretending to ignore it)

Ah.

I'd like to try my hand at patching that then. It *sounds* simple, unless
your note about the character set means it's going to be far more
complicated...

-- Matthew Horsfall (alh)

From @nwc10

On Fri, Aug 30, 2013 at 08​:45​:42AM -0400, Matthew Horsfall (alh) wrote​:

On Fri, Aug 30, 2013 at 8​:38 AM, Nicholas Clark <nick@​ccl4.org> wrote​:

On Fri, Aug 30, 2013 at 08​:18​:59AM -0400, Matthew Horsfall (alh) wrote​:

How would we avoid the strlen call?

It looks like newXS_len_flags() still needs the length of char *
perl_name
passed to it.

Yes. The length is known, but being throw away by the XS typemap​:

void
dl_install_xsub(perl_name, symref, filename="$Package")
char * perl_name
void * symref
const char * filename

Change char *perl_name to SV *perl name, and one can get the length.
(And, like every other filename interaction, start to wonder which
character
set the OS was expecting. Rather than pretending to ignore it)

Ah.

I'd like to try my hand at patching that then. It *sounds* simple, unless
your note about the character set means it's going to be far more
complicated...

Well, I think that the least worst approach for now is to change perl_name
to be SV *, and then use SvPVbyte() to get a char * pointer and the length.

Nicholas Clark

From @cpansprout

On Fri Aug 30 04​:43​:57 2013, nicholas wrote​:

On Thu, Aug 29, 2013 at 07​:15​:12AM -0700, Matthew Horsfall wrote​:

The DynaLoader documentation for dl_install_xsub says​:

This is simply a direct call to newXSUB().

It looks like newXSUB() was removed a long time ago. I've updated
the doc
with​:

This is simply a direct call to newXS()/newXS_flags().

Attached is the patch.

Good catch.

Subject​: [PATCH] Replace reference to newXSUB with newXS.

newXSUB hasn't been around for a long time
---
ext/DynaLoader/DynaLoader_pm.PL | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/DynaLoader/DynaLoader_pm.PL
b/ext/DynaLoader/DynaLoader_pm.PL
index c67b4ab..03d45d1 100644
--- a/ext/DynaLoader/DynaLoader_pm.PL
+++ b/ext/DynaLoader/DynaLoader_pm.PL
@​@​ -870,7 +870,7 @​@​ Syntax​:

Create a new Perl external subroutine named $perl_name using
$symref as
a pointer to the function which implements the routine. This is
simply
-a direct call to newXSUB(). Returns a reference to the installed
+a direct call to newXS()/newXS_flags(). Returns a reference to the
installed
function.

The $filename parameter is used by Perl to identify the source file
for

win32 seems to be the only code using newXS(). newXS() And
newXS_flags()
are both (now) wrappers to newXS_len_flags(), and possibly some of the
callers of newXS_flags() might change to the latter. So is it better
just
to word it as "newXS_len_flags() or one of its wrappers" ?

No. newXS_len_flags is not API.

--

Father Chrysostomos

From @wolfsage

On Fri, Aug 30, 2013 at 11​:20 AM, Father Chrysostomos via RT <
perlbug-followup@​perl.org> wrote​:

On Fri Aug 30 04​:43​:57 2013, nicholas wrote​:

The $filename parameter is used by Perl to identify the source file
for

win32 seems to be the only code using newXS(). newXS() And
newXS_flags()
are both (now) wrappers to newXS_len_flags(), and possibly some of the
callers of newXS_flags() might change to the latter. So is it better
just
to word it as "newXS_len_flags() or one of its wrappers" ?

No. newXS_len_flags is not API.

Should we add it to the api and save the strlen call, or forget it and
document as is?

Or is there another option?

Thanks,

-- Matthew Horsfall (alh)

From @nwc10

On Fri, Aug 30, 2013 at 12​:45​:20PM -0400, Matthew Horsfall (alh) wrote​:

On Fri, Aug 30, 2013 at 11​:20 AM, Father Chrysostomos via RT <
perlbug-followup@​perl.org> wrote​:

On Fri Aug 30 04​:43​:57 2013, nicholas wrote​:

The $filename parameter is used by Perl to identify the source file
for

win32 seems to be the only code using newXS(). newXS() And
newXS_flags()
are both (now) wrappers to newXS_len_flags(), and possibly some of the
callers of newXS_flags() might change to the latter. So is it better
just
to word it as "newXS_len_flags() or one of its wrappers" ?

No. newXS_len_flags is not API.

Should we add it to the api and save the strlen call, or forget it and
document as is?

Or is there another option?

I don't know. I'm dithering. It's ugly, with too many parameters. But I think
that they are all there for a reason. One question to ask is "how long has it
been around, and are the parameters stable?" (ie, has there been need to
change them).

And what we have isn't on a hot path.
So there are likely better things to fix first.

Also, technically DynaLoader is the only thing outside of the top level
directory that doesn't need to care about API link restrictions, as it has
to be linked statically to the rest of perl. But it would be bad form to use
something that no-one else is supposed to use.

Nicholas Clark

From @cpansprout

On Fri Aug 30 09​:56​:19 2013, nicholas wrote​:

On Fri, Aug 30, 2013 at 12​:45​:20PM -0400, Matthew Horsfall (alh)
wrote​:

On Fri, Aug 30, 2013 at 11​:20 AM, Father Chrysostomos via RT <
perlbug-followup@​perl.org> wrote​:

On Fri Aug 30 04​:43​:57 2013, nicholas wrote​:

The $filename parameter is used by Perl to identify the
source file
for

win32 seems to be the only code using newXS(). newXS() And
newXS_flags()
are both (now) wrappers to newXS_len_flags(), and possibly some
of the
callers of newXS_flags() might change to the latter. So is it
better
just
to word it as "newXS_len_flags() or one of its wrappers" ?

No. newXS_len_flags is not API.

Should we add it to the api and save the strlen call, or forget it
and
document as is?

Or is there another option?

I don't know. I'm dithering. It's ugly,

Thank you. :-)

with too many parameters. But
I think
that they are all there for a reason. One question to ask is "how long
has it
been around,

032a044 in November of 2011.

and are the parameters stable?" (ie, has there been need
to
change them).

8f82b56 in the same month.

The parameter list is absolutely awful for an API. I added that
function (I renamed newXS_flags and made it a wrapper) because
newXS_flags didn’t do what I needed. The parameters are definitely
*not* stable. Making them stable will just guarantee more wrappers in
the future, which all wrap around newXS_not_api_DONT_USE_OR_OSSIFY_THIS.

--

Father Chrysostomos

From @tonycoz

On Thu Aug 29 07​:15​:12 2013, alh wrote​:

From 8e0f3b59235138c0fa6f2220a0184c85135a3b2d Mon Sep 17 00​:00​:00 2001
From​: "Matthew Horsfall (alh)" <wolfsage@​gmail.com>
Date​: Thu, 29 Aug 2013 08​:41​:06 -0400
Subject​: [PATCH] Replace reference to newXSUB with newXS.

newXSUB hasn't been around for a long time
---
ext/DynaLoader/DynaLoader_pm.PL | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/DynaLoader/DynaLoader_pm.PL
b/ext/DynaLoader/DynaLoader_pm.PL
index c67b4ab..03d45d1 100644
--- a/ext/DynaLoader/DynaLoader_pm.PL
+++ b/ext/DynaLoader/DynaLoader_pm.PL
@​@​ -870,7 +870,7 @​@​ Syntax​:

Create a new Perl external subroutine named $perl_name using $symref
as
a pointer to the function which implements the routine. This is
simply
-a direct call to newXSUB(). Returns a reference to the installed
+a direct call to newXS()/newXS_flags(). Returns a reference to the
installed
function.

The $filename parameter is used by Perl to identify the source file
for

Thanks, applied as 6114e45.

The other discussion here might need a new ticket, leaving this open (for now.)

Tony