[cpp] Add unit test for pyos::ReadByte()

This uncovered the fact that the macro hackery for posix::O_RDONLY and
errno_::EINTR wasn't working.  And it was causing problems in the tests.

So I removed all of it, and we just use unqualified EINTR like in
C-style code.  We're doing this for WUNTRACED too, so now it's
consistent.

build/mycpp.sh

@@ -90,6 +90,8 @@ cpp-skeleton() {
 
 #include "preamble.h"  // hard-coded stuff
 
+#undef errno  // for e->errno to work; see mycpp/myerror.h
+
 EOF
 
   cat "$@"
@@ -244,7 +246,6 @@ compile-slice() {
     _build/cpp/arith_parse.cc \
     _build/cpp/arg_types.cc \
     cpp/dumb_alloc.cc \
-    cpp/errno_.cc \
     cpp/fcntl_.cc \
     cpp/posix.cc \
     cpp/signal_.cc \

core/completion.py

@@ -55,6 +55,8 @@
 
 import libc
 import posix_ as posix
+from posix_ import X_OK  # translated directly to C macro
+
 from typing import (
     Dict, Tuple, List, Optional, Iterator, Union, Callable, Any, TYPE_CHECKING
 )
@@ -399,7 +401,7 @@ def Matches(self, comp):
         if self.exec_only:
           # TODO: Handle exception if file gets deleted in between listing and
           # check?
-          if not posix.access(path, posix.X_OK):
+          if not posix.access(path, X_OK):
             continue
 
         if self.add_slash and path_stat.isdir(path):
@@ -578,7 +580,7 @@ def Matches(self, comp):
           path = os_path.join(d, name)
           # TODO: Handle exception if file gets deleted in between listing and
           # check?
-          if not posix.access(path, posix.X_OK):
+          if not posix.access(path, X_OK):
             continue
           dir_exes.append(name)  # append the name, not the path
 

core/executor.py

@@ -3,7 +3,7 @@
 """
 from __future__ import print_function
 
-import errno as errno_
+from errno import EINTR
 import sys
 
 #from _devbuild.gen.option_asdl import builtin_i
@@ -363,7 +363,7 @@ def RunCommandSub(self, cs_part):
       n, err_num = pyos.Read(r, 4096, chunks)
 
       if n < 0:
-        if err_num == errno_.EINTR:
+        if err_num == EINTR:
           pass  # retry
         else:
           # Like the top level IOError handler

core/process.py

@@ -9,7 +9,7 @@
 """
 from __future__ import print_function
 
-import errno as errno_  # avoid macro name conflict after translation
+from errno import EACCES, EBADF, ECHILD, EINTR, ENOENT, ENOEXEC
 import fcntl as fcntl_
 import signal as signal_
 from sys import exit  # mycpp translation directly calls exit(int status)!
@@ -45,6 +45,7 @@
     WUNTRACED,
     WIFSIGNALED, WIFEXITED, WIFSTOPPED,
     WEXITSTATUS, WTERMSIG,
+    O_APPEND, O_CREAT, O_RDONLY, O_RDWR, O_WRONLY, O_TRUNC,
 )
 
 from typing import List, Tuple, Dict, Optional, cast, TYPE_CHECKING
@@ -135,14 +136,14 @@ def Open(self, path):
     Raises:
       OSError if the path can't be found.
     """
-    fd_mode = posix.O_RDONLY
+    fd_mode = O_RDONLY
     return self._Open(path, 'r', fd_mode)
 
   if mylib.PYTHON:
     # used for util.DebugFile
     def OpenForWrite(self, path):
       # type: (str) -> mylib.Writer
-      fd_mode = posix.O_CREAT | posix.O_RDWR
+      fd_mode = O_CREAT | O_RDWR
       f = self._Open(path, 'w', fd_mode)
       # Hack to change mylib.LineReader into mylib.Writer.  In reality the file
       # object supports both interfaces.
@@ -189,7 +190,7 @@ def _PushSave(self, fd):
       # Example program that causes this error: exec 4>&1.  Descriptor 4 isn't
       # open.
       # This seems to be ignored in dash too in savefd()?
-      if e.errno == errno_.EBADF:
+      if e.errno == EBADF:
         #log('ERROR fd %d: %s', fd, e)
         need_restore = False
       else:
@@ -221,7 +222,7 @@ def _PushDup(self, fd1, loc):
         # F_DUPFD: GREATER than range
         new_fd = fcntl_.fcntl(fd1, fcntl_.F_DUPFD, _SHELL_MIN_FD)  # type: int
       except IOError as e:
-        if e.errno == errno_.EBADF:
+        if e.errno == EBADF:
           self.errfmt.Print_('%d: %s' % (fd1, pyutil.strerror(e)))
           return NO_FD
         else:
@@ -310,15 +311,15 @@ def _ApplyRedirect(self, r):
         if r.op_id in (Id.Redir_Great, Id.Redir_AndGreat):  # >   &>
           # NOTE: This is different than >| because it respects noclobber, but
           # that option is almost never used.  See test/wild.sh.
-          mode = posix.O_CREAT | posix.O_WRONLY | posix.O_TRUNC
+          mode = O_CREAT | O_WRONLY | O_TRUNC
         elif r.op_id == Id.Redir_Clobber:  # >|
-          mode = posix.O_CREAT | posix.O_WRONLY | posix.O_TRUNC
+          mode = O_CREAT | O_WRONLY | O_TRUNC
         elif r.op_id in (Id.Redir_DGreat, Id.Redir_AndDGreat):  # >>   &>>
-          mode = posix.O_CREAT | posix.O_WRONLY | posix.O_APPEND
+          mode = O_CREAT | O_WRONLY | O_APPEND
         elif r.op_id == Id.Redir_Less:  # <
-          mode = posix.O_RDONLY
+          mode = O_RDONLY
         elif r.op_id == Id.Redir_LessGreat:  # <>
-          mode = posix.O_CREAT | posix.O_RDWR
+          mode = O_CREAT | O_RDWR
         else:
           raise NotImplementedError(r.op_id)
 
@@ -592,7 +593,7 @@ def _Exec(self, argv0_path, argv, argv0_spid, environ, should_retry):
       posix.execve(argv0_path, argv, environ)
     except OSError as e:
       # Run with /bin/sh when ENOEXEC error (no shebang).  All shells do this.
-      if e.errno == errno_.ENOEXEC and should_retry:
+      if e.errno == ENOEXEC and should_retry:
         new_argv = ['/bin/sh', argv0_path]
         new_argv.extend(argv[1:])
         self._Exec('/bin/sh', new_argv, argv0_spid, environ, False)
@@ -609,9 +610,9 @@ def _Exec(self, argv0_path, argv, argv0_spid, environ, should_retry):
       # unspecified.
       #
       # http://pubs.opengroup.org/onlinepubs/9699919799.2016edition/utilities/V3_chap02.html#tag_18_08_02
-      if e.errno == errno_.EACCES:
+      if e.errno == EACCES:
         status = 126
-      elif e.errno == errno_.ENOENT:
+      elif e.errno == ENOENT:
         # TODO: most shells print 'command not found', rather than strerror()
         # == "No such file or directory".  That's better because it's at the
         # end of the path search, and we're never searching for a directory.
@@ -1418,9 +1419,9 @@ def WaitForOne(self, eintr_retry):
       pid, status = posix.waitpid(-1, WUNTRACED)
     except OSError as e:
       #log('wait() error: %s', e)
-      if e.errno == errno_.ECHILD:
+      if e.errno == ECHILD:
         return -1  # nothing to wait for caller should stop
-      elif e.errno == errno_.EINTR:  # Bug #858 fix
+      elif e.errno == EINTR:  # Bug #858 fix
         # Examples:
         # - 128 + SIGUSR1 = 128 + 10 = 138
         # - 128 + SIGUSR2 = 128 + 12 = 140

core/pyos.py

@@ -5,7 +5,7 @@
 """
 from __future__ import print_function
 
-import errno as errno_
+from errno import EINTR
 import pwd
 import resource
 import signal
@@ -92,7 +92,7 @@ def ReadLine():
     ch, err_num = ReadByte(0)
 
     if ch < 0:
-      if err_num == errno_.EINTR:
+      if err_num == EINTR:
         # Instead of retrying, return EOF, which is what libc.stdin_readline()
         # did.  I think this interface is easier with getline().
         # This causes 'read --line' to return status 1.

core/state.py

@@ -38,6 +38,7 @@
 
 import libc
 import posix_ as posix
+from posix_ import X_OK  # translated directly to C macro
 
 from typing import Tuple, List, Dict, Optional, Any, cast, TYPE_CHECKING
 
@@ -102,7 +103,7 @@ def Lookup(self, name, exec_required=True):
       # -t').  OSH follows mksh and zsh.  Note that we can still get EPERM if
       # the permissions are changed between check and use.
       if exec_required:
-        found = posix.access(full_path, posix.X_OK)
+        found = posix.access(full_path, X_OK)
       else:
         found = path_stat.exists(full_path)  # for 'source'
 

cpp/core_pyos.cc

@@ -21,13 +21,13 @@ Tuple2<int, int> Read(int fd, int n, List<Str*>* chunks) {
 }
 
 Tuple2<int, int> ReadByte(int fd) {
-  char c;
+  int c;  // avoid unsigned issue with char
   ssize_t n = read(fd, &c, 1);
   if (n < 0) {  // read error
     return Tuple2<int, int>(-1, errno);
   } else if (n == 0) {  // EOF
     return Tuple2<int, int>(EOF_SENTINEL, 0);
-  } else { // return character
+  } else {  // return character
     return Tuple2<int, int>(c, 0);
   }
 }

cpp/core_pyutil.cc

@@ -16,12 +16,12 @@ bool IsValidCharEscape(int c) {
 
 Str* ChArrayToString(List<int>* ch_array) {
   int n = len(ch_array);
-  char* buf = static_cast<char*>(malloc(n + 1));
+  unsigned char* buf = static_cast<unsigned char*>(malloc(n + 1));
   for (int i = 0; i < n; ++i) {
     buf[i] = ch_array->index(i);
   }
   buf[n] = '\0';
-  return new Str(buf, n);
+  return new Str(reinterpret_cast<char*>(buf), n);
 }
 
 Str* _ResourceLoader::Get(Str* path) {

cpp/posix.cc

@@ -6,33 +6,19 @@
 #include <sys/wait.h>  // WUNTRACED
 #include <unistd.h>
 
-// Why do I need these again here?  They are undefined in the header.
-#undef X_OK
-#undef R_OK
-#undef W_OK
-#undef O_APPEND
-#undef O_CREAT
-#undef O_RDONLY
-#undef O_RDWR
-#undef O_WRONLY
-#undef O_TRUNC
-
 namespace posix {
 
-// Aliases in this namespace.  Avoid name conflicts with macros.
-int X_OK = X_OK;
-int R_OK = R_OK;
-int W_OK = W_OK;
-int O_APPEND = O_APPEND_;
-int O_CREAT = O_CREAT_;
-int O_RDONLY = O_RDONLY_;
-int O_RDWR = O_RDWR_;
-int O_WRONLY = O_WRONLY_;
-int O_TRUNC = O_TRUNC_;
-
-int open(Str* path, int mode, int perms) {
+int open(Str* path, int flags, int perms) {
   mylib::Str0 path0(path);
-  return ::open(path0.Get(), mode, perms);
+  log("OPEN %s\n", path0.Get());
+  log("flags %d\n", flags);
+  log("O_CREAT %d\n", O_CREAT);
+  log("O_RDWR %d\n", O_RDWR);
+
+  log("perms %d\n", perms);
+  int fd = ::open(path0.Get(), flags, perms);
+  log("fd = %d\n", fd);
+  return fd;
 }
 
 }  // namespace posix

cpp/posix.h

@@ -8,40 +8,8 @@
 
 #include "mylib.h"
 
-// Save as a different name
-#define X_OK_ X_OK
-#define R_OK_ R_OK
-#define W_OK_ W_OK
-#define O_APPEND_ O_APPEND
-#define O_CREAT_ O_CREAT
-#define O_RDONLY_ O_RDONLY
-#define O_RDWR_ O_RDWR
-#define O_WRONLY_ O_WRONLY
-#define O_TRUNC_ O_TRUNC
-
-#undef X_OK
-#undef R_OK
-#undef W_OK
-#undef O_APPEND
-#undef O_CREAT
-#undef O_RDONLY
-#undef O_RDWR
-#undef O_WRONLY
-#undef O_TRUNC
-
 namespace posix {
 
-// aliases in this namespace
-extern int X_OK;
-extern int R_OK;
-extern int W_OK;
-extern int O_APPEND;
-extern int O_CREAT;
-extern int O_RDONLY;
-extern int O_RDWR;
-extern int O_WRONLY;
-extern int O_TRUNC;
-
 inline int access(Str* pathname, int mode) {
   // Are there any errno I care about?
   mylib::Str0 pathname0(pathname);
@@ -143,7 +111,7 @@ inline void dup2(int oldfd, int newfd) {
   assert(0);
 }
 
-int open(Str* path, int mode, int perms);
+int open(Str* path, int flags, int perms);
 
 inline mylib::LineReader* fdopen(int fd, Str* c_mode) {
   mylib::Str0 c_mode0(c_mode);

cpp/preamble.h

@@ -27,7 +27,6 @@ using id_kind_asdl::Kind_t;
 #include "core_pyerror.h"
 #include "core_pyos.h"
 #include "core_pyutil.h"
-#include "errno_.h"
 #include "fcntl_.h"
 #include "frontend_flag_spec.h"
 #include "frontend_match.h"