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

[clang] Define ATOMIC_FLAG_INIT correctly for C++. #97534

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

chrisnc
Copy link
Contributor

@chrisnc chrisnc commented Jul 3, 2024

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Jul 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Chris Copeland (chrisnc)

Changes

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

2 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Headers/stdatomic.h (+4)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1537eaaba0c66..3d85065a7ff8a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -750,6 +750,8 @@ Bug Fixes in This Version
 
 - Fixed `static_cast` to array of unknown bound. Fixes (#GH62863).
 
+- Fixed the definition of ATOMIC_FLAG_INIT in stdatomic.h so it can be used in C++.
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/lib/Headers/stdatomic.h b/clang/lib/Headers/stdatomic.h
index 9c103d98af8c5..88172038be5c4 100644
--- a/clang/lib/Headers/stdatomic.h
+++ b/clang/lib/Headers/stdatomic.h
@@ -166,7 +166,11 @@ typedef _Atomic(uintmax_t)          atomic_uintmax_t;
 
 typedef struct atomic_flag { atomic_bool _Value; } atomic_flag;
 
+#ifdef __cplusplus
+#define ATOMIC_FLAG_INIT { false }
+#else
 #define ATOMIC_FLAG_INIT { 0 }
+#endif
 
 /* These should be provided by the libc implementation. */
 #ifdef __cplusplus

@chrisnc
Copy link
Contributor Author

chrisnc commented Jul 3, 2024

Without this change, trying to use ATOMIC_FLAG_INIT in a constant initialization context in C++ yields the following:

https://godbolt.org/z/sTvMs5a95

<source>:3:17: error: non-constant-expression cannot be narrowed from type 'int' to 'atomic_bool' (aka '_Atomic(bool)') in initializer list [-Wc++11-narrowing]
    3 | atomic_flag x = ATOMIC_FLAG_INIT;
      |                 ^~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-trunk-20240703/lib/clang/19/include/stdatomic.h:169:28: note: expanded from macro 'ATOMIC_FLAG_INIT'
  169 | #define ATOMIC_FLAG_INIT { 0 }
      |                            ^
<source>:3:17: note: insert an explicit cast to silence this issue
    3 | atomic_flag x = ATOMIC_FLAG_INIT;
      |                 ^~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-trunk-20240703/lib/clang/19/include/stdatomic.h:169:28: note: expanded from macro 'ATOMIC_FLAG_INIT'
  169 | #define ATOMIC_FLAG_INIT { 0 }
      |                            ^
1 error generated.
Compiler returned: 1

Copy link

github-actions bot commented Jul 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@chrisnc
Copy link
Contributor Author

chrisnc commented Jul 9, 2024

ping

1 similar comment
@chrisnc
Copy link
Contributor Author

chrisnc commented Jul 20, 2024

ping

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for catching this! Can you also add some tests to clang/test/Headers/? We have some coverage for stdatomic.h in C, but not in C++ (I'd recommend trying to add a RUN line to the .c file with -x c++ so it's tested in C++ mode and then ensure there's sufficient coverage for ATOMIC_FLAG_INIT.)

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved
@chrisnc
Copy link
Contributor Author

chrisnc commented Jul 23, 2024

Rebased and added a test case. I confirmed that the test fails using the current definition of ATOMIC_FLAG_INIT:

error: 'expected-error' diagnostics seen but not expected:
  Line 38: non-constant-expression cannot be narrowed from type 'int' to 'atomic_bool' (aka '_Atomic(bool)') in initializer list
error: 'expected-note' diagnostics seen but not expected:
  Line 38: insert an explicit cast to silence this issue
2 errors generated.

@chrisnc chrisnc changed the title [clang] Define ATOMIC_FLAG_INIT correctly for C++. [clang] Define ATOMIC_FLAG_INIT correctly for C++. Jul 23, 2024
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

@AaronBallman
Copy link
Collaborator

Btw, do you need someone to merge this on your behalf?

@chrisnc
Copy link
Contributor Author

chrisnc commented Jul 23, 2024

@AaronBallman yes, that would be great! I'm just a contributor, not a collaborator.

@AaronBallman AaronBallman merged commit 4bb3a1e into llvm:main Jul 24, 2024
8 checks passed
@AaronBallman
Copy link
Collaborator

I think this should be picked for the Clang 19 branch, so I filed #100362

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 24, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 24, 2024
@ldionne
Copy link
Member

ldionne commented Jul 24, 2024

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 24, 2024

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

Error: Command failed due to missing milestone.

@ldionne
Copy link
Member

ldionne commented Jul 24, 2024

It seems like the bot looks for the command anywhere in any comment :)

@AaronBallman
Copy link
Collaborator

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

That's good to know! I was going off the documentation at: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches because it says "issue" specifically. I'll post a patch to update those docs, thanks!

@AaronBallman
Copy link
Collaborator

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

That's good to know! I was going off the documentation at: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches because it says "issue" specifically. I'll post a patch to update those docs, thanks!

#100401

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 24, 2024

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

That's good to know! I was going off the documentation at: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches because it says "issue" specifically. I'll post a patch to update those docs, thanks!

Error: Command failed due to missing milestone.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 24, 2024

@AaronBallman Just FYI you can also add the Milestone to the PR directly and use the /cherry-pick command right here. This is a bit simpler.

That's good to know! I was going off the documentation at: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches because it says "issue" specifically. I'll post a patch to update those docs, thanks!

#100401

Error: Command failed due to missing milestone.

@chrisnc chrisnc deleted the fix-cxx-atomic-flag-init branch July 24, 2024 22:15
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary: 

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250653
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants