This is an archive of the discontinued LLVM Phabricator instance.

[libc++] add FreeBSD atomic wait support
ClosedPublic

Authored by emaste on Jan 19 2023, 9:23 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Commits
rG83387dbc18e7: [libc++] add FreeBSD atomic wait support
Summary

From Konstantin Belousov <kib@freebsd.org>

Diff Detail

Event Timeline

emaste created this revision.Jan 19 2023, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 9:23 AM
emaste requested review of this revision.Jan 19 2023, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 9:23 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this! Does this fix existing tests?

emaste added inline comments.Jan 19 2023, 9:37 AM
libcxx/src/atomic.cpp
29–33

The CI format job complained about the indentation on the changes in this file, but they are consistent with what is in the file now. Should we run clang-format on it first, and then update this to match?

emaste added a comment.EditedJan 19 2023, 9:39 AM

Thanks for working on this!

In fact Kostik found this independent of my work on bringing up CI. It's fortuitous timing.

Does this fix existing tests?

There were a number of atomic tests that timed out on occasion or were inconsistent before (and I temporarily removed them in my work to get the CI going).
I applied this change and restored those tests and (in the one run so far) they passed.

I applied this change and restored those tests and (in the one run so far) they passed.

After a couple more runs it looks like the tests now pass consistently.

philnik accepted this revision.Jan 19 2023, 8:47 PM
philnik added a subscriber: philnik.

LGTM % nit.

libcxx/src/atomic.cpp
29–33

Just leave it as-is for now. Most of the code-base isn't formatted and we don't enforce it currently in any way.

82

You don't need the underscores in the argument names, since this is dylib code.

This revision is now accepted and ready to land.Jan 19 2023, 8:47 PM
kib added a subscriber: kib.Jan 20 2023, 4:08 AM
kib added inline comments.
libcxx/src/atomic.cpp
82

It is just consistent with the rest of the file (in other words, functions were copy/pasted from the Linux block and edited for FreeBSD).

emaste added inline comments.Jan 20 2023, 8:29 AM
libcxx/src/atomic.cpp
82

OK, I would rather keep this consistent with the other implementations, similar to the clang-format discussion above.

This revision was automatically updated to reflect the committed changes.