Fixes for builds that require strict X/Open and POSIX compatiblity

[daltenty]

Summary

• use alternative to MAP_ANONYMOUS for allocating mapped memory if it isn't available
• Use strtok_r instead of strsep as part of getting program path
• Don't try to find the width of a terminal using "struct winsize" and TIOCGWINSZ on POSIX builds. These aren't defined under POSIX (even though some platforms make them available when they shouldn't), so just check if we are doing a X/Open or POSIX compliant build first.

[hubert.reinterpretcast]

These are defined under POSIX

Do you mean that these aren't defined under POSIX?

llvm/lib/Support/Unix/Memory.inc

94 ↗

(On Diff #197371)

<ins>On</ins><del>on</del> platforms that have it<ins>,</ins> we can use MAP_ANONYMOUS to get a memory<ins>-</ins>mapped page without file backing, but we need a fallback <del>by</del><ins>of</ins> opening /dev/zero for strictly POSIX platforms instead<ins>.</ins>

97 ↗

(On Diff #197371)

I would rather have fd assigned separately on each branch of the #if than to have the rest of the statement inside conditional inclusion blocks.

102 ↗

(On Diff #197371)

Add a space after ==.

112 ↗

(On Diff #197371)

To be consistent with the ifdef, this should be #elif defined(MAP_ANON).

135 ↗

(On Diff #197371)

The file descriptor returned by open and stored in fd is not closed on this path.

llvm/lib/Support/Unix/Path.inc

157 ↗

(On Diff #197371)

Once assigned, s always has the same value as pv. Do we need s?

161 ↗

(On Diff #197371)

Add spaces after the commas in the argument list.

167 ↗

(On Diff #197371)

Add spaces after the commas in the argument list.
Add space after while (or at least that is what clang-format does for me).

[MaskRay]

llvm/lib/Support/Unix/Memory.inc

110 ↗

(On Diff #197371)

I think you can just keep MAP_ANON and remove MAP_ANONYMOUS. Though marked as deprecated in Linux manpages, it is most widely used. It is also used by compiler-rt.

[MaskRay]

llvm/lib/Support/Unix/Memory.inc

110 ↗

(On Diff #197371)

You may need to rebase. I've deleted MAP_ANONYMOUS.

[daltenty]

• Address formatting comments
• Revert to using MAP_ANON
• Close the file descriptor on all return paths in Memory::allocateMappedMemory
• Cleanup the pointers we keep in getprogpath

[hubert.reinterpretcast]

llvm/lib/Support/Unix/Memory.inc

111 ↗

(On Diff #197875)

I think using MMFlags |= MAP_ANON; will help readability/maintainability.

146 ↗

(On Diff #197875)

If it's okay to close the file here, then it is okay to close the file right after the call to mmap before checking whether mmap failed. We would only need to add one call to close (instead of three).

llvm/lib/Support/Unix/Path.inc

156 ↗

(On Diff #197875)

If we're going to move where the pv, etc. is declared, then we might as well move the declaration of s to this line...

160 ↗

(On Diff #197875)

... and declare t in the if:

if (char *t = strtok_r(s, ":", &state)) {

[daltenty]

llvm/lib/Support/Unix/Memory.inc

146 ↗

(On Diff #197875)

If we close right after mmap, that would technically render the value of errno undefined or clobber it, and we may need to inspect it if the mmap failed (and according to spec we should only manipulate it if mmap failed)

[hubert.reinterpretcast]

llvm/lib/Support/Unix/Path.inc

156 ↗

(On Diff #199115)

Minor nit: Add a space before the =.

160 ↗

(On Diff #199115)

Does this compile?

[daltenty]

Fix style

[daltenty]

• Fix loop on strtok_r

[daltenty]

llvm/lib/Support/Unix/Path.inc

160 ↗

(On Diff #199115)

Yes it does. Can you clarify?

[hubert.reinterpretcast]

LGTM.

llvm/trunk/lib/Support/Unix/Memory.inc

 Memory::allocateMappedMemory(size_t NumBytes,
                              const MemoryBlock *const NearBlock,
                              unsigned PFlags,
                              std::error_code &EC) {
   EC = std::error_code();
   if (NumBytes == 0)
     return MemoryBlock();
 
-  int fd = -1;
+  // On platforms that have it, we can use MAP_ANON to get a memory-mapped
+  // page without file backing, but we need a fallback of opening /dev/zero
+  // for strictly POSIX platforms instead.
+  int fd;
+#if defined(MAP_ANON)
+  fd = -1;
+#else
+  fd = open("/dev/zero", O_RDWR);
+  if (fd == -1) {
+    EC = std::error_code(errno, std::generic_category());
+    return MemoryBlock();
+  }
+#endif
 
-  int MMFlags = MAP_PRIVATE | MAP_ANON;
+  int MMFlags = MAP_PRIVATE;
+#if defined(MAP_ANON)
+  MMFlags |= MAP_ANON;
+#endif
   int Protect = getPosixProtectionFlags(PFlags);
 
 #if defined(__NetBSD__) && defined(PROT_MPROTECT)
   Protect |= PROT_MPROTECT(PROT_READ | PROT_WRITE | PROT_EXEC);
 #endif
 
   // Use any near hint and the page size to set a page-aligned starting address
   uintptr_t Start = NearBlock ? reinterpret_cast<uintptr_t>(NearBlock->base()) +
                                       NearBlock->size() : 0;
   static const size_t PageSize = Process::getPageSizeEstimate();
   if (Start && Start % PageSize)
     Start += PageSize - Start % PageSize;
 
   // FIXME: Handle huge page requests (MF_HUGE_HINT).
   void *Addr = ::mmap(reinterpret_cast<void *>(Start), NumBytes, Protect,
                       MMFlags, fd, 0);
   if (Addr == MAP_FAILED) {
-    if (NearBlock) //Try again without a near hint
+    if (NearBlock) { //Try again without a near hint
+#if !defined(MAP_ANON)
+      close(fd);
+#endif
       return allocateMappedMemory(NumBytes, nullptr, PFlags, EC);
+    }
 
     EC = std::error_code(errno, std::generic_category());
+#if !defined(MAP_ANON)
+    close(fd);
+#endif
     return MemoryBlock();
   }
 
+#if !defined(MAP_ANON)
+  close(fd);
+#endif
+
   MemoryBlock Result;
   Result.Address = Addr;
   Result.Size = NumBytes;
   Result.Flags = PFlags;
 
   // Rely on protectMappedMemory to invalidate instruction cache.
   if (PFlags & MF_EXEC) {
     EC = Memory::protectMappedMemory (Result, PFlags);

llvm/trunk/lib/Support/Unix/Path.inc

if (stat(fullpath, &sb) != 0)
     return 1;
 
   return 0;
 }
 
 static char *
 getprogpath(char ret[PATH_MAX], const char *bin)
 {
-  char *pv, *s, *t;
-
   /* First approach: absolute path. */
   if (bin[0] == '/') {
     if (test_dir(ret, "/", bin) == 0)
       return ret;
     return nullptr;
   }
 
   /* Second approach: relative path. */
   if (strchr(bin, '/')) {
     char cwd[PATH_MAX];
     if (!getcwd(cwd, PATH_MAX))
       return nullptr;
     if (test_dir(ret, cwd, bin) == 0)
       return ret;
     return nullptr;
   }
 
   /* Third approach: $PATH */
+  char *pv;
   if ((pv = getenv("PATH")) == nullptr)
     return nullptr;
-  s = pv = strdup(pv);
-  if (!pv)
+  char *s = strdup(pv);
+  if (!s)
     return nullptr;
-  while ((t = strsep(&s, ":")) != nullptr) {
+  char *state;
+  for (char *t = strtok_r(s, ":", &state); t != nullptr;
+       t = strtok_r(nullptr, ":", &state)) {
     if (test_dir(ret, t, bin) == 0) {
-      free(pv);
+      free(s);
       return ret;
     }
   }
-  free(pv);
+  free(s);
   return nullptr;
 }
 #endif // __FreeBSD__ || __NetBSD__ || __FreeBSD_kernel__
 
 /// GetMainExecutable - Return the path to the main executable, given the
 /// value of argv[0] from program startup.
 std::string getMainExecutable(const char *argv0, void *MainAddr) {
 #if defined(__APPLE__)

llvm/trunk/lib/Support/Unix/Process.inc

static unsigned getColumns(int FileID) {
   if (const char *ColumnsStr = std::getenv("COLUMNS")) {
     int Columns = std::atoi(ColumnsStr);
     if (Columns > 0)
       return Columns;
   }
 
   unsigned Columns = 0;
 
-#if defined(HAVE_SYS_IOCTL_H) && defined(HAVE_TERMIOS_H)
+#if defined(HAVE_SYS_IOCTL_H) && defined(HAVE_TERMIOS_H) \
+  && !(defined(_XOPEN_SOURCE) || defined(_POSIX_C_SOURCE))
   // Try to determine the width of the terminal.
   struct winsize ws;
   if (ioctl(FileID, TIOCGWINSZ, &ws) == 0)
     Columns = ws.ws_col;
 #endif
 
   return Columns;
 }