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
[PATCH] reduce # of calls to errno #13919
Comments
From @bulk88Created by @bulk88See attached patch. Perl Info
|
From @bulk880001-reduce-of-calls-to-errno.patchFrom 371a4965c333bbb8816e48322984a5cd85ddc4c5 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 12 Jun 2014 23:00:53 -0400
Subject: [PATCH] reduce # of calls to errno
On some OSes like Win32 and Linux, errno is a macro, which contains a
func call that returns a int *. Excessive getting/calling of it is
undesirable. Use stack vars, factor out, reposition the errno to the
branch it is used in, or change the end of a branch to be more friendly
to being merged by CC. In win32.c, neither GCC 02 or VC 01 factored out,
or attempted to merge the func calls when assigning to errno, each time
errno was written, machine code showed a unique call. So do it explictly.
No attempt was made at fixing any errno bugs, such as lack of setting
errno or what constant errno is being set to. This search for errno
callers was not exhaustive. More will need to be done. VC2003 32bits
perl521.dll's .text section dropped from 0xC281F to 0xC278f bytes of
machine code with this patch.
---
doio.c | 2 +-
mg.c | 10 ++++----
perl.c | 10 ++++---
perl.h | 1 +
pp_ctl.c | 7 ++++-
win32/perlhost.h | 6 +++-
win32/win32.c | 70 +++++++++++++++++++++++++++++++++--------------------
7 files changed, 65 insertions(+), 41 deletions(-)
diff --git a/doio.c b/doio.c
index c868b29..05ed652 100644
--- a/doio.c
+++ b/doio.c
@@ -979,7 +979,6 @@ Perl_nextargv(pTHX_ GV *gv)
} /* successful do_open_raw(), PL_inplace non-NULL */
if (ckWARN_d(WARN_INPLACE)) {
- const int eno = errno;
if (PerlLIO_stat(PL_oldname, &PL_statbuf) >= 0
&& !S_ISREG(PL_statbuf.st_mode)) {
Perl_warner(aTHX_ packWARN(WARN_INPLACE),
@@ -987,6 +986,7 @@ Perl_nextargv(pTHX_ GV *gv)
PL_oldname);
}
else {
+ const int eno = errno;
Perl_warner(aTHX_ packWARN(WARN_INPLACE), "Can't open %s: %s",
PL_oldname, Strerror(eno));
}
diff --git a/mg.c b/mg.c
index 76912bd..9e46549 100644
--- a/mg.c
+++ b/mg.c
@@ -888,16 +888,16 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
{
dSAVE_ERRNO;
#ifdef VMS
- sv_setnv(sv, (NV)((errno == EVMSERR) ? vaxc$errno : errno));
+ sv_setnv(sv, (NV)((saved_errno == EVMSERR) ? vaxc$errno : saved_errno));
#else
- sv_setnv(sv, (NV)errno);
+ sv_setnv(sv, (NV)saved_errno);
#endif
#ifdef OS2
- if (errno == errno_isOS2 || errno == errno_isOS2_set)
+ if (saved_errno == errno_isOS2 || saved_errno == errno_isOS2_set)
sv_setpv(sv, os2error(Perl_rc));
else
#endif
- if (! errno) {
+ if (! saved_errno) {
sv_setpvs(sv, "");
}
else {
@@ -905,7 +905,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
/* Strerror can return NULL on some platforms, which will
* result in 'sv' not being considered SvOK. The SvNOK_on()
* below will cause just the number part to be valid */
- sv_setpv(sv, Strerror(errno));
+ sv_setpv(sv, Strerror(saved_errno));
if (SvOK(sv)) {
fixup_errno_string(sv);
}
diff --git a/perl.c b/perl.c
index 35282f8..75e524e 100644
--- a/perl.c
+++ b/perl.c
@@ -3792,12 +3792,13 @@ S_open_script(pTHX_ const char *scriptname, bool dosearch, bool *suidscript)
#endif
}
if (!rsfp) {
+ const char * const error_str = Strerror(errno);
/* PSz 16 Sep 03 Keep neat error message */
if (PL_e_script)
- Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", Strerror(errno));
+ Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", error_str);
else
Perl_croak(aTHX_ "Can't open perl script \"%s\": %s\n",
- CopFILE(PL_curcop), Strerror(errno));
+ CopFILE(PL_curcop), error_str);
}
#if defined(HAS_FCNTL) && defined(F_SETFD)
/* ensure close-on-exec */
@@ -5020,8 +5021,9 @@ Perl_my_failure_exit(pTHX)
#else
int exitstatus;
- if (errno & 255)
- STATUS_UNIX_SET(errno);
+ int eno = errno;
+ if (eno & 255)
+ STATUS_UNIX_SET(eno);
else {
exitstatus = STATUS_UNIX >> 8;
if (exitstatus & 255)
diff --git a/perl.h b/perl.h
index 6da39f3..acef724 100644
--- a/perl.h
+++ b/perl.h
@@ -1199,6 +1199,7 @@ EXTERN_C char *crypt(const char *, const char *);
# define SETERRNO(errcode,vmserrcode) (errno = (errcode))
#endif
+/* saved_errno, is used in other parts of interp, careful if you rename it */
#ifndef dSAVEDERRNO
# define dSAVEDERRNO int saved_errno
# define dSAVE_ERRNO int saved_errno = errno
diff --git a/pp_ctl.c b/pp_ctl.c
index 380a7fe..0dfe736 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -4030,13 +4030,15 @@ PP(pp_require)
}
break;
}
- else if (errno == EMFILE || errno == EACCES) {
+ else {
+ saved_errno = errno;
+ if (saved_errno == EMFILE || saved_errno == EACCES)
/* no point in trying other paths if out of handles;
* on the other hand, if we couldn't open one of the
* files, then going on with the search could lead to
* unexpected results; see perl #113422
*/
- break;
+ goto break_loop_with_errno;
}
}
}
@@ -4044,6 +4046,7 @@ PP(pp_require)
}
}
saved_errno = errno; /* sv_2mortal can realloc things */
+ break_loop_with_errno:
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
diff --git a/win32/perlhost.h b/win32/perlhost.h
index 265328b..89296e6 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2416,12 +2416,14 @@ int
CPerlHost::Chdir(const char *dirname)
{
int ret;
+ int eno;
if (!dirname) {
- errno = ENOENT;
- return -1;
+ ret = -1;
+ goto no_entry;
}
ret = m_pvDir->SetCurrentDirectoryA((char*)dirname);
if(ret < 0) {
+ no_entry:
errno = ENOENT;
}
return ret;
diff --git a/win32/win32.c b/win32/win32.c
index 0e759d3..0faffcb 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -599,6 +599,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
int status;
int flag = P_WAIT;
int index = 0;
+ int eno;
PERL_ARGS_ASSERT_DO_ASPAWN;
@@ -625,7 +626,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
(const char*)(really ? SvPV_nolen(really) : argv[0]),
(const char* const*)argv);
- if (status < 0 && (errno == ENOEXEC || errno == ENOENT)) {
+ if (status < 0 && ((eno = errno),(eno == ENOEXEC || eno == ENOENT))) {
/* possible shell-builtin, invoke with shell */
int sh_items;
sh_items = w32_perlshell_items;
@@ -816,15 +817,16 @@ win32_opendir(const char *filename)
WIN32_FIND_DATAW wFindData;
char buffer[MAX_PATH*2];
BOOL use_default;
+ int ret_eno;
len = strlen(filename);
if (len == 0) {
- errno = ENOENT;
- return NULL;
+ ret_eno = ENOENT;
+ goto ret_w_errno;
}
if (len > MAX_PATH) {
- errno = ENAMETOOLONG;
- return NULL;
+ ret_eno = ENAMETOOLONG;
+ goto ret_w_errno;
}
/* Get us a DIR structure */
@@ -852,22 +854,23 @@ win32_opendir(const char *filename)
if (dirp->handle == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
/* FindFirstFile() fails on empty drives! */
- switch (err) {
- case ERROR_FILE_NOT_FOUND:
+ if(err == ERROR_FILE_NOT_FOUND){
return dirp;
+ }
+ Safefree(dirp);
+ switch (err) {
case ERROR_NO_MORE_FILES:
case ERROR_PATH_NOT_FOUND:
- errno = ENOENT;
+ ret_eno = ENOENT;
break;
case ERROR_NOT_ENOUGH_MEMORY:
- errno = ENOMEM;
+ ret_eno = ENOMEM;
break;
default:
- errno = EINVAL;
+ ret_eno = EINVAL;
break;
}
- Safefree(dirp);
- return NULL;
+ goto ret_w_errno;
}
use_default = FALSE;
@@ -894,6 +897,10 @@ win32_opendir(const char *filename)
dirp->end = dirp->curr = dirp->start;
dirp->end += idx;
return dirp;
+
+ ret_w_errno:
+ errno = ret_eno;
+ return NULL;
}
@@ -1438,6 +1445,7 @@ win32_stat(const char *path, Stat_t *sbuf)
int l = strlen(path);
dTHX;
int res;
+ int fail_eno;
int nlink = 1;
BOOL expect_dir = FALSE;
@@ -1452,8 +1460,8 @@ win32_stat(const char *path, Stat_t *sbuf)
case '\\':
case '/':
if (l > sizeof(buffer)) {
- errno = ENAMETOOLONG;
- return -1;
+ fail_eno = ENAMETOOLONG;
+ goto fail_with_error;
}
--l;
strncpy(buffer, path, l);
@@ -1528,13 +1536,13 @@ win32_stat(const char *path, Stat_t *sbuf)
{
/* The drive can be inaccessible, some _stat()s are buggy */
if (!GetVolumeInformationA(path,NULL,0,NULL,NULL,NULL,NULL,0)) {
- errno = ENOENT;
- return -1;
+ fail_eno = ENOENT;
+ goto fail_with_error;
}
}
if (expect_dir && !S_ISDIR(sbuf->st_mode)) {
- errno = ENOTDIR;
- return -1;
+ fail_eno = ENOTDIR;
+ goto fail_with_error;
}
if (S_ISDIR(sbuf->st_mode)) {
/* Ensure the "write" bit is switched off in the mode for
@@ -1550,6 +1558,10 @@ win32_stat(const char *path, Stat_t *sbuf)
}
}
return res;
+
+ fail_with_error:
+ errno = fail_eno;
+ return -1;
}
#define isSLASH(c) ((c) == '/' || (c) == '\\')
@@ -1631,14 +1643,14 @@ win32_longpath(char *path)
fhand = FindFirstFile(path,&fdata);
*start = sep;
if (fhand != INVALID_HANDLE_VALUE) {
- STRLEN len = strlen(fdata.cFileName);
+ STRLEN len;
+ FindClose(fhand);
+ len = strlen(fdata.cFileName);
if ((STRLEN)(tmpbuf + sizeof(tmpbuf) - tmpstart) > len) {
strcpy(tmpstart, fdata.cFileName);
tmpstart += len;
- FindClose(fhand);
}
else {
- FindClose(fhand);
errno = ERANGE;
return NULL;
}
@@ -2326,7 +2338,7 @@ win32_internal_wait(pTHX_ int *status, DWORD timeout)
}
}
- errno = GetLastError();
+ errno = GetLastError(); /* is this correct ? */
return -1;
}
@@ -2475,11 +2487,12 @@ win32_flock(int fd, int oper)
{
OVERLAPPED o;
int i = -1;
+ int eno;
HANDLE fh;
fh = (HANDLE)_get_osfhandle(fd);
if (fh == (HANDLE)-1) /* _get_osfhandle() already sets errno to EBADF */
- return -1;
+ return i;
memset(&o, 0, sizeof(o));
@@ -2506,14 +2519,17 @@ win32_flock(int fd, int oper)
i = 0;
break;
default: /* unknown */
- errno = EINVAL;
- return -1;
+ assert(i == -1);
+ eno = EINVAL;
+ goto fail;
}
if (i == -1) {
if (GetLastError() == ERROR_LOCK_VIOLATION)
- errno = EWOULDBLOCK;
+ eno = EWOULDBLOCK;
else
- errno = EINVAL;
+ eno = EINVAL;
+ fail:
+ errno = eno;
}
return i;
}
--
1.7.9.msysgit.0
|
From @bulk88On Thu Jun 12 20:09:15 2014, bulk88 wrote:
I am also worried about http://perl5.git.perl.org/perl.git/commitdiff/2ad48547234bdf521daff432b65a0b173efd2a19 which is a Father C commit, there are -T related commits above and below that one. It adds a SETERRNO before and after report_evil_fh. Every other caller of report_evil_fh only calls SETERRNO after report_evil_fh. It is is very inconsistent, since its the only one in the interp doing that. -- |
From @tonycozOn Thu Jun 12 20:09:15 2014, bulk88 wrote:
This is incorrect: --- a/doio.c if (ckWARN_d(WARN_INPLACE)) { This is reporting the failures of the opens in both sides of the if() above the C<< if (ckWARN_d(WARN_INPLACE)) { >>. Moving the initialization down here means it will be the errno from PerlLIO_stat() failing instead. } Doesn't match our usual label indentation (and a few other places too). --- a/win32/perlhost.h You added eno but didn't use it. I'm not sure this level of micro-optimization is worthwhile. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn 06/15/2014 10:10 PM, Tony Cook via RT wrote:
I have 2 more issues with this patch. One is that I don't like using variables that a macro sets, outside it. More importantly is that it isn't clear to me that the errno isn't errno is a volatile value, and it scares me to treat it otherwise. The |
From @bulk88On Tue Jun 17 19:36:28 2014, public@khwilliamson.com wrote:
http://perl5.git.perl.org/perl.git/blob/9bc33472025546a667b2f318255b6fa650b901e8:/cop.h#l859
ok, something new will be made
Code is for running, not hanging on a wall. This patch isn't Win32-specific (except for win32.c) since errno is a macro to __errno_location (a function) on Linux. All the errno/goto usage is within a screenful of code, except for pp_require which is hard to understand since its a huge for loop with break and continue all over the place deep inside many if()s. On Sun Jun 15 21:10:53 2014, tonyc wrote:
Will fix.
Yes, I agree the label names need to be harmonized. What should be the label name? or better label names
gcc version 4.6.3 (gcc-4.6.3 release with patches [build 20121012 by perlmingw.s It wasn't just old VC that failed to optimize but GCC too, this is from Strawberry 5.18 with my comments 63418E90 53 push ebx I probably wont get around to posting a new patch for 2 weeks. -- |
From @ikegamiOn Tue, Jun 17, 2014 at 11:56 PM, bulk88 via RT <perlbug-followup@perl.org>
You appeared to have misread Karl's comment. |
From @tonycozOn Wed Jun 18 08:06:23 2014, ikegami@adaelis.com wrote:
He did misread the comment, but it was my comment. He started quoting my reply part way down and attributed it correctly. Tony |
From @bulk88On Sun Jun 15 21:10:53 2014, tonyc wrote:
......
Revised patch attached. doio.c errno capture change removed. Unused var removed. labels indented differently. API made instead of directly accessing the saved_errno that was declared in a macro (in one place int saved_errno is declared as a var directly in src , not through macro, so that wont use the macro obviously). My last revision after rebasing failed a test ../lib/File/stat.t (Wstat: 256 Tes but the failure went away after removing the doio.c change. -- |
From @bulk880001-reduce-of-calls-to-errno.patchFrom a427853ee0988b673cdf3f6131f9ad899d832c60 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 12 Dec 2014 23:43:03 -0500
Subject: [PATCH] reduce # of calls to errno
On some OSes like Win32 and Linux, errno is a macro, which contains a
func call that returns a int *. Excessive getting/calling of it is
undesirable. Use stack vars, factor out, reposition the errno to the
branch it is used in, or change the end of a branch to be more friendly
to being merged by CC. In win32.c, neither GCC 02 or VC 01 factored out,
or attempted to merge the func calls when assigning to errno, each time
errno was written, machine code showed a unique call. So do it explictly.
No attempt was made at fixing any errno bugs, such as lack of setting
errno or what constant errno is being set to. This search for errno
callers was not exhaustive. More will need to be done. VC2003 32bits
perl521.dll's .text section dropped from 0xC88D3 to 0xC8833 bytes of
machine code with this patch.
---
mg.c | 10 ++++----
perl.c | 10 ++++---
perl.h | 5 ++++
pp_ctl.c | 7 ++++-
win32/perlhost.h | 5 ++-
win32/win32.c | 70 +++++++++++++++++++++++++++++++++--------------------
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/mg.c b/mg.c
index 77dd9c0..dbc5d6b 100644
--- a/mg.c
+++ b/mg.c
@@ -914,16 +914,16 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
{
dSAVE_ERRNO;
#ifdef VMS
- sv_setnv(sv, (NV)((errno == EVMSERR) ? vaxc$errno : errno));
+ sv_setnv(sv, (NV)((SAVEDCERRNO == EVMSERR) ? vaxc$errno : SAVEDCERRNO));
#else
- sv_setnv(sv, (NV)errno);
+ sv_setnv(sv, (NV)SAVEDCERRNO);
#endif
#ifdef OS2
- if (errno == errno_isOS2 || errno == errno_isOS2_set)
+ if (SAVEDCERRNO == errno_isOS2 || SAVEDCERRNO == errno_isOS2_set)
sv_setpv(sv, os2error(Perl_rc));
else
#endif
- if (! errno) {
+ if (! SAVEDCERRNO) {
sv_setpvs(sv, "");
}
else {
@@ -931,7 +931,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
/* Strerror can return NULL on some platforms, which will
* result in 'sv' not being considered SvOK. The SvNOK_on()
* below will cause just the number part to be valid */
- sv_setpv(sv, my_strerror(errno));
+ sv_setpv(sv, my_strerror(SAVEDCERRNO));
if (SvOK(sv)) {
fixup_errno_string(sv);
}
diff --git a/perl.c b/perl.c
index be9932d..d4ea00c 100644
--- a/perl.c
+++ b/perl.c
@@ -3744,12 +3744,13 @@ S_open_script(pTHX_ const char *scriptname, bool dosearch, bool *suidscript)
#endif
}
if (!rsfp) {
+ const char * const error_str = Strerror(errno);
/* PSz 16 Sep 03 Keep neat error message */
if (PL_e_script)
- Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", Strerror(errno));
+ Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", error_str);
else
Perl_croak(aTHX_ "Can't open perl script \"%s\": %s\n",
- CopFILE(PL_curcop), Strerror(errno));
+ CopFILE(PL_curcop), error_str);
}
fd = PerlIO_fileno(rsfp);
#if defined(HAS_FCNTL) && defined(F_SETFD)
@@ -4979,8 +4980,9 @@ Perl_my_failure_exit(pTHX)
#else
int exitstatus;
- if (errno & 255)
- STATUS_UNIX_SET(errno);
+ int eno = errno;
+ if (eno & 255)
+ STATUS_UNIX_SET(eno);
else {
exitstatus = STATUS_UNIX >> 8;
if (exitstatus & 255)
diff --git a/perl.h b/perl.h
index e9a1477..2b4d9c8 100644
--- a/perl.h
+++ b/perl.h
@@ -1209,6 +1209,7 @@ EXTERN_C char *crypt(const char *, const char *);
set_vaxc_errno(vmserrcode); \
} STMT_END
# define dSAVEDERRNO int saved_errno; unsigned saved_vms_errno
+# define SAVEDCERRNO saved_errno
# define dSAVE_ERRNO int saved_errno = errno; unsigned saved_vms_errno = vaxc$errno
# define SAVE_ERRNO ( saved_errno = errno, saved_vms_errno = vaxc$errno )
# define RESTORE_ERRNO SETERRNO(saved_errno, saved_vms_errno)
@@ -1244,6 +1245,7 @@ EXTERN_C char *crypt(const char *, const char *);
#ifdef WIN32
# define dSAVEDERRNO int saved_errno; DWORD saved_win32_errno
+# define SAVEDCERRNO saved_errno
# define dSAVE_ERRNO int saved_errno = errno; DWORD saved_win32_errno = GetLastError()
# define SAVE_ERRNO ( saved_errno = errno, saved_win32_errno = GetLastError() )
# define RESTORE_ERRNO ( errno = saved_errno, SetLastError(saved_win32_errno) )
@@ -1252,6 +1254,7 @@ EXTERN_C char *crypt(const char *, const char *);
#ifdef OS2
# define dSAVEDERRNO int saved_errno; unsigned long saved_os2_errno
# define dSAVE_ERRNO int saved_errno = errno; unsigned long saved_os2_errno = Perl_rc
+# define SAVEDCERRNO saved_errno
# define SAVE_ERRNO ( saved_errno = errno, saved_os2_errno = Perl_rc )
# define RESTORE_ERRNO ( errno = saved_errno, Perl_rc = saved_os2_errno )
#endif
@@ -1263,6 +1266,8 @@ EXTERN_C char *crypt(const char *, const char *);
#ifndef dSAVEDERRNO
# define dSAVEDERRNO int saved_errno
# define dSAVE_ERRNO int saved_errno = errno
+/* SAVEDCERRNO can never be a function call, it is multi-evaled */
+# define SAVEDCERRNO saved_errno
# define SAVE_ERRNO (saved_errno = errno)
# define RESTORE_ERRNO (errno = saved_errno)
#endif
diff --git a/pp_ctl.c b/pp_ctl.c
index 0bbc626..97ba22a 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -4029,13 +4029,15 @@ PP(pp_require)
}
break;
}
- else if (errno == EMFILE || errno == EACCES) {
+ else {
+ saved_errno = errno;
+ if (saved_errno == EMFILE || saved_errno == EACCES)
/* no point in trying other paths if out of handles;
* on the other hand, if we couldn't open one of the
* files, then going on with the search could lead to
* unexpected results; see perl #113422
*/
- break;
+ goto break_loop_with_errno;
}
}
}
@@ -4043,6 +4045,7 @@ PP(pp_require)
}
}
saved_errno = errno; /* sv_2mortal can realloc things */
+ break_loop_with_errno:
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
diff --git a/win32/perlhost.h b/win32/perlhost.h
index 0637f8e..bd70f8c 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2473,11 +2473,12 @@ CPerlHost::Chdir(const char *dirname)
{
int ret;
if (!dirname) {
- errno = ENOENT;
- return -1;
+ ret = -1;
+ goto no_entry;
}
ret = m_pvDir->SetCurrentDirectoryA((char*)dirname);
if(ret < 0) {
+ no_entry:
errno = ENOENT;
}
return ret;
diff --git a/win32/win32.c b/win32/win32.c
index d82cf11..388b35e 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -601,6 +601,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
int status;
int flag = P_WAIT;
int index = 0;
+ int eno;
PERL_ARGS_ASSERT_DO_ASPAWN;
@@ -627,7 +628,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
(const char*)(really ? SvPV_nolen(really) : argv[0]),
(const char* const*)argv);
- if (status < 0 && (errno == ENOEXEC || errno == ENOENT)) {
+ if (status < 0 && ((eno = errno),(eno == ENOEXEC || eno == ENOENT))) {
/* possible shell-builtin, invoke with shell */
int sh_items;
sh_items = w32_perlshell_items;
@@ -818,15 +819,16 @@ win32_opendir(const char *filename)
WIN32_FIND_DATAW wFindData;
char buffer[MAX_PATH*2];
BOOL use_default;
+ int ret_eno;
len = strlen(filename);
if (len == 0) {
- errno = ENOENT;
- return NULL;
+ ret_eno = ENOENT;
+ goto ret_w_errno;
}
if (len > MAX_PATH) {
- errno = ENAMETOOLONG;
- return NULL;
+ ret_eno = ENAMETOOLONG;
+ goto ret_w_errno;
}
/* Get us a DIR structure */
@@ -854,22 +856,23 @@ win32_opendir(const char *filename)
if (dirp->handle == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
/* FindFirstFile() fails on empty drives! */
- switch (err) {
- case ERROR_FILE_NOT_FOUND:
+ if(err == ERROR_FILE_NOT_FOUND){
return dirp;
+ }
+ Safefree(dirp);
+ switch (err) {
case ERROR_NO_MORE_FILES:
case ERROR_PATH_NOT_FOUND:
- errno = ENOENT;
+ ret_eno = ENOENT;
break;
case ERROR_NOT_ENOUGH_MEMORY:
- errno = ENOMEM;
+ ret_eno = ENOMEM;
break;
default:
- errno = EINVAL;
+ ret_eno = EINVAL;
break;
}
- Safefree(dirp);
- return NULL;
+ goto ret_w_errno;
}
use_default = FALSE;
@@ -896,6 +899,10 @@ win32_opendir(const char *filename)
dirp->end = dirp->curr = dirp->start;
dirp->end += idx;
return dirp;
+
+ ret_w_errno:
+ errno = ret_eno;
+ return NULL;
}
@@ -1440,6 +1447,7 @@ win32_stat(const char *path, Stat_t *sbuf)
int l = strlen(path);
dTHX;
int res;
+ int fail_eno;
int nlink = 1;
BOOL expect_dir = FALSE;
@@ -1454,8 +1462,8 @@ win32_stat(const char *path, Stat_t *sbuf)
case '\\':
case '/':
if (l > sizeof(buffer)) {
- errno = ENAMETOOLONG;
- return -1;
+ fail_eno = ENAMETOOLONG;
+ goto fail_with_error;
}
--l;
strncpy(buffer, path, l);
@@ -1530,13 +1538,13 @@ win32_stat(const char *path, Stat_t *sbuf)
{
/* The drive can be inaccessible, some _stat()s are buggy */
if (!GetVolumeInformationA(path,NULL,0,NULL,NULL,NULL,NULL,0)) {
- errno = ENOENT;
- return -1;
+ fail_eno = ENOENT;
+ goto fail_with_error;
}
}
if (expect_dir && !S_ISDIR(sbuf->st_mode)) {
- errno = ENOTDIR;
- return -1;
+ fail_eno = ENOTDIR;
+ goto fail_with_error;
}
if (S_ISDIR(sbuf->st_mode)) {
/* Ensure the "write" bit is switched off in the mode for
@@ -1552,6 +1560,10 @@ win32_stat(const char *path, Stat_t *sbuf)
}
}
return res;
+
+ fail_with_error:
+ errno = fail_eno;
+ return -1;
}
#define isSLASH(c) ((c) == '/' || (c) == '\\')
@@ -1633,14 +1645,14 @@ win32_longpath(char *path)
fhand = FindFirstFile(path,&fdata);
*start = sep;
if (fhand != INVALID_HANDLE_VALUE) {
- STRLEN len = strlen(fdata.cFileName);
+ STRLEN len;
+ FindClose(fhand);
+ len = strlen(fdata.cFileName);
if ((STRLEN)(tmpbuf + sizeof(tmpbuf) - tmpstart) > len) {
strcpy(tmpstart, fdata.cFileName);
tmpstart += len;
- FindClose(fhand);
}
else {
- FindClose(fhand);
errno = ERANGE;
return NULL;
}
@@ -2328,7 +2340,7 @@ win32_internal_wait(pTHX_ int *status, DWORD timeout)
}
}
- errno = GetLastError();
+ errno = GetLastError(); /* is this correct ? */
return -1;
}
@@ -2477,11 +2489,12 @@ win32_flock(int fd, int oper)
{
OVERLAPPED o;
int i = -1;
+ int eno;
HANDLE fh;
fh = (HANDLE)_get_osfhandle(fd);
if (fh == (HANDLE)-1) /* _get_osfhandle() already sets errno to EBADF */
- return -1;
+ return i;
memset(&o, 0, sizeof(o));
@@ -2508,14 +2521,17 @@ win32_flock(int fd, int oper)
i = 0;
break;
default: /* unknown */
- errno = EINVAL;
- return -1;
+ assert(i == -1);
+ eno = EINVAL;
+ goto fail;
}
if (i == -1) {
if (GetLastError() == ERROR_LOCK_VIOLATION)
- errno = EWOULDBLOCK;
+ eno = EWOULDBLOCK;
else
- errno = EINVAL;
+ eno = EINVAL;
+ fail:
+ errno = eno;
}
return i;
}
--
1.7.9.msysgit.0
|
From @bulk88On Fri Dec 12 20:47:36 2014, bulk88 wrote:
Bump, rebased patch attached with no changes AFAIK. -- |
From @bulk880001-reduce-of-calls-to-errno.patchFrom 0481d27d9fc701d6987f3ccccf905f652a963a64 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 12 Dec 2014 23:43:03 -0500
Subject: [PATCH] reduce # of calls to errno
On some OSes like Win32 and Linux, errno is a macro, which contains a
func call that returns a int *. Excessive getting/calling of it is
undesirable. Use stack vars, factor out, reposition the errno to the
branch it is used in, or change the end of a branch to be more friendly
to being merged by CC. In win32.c, neither GCC 02 or VC 01 factored out,
or attempted to merge the func calls when assigning to errno, each time
errno was written, machine code showed a unique call. So do it explictly.
No attempt was made at fixing any errno bugs, such as lack of setting
errno or what constant errno is being set to. This search for errno
callers was not exhaustive. More will need to be done. VC2003 32bits
perl521.dll's .text section dropped from 0xC88D3 to 0xC8833 bytes of
machine code with this patch.
---
mg.c | 10 ++++----
perl.c | 10 ++++---
perl.h | 5 ++++
pp_ctl.c | 7 ++++-
win32/perlhost.h | 5 ++-
win32/win32.c | 70 +++++++++++++++++++++++++++++++++--------------------
6 files changed, 67 insertions(+), 40 deletions(-)
diff --git a/mg.c b/mg.c
index b4a368d..d703f74 100644
--- a/mg.c
+++ b/mg.c
@@ -914,16 +914,16 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
{
dSAVE_ERRNO;
#ifdef VMS
- sv_setnv(sv, (NV)((errno == EVMSERR) ? vaxc$errno : errno));
+ sv_setnv(sv, (NV)((SAVEDCERRNO == EVMSERR) ? vaxc$errno : SAVEDCERRNO));
#else
- sv_setnv(sv, (NV)errno);
+ sv_setnv(sv, (NV)SAVEDCERRNO);
#endif
#ifdef OS2
- if (errno == errno_isOS2 || errno == errno_isOS2_set)
+ if (SAVEDCERRNO == errno_isOS2 || SAVEDCERRNO == errno_isOS2_set)
sv_setpv(sv, os2error(Perl_rc));
else
#endif
- if (! errno) {
+ if (! SAVEDCERRNO) {
sv_setpvs(sv, "");
}
else {
@@ -931,7 +931,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
/* Strerror can return NULL on some platforms, which will
* result in 'sv' not being considered SvOK. The SvNOK_on()
* below will cause just the number part to be valid */
- sv_setpv(sv, my_strerror(errno));
+ sv_setpv(sv, my_strerror(SAVEDCERRNO));
if (SvOK(sv)) {
fixup_errno_string(sv);
}
diff --git a/perl.c b/perl.c
index f309cea..d40b3e1 100644
--- a/perl.c
+++ b/perl.c
@@ -3774,12 +3774,13 @@ S_open_script(pTHX_ const char *scriptname, bool dosearch, bool *suidscript)
#endif
}
if (!rsfp) {
+ const char * const error_str = Strerror(errno);
/* PSz 16 Sep 03 Keep neat error message */
if (PL_e_script)
- Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", Strerror(errno));
+ Perl_croak(aTHX_ "Can't open "BIT_BUCKET": %s\n", error_str);
else
Perl_croak(aTHX_ "Can't open perl script \"%s\": %s\n",
- CopFILE(PL_curcop), Strerror(errno));
+ CopFILE(PL_curcop), error_str);
}
fd = PerlIO_fileno(rsfp);
#if defined(HAS_FCNTL) && defined(F_SETFD)
@@ -5007,8 +5008,9 @@ Perl_my_failure_exit(pTHX)
#else
int exitstatus;
- if (errno & 255)
- STATUS_UNIX_SET(errno);
+ int eno = errno;
+ if (eno & 255)
+ STATUS_UNIX_SET(eno);
else {
exitstatus = STATUS_UNIX >> 8;
if (exitstatus & 255)
diff --git a/perl.h b/perl.h
index 9114176..4cff03c 100644
--- a/perl.h
+++ b/perl.h
@@ -1189,6 +1189,7 @@ EXTERN_C char *crypt(const char *, const char *);
set_vaxc_errno(vmserrcode); \
} STMT_END
# define dSAVEDERRNO int saved_errno; unsigned saved_vms_errno
+# define SAVEDCERRNO saved_errno
# define dSAVE_ERRNO int saved_errno = errno; unsigned saved_vms_errno = vaxc$errno
# define SAVE_ERRNO ( saved_errno = errno, saved_vms_errno = vaxc$errno )
# define RESTORE_ERRNO SETERRNO(saved_errno, saved_vms_errno)
@@ -1226,6 +1227,7 @@ EXTERN_C char *crypt(const char *, const char *);
#ifdef WIN32
# define dSAVEDERRNO int saved_errno; DWORD saved_win32_errno
+# define SAVEDCERRNO saved_errno
# define dSAVE_ERRNO int saved_errno = errno; DWORD saved_win32_errno = GetLastError()
# define SAVE_ERRNO ( saved_errno = errno, saved_win32_errno = GetLastError() )
# define RESTORE_ERRNO ( errno = saved_errno, SetLastError(saved_win32_errno) )
@@ -1234,6 +1236,7 @@ EXTERN_C char *crypt(const char *, const char *);
#ifdef OS2
# define dSAVEDERRNO int saved_errno; unsigned long saved_os2_errno
# define dSAVE_ERRNO int saved_errno = errno; unsigned long saved_os2_errno = Perl_rc
+# define SAVEDCERRNO saved_errno
# define SAVE_ERRNO ( saved_errno = errno, saved_os2_errno = Perl_rc )
# define RESTORE_ERRNO ( errno = saved_errno, Perl_rc = saved_os2_errno )
#endif
@@ -1245,6 +1248,8 @@ EXTERN_C char *crypt(const char *, const char *);
#ifndef dSAVEDERRNO
# define dSAVEDERRNO int saved_errno
# define dSAVE_ERRNO int saved_errno = errno
+/* SAVEDCERRNO can never be a function call, it is multi-evaled */
+# define SAVEDCERRNO saved_errno
# define SAVE_ERRNO (saved_errno = errno)
# define RESTORE_ERRNO (errno = saved_errno)
#endif
diff --git a/pp_ctl.c b/pp_ctl.c
index 58b62d9..2fe9250 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3965,13 +3965,15 @@ PP(pp_require)
}
break;
}
- else if (errno == EMFILE || errno == EACCES) {
+ else {
+ saved_errno = errno;
+ if (saved_errno == EMFILE || saved_errno == EACCES)
/* no point in trying other paths if out of handles;
* on the other hand, if we couldn't open one of the
* files, then going on with the search could lead to
* unexpected results; see perl #113422
*/
- break;
+ goto break_loop_with_errno;
}
}
}
@@ -3979,6 +3981,7 @@ PP(pp_require)
}
}
saved_errno = errno; /* sv_2mortal can realloc things */
+ break_loop_with_errno:
sv_2mortal(namesv);
if (!tryrsfp) {
if (PL_op->op_type == OP_REQUIRE) {
diff --git a/win32/perlhost.h b/win32/perlhost.h
index 7a0c3b3..c624aec 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2416,11 +2416,12 @@ CPerlHost::Chdir(const char *dirname)
{
int ret;
if (!dirname) {
- errno = ENOENT;
- return -1;
+ ret = -1;
+ goto no_entry;
}
ret = m_pvDir->SetCurrentDirectoryA((char*)dirname);
if(ret < 0) {
+ no_entry:
errno = ENOENT;
}
return ret;
diff --git a/win32/win32.c b/win32/win32.c
index 4c3e3d4..fa7f6c1 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -595,6 +595,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
int status;
int flag = P_WAIT;
int index = 0;
+ int eno;
PERL_ARGS_ASSERT_DO_ASPAWN;
@@ -621,7 +622,7 @@ Perl_do_aspawn(pTHX_ SV *really, SV **mark, SV **sp)
(const char*)(really ? SvPV_nolen(really) : argv[0]),
(const char* const*)argv);
- if (status < 0 && (errno == ENOEXEC || errno == ENOENT)) {
+ if (status < 0 && ((eno = errno),(eno == ENOEXEC || eno == ENOENT))) {
/* possible shell-builtin, invoke with shell */
int sh_items;
sh_items = w32_perlshell_items;
@@ -812,15 +813,16 @@ win32_opendir(const char *filename)
WIN32_FIND_DATAW wFindData;
char buffer[MAX_PATH*2];
BOOL use_default;
+ int ret_eno;
len = strlen(filename);
if (len == 0) {
- errno = ENOENT;
- return NULL;
+ ret_eno = ENOENT;
+ goto ret_w_errno;
}
if (len > MAX_PATH) {
- errno = ENAMETOOLONG;
- return NULL;
+ ret_eno = ENAMETOOLONG;
+ goto ret_w_errno;
}
/* Get us a DIR structure */
@@ -848,22 +850,23 @@ win32_opendir(const char *filename)
if (dirp->handle == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
/* FindFirstFile() fails on empty drives! */
- switch (err) {
- case ERROR_FILE_NOT_FOUND:
+ if(err == ERROR_FILE_NOT_FOUND){
return dirp;
+ }
+ Safefree(dirp);
+ switch (err) {
case ERROR_NO_MORE_FILES:
case ERROR_PATH_NOT_FOUND:
- errno = ENOENT;
+ ret_eno = ENOENT;
break;
case ERROR_NOT_ENOUGH_MEMORY:
- errno = ENOMEM;
+ ret_eno = ENOMEM;
break;
default:
- errno = EINVAL;
+ ret_eno = EINVAL;
break;
}
- Safefree(dirp);
- return NULL;
+ goto ret_w_errno;
}
use_default = FALSE;
@@ -890,6 +893,10 @@ win32_opendir(const char *filename)
dirp->end = dirp->curr = dirp->start;
dirp->end += idx;
return dirp;
+
+ ret_w_errno:
+ errno = ret_eno;
+ return NULL;
}
@@ -1436,6 +1443,7 @@ win32_stat(const char *path, Stat_t *sbuf)
int l = strlen(path);
dTHX;
int res;
+ int fail_eno;
int nlink = 1;
BOOL expect_dir = FALSE;
@@ -1450,8 +1458,8 @@ win32_stat(const char *path, Stat_t *sbuf)
case '\\':
case '/':
if (l > sizeof(buffer)) {
- errno = ENAMETOOLONG;
- return -1;
+ fail_eno = ENAMETOOLONG;
+ goto fail_with_error;
}
--l;
strncpy(buffer, path, l);
@@ -1526,13 +1534,13 @@ win32_stat(const char *path, Stat_t *sbuf)
{
/* The drive can be inaccessible, some _stat()s are buggy */
if (!GetVolumeInformationA(path,NULL,0,NULL,NULL,NULL,NULL,0)) {
- errno = ENOENT;
- return -1;
+ fail_eno = ENOENT;
+ goto fail_with_error;
}
}
if (expect_dir && !S_ISDIR(sbuf->st_mode)) {
- errno = ENOTDIR;
- return -1;
+ fail_eno = ENOTDIR;
+ goto fail_with_error;
}
if (S_ISDIR(sbuf->st_mode)) {
/* Ensure the "write" bit is switched off in the mode for
@@ -1548,6 +1556,10 @@ win32_stat(const char *path, Stat_t *sbuf)
}
}
return res;
+
+ fail_with_error:
+ errno = fail_eno;
+ return -1;
}
#define isSLASH(c) ((c) == '/' || (c) == '\\')
@@ -1629,14 +1641,14 @@ win32_longpath(char *path)
fhand = FindFirstFile(path,&fdata);
*start = sep;
if (fhand != INVALID_HANDLE_VALUE) {
- STRLEN len = strlen(fdata.cFileName);
+ STRLEN len;
+ FindClose(fhand);
+ len = strlen(fdata.cFileName);
if ((STRLEN)(tmpbuf + sizeof(tmpbuf) - tmpstart) > len) {
strcpy(tmpstart, fdata.cFileName);
tmpstart += len;
- FindClose(fhand);
}
else {
- FindClose(fhand);
errno = ERANGE;
return NULL;
}
@@ -2324,7 +2336,7 @@ win32_internal_wait(pTHX_ int *status, DWORD timeout)
}
}
- errno = GetLastError();
+ errno = GetLastError(); /* is this correct ? */
return -1;
}
@@ -2473,11 +2485,12 @@ win32_flock(int fd, int oper)
{
OVERLAPPED o;
int i = -1;
+ int eno;
HANDLE fh;
fh = (HANDLE)_get_osfhandle(fd);
if (fh == (HANDLE)-1) /* _get_osfhandle() already sets errno to EBADF */
- return -1;
+ return i;
memset(&o, 0, sizeof(o));
@@ -2504,14 +2517,17 @@ win32_flock(int fd, int oper)
i = 0;
break;
default: /* unknown */
- errno = EINVAL;
- return -1;
+ assert(i == -1);
+ eno = EINVAL;
+ goto fail;
}
if (i == -1) {
if (GetLastError() == ERROR_LOCK_VIOLATION)
- errno = EWOULDBLOCK;
+ eno = EWOULDBLOCK;
else
- errno = EINVAL;
+ eno = EINVAL;
+ fail:
+ errno = eno;
}
return i;
}
--
1.7.9.msysgit.0
|
From @tonycozOn Sun Jul 05 11:44:20 2015, bulk88 wrote:
This fixes the issues with the original, but I'm still not sure this level Anyone else? Tony |
From @craigberryOn Tue, Jul 7, 2015 at 12:37 AM, Tony Cook via RT
I think it hurts readability. Things like doing a goto out of the |
From zefram@fysh.orgWe should not apply the hunks that break macro encapsulation -zefram |
@cpansprout - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#122096 (status was 'resolved')
Searchable as RT122096$
The text was updated successfully, but these errors were encountered: