Page MenuHomePhabricator

[libcxx] limit debug mode tests on platforms without fork
Needs ReviewPublic

Authored by bcain on Apr 30 2019, 3:13 PM.

Details

Summary

Add a has-fork feature flag and only enable debug-mode tests on systems with fork() support.

Diff Detail

Event Timeline

bcain created this revision.Apr 30 2019, 3:13 PM
EricWF requested changes to this revision.Apr 30 2019, 3:57 PM

These tests turn on debug mode themselves - they don't need to be limited the configurations where the user turns it on.

I don't want the debug tests to be their "own configuration". They should be run in all configurations (except testing the Apple system libc++)

This revision now requires changes to proceed.Apr 30 2019, 3:57 PM

These tests turn on debug mode themselves - they don't need to be limited the configurations where the user turns it on.

These tests need to be fixed so that they DTRT when the user specifies -debug_level=X (where X is 0/1) as a LIT param as well.

bcain added a comment.May 1 2019, 7:33 AM

These tests turn on debug mode themselves - they don't need to be limited the configurations where the user turns it on.

These tests need to be fixed so that they DTRT when the user specifies -debug_level=X (where X is 0/1) as a LIT param as well.

Ok but does that mean that the REQUIRES only belongs on a subset of the tests? Ones with MODULES_DEFINES: _LIBCPP_DEBUG=1 maybe?

I got here (to this change) indirectly. One of our hexagon targets doesn't support waitpid() so we see compilation failures on the tests that include debug_mode_helper.h. When I read this comment regarding system lib I think I misunderstood and assumed that these tests only are necessary for some library configurations.

I can follow-through on this change if you and Eric agree on what it should be. But maybe I will work on a feature test for waitpid (LIBCXX_HAVE_WAITPID?) instead (or in addition) and somehow gate these tests that way.

I can follow-through on this change if you and Eric agree on what it should be.

We're talking about that ;-)

bcain updated this revision to Diff 197588.May 1 2019, 10:40 AM
bcain edited the summary of this revision. (Show Details)

Changed debug-mode feature to be deactivated when the LIBCXX_TEST_HAVE_FORK option is disabled. This hopefully sidesteps the debate triggered by the previous revision.

Added REQUIRES: debug-mode to libcxx/input.output/filesystems/class.path/path.itr/iterator_db.pass.cpp, this was unintentionally omitted before.

bcain added a comment.May 8 2019, 8:08 AM

Changed debug-mode feature to be deactivated when the LIBCXX_TEST_HAVE_FORK option is disabled. This hopefully sidesteps the debate triggered by the previous revision.

I think this change makes sense and avoids the controversy. But I suppose the 'debug-mode' feature label might be misleading. Maybe it be better if I just changed it to 'has-fork'?

bcain updated this revision to Diff 198833.May 9 2019, 8:18 AM
bcain retitled this revision from [libcxx] add debug-mode feature flag to [libcxx] limit debug mode tests on platforms without fork.
bcain edited the summary of this revision. (Show Details)

Changed to use 'has-fork' feature instead of 'debug-mode'.

bcain updated this revision to Diff 198836.May 9 2019, 8:20 AM

Previous patch was missing update to config.py.