Replace use of PL_statbuf and PL_timesbuf with auto variables #13632
Comments
From @nwc10intrpvar.h contains these two lines: PERLVAR(I, statbuf, Stat_t) and PERLVAR(I, timesbuf, struct tms) (Note that the stat struct used to implement _ is in PL_statcache, These variables are not used to pass any state between functions in http://grep.cpan.me/?q=PL_statbuf (note that PAR-Packer must already work just fine without PL_statbuf, These seem to be vestiges of Perl 1, which has this in its perl.h: EXT struct stat statbuf; I asked Larry if he remembered why: 16:05 < nwc10> TimToady: in perl 1, why does it have EXT struct stat statbuf; and EXT struct tms timesbuf; in perl.h, rather than using local variables in functions? Was there some compiler back then that screwed up allocating structs on the stack? http://irclog.perlgeek.de/perl6/2014-02-28#i_8363520 I think that we should abolish these two variables, and replace them I think that we shouldn't do this before v5.20.0 escapes. Given the complete lack of CPAN usage, I'm also not sure whether we Nicholas Clark |
From zefram@fysh.orgNicholas Clark wrote:
Agreed.
Just remove them, early in 5.21. Even if they are used, a year is plenty -zefram |
|
The RT System itself - Status changed from 'new' to 'open' |
From @nwc10On Sat, Mar 01, 2014 at 02:46:47PM +0000, Zefram wrote:
Thanks. I'd reached this conclusion too. I don't think that there's much Nicholas Clark |
From @bulk88Can PL_statcache also be removed? -- |
From @tonycozOn Sat, Mar 15, 2014 at 11:56:01PM -0700, bulk88 via RT wrote:
No, it's used to save the result of the last stat so that -s _ etc Tony |
From @bulk88On Sat Mar 01 08:09:26 2014, nicholas wrote:
This ticket got a commit from NC at http://perl5.git.perl.org/perl.git/commit/25983af42cdcf2dc1fea6717dac7aac441b6301d I think this ticket can be closed. -- |
From @bulk88On Sun Mar 22 20:15:53 2015, bulk88 wrote:
Actually its a 5.23 blocker now, that commit didn't remove the interp var http://perl5.git.perl.org/perl.git/blobdiff/c79d007613fa174f6f5e1588ca5374f505fc44af..25983af:/intrpvar.h so that has to be done, it should have been done after blead was thawed after 5.20 was released, but NC never got around to it. -- |
From @bulk88Patch attached. This ticket needs to still stay open since PL_statbuf hasn't been removed, it isn't since simple since I found an uninit memory problem with PL_statbuf that will take a while to solve. -- |
From @bulk880001-121351-remove-deprecated-PL_timesbuf.patchFrom 5856cdd25bb313c61eaddb62ce7cac58d5a22fc3 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 11 Aug 2015 14:17:52 -0400
Subject: [PATCH] [#121351] remove deprecated PL_timesbuf
Saves memory in interp struct.
---
embedvar.h | 1 -
intrpvar.h | 5 -----
pod/perldelta.pod | 3 ++-
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/embedvar.h b/embedvar.h
index 9ed30e0..f6aaadf 100644
--- a/embedvar.h
+++ b/embedvar.h
@@ -323,7 +323,6 @@
#define PL_tainted (vTHX->Itainted)
#define PL_tainting (vTHX->Itainting)
#define PL_threadhook (vTHX->Ithreadhook)
-#define PL_timesbuf (vTHX->Itimesbuf)
#define PL_tmps_floor (vTHX->Itmps_floor)
#define PL_tmps_ix (vTHX->Itmps_ix)
#define PL_tmps_max (vTHX->Itmps_max)
diff --git a/intrpvar.h b/intrpvar.h
index 6ee88b3..337bf4f 100644
--- a/intrpvar.h
+++ b/intrpvar.h
@@ -178,11 +178,6 @@ PERLVAR(I, statcache, Stat_t) /* _ */
PERLVAR(I, statgv, GV *)
PERLVARI(I, statname, SV *, NULL)
-#ifdef HAS_TIMES
-/* Will be removed soon after v5.23.2. See RT #121351 */
-PERLVAR(I, timesbuf, struct tms)
-#endif
-
/*
=for apidoc mn|SV*|PL_rs
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index 6ace170..38fd118 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -334,7 +334,8 @@ well.
=item *
-XXX
+Deprecated in 5.20 interpreter variable C<PL_timesbuf> has been removed.
+[perl #121351]
=back
--
1.7.9.msysgit.0
|
From @bulk88On Tue Aug 11 11:20:13 2015, bulk88 wrote:
Another patch removing most of the usage of PL_statbuf. Since the PL_statbuf code is complicated and using global state, it is best to remove it slowly to identify breakage/bisecting. -- |
From @bulk880001-121351-partial-PL_statbuf-removal.patchFrom f3f2517215490c31e1646dfdcf6789f382d142e8 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 14 Aug 2015 18:34:09 -0400
Subject: [PATCH] [#121351] partial PL_statbuf removal
Perl_nextargv has to have access to the Stat_t that is written to inside
S_openn_cleanup or else run/switches.t, io/argv.t, io/inplace.t, and
io/iprefix.t will fail. Removing the uses of PL_statbuf that are using
PL_statbuf due to historical reason, and not using PL_statbuf to pass data
between different funcs/different callstacks. This patch makes it easier to
remove PL_statbuf in the future since the number uses of it has been
reduced.
-in Perl_apply move SETERRNO before tot--; so the branch can be combined
with other "tot--;" branches by CC optmizer
-combine 2 Perl_croak(aTHX_ "Illegal suidscript"); statements in
S_validate_suid to make code look simpler, drop my_perl arg for space
efficiency on threads of rarely executed code
---
doio.c | 16 +++++++++-------
perl.c | 13 +++++--------
pp_hot.c | 3 ++-
pp_sys.c | 10 +++++++---
util.c | 37 +++++++++++++++++++++++--------------
5 files changed, 46 insertions(+), 33 deletions(-)
diff --git a/doio.c b/doio.c
index 39e5ce7..c196d74 100644
--- a/doio.c
+++ b/doio.c
@@ -835,6 +835,7 @@ Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
if (!GvAV(gv))
return NULL;
while (av_tindex(GvAV(gv)) >= 0) {
+ Stat_t statbuf;
STRLEN oldlen;
SV *const sv = av_shift(GvAV(gv));
SAVEFREESV(sv);
@@ -976,13 +977,13 @@ Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
setdefout(PL_argvoutgv);
PL_lastfd = PerlIO_fileno(IoIFP(GvIOp(PL_argvoutgv)));
if (PL_lastfd >= 0) {
- (void)PerlLIO_fstat(PL_lastfd,&PL_statbuf);
+ (void)PerlLIO_fstat(PL_lastfd,&statbuf);
#ifdef HAS_FCHMOD
(void)fchmod(PL_lastfd,PL_filemode);
#else
(void)PerlLIO_chmod(PL_oldname,PL_filemode);
#endif
- if (fileuid != PL_statbuf.st_uid || filegid != PL_statbuf.st_gid) {
+ if (fileuid != statbuf.st_uid || filegid != statbuf.st_gid) {
/* XXX silently ignore failures */
#ifdef HAS_FCHOWN
PERL_UNUSED_RESULT(fchown(PL_lastfd,fileuid,filegid));
@@ -999,8 +1000,8 @@ Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
if (ckWARN_d(WARN_INPLACE)) {
const int eno = errno;
- if (PerlLIO_stat(PL_oldname, &PL_statbuf) >= 0
- && !S_ISREG(PL_statbuf.st_mode)) {
+ if (PerlLIO_stat(PL_oldname, &statbuf) >= 0
+ && !S_ISREG(statbuf.st_mode)) {
Perl_warner(aTHX_ packWARN(WARN_INPLACE),
"Can't do inplace edit: %s is not a regular file",
PL_oldname);
@@ -1955,11 +1956,12 @@ nothing in the core.
tot--;
}
else { /* don't let root wipe out directories without -U */
- if (PerlLIO_lstat(s,&PL_statbuf) < 0)
- tot--;
- else if (S_ISDIR(PL_statbuf.st_mode)) {
+ Stat_t statbuf;
+ if (PerlLIO_lstat(s, &statbuf) < 0)
tot--;
+ else if (S_ISDIR(statbuf.st_mode)) {
SETERRNO(EISDIR, SS_NOPRIV);
+ tot--;
}
else {
if (UNLINK(s))
diff --git a/perl.c b/perl.c
index 1823c99..63d89c4 100644
--- a/perl.c
+++ b/perl.c
@@ -3840,16 +3840,13 @@ S_validate_suid(pTHX_ PerlIO *rsfp)
if (my_euid != my_uid || my_egid != my_gid) { /* (suidperl doesn't exist, in fact) */
dVAR;
int fd = PerlIO_fileno(rsfp);
- if (fd < 0) {
- Perl_croak(aTHX_ "Illegal suidscript");
- } else {
- if (PerlLIO_fstat(fd, &PL_statbuf) < 0) { /* may be either wrapped or real suid */
- Perl_croak(aTHX_ "Illegal suidscript");
- }
+ Stat_t statbuf;
+ if (fd < 0 || PerlLIO_fstat(fd, &statbuf) < 0) { /* may be either wrapped or real suid */
+ Perl_croak_nocontext( "Illegal suidscript");
}
- if ((my_euid != my_uid && my_euid == PL_statbuf.st_uid && PL_statbuf.st_mode & S_ISUID)
+ if ((my_euid != my_uid && my_euid == statbuf.st_uid && statbuf.st_mode & S_ISUID)
||
- (my_egid != my_gid && my_egid == PL_statbuf.st_gid && PL_statbuf.st_mode & S_ISGID)
+ (my_egid != my_gid && my_egid == statbuf.st_gid && statbuf.st_mode & S_ISGID)
)
if (!PL_do_undump)
Perl_croak(aTHX_ "YOU HAVEN'T DISABLED SET-ID SCRIPTS IN THE KERNEL YET!\n\
diff --git a/pp_hot.c b/pp_hot.c
index 1094510..dc484f7 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1727,6 +1727,7 @@ Perl_do_readline(pTHX)
XPUSHs(sv);
if (type == OP_GLOB) {
const char *t1;
+ Stat_t statbuf;
if (SvCUR(sv) > 0 && SvCUR(PL_rs) > 0) {
char * const tmps = SvEND(sv) - 1;
@@ -1742,7 +1743,7 @@ Perl_do_readline(pTHX)
if (strchr("$&*(){}[]'\";\\|?<>~`", *t1))
#endif
break;
- if (*t1 && PerlLIO_lstat(SvPVX_const(sv), &PL_statbuf) < 0) {
+ if (*t1 && PerlLIO_lstat(SvPVX_const(sv), &statbuf) < 0) {
(void)POPs; /* Unmatched wildcard? Chuck it... */
continue;
}
diff --git a/pp_sys.c b/pp_sys.c
index ebd675b..6010c06 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -3706,17 +3706,20 @@ PP(pp_rename)
{
dSP; dTARGET;
int anum;
+#ifndef HAS_RENAME
+ Stat_t statbuf;
+#endif
const char * const tmps2 = POPpconstx;
const char * const tmps = SvPV_nolen_const(TOPs);
TAINT_PROPER("rename");
#ifdef HAS_RENAME
anum = PerlLIO_rename(tmps, tmps2);
#else
- if (!(anum = PerlLIO_stat(tmps, &PL_statbuf))) {
+ if (!(anum = PerlLIO_stat(tmps, &statbuf))) {
if (same_dirent(tmps2, tmps)) /* can always rename to same name */
anum = 1;
else {
- if (PerlProc_geteuid() || PerlLIO_stat(tmps2, &PL_statbuf) < 0 || !S_ISDIR(PL_statbuf.st_mode))
+ if (PerlProc_geteuid() || PerlLIO_stat(tmps2, &statbuf) < 0 || !S_ISDIR(statbuf.st_mode))
(void)UNLINK(tmps2);
if (!(anum = link(tmps, tmps2)))
anum = UNLINK(tmps);
@@ -3878,7 +3881,8 @@ S_dooneliner(pTHX_ const char *cmd, const char *filename)
return 0;
}
else { /* some mkdirs return no failure indication */
- anum = (PerlLIO_stat(save_filename, &PL_statbuf) >= 0);
+ Stat_t statbuf;
+ anum = (PerlLIO_stat(save_filename, &statbuf) >= 0);
if (PL_op->op_type == OP_RMDIR)
anum = !anum;
if (anum)
diff --git a/util.c b/util.c
index 0a2c11e..049f997 100644
--- a/util.c
+++ b/util.c
@@ -3259,13 +3259,16 @@ Perl_find_script(pTHX_ const char *scriptname, bool dosearch,
#endif
DEBUG_p(PerlIO_printf(Perl_debug_log,
"Looking for %s\n",cur));
- if (PerlLIO_stat(cur,&PL_statbuf) >= 0
- && !S_ISDIR(PL_statbuf.st_mode)) {
- dosearch = 0;
- scriptname = cur;
+ {
+ Stat_t statbuf;
+ if (PerlLIO_stat(cur,&statbuf) >= 0
+ && !S_ISDIR(statbuf.st_mode)) {
+ dosearch = 0;
+ scriptname = cur;
#ifdef SEARCH_EXTS
- break;
+ break;
#endif
+ }
}
#ifdef SEARCH_EXTS
if (cur == scriptname) {
@@ -3291,6 +3294,7 @@ Perl_find_script(pTHX_ const char *scriptname, bool dosearch,
bufend = s + strlen(s);
while (s < bufend) {
+ Stat_t statbuf;
# ifdef DOSISH
for (len = 0; *s
&& *s != ';'; len++, s++) {
@@ -3327,8 +3331,8 @@ Perl_find_script(pTHX_ const char *scriptname, bool dosearch,
do {
#endif
DEBUG_p(PerlIO_printf(Perl_debug_log, "Looking for %s\n",tmpbuf));
- retval = PerlLIO_stat(tmpbuf,&PL_statbuf);
- if (S_ISDIR(PL_statbuf.st_mode)) {
+ retval = PerlLIO_stat(tmpbuf,&statbuf);
+ if (S_ISDIR(statbuf.st_mode)) {
retval = -1;
}
#ifdef SEARCH_EXTS
@@ -3339,10 +3343,10 @@ Perl_find_script(pTHX_ const char *scriptname, bool dosearch,
#endif
if (retval < 0)
continue;
- if (S_ISREG(PL_statbuf.st_mode)
- && cando(S_IRUSR,TRUE,&PL_statbuf)
+ if (S_ISREG(statbuf.st_mode)
+ && cando(S_IRUSR,TRUE,&statbuf)
#if !defined(DOSISH)
- && cando(S_IXUSR,TRUE,&PL_statbuf)
+ && cando(S_IXUSR,TRUE,&statbuf)
#endif
)
{
@@ -3353,11 +3357,16 @@ Perl_find_script(pTHX_ const char *scriptname, bool dosearch,
xfailed = savepv(tmpbuf);
}
#ifndef DOSISH
- if (!xfound && !seen_dot && !xfailed &&
- (PerlLIO_stat(scriptname,&PL_statbuf) < 0
- || S_ISDIR(PL_statbuf.st_mode)))
+ {
+ Stat_t statbuf;
+ if (!xfound && !seen_dot && !xfailed &&
+ (PerlLIO_stat(scriptname,&statbuf) < 0
+ || S_ISDIR(statbuf.st_mode)))
+#endif
+ seen_dot = 1; /* Disable message. */
+#ifndef DOSISH
+ }
#endif
- seen_dot = 1; /* Disable message. */
if (!xfound) {
if (flags & 1) { /* do or die? */
/* diag_listed_as: Can't execute %s */
--
1.7.9.msysgit.0
|
From @bulk88On Fri Aug 14 21:14:28 2015, bulk88 wrote:
Bump. -- |
From @iabynOn Fri, Sep 25, 2015 at 03:08:49PM -0700, bulk88 via RT wrote:
Thanks, applied as -- |
|
@iabyn - Status changed from 'open' to 'resolved' |
From @bulk88On Thu Oct 08 08:57:29 2015, davem wrote:
This ticket needs to be reopened, it is not resolved. Patch "remove deprecated PL_timesbuf" was never applied. And this ticket needs to stays open until the original purpose of it "Replace use of PL_statbuf and PL_timesbuf with auto variables" is satisfied. Patch "partial PL_statbuf removal" didn't remove entirely remove PL_statbuf. Removing the last piece of PL_statbuf will probably be part of a refactor of "S_openn_cleanup" and "openn_setup" since those 2 take half a dozen or more "&some_c_auto", which sounds like a OPEN_STATE struct needs to created for doio.c and just one "OPEN_STATE *" passed to openn_cleanup/openn_setup instead of each member of a future OPEN_STATE being passed by reference one by one as args. -- |
|
@tonycoz - Status changed from 'resolved' to 'open' |
From @arcbulk88 via RT <perlbug-followup@perl.org> wrote:
Thanks, applied (with tweaks to the perldelta entry, and with a couple My understanding is that there are other parts of this ticket that are -- |
From @ilmariAaron Crane <arc@cpan.org> writes:
Yes, thre's some uses left in os2/os2.c and ext/ODBM_File/ODBM_File.xs, I'll push the following patch to remove the two former, if nobody |
From @ilmari0001-perl-1369717-Remove-non-doio.c-uses-of-PL_statbuf.patchFrom c8879096cae5c2bfc008cf50ef582ab19595b9f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Sun, 17 Jan 2016 15:52:30 +0000
Subject: [PATCH] [perl #1369717] Remove non-doio.c uses of PL_statbuf
These are the last remaining uses outside the interwoven mess in
S_openn_cleanup and openn_setup in doio.c.
---
ext/ODBM_File/ODBM_File.pm | 2 +-
ext/ODBM_File/ODBM_File.xs | 3 ++-
os2/os2.c | 5 +++--
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/ext/ODBM_File/ODBM_File.pm b/ext/ODBM_File/ODBM_File.pm
index 958232c..dd92fd3 100644
--- a/ext/ODBM_File/ODBM_File.pm
+++ b/ext/ODBM_File/ODBM_File.pm
@@ -7,7 +7,7 @@ require Tie::Hash;
require XSLoader;
our @ISA = qw(Tie::Hash);
-our $VERSION = "1.12";
+our $VERSION = "1.13";
XSLoader::load();
diff --git a/ext/ODBM_File/ODBM_File.xs b/ext/ODBM_File/ODBM_File.xs
index d1ece7f..f26abda 100644
--- a/ext/ODBM_File/ODBM_File.xs
+++ b/ext/ODBM_File/ODBM_File.xs
@@ -92,6 +92,7 @@ odbm_TIEHASH(dbtype, filename, flags, mode)
{
char *tmpbuf;
void * dbp ;
+ struct stat statbuf;
dMY_CXT;
if (dbmrefcnt++)
@@ -99,7 +100,7 @@ odbm_TIEHASH(dbtype, filename, flags, mode)
Newx(tmpbuf, strlen(filename) + 5, char);
SAVEFREEPV(tmpbuf);
sprintf(tmpbuf,"%s.dir",filename);
- if (stat(tmpbuf, &PL_statbuf) < 0) {
+ if (stat(tmpbuf, &statbuf) < 0) {
if (flags & O_CREAT) {
if (mode < 0 || close(creat(tmpbuf,mode)) < 0)
croak("ODBM_File: Can't create %s", filename);
diff --git a/os2/os2.c b/os2/os2.c
index 8c5e941..a4f5015 100644
--- a/os2/os2.c
+++ b/os2/os2.c
@@ -1140,6 +1140,7 @@ do_spawn_ve(pTHX_ SV *really, U32 flag, U32 execf, char *inicmd, U32 addflag)
if (!buf)
buf = ""; /* XXX Needed? */
if (!buf[0]) { /* Empty... */
+ struct stat statbuf;
PerlIO_close(file);
/* Special case: maybe from -Zexe build, so
there is an executable around (contrary to
@@ -1148,8 +1149,8 @@ do_spawn_ve(pTHX_ SV *really, U32 flag, U32 execf, char *inicmd, U32 addflag)
reached this place). */
sv_catpv(scrsv, ".exe");
PL_Argv[0] = scr = SvPV(scrsv, n_a); /* Reload */
- if (PerlLIO_stat(scr,&PL_statbuf) >= 0
- && !S_ISDIR(PL_statbuf.st_mode)) { /* Found */
+ if (PerlLIO_stat(scr,&statbuf) >= 0
+ && !S_ISDIR(statbuf.st_mode)) { /* Found */
real_name = scr;
pass++;
goto reread;
--
2.7.0.rc3
|
From @ilmari-- |
From @ilmariilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
Pushed as d50541e with the minor tweak -- |
From zefram@fysh.orgPL_statbuf was finally removed in 5.27.1, in commit -zefram |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#121351 (status was 'resolved')
Searchable as RT121351$
The text was updated successfully, but these errors were encountered: