This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] limit debug mode tests on platforms without fork
AbandonedPublic

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

Details

Reviewers
mclow.lists
EricWF
bcain
Group Reviewers
Restricted Project
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.

ldionne requested changes to this revision.Nov 2 2020, 2:41 PM
ldionne added a reviewer: Restricted Project.

Is this still relevant? We now have a way of disabling all debug mode tests by using --param enable_debug_tests=False. If not, let's drop this to clean up the review queue.

This revision now requires changes to proceed.Nov 2 2020, 2:41 PM
ldionne commandeered this revision.Aug 31 2023, 12:35 PM
ldionne edited reviewers, added: bcain; removed: ldionne.

[Github PR transition cleanup]

We also use has-unix-headers now as a proxy to this. Closing since it's inactive and I don't think it is directly relevant anymore.

This revision now requires review to proceed.Aug 31 2023, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 12:35 PM
ldionne abandoned this revision.Aug 31 2023, 12:35 PM