[PATCH] D40032: [compiler-rt] Replace forkpty with posix_spawn

[George Karpenkov via Phabricator via llvm-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.

LGTM with a few nits, but I'm not 100% sure what's going on either..



================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:216
+
+  master_fd = posix_openpt(O_RDWR);
+  if (master_fd == -1) goto cleanup;
----------------
I would appreciate a comment on why do you need a pseudo-terminal here.


================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:217
+  master_fd = posix_openpt(O_RDWR);
+  if (master_fd == -1) goto cleanup;
+
----------------
Is `kInvalidFd` equal to `-1` ?


================
Comment at: lib/sanitizer_common/sanitizer_mac.cc:261
+ cleanup:
+  if (master_fd != kInvalidFd) internal_close(master_fd);
+  if (slave_fd != kInvalidFd) internal_close(slave_fd);
----------------
could the error happen after the file descriptor was set? (e.g. system call fails, but the pointer was still written into?) Might be safer to have multiple cleanup labels to jump to the one where you know it's safe to close the descriptor?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D40032