Skip to content

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Aug 26, 2024

No description provided.

@ldionne ldionne requested a review from a team as a code owner August 26, 2024 14:11
@ldionne ldionne added this to the LLVM 19.X Release milestone Aug 26, 2024
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 26, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/106077.diff

1 Files Affected:

  • (renamed) libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp ()
diff --git a/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp b/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp
similarity index 100%
rename from libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp
rename to libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp

@tru
Copy link
Collaborator

tru commented Sep 1, 2024

Seems like this PR caused tests to fail. Can you have a look @ldionne

@tru
Copy link
Collaborator

tru commented Sep 10, 2024

ping

@tru
Copy link
Collaborator

tru commented Sep 11, 2024

@philnik777 you approved it - but the tests are failing - I did re-run them and it still failed. Would it still be safe to merge this?

@philnik777
Copy link
Contributor

@philnik777 you approved it - but the tests are failing - I did re-run them and it still failed. Would it still be safe to merge this?

Not as-is. We need to suppress the diagnostic that causes the test to fail, but that's a minor change.

@ldionne
Copy link
Member Author

ldionne commented Sep 17, 2024

Sorry, I dropped the ball on this cause I was absorbed by the modules work. Looking now.

@ldionne ldionne force-pushed the review/is-always-lock-free-test-name branch from 6ff7ed8 to eddceb7 Compare September 17, 2024 14:31
@tru
Copy link
Collaborator

tru commented Sep 24, 2024

Seems like there are still a lot of CI problems here?

@ldionne ldionne force-pushed the review/is-always-lock-free-test-name branch 3 times, most recently from 17e0d67 to 8225a05 Compare September 30, 2024 18:29
@tru tru force-pushed the review/is-always-lock-free-test-name branch from 8225a05 to 037efef Compare October 1, 2024 07:04
@ldionne ldionne force-pushed the review/is-always-lock-free-test-name branch from 037efef to 74dd036 Compare October 1, 2024 13:33
// UNSUPPORTED: c++03, c++11, c++14
// XFAIL: LIBCXX-PICOLIBC-FIXME
Copy link
Member Author

Choose a reason for hiding this comment

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

@domin144 @mplatings I don't quite understand why this fails. There seems to be missing the __atomic_is_lock_free intrinsic on the Picolibc setup, but the other atomic tests are working. Could you take a look?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I will merge this because we want to cherry-pick onto LLVM 19 and it's better to have this test for most platforms than for none of them, but I would really appreciate if we could address this in a follow-up.

@ldionne ldionne merged commit b456619 into llvm:main Oct 2, 2024
63 checks passed
@ldionne ldionne deleted the review/is-always-lock-free-test-name branch October 2, 2024 12:54
@ldionne
Copy link
Member Author

ldionne commented Oct 2, 2024

/cherry-pick b456619

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Oct 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

/pull-request #110838

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
ldionne pushed a commit that referenced this pull request Oct 9, 2024
Currently this test is completely xfailed as part of the patch
#106077. But this test works on
A and R profile, not in v7M profile. Because the test contain cases in
which m-profile will fail for atomic types greater than 4 bytes in size.
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Oct 11, 2024
@@ -45,7 +51,8 @@ void check_always_lock_free(std::atomic<T> const& a) {

// In all cases, also sanity-check it based on the implication always-lock-free => lock-free.
if (is_always_lock_free) {
std::same_as<bool> decltype(auto) is_lock_free = a.is_lock_free();
auto is_lock_free = a.is_lock_free();
ASSERT_SAME_TYPE(decltype(is_always_lock_free), bool const);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ASSERT_SAME_TYPE(decltype(is_always_lock_free), bool const);
ASSERT_SAME_TYPE(decltype(is_lock_free), bool);

Copy link
Member

Choose a reason for hiding this comment

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

Would you open a PR to fix this?

Copy link
Member Author

Choose a reason for hiding this comment

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

StephanTLavavej added a commit that referenced this pull request Oct 25, 2024
… `-Wno-psabi` (#113608)

MSVC doesn't understand `-Wno-psabi`, which was introduced here by
@ldionne in #106077.

Using `ADDITIONAL_COMPILE_FLAGS(gcc-style-warnings)` (implemented by
#75317) avoids passing this to MSVC.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
… `-Wno-psabi` (llvm#113608)

MSVC doesn't understand `-Wno-psabi`, which was introduced here by
@ldionne in llvm#106077.

Using `ADDITIONAL_COMPILE_FLAGS(gcc-style-warnings)` (implemented by
llvm#75317) avoids passing this to MSVC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

7 participants