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

Mon Mar 18 12:41:46 PDT 2019

EricWF marked 19 inline comments as done.
EricWF added inline comments.

================
Comment at: lib/abi/x86_64-apple-darwin.v1.abilist:264
 {'is_defined': True, 'name': '__ZNKSt3__123__match_any_but_newlineIwE6__execERNS_7__stateIwEE', 'type': 'FUNC'}
-{'is_defined': True, 'name': '__ZNKSt3__124__libcpp_debug_exception4whatEv', 'type': 'FUNC'}
 {'is_defined': True, 'name': '__ZNKSt3__15ctypeIcE10do_tolowerEPcPKc', 'type': 'FUNC'}
----------------
ldionne wrote:
> Also, this "ABI break" is fine as far as Apple is concerned because we don't ship the debug mode in our dylib. So this is not an ABI break for us.
Cool. Maybe we should omit `__libcpp_debug` symbols from the ABI list all together. I'll confirm with FreeBSD that they're not concerned about ABI stability here either.

================
Comment at: test/support/debug_mode_helper.h:30

+#include <unistd.h>
+#include <sys/wait.h>
----------------
ldionne wrote:
> So these tests are not going to work on Windows -- correct?
Not right away. But I'll work on it once we get Windows CI up and running.

================
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;
----------------
ldionne wrote:
> So, if `file` is `"f"` and `got.__file_` is `"foo"`, this is going to consider it a match. Is that intended?
That's not intentional. Good catch.

I changed it so the match has to go until the end and the match must start at the beginning or immediately after a directory separator. 

================
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)) {}
----------------
ldionne wrote:
> 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`.
I believe your understanding is correct.

I've tested that this all works when a `EXPECT_DEATH` assertion fails.

================
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_;
----------------
ldionne wrote:
> 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?
I've added logic to capture both stderr and stdout.

I agree it's better to fail loud and early.
Instead of handling `EMFILE` and `EBADF`, I've changed the test runner to exit with a specific value when the setup code fails, so that the parent will know what happened.

================
Comment at: test/support/debug_mode_helper.h:150
+    int status_value;
+    pid_t result = waitpid(child_pid_, &status_value, 0);
+    assert(result != 1);
----------------
ldionne wrote:
> Why don't you pass `&status_` directly?
I'm going to remove `status_` entirely. So that will clean this up.

================
Comment at: test/support/debug_mode_helper.h:170
+  int status_ = -1;
+  int pipe_fd[2];
+  std::string error_msg_;
----------------
ldionne wrote:
> 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.
> 
I hid most of this behind named methods now. I think that addresses your concerns.

================
Comment at: test/support/debug_mode_helper.h:204

-} // namespace IteratorDebugChecks
+#define EXPECT_DEATH_MATCHES(Matcher, ...) assert((ExpectDeath(#__VA_ARGS__, [&]() { __VA_ARGS__; }, Matcher)))

----------------
ldionne wrote:
> 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.
I'm not really taking precautions to make this C++03 compliant. For example, I don't want or need an `enum class` for `ResultKind`.

But I'll add an explicit dialect check at the top of the file requiring C++11.

Repository:
  rCXX libc++

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

https://reviews.llvm.org/D59166