Skip to content
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

Closed
p5pRT opened this issue Jun 13, 2014 · 17 comments
Closed

[PATCH] reduce # of calls to errno #13919

p5pRT opened this issue Jun 13, 2014 · 17 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 13, 2014

Migrated from rt.perl.org#122096 (status was 'resolved')

Searchable as RT122096$

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2014

From @bulk88

Created by @bulk88

See attached patch.

Perl Info

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)




@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2014

From @bulk88

0001-reduce-of-calls-to-errno.patch
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

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2014

From @bulk88

On Thu Jun 12 20​:09​:15 2014, bulk88 wrote​:

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

@p5pRT
Copy link
Author

p5pRT commented Jun 16, 2014

From @tonycoz

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.

  }
  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

@p5pRT
Copy link
Author

p5pRT commented Jun 16, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2014

From @khwilliamson

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. 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.

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2014

From @bulk88

On Tue Jun 17 19​:36​:28 2014, public@​khwilliamson.com wrote​:

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

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

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​:

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.

}
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

--- 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

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2014

From @ikegami

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.

@p5pRT
Copy link
Author

p5pRT commented Jul 9, 2014

From @tonycoz

On Wed Jun 18 08​:06​:23 2014, ikegami@​adaelis.com wrote​:

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

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2014

From @bulk88

On Sun Jun 15 21​:10​:53 2014, tonyc wrote​:

On Thu Jun 12 20​:09​:15 2014, bulk88 wrote​:

See attached patch.

This is incorrect​:
.......
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).

......

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

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2014

From @bulk88

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

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2015

From @bulk88

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.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Jul 5, 2015

From @bulk88

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

@p5pRT
Copy link
Author

p5pRT commented Jul 7, 2015

From @tonycoz

On Sun Jul 05 11​:44​:20 2015, bulk88 wrote​:

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

@p5pRT
Copy link
Author

p5pRT commented Jul 7, 2015

From @craigberry

On Tue, Jul 7, 2015 at 12​:37 AM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

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.

@p5pRT
Copy link
Author

p5pRT commented Nov 12, 2017

From zefram@fysh.org

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
69374fe.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Nov 12, 2017

@cpansprout - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant