search

Perl / perl5

šŸŖ The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

provide originating context of subroutine definitions #10628

Open p5pRT opened 13 years ago

p5pRT commented 13 years ago

Migrated from rt.perl.org#77816 (status was 'open')

Searchable as RT77816$

p5pRT commented 13 years ago

From jawnsy@cpan.org

Created by jawnsy@cpan.org

I noticed that subroutine redefinition messages from perl are not as helpful as they could be.

If we redefine something in perl\, we get​:

$ perl -we 'sub blah { 1; }; sub blah { 2; }' Subroutine blah redefined at -e line 1.

Compiling a C program (using gcc) with redefinitions results in​:

$ cc -std=c99 -pedantic -Wall -g -O0 -DDEBUG -I. -fPIC -c display.c -o display.o display.c​:20​: error​: conflicting types for ā€˜usageā€™ display.c​:5​: note​: previous declaration of ā€˜usageā€™ was here

In a brief chat with the #toolchain folks\, at first glance\, it might be possible to add this feature (though it could be non-trivial to handle some of the obscure cases)

  \ question​: why isn't perl's message -- Subroutine blah redefined at -e line 1. -- more informative\, like gcc's? (gcc shows the origin of the first definition)   \ is it something that's not possible based on how perl parses Perl\, or is it just something nobody's ever thought about doing?   \ jawnsy\, my guess is no one ever really considered it   \ Or maybe it's hard to find where a sub came from once it's been compiled?   \ xdg​: I guess at this point it's not really worth adding... maybe I should add it as a wishlist thing in perl's RT\, in case anyone is so inclined   \ jawnsy\, sure. I'm glancing at the core code right now in case it's trivial.   \ there is some cases where there isn't more information available   \ but most of the time it could be much more informative\, yes   \ rafl\, looks like every CV has ->FILE so that could help locate the original. But without line number\, it might be more annoying than helpful.   \ yeah\, but the line is also around somewhere. in the COPs\, i guess   \ except for xsubs and such   \ right. I just don't know if there's a backreference to it.   \ but that's all just where it was compiled   \ not where it was added to the symbol table   \ jawnsy\, it's probably possible. open up an RT wishlist for it and maybe someone will get to it   \ that's the same most of the time\, but doesn't have to be   \ it would be fixable with line debugging enabled tho   \ because that's what %DB​::sub is intended for   \ we'd just need to update that on sub assignments to globs   \ not a big deal\, of course. I was just trying to write similar type output for something I'm working on\, and looked at gcc and perl   \ as for the place it's compiled at​: perl -MO=Concise\,foo -e'sub foo { 42 }'   \ \<;> nextstate(main 1 -e​:1) v ->2   \ so it's *somewhere* in the optree :)   \ Ah. CV->START->line perhaps   \ something like that\, yes   \ but still.. that isn't necessarily the right answer to "where was that sub previously defined"\, only very often :)   \ true\, but it's a pointer to what subroutine is being replaced   \ that's not too useful\, i'd think   \ you'd really need to know where the symbol was assigned to   \ I'm not sure anything actually does that kind of bookkeeping

In case this actually isn't possible\, then I apologize for the noise. I just thought it'd be a nice feature\, and could help to make perl (the binary) more friendly for new users.

Perl Info ``` Flags: category=core severity=wishlist Site configuration information for perl 5.10.1: Configured by Debian Project at Fri Aug 6 10:46:59 UTC 2010. Summary of my perl5 (revision 5 version 10 subversion 1) configuration: Platform: osname=linux, osvers=2.6.32.17-dsa-ia32, archname=i486-linux-gnu-thread-multi uname='linux murphy 2.6.32.17-dsa-ia32 #1 smp tue aug 3 15:32:33 cest 2010 i686 gnulinux ' config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i486-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.1 -Dsitearch=/usr/local/lib/perl/5.10.1 -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.1 -Dd_dosuid -des' hint=recommended, useposix=true, d_sigaction=define useithreads=define, usemultiplicity=define useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=undef, use64bitall=undef, 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.4.5 20100728 (prerelease)', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib /usr/lib64 libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt perllibs=-ldl -lm -lpthread -lc -lcrypt libc=/lib/libc-2.11.2.so, so=so, useshrplib=true, libperl=libperl.so.5.10.1 gnulibc_version='2.11.2' 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: DEBPKG:debian/arm_thread_stress_timeout - http://bugs.debian.org/501970 Raise the timeout of ext/threads/shared/t/stress.t to accommodate slower build hosts DEBPKG:debian/cpan_config_path - Set location of CPAN::Config to /etc/perl as /usr may not be writable. DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN. DEBPKG:debian/db_file_ver - http://bugs.debian.org/340047 Remove overly restrictive DB_File version check. DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information. DEBPKG:debian/enc2xs_inc - http://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories. DEBPKG:debian/errno_ver - http://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes. DEBPKG:debian/extutils_hacks - Various debian-specific ExtUtils changes DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets. DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor. DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy. DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable. DEBPKG:debian/m68k_thread_stress - http://bugs.debian.org/495826 Disable some threads tests on m68k for now due to missing TLS. DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian DEBPKG:debian/module_build_man_extensions - http://bugs.debian.org/479460 Adjust Module::Build manual page extensions for the Debian Perl policy DEBPKG:debian/perl_synopsis - http://bugs.debian.org/278323 Rearrange perl.pod DEBPKG:debian/prune_libs - http://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need. DEBPKG:debian/use_gdbm - Explicitly link against -lgdbm_compat in ODBM_File/NDBM_File. DEBPKG:fixes/assorted_docs - http://bugs.debian.org/443733 [384f06a] Math::BigInt::CalcEmu documentation grammar fix DEBPKG:fixes/net_smtp_docs - http://bugs.debian.org/100195 [rt.cpan.org #36038] Document the Net::SMTP 'Port' option DEBPKG:fixes/processPL - http://bugs.debian.org/357264 [rt.cpan.org #17224] Always use PERLRUNINST when building perl modules. DEBPKG:debian/perlivp - http://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local DEBPKG:fixes/pod2man-index-backslash - http://bugs.debian.org/521256 Escape backslashes in .IX entries DEBPKG:debian/disable-zlib-bundling - Disable zlib bundling in Compress::Raw::Zlib DEBPKG:fixes/kfreebsd_cppsymbols - http://bugs.debian.org/533098 [3b910a0] Add gcc predefined macros to $Config{cppsymbols} on GNU/kFreeBSD. DEBPKG:debian/cpanplus_definstalldirs - http://bugs.debian.org/533707 Configure CPANPLUS to use the site directories by default. DEBPKG:debian/cpanplus_config_path - Save local versions of CPANPLUS::Config::System into /etc/perl. DEBPKG:fixes/kfreebsd-filecopy-pipes - http://bugs.debian.org/537555 [16f708c] Fix File::Copy::copy with pipes on GNU/kFreeBSD DEBPKG:fixes/anon-tmpfile-dir - http://bugs.debian.org/528544 [perl #66452] Honor TMPDIR when open()ing an anonymous temporary file DEBPKG:fixes/abstract-sockets - http://bugs.debian.org/329291 [89904c0] Add support for Abstract namespace sockets. DEBPKG:fixes/hurd_cppsymbols - http://bugs.debian.org/544307 [eeb92b7] Add gcc predefined macros to $Config{cppsymbols} on GNU/Hurd. DEBPKG:fixes/autodie-flock - http://bugs.debian.org/543731 Allow for flock returning EAGAIN instead of EWOULDBLOCK on linux/parisc DEBPKG:fixes/archive-tar-instance-error - http://bugs.debian.org/539355 [rt.cpan.org #48879] Separate Archive::Tar instance error strings from each other DEBPKG:fixes/positive-gpos - http://bugs.debian.org/545234 [perl #69056] [c584a96] Fix \\G crash on first match DEBPKG:debian/devel-ppport-ia64-optim - http://bugs.debian.org/548943 Work around an ICE on ia64 DEBPKG:fixes/trie-logic-match - http://bugs.debian.org/552291 [perl #69973] [0abd0d7] Fix a DoS in Unicode processing [CVE-2009-3626] DEBPKG:fixes/hppa-thread-eagain - http://bugs.debian.org/554218 make the threads-shared test suite more robust, fixing failures on hppa DEBPKG:fixes/crash-on-undefined-destroy - http://bugs.debian.org/564074 [perl #71952] [1f15e67] Fix a NULL pointer dereference when looking for a DESTROY method DEBPKG:fixes/tainted-errno - http://bugs.debian.org/574129 [perl #61976] [be1cf43] fix an errno stringification bug in taint mode DEBPKG:fixes/safe-upgrade - http://bugs.debian.org/582978 Upgrade Safe.pm to 2.25, fixing CVE-2010-1974 DEBPKG:fixes/tell-crash - http://bugs.debian.org/578577 [f4817f3] Fix a tell() crash on bad arguments. DEBPKG:fixes/format-write-crash - http://bugs.debian.org/579537 [perl #22977] [421f30e] Fix a crash in format/write DEBPKG:fixes/arm-alignment - http://bugs.debian.org/289884 [f1c7503] Prevent gcc from optimizing the alignment test away on armel DEBPKG:fixes/fcgi-test - Fix a failure in CGI/t/fast.t when FCGI is installed DEBPKG:fixes/hurd-ccflags - http://bugs.debian.org/587901 Make hints/gnu.sh append to $ccflags rather than overriding them DEBPKG:patchlevel - http://bugs.debian.org/567489 List packaged patches for 5.10.1-14 in patchlevel.h @INC for perl 5.10.1: /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /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.1: HOME=/home/jon LANG=en_CA.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=~/.bin:/usr/local/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/sbin:/bin PERL_BADLANG (unset) SHELL=/bin/bash ```
p5pRT commented 13 years ago

From jawnsy@cpan.org

Additional information was sent to me via e-mail from Brad Gilbert \b2gills@&#8203;gmail\.com​:

This code should also look for places where the code was set by a GLOB assignment.

  *newsub = \&oldsub   sub newsub{} # redefinition warning

If you don't check for this\, your message might say where oldsub() was defined\, which wouldn't be nearly as helpful as pointing the programmer to where the GLOB assignment was.

p5pRT commented 12 years ago

From @doy

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

p5pRT commented 12 years ago

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

p5pRT commented 12 years ago

From @cpansprout

On Sat Jun 23 21​:32​:09 2012\, doy wrote​:

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

Maybe 28333232 was premature. What happens if you revert 28333232?

--

Father Chrysostomos

p5pRT commented 12 years ago

From @doy

On Sat\, Jun 23\, 2012 at 11​:21​:20PM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 21​:32​:09 2012\, doy wrote​:

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

Maybe 28333232 was premature. What happens if you revert 28333232?

SAVECOPSTASH no longer exists\, and I'm not sure what to use instead. Any pointers?

-doy

p5pRT commented 12 years ago

From @cpansprout

On Sat Jun 23 23​:43​:17 2012\, doy@​tozt.net wrote​:

On Sat\, Jun 23\, 2012 at 11​:21​:20PM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 21​:32​:09 2012\, doy wrote​:

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

Maybe 28333232 was premature. What happens if you revert 28333232?

SAVECOPSTASH no longer exists\, and I'm not sure what to use instead. Any pointers?

SAVECOPSTASH_FREE will do. It doesnā€™t actually free anything any more.

--

Father Chrysostomos

p5pRT commented 12 years ago

From @doy

On Sun\, Jun 24\, 2012 at 12​:05​:21AM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 23​:43​:17 2012\, doy@​tozt.net wrote​:

On Sat\, Jun 23\, 2012 at 11​:21​:20PM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 21​:32​:09 2012\, doy wrote​:

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

Maybe 28333232 was premature. What happens if you revert 28333232?

SAVECOPSTASH no longer exists\, and I'm not sure what to use instead. Any pointers?

SAVECOPSTASH_FREE will do. It doesnā€™t actually free anything any more.

No difference. This actually looks like an off-by-one error of some kind - the line number in the first warning points to the previous time that an anonymous sub was assigned to something in the script\, and then the second warning points to where the first warning should be pointing to (which is assigning an anon sub to a glob slot). Not sure if that is enlightening at all.

-doy

p5pRT commented 12 years ago

From @cpansprout

On Sun Jun 24 01​:20​:59 2012\, doy@​tozt.net wrote​:

On Sun\, Jun 24\, 2012 at 12​:05​:21AM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 23​:43​:17 2012\, doy@​tozt.net wrote​:

On Sat\, Jun 23\, 2012 at 11​:21​:20PM -0700\, Father Chrysostomos via RT wrote​:

On Sat Jun 23 21​:32​:09 2012\, doy wrote​:

Pushed an implementation of this to the doy/better_redefinition_warnings branch. I didn't merge it because I was having trouble with some of the tests in XS-APItest - it seems that newCONSTSUB can sometimes generate warnings with nonsensical line numbers. Looking at the source\, they seem to be coming from op.c​:7252-7259\, which does some weird hacking around to try to force in the right line number\, which is probably getting it wrong somehow. If people don't consider this to be too much of a problem (or if somebody else comes up with an actual fix!)\, I'll go ahead and merge this soon.

Maybe 28333232 was premature. What happens if you revert 28333232?

SAVECOPSTASH no longer exists\, and I'm not sure what to use instead. Any pointers?

SAVECOPSTASH_FREE will do. It doesnā€™t actually free anything any more.

No difference.

Sorry. I wasnā€™t thinking.

This actually looks like an off-by-one error of some kind - the line number in the first warning points to the previous time that an anonymous sub was assigned to something in the script\, and then the second warning points to where the first warning should be pointing to (which is assigning an anon sub to a glob slot). Not sure if that is enlightening at all.

What I see is this​:

sensical line numbers # Failed (TODO) test 'newCONSTSUB redefinition warning + utf8' # at ext/XS-APItest/t/newCONSTSUB.t line 70. # 'Subroutine Ā redefined (previous definition at ext/XS-APItest/t/newCONSTSUB.t line 16) at ext/XS-APItest/t/newCONSTSUB.t line -1. # ' # doesn't match '(?^u​:Subroutine \x{100} redefined \(previous definition at ext/XS-APItest/t/newCONSTSUB.t line 63\) at )'

Iā€™m getting line -1.

This code for setting the line number is very odd indeed.

newCONSTSUB has everything within an ENTER/LEAVE pair\, in which it does this​:

  SAVECOPLINE(PL_curcop);   CopLINE_set(PL_curcop\, PL_parser ? PL_parser->copline : NOLINE);

I donā€™t fully understand how the parser setup happens\, but I think that code is wrong.

It needs to set PL_curcop to &PL_compiling\, because it has to modify it temporarily. Hence\, it needs to make sure the line number is set in &PL_compiling (Iā€™m assuming thatā€™s the logic behind it). But then it gets the line number from the current parser\, instead of the current op. That probably makes sense if newCONSTSUB is called at compile time\, but not run time.

But this test script gives the right line number (it does go through newCONSTSUB; gdb told me so)\, and I donā€™t know why​:

use constant { foo => 1\, bar => 2}; study *foo; # autovivify *foo = \&{"bar"};

And then newXS_len_flags also plays around with the line number\, as you saw​:

  const line_t oldline = CopLINE(PL_curcop);   if (PL_parser && PL_parser->copline != NOLINE)   CopLINE_set(PL_curcop\, PL_parser->copline);   report_redefined_cv(newSVpvn_flags(   name\,len\,(flags&SVf_UTF8)|SVs_TEMP   )\,   cv\, const_svp);   CopLINE_set(PL_curcop\, oldline);

Iā€™m not even sure thatā€™s thread-safe. newXS can be called from XSLoader​::load\, which many modules call at run-time. So PL_curcop is a an op in an op tree\, which is shared between threads. And here it is modified. Also\, if fatal warnings are enabled\, it will leave the line number modified.

Also\, itā€™s probably wrongly using the parserā€™s line number.

Iā€™m not sure I want to debug this quagmire just yet. :-)

Iā€™ve had a look at your patch\, and there is one problem with it (apart from the above). It uses the line number from the GV\, which is set when the GV is created​:

#!perl -w

$foo = 'squiggles';

sub foo { 1 }

sub foo { 2 } __END__ Subroutine foo redefined (previous definition at - line 3) at - line 11.

I donā€™t even know what the line number in the GV is for. Maybe it would be acceptable to set it when a sub is defined (after the warning\, and if CvGV(cv) == gv). I donā€™t know.

--

Father Chrysostomos

p5pRT commented 11 years ago

From @cpansprout

On Sun Jun 24 11​:01​:21 2012\, sprout wrote​:

I donā€™t even know what the line number in the GV is for. Maybe it would be acceptable to set it when a sub is defined (after the warning\, and if CvGV(cv) == gv). I donā€™t know.

I do now. :-)

It is used for ā€˜use onceā€™ warnings. If the GV is accessed multiple times; e.g.\, for $x and then sub x\, the GvFILE nad GvLINE fields are never used. So it should be safe to overwrite them when defining subs\, for the sake of putting this warning on the right line.

--

Father Chrysostomos