This is an archive of the discontinued LLVM Phabricator instance.

[libc++] add basic runtime assertions to <semaphore>
ClosedPublic

Authored by diamante0018 on Jul 16 2023, 3:53 AM.

Details

Reviewers
ldionne
EricWF
Group Reviewers
Restricted Project
Commits
rG42a024fad9fc: [libc++] Add basic runtime assertions to <semaphore>
Summary

Adding assertions will aid users that have bugs or logic mistakes in their code to receive error messages when debugging.

Diff Detail

Event Timeline

diamante0018 created this revision.Jul 16 2023, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 3:53 AM
diamante0018 requested review of this revision.Jul 16 2023, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 3:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Fix typo in my code

Remove static_assert. Do not want to add chrono include unless asked to.

Fix logic error in test file

Use std::counting_semaphore<>::max()

Undo last change to test file

Use new define for assertions

remove xfail flag and see if tests pass on mac

Add XFAILS flag for macos runners

Use std::counting_semaphore<>::max()

diamante0018 edited the summary of this revision. (Show Details)Jul 17 2023, 10:20 AM
EricWF accepted this revision.Jul 17 2023, 10:48 AM
EricWF added a subscriber: EricWF.

LGTM, though I spot 4 new checks and 3 new tests. Is there one missing?

libcxx/include/semaphore
99

Shouldn't this check be using ::max() rather than the default _LIBPCP_SEMAPHORE_MAX?

102

Please put an empty { } block here to make it more clear that this is intentional. Maybe a comment as well.

141

We should probably add a static assertion that __least_max_value is positive.

This revision is now accepted and ready to land.Jul 17 2023, 10:48 AM
diamante0018 added a comment.EditedJul 17 2023, 11:24 AM

You are correct in observing there are four new assertion but only three checks. There are three checks because one cannot be tested for. max::() is equivalent to the max value of ptrdiff_t so there is no number that can violate this precondition.
I was asked by @ldionne in a previous review to leave the assertion anyway for an identical situation in D154425.
I will see if I can address your other comments with a diff. Thanks for the review.

libcxx/include/semaphore
99

Unfortunately I don't think I can. ::max() is a definition that is not part of this class and is defined down in this file so it should not be an available definition right here.

Address review comments.

diamante0018 marked 2 inline comments as done.Jul 17 2023, 11:35 AM

Add test for new static_assert

Add xfail flags that should be required

Address review comments from Discord

maybe unused requires c++17, used void

ldionne requested changes to this revision.Jul 17 2023, 3:44 PM
ldionne added inline comments.
libcxx/include/semaphore
107

I suggest we move to __old > 0 as a drive-by fix too, since it's more consistent with the rest of the code (and it's easier to read, although that is subjective).

libcxx/test/libcxx/thread/thread.semaphore/assert.ctor.pass.cpp
12

This REQUIRES shouldn't be there, otherwise we're basically requiring an old system that didn't have the synchronization library. Along with the XFAIL below, this means that this test was never run.

libcxx/test/libcxx/thread/thread.semaphore/assert.release.pass.cpp
12–13

Same, both these lines should be removed.

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp
33–36 ↗(On Diff #541192)

This test is a .compile.pass.cpp, so your expected-error comment is never picked up. You want to move this to a separate .verify.cpp test instead.

This revision now requires changes to proceed.Jul 17 2023, 3:44 PM

Address review comments

diamante0018 marked 4 inline comments as done.Jul 17 2023, 3:54 PM
diamante0018 marked an inline comment as done.

I hope the tests succeed on macosx because they always seem to fail there and that's why I thought about adding the xfail flag. Let's see if removing both does the trick.

ldionne accepted this revision.Jul 17 2023, 4:18 PM

LGTM assuming CI is green. Thanks!

This revision is now accepted and ready to land.Jul 17 2023, 4:18 PM

fix warnings in test file

Use better macro for enabling assertions

FIx verify test

Attempt to fix test

Try to fix verify.cpp test file

Fix test finally

Added new UNSUPPORTED flag, fixing tests.

diamante0018 added a comment.EditedJul 19 2023, 1:46 PM

Since CI could be considered green since only one runner caused unrelated issues I'm gonna leave my details here for the landing of this revision since I don't have commit access. Thanks to the reviewers.

Edoardo Sanguineti <edoardo.sanguineti222@gmail.com>

This revision was landed with ongoing or failed builds.Jul 19 2023, 2:30 PM
This revision was automatically updated to reflect the committed changes.