[libcxx-commits] [PATCH] D59166: Remove exception throwing debug mode handler support.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I really like that. I wasn't aware of the exception business in the debug mode, but I agree that the new approach makes more sense.



================
Comment at: test/support/debug_mode_helper.h:30
 
+#include <unistd.h>
+#include <sys/wait.h>
----------------
So these tests are not going to work on Windows -- correct?


================
Comment at: test/support/debug_mode_helper.h:50
+        return false;
+      if (file != std::string(any_file) && std::string(got.__file_).find(file) == std::string::npos)
+        return false;
----------------
So, if `file` is `"f"` and `got.__file_` is `"foo"`, this is going to consider it a match. Is that intended?


================
Comment at: test/support/debug_mode_helper.h:110
+    pid_t child_pid = fork();
+    assert(child_pid != -1);
+    child_pid_ = child_pid;
----------------
`assert(child_pid != -1 && "failed to fork a process to perform a death test")`


================
Comment at: test/support/debug_mode_helper.h:124
+  TEST_NORETURN void RunForChild(Func&& f) {
+    close(pipe_fd[0]);
+    while ((dup2(pipe_fd[1], STDOUT_FILENO) == -1) && (errno == EINTR)) {}
----------------
So, when you fork, the child gets its own copy of the parent's file descriptors. Here I understand you're closing the _read_ end of the pipe because you won't need it, but this does not affect the read end of the pipe for the parent process. Is my understanding correct?

Also, please add `// don't need to read from the pipe in the child`.


================
Comment at: test/support/debug_mode_helper.h:125
+    close(pipe_fd[0]);
+    while ((dup2(pipe_fd[1], STDOUT_FILENO) == -1) && (errno == EINTR)) {}
+    GlobalMatcher() = matcher_;
----------------
What about `stderr`? Also, I would suggest doing something like this instead:

```
int dup_result = dup2(pipe_fd[1], STDOUT_FILENO);
assert(dup_result != -1 && "Something went wrong while redirecting stdout to the pipe");
```

Otherwise, with the `while`, you should probably also handle `errno == EMFILE`. And if `errno == EBADF`, we'll continue executing the death test and we'll notice that there's no match, when the problem really is that the redirection of stdout failed. This could be tricky to debug. I think it's better to fail early and loud here, even though we _might_ sometimes (but rarely) error out when we could have instead tried again and succeeded. WDYT?


================
Comment at: test/support/debug_mode_helper.h:133
+  void CaptureIO() {
+    close(pipe_fd[1]);
+    int read_fd = pipe_fd[0];
----------------
`// no need to write from the parent process`


================
Comment at: test/support/debug_mode_helper.h:150
+    int status_value;
+    pid_t result = waitpid(child_pid_, &status_value, 0);
+    assert(result != 1);
----------------
Why don't you pass `&status_` directly?


================
Comment at: test/support/debug_mode_helper.h:151
+    pid_t result = waitpid(child_pid_, &status_value, 0);
+    assert(result != 1);
+    status_ = status_value;
----------------
You mean `result != -1`, right?

Also, `assert(result != -1 && "there is no child process to wait for")`.


================
Comment at: test/support/debug_mode_helper.h:170
+  int status_ = -1;
+  int pipe_fd[2];
+  std::string error_msg_;
----------------
Instead, I might have `int read_fd_, write_fd_`, and set them up in the constructor or in `Run`:

```
int pipe_fd[2];
int pipe_res = pipe(pipe_fd);
assert(pipe_res != -1);
read_fd_ = pipe_fd[0];
write_fd_ = pipe_fd[1];
```

This would allow you to use `read_fd_` and `write_fd_` everywhere else in the class, which is more readable IMO.



================
Comment at: test/support/debug_mode_helper.h:204
 
-} // namespace IteratorDebugChecks
+#define EXPECT_DEATH_MATCHES(Matcher, ...) assert((ExpectDeath(#__VA_ARGS__, [&]() { __VA_ARGS__; }, Matcher)))
 
----------------
I notice you're taking precautions to make the rest of this file C++03 compliant (e.g. no `enum class`, etc..), however you require a lambda here. I'd be fine if the debug tests were only enabled with C++11 and above.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59166/new/

https://reviews.llvm.org/D59166