Page MenuHomePhabricator

Remove exception throwing debug mode handler support.
ClosedPublic

Authored by EricWF on Mar 8 2019, 4:49 PM.

Details

Summary

The reason libc++ implemented a throwing debug mode handler was for ease of testing. Specifically,
I thought that if a debug violation aborted, we could only test one violation per file. This made
it impossible to test debug mode. Which throwing behavior we could test more!

However, the throwing approach didn't work either, since there are debug violations underneath noexcept
functions. This lead to the introduction of _NOEXCEPT_DEBUG, which was only noexcept when debug
mode was off.

Having thought more and having grown wiser, _NOEXCEPT_DEBUG was a horrible decision. It was
viral, it didn't cover all the cases it needed to, and it was observable to the user -- at worst
changing the behavior of their program.

This patch removes the throwing debug handler, and rewrites the debug tests using 'fork-ing' style
death tests.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Mar 8 2019, 4:49 PM
EricWF edited the summary of this revision. (Show Details)Mar 8 2019, 4:49 PM
ldionne requested changes to this revision.Mar 14 2019, 8:42 AM

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.

test/support/debug_mode_helper.h
30

So these tests are not going to work on Windows -- correct?

153

So, if file is "f" and got.__file_ is "foo", this is going to consider it a match. Is that intended?

243

assert(child_pid != -1 && "failed to fork a process to perform a death test")

244

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.

245

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?

249

Why don't you pass &status_ directly?

250

You mean result != -1, right?

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

253

// no need to write from the parent process

295

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.

300

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.

This revision now requires changes to proceed.Mar 14 2019, 8:42 AM
ldionne added inline comments.Mar 14 2019, 8:44 AM
lib/abi/x86_64-apple-darwin.v1.abilist
264

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.

mclow.lists requested changes to this revision.Mar 14 2019, 5:21 PM

I have marked test/libcxx/debug/containers/db_sequence_container_iterators.pass.cpp and libcxx/containers/sequences/array/array.zero/db_indexing.pass.cpp because I marked operator[] and front and back as noexcept.

These tests need to be un-XFAIL-ed when they are rewritten.

EricWF marked 19 inline comments as done.Mar 18 2019, 12:40 PM
EricWF added inline comments.
lib/abi/x86_64-apple-darwin.v1.abilist
264

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.

test/support/debug_mode_helper.h
30

Not right away. But I'll work on it once we get Windows CI up and running.

153

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.

244

I believe your understanding is correct.

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

245

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.

249

I'm going to remove status_ entirely. So that will clean this up.

295

I hid most of this behind named methods now. I think that addresses your concerns.

300

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.

EricWF updated this revision to Diff 191154.Mar 18 2019, 1:08 PM
EricWF marked 8 inline comments as done.

Address review comments. I think this is good to go.

ldionne accepted this revision.Mar 18 2019, 2:05 PM

LGTM with inline nit.

test/support/debug_mode_helper.h
299

Trailing underscore for member variables?

EricWF updated this revision to Diff 191181.Mar 18 2019, 2:47 PM
EricWF marked an inline comment as done.

Address final inline comments.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2019, 2:50 PM
This revision was automatically updated to reflect the committed changes.