Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SingleSource/Atomic] Add preliminary tests for atomic builtins. #94

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Logikable
Copy link

There exist atomic IR unit tests and libatomic unit tests, but neither can test the atomicity and interoperability of atomic builtins and compiler-rt's atomic library. These tests aim to approximate behaviour encountered in user code.

These tests have caught issues in Clang. See llvm/llvm-project#74349 and llvm/llvm-project#73176 for LLVM changes inspired by these tests.

Note to reviewers: this is a recreation of the accidentally closed #78.

There exist atomic IR unit tests and libatomic unit tests, but neither
can test the atomicity and interoperability of atomic builtins and
compiler-rt's atomic library. These tests aim to approximate behaviour
encountered in user code.

These tests have caught issues in Clang. See
llvm/llvm-project#74349 and
llvm/llvm-project#73176 for LLVM changes
inspired by these tests.
Copy link
Member

@jyknight jyknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, modulo the one comment/question.

@@ -0,0 +1,9 @@
# Link the Clang built libatomic.
execute_process(COMMAND ${CMAKE_C_COMPILER} --print-file-name=libclang_rt.atomic.so
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That library isn't built by default, so almost nobody will have a copy. I assume we need to have some conditionals here to skip testing if the library isn't present?

Potentially we should additionally integration-test against -latomic (even if that comes via GCC), since that's what real users actually specify.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I'm not great with CMake -- what are better ways to do what I have here?

@Logikable Logikable requested a review from jyknight February 27, 2024 22:04
@Logikable
Copy link
Author

Gentle bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants