Skip Menu |
Report information
Id: 122096
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: bulk88 <bulk88 [at] hotmail.com>
Cc:
AdminCc:

Operating System: mswin32
PatchStatus: HasPatch
Severity: low
Type:
Perl Version: 5.21.1
Fixed In: (no value)



Subject: [PATCH] reduce # of calls to errno
To: perlbug [...] perl.org
Date: Thu, 12 Jun 2014 23:09:00 -0400
From: bulk88 <bulk88 [...] hotmail.com>
Download (untitled) / with headers
text/plain 3.4k
This is a bug report for perl from bulk88@hotmail.com, generated with the help of perlbug 1.40 running under perl 5.21.1. ----------------------------------------------------------------- [Please describe your issue here] See attached patch. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.21.1: Configured by Owner at Wed May 28 03:57:28 2014. Summary of my perl5 (revision 5 version 21 subversion 1) configuration: Local Commit: 1abbcfa06576bf8a6937c566bb4d18ba803b59d8 Ancestor: 234105dd8a732de5fb48ccb1838c99281f89f669 Platform: osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread uname='' config_args='undef' hint=recommended, useposix=true, d_sigaction=undef useithreads=define, usemultiplicity=define use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -G7 -GL -DWIN32 -D_CONSOLE -DNO_STRICT -DPERL_TEXTMODE_SCRIPTS -DPERL_HASH_FUNC_ONE_AT_A_TIME -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T', optimize='-O1 -MD -Zi -DNDEBUG -G7 -GL', cppflags='-DWIN32' ccversion='13.10.6030', gccversion='', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf -ltcg -libpath:"c:\perl521\lib\CORE" -machine:x86' libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib" libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl521.lib gnulibc_version='' Dynamic Linking: dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' ' cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug -opt:ref,icf -ltcg -libpath:"c:\perl521\lib\CORE" -machine:x86' Locally applied patches: 7494266ea378a3cdc4bfd51725012c1e432db0f1 61961437d9453dd0d4053ad100e97a029a24edbb cd30b936fc5177ce169d776445d09c9898c15da1 1abbcfa06576bf8a6937c566bb4d18ba803b59d8 --- @INC for perl 5.21.1: C:/perl521/site/lib C:/perl521/lib . --- Environment for perl 5.21.1: HOME (unset) LANG (unset) LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=C:\perl521\bin;C:\Program Files\Microsoft Visual Studio .NET 2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET 2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET 2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem; PERL_BADLANG (unset) SHELL (unset)
From 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 745b
On Thu Jun 12 20:09:15 2014, bulk88 wrote: Show quoted text
> This is a bug report for perl from bulk88@hotmail.com, > generated with the help of perlbug 1.40 running under perl 5.21.1. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > See attached patch.
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. -- bulk88 ~ bulk88 at hotmail.com
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Thu Jun 12 20:09:15 2014, bulk88 wrote: Show quoted text
> See attached patch.
This is incorrect: --- 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)); } 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. } saved_errno = errno; /* sv_2mortal can realloc things */ + break_loop_with_errno: sv_2mortal(namesv); if (!tryrsfp) { if (PL_op->op_type == OP_REQUIRE) { Doesn't match our usual label indentation (and a few other places too). --- 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; You added eno but didn't use it. I'm not sure this level of micro-optimization is worthwhile. Tony
From: Karl Williamson <public [...] khwilliamson.com>
Date: Tue, 17 Jun 2014 20:36:00 -0600
Subject: Re: [perl #122096] [PATCH] reduce # of calls to errno
To: perlbug-followup [...] perl.org, "OtherRecipients of perl Ticket #122096:;" [...] smtp.indra.com
CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On 06/15/2014 10:10 PM, Tony Cook via RT wrote: Show quoted text
> You added eno but didn't use it. I'm not sure this level of micro-optimization is worthwhile.
I have 2 more issues with this patch. One is that I don't like using variables that a macro sets, outside it. It breaks encapsulation. dSAVEDERRNO uses a variable saved_errno, and this patch knows that and uses it. It may be that Perl already does this kind of thing, and maybe even for this variable, but I don't think we should worsen the situation. This issue could be solved to assuage me by creating a macro that evaluates to the variable and using that macro inside dSAVEDERRNO to evaluate to the variable name. That way if it gets changed, it changes everywhere. More importantly is that it isn't clear to me that the errno isn't getting changed between the places it's getting saved in this patch and the places where it gets used. I'd have to check each and every one to be sure. But suppose that the patch is correct and there are no such problems. I think it would be too easy for code to be inadvertently added that might break it. errno is a volatile value, and it scares me to treat it otherwise. The maintainability decreases.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 5.7k
On Tue Jun 17 19:36:28 2014, public@khwilliamson.com wrote: Show quoted text
> On 06/15/2014 10:10 PM, Tony Cook via RT wrote:
> > You added eno but didn't use it. I'm not sure this level of micro- > > optimization is worthwhile.
> > I have 2 more issues with this patch. > > One is that I don't like using variables that a macro sets, outside > it. > It breaks encapsulation. dSAVEDERRNO uses a variable saved_errno, > and > this patch knows that and uses it. It may be that Perl already does > this kind of thing, and maybe even for this variable, but I don't > think > we should worsen the situation.
http://perl5.git.perl.org/perl.git/blob/9bc33472025546a667b2f318255b6fa650b901e8:/cop.h#l859 Show quoted text
> This issue could be solved to assuage > me by creating a macro that evaluates to the variable and using that > macro inside dSAVEDERRNO to evaluate to the variable name. That way > if > it gets changed, it changes everywhere.
ok, something new will be made Show quoted text
> > More importantly is that it isn't clear to me that the errno isn't > getting changed between the places it's getting saved in this patch > and > the places where it gets used. I'd have to check each and every one > to > be sure. But suppose that the patch is correct and there are no such > problems. I think it would be too easy for code to be inadvertently > added that might break it.
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: Show quoted text
> On Thu Jun 12 20:09:15 2014, bulk88 wrote:
> > See attached patch.
> > This is incorrect: > > --- 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)); > } > > 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.
Will fix. Show quoted text
> > } > saved_errno = errno; /* sv_2mortal can realloc things */ > + break_loop_with_errno: > sv_2mortal(namesv); > if (!tryrsfp) { > if (PL_op->op_type == OP_REQUIRE) { > > Doesn't match our usual label indentation (and a few other places > too).
Yes, I agree the label names need to be harmonized. What should be the label name? or better label names Show quoted text
> > --- 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; > > You added eno but didn't use it. I'm not sure this level of micro- > optimization is worthwhile. > > Tony
gcc version 4.6.3 (gcc-4.6.3 release with patches [build 20121012 by perlmingw.s f.net]) 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 63418E91 83 EC 18 sub esp,18h 63418E94 8B 44 24 24 mov eax,dword ptr [esp+24h] 63418E98 85 C0 test eax,eax 63418E9A 74 37 je 63418ED3 63418E9C 89 44 24 04 mov dword ptr [esp+4],eax ; put char * dirname on outgoing stack 63418EA0 8B 44 24 20 mov eax,dword ptr [esp+20h] 63418EA4 8B 80 18 03 00 00 mov eax,dword ptr [eax+318h] 63418EAA 89 04 24 mov dword ptr [esp],eax ; put this * on outgoing stack 63418EAD E8 EE FE FF FF call 63418DA0 ; call VDir::SetCurrentDirectoryA 63418EB2 85 C0 test eax,eax 63418EB4 89 C3 mov ebx,eax 63418EB6 78 08 js 63418EC0 63418EB8 83 C4 18 add esp,18h 63418EBB 89 D8 mov eax,ebx 63418EBD 5B pop ebx 63418EBE C3 ret 63418EBF 90 nop ; hmmm GCC is aligning instructions 63418EC0 FF 15 DC 57 46 63 call dword ptr ds:[634657DCh] ; call __errno 63418EC6 C7 00 02 00 00 00 mov dword ptr [eax],2 ; assign ENOENT 63418ECC 83 C4 18 add esp,18h 63418ECF 89 D8 mov eax,ebx 63418ED1 5B pop ebx 63418ED2 C3 ret ; 2 tails to leave this function 63418ED3 FF 15 DC 57 46 63 call dword ptr ds:[634657DCh] ; call __errno 63418ED9 BB FF FF FF FF mov ebx,0FFFFFFFFh ; assign -1 to returned val; this statement somehow wasn't moved around the function call to errno to make the 2 "fail" tails identical, then merged out 63418EDE C7 00 02 00 00 00 mov dword ptr [eax],2 ; assign ENOENT 63418EE4 EB D2 jmp 63418EB8 ----------------------------------------------- I probably wont get around to posting a new patch for 2 weeks. -- bulk88 ~ bulk88 at hotmail.com
From: Eric Brine <ikegami [...] adaelis.com>
Subject: Re: [perl #122096] [PATCH] reduce # of calls to errno
Date: Wed, 18 Jun 2014 11:05:58 -0400
To: perlbug-followup <perlbug-followup [...] perl.org>
CC: perl5 porters <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 579b
On Tue, Jun 17, 2014 at 11:56 PM, bulk88 via RT <perlbug-followup@perl.org> wrote:
Show quoted text
On Tue Jun 17 19:36:28 2014, public@khwilliamson.com wrote:
>
> }
> saved_errno = errno; /* sv_2mortal can realloc things */
> +    break_loop_with_errno:
> sv_2mortal(namesv);
> if (!tryrsfp) {
>    if (PL_op->op_type == OP_REQUIRE) {
>
> Doesn't match our usual label indentation (and a few other places
> too).

Yes, I agree the label names need to be harmonized. What should be the label name? or better label names

You appeared to have misread Karl's comment.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 805b
On Wed Jun 18 08:06:23 2014, ikegami@adaelis.com wrote: Show quoted text
> On Tue, Jun 17, 2014 at 11:56 PM, bulk88 via RT <perlbug-followup@perl.org> > wrote: >
> > On Tue Jun 17 19:36:28 2014, public@khwilliamson.com wrote:
> > > > > > } > > > saved_errno = errno; /* sv_2mortal can realloc things */ > > > + break_loop_with_errno: > > > sv_2mortal(namesv); > > > if (!tryrsfp) { > > > if (PL_op->op_type == OP_REQUIRE) { > > > > > > Doesn't match our usual label indentation (and a few other places > > > too).
> > > > Yes, I agree the label names need to be harmonized. What should be the > > label name? or better label names > >
> > You appeared to have misread Karl's comment.
He did misread the comment, but it was my comment. He started quoting my reply part way down and attributed it correctly. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Sun Jun 15 21:10:53 2014, tonyc wrote: Show quoted text
> On Thu Jun 12 20:09:15 2014, bulk88 wrote:
> > See attached patch.
> > This is incorrect:
....... Show quoted text
> 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.
....... Show quoted text
> Doesn't match our usual label indentation (and a few other places > too). >
...... Show quoted text
> You added eno but didn't use it. I'm not sure this level of micro- > optimization is worthwhile. > > Tony
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 ts: 5008 Failed: 1) Failed test: 4983 Non-zero exit status: 1 but the failure went away after removing the doio.c change. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-reduce-of-calls-to-errno.patch
From 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 693b
On Fri Dec 12 20:47:36 2014, bulk88 wrote: Show quoted text
> 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 > ts: 5008 Failed: 1) > Failed test: 4983 > Non-zero exit status: 1 > > but the failure went away after removing the doio.c change.
Bump, rebased patch attached with no changes AFAIK. -- bulk88 ~ bulk88 at hotmail.com
Subject: 0001-reduce-of-calls-to-errno.patch
From 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 869b
On Sun Jul 05 11:44:20 2015, bulk88 wrote: Show quoted text
> On Fri Dec 12 20:47:36 2014, bulk88 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 > > ts: 5008 Failed: 1) > > Failed test: 4983 > > Non-zero exit status: 1 > > > > but the failure went away after removing the doio.c change.
> > Bump, rebased patch attached with no changes AFAIK.
This fixes the issues with the original, but I'm still not sure this level of micro-optimization is worthwhile. Anyone else? Tony
Date: Tue, 7 Jul 2015 10:23:49 -0500
To: Craig Berry via RT <perlbug-followup [...] perl.org>
CC: "Perl5 Porters (E-mail)" <perl5-porters [...] perl.org>
Subject: Re: [perl #122096] [PATCH] reduce # of calls to errno
From: "Craig A. Berry" <craig.a.berry [...] gmail.com>
Download (untitled) / with headers
text/plain 547b
On Tue, Jul 7, 2015 at 12:37 AM, Tony Cook via RT <perlbug-followup@perl.org> wrote: Show quoted text
> This fixes the issues with the original, but I'm still not sure this level > of micro-optimization is worthwhile. > > Anyone else?
I think it hurts readability. Things like doing a goto out of the middle of one if block into the middle of another just aren't worth it. Plus I don't see much savings. In most cases errno will only be set once before the function returns anyway because the different references to it are in mutually exclusive code paths.
Date: Sun, 12 Nov 2017 00:07:22 +0000
Subject: Re: [perl #122096] [PATCH] reduce # of calls to errno
To: perl5-porters [...] perl.org
From: Zefram <zefram [...] fysh.org>
Download (untitled) / with headers
text/plain 239b
We should not apply the hunks that break macro encapsulation or the hunks that don't actually reduce the runtime number of calls to errno. I've applied the remaining good hunks as commit 69374fe705978962b85217f3eb828a93f836fd8d. -zefram


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org