Skip to content

[C23] Use thread_local semantics#70107

Merged
AaronBallman merged 4 commits intollvm:mainfrom
AaronBallman:aballman-70068
Oct 25, 2023
Merged

[C23] Use thread_local semantics#70107
AaronBallman merged 4 commits intollvm:mainfrom
AaronBallman:aballman-70068

Conversation

@AaronBallman
Copy link
Copy Markdown
Collaborator

When implementing thread_local as a keyword in C23, we accidentally started using C++11 thread_local semantics when using that keyword instead of using C11 _Thread_local semantics.

This oversight is fixed by pretending the user wrote _Thread_local instead. This doesn't have the best behavior in terms of diagnostics, but it does correct the semantic behavior.

Fixes #70068

When implementing thread_local as a keyword in C23, we accidentally
started using C++11 thread_local semantics when using that keyword
instead of using C11 _Thread_local semantics.

This oversight is fixed by pretending the user wrote _Thread_local
instead. This doesn't have the best behavior in terms of diagnostics,
but it does correct the semantic behavior.

Fixes llvm#70068
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 24, 2023
@llvmbot
Copy link
Copy Markdown
Member

llvmbot commented Oct 24, 2023

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

When implementing thread_local as a keyword in C23, we accidentally started using C++11 thread_local semantics when using that keyword instead of using C11 _Thread_local semantics.

This oversight is fixed by pretending the user wrote _Thread_local instead. This doesn't have the best behavior in terms of diagnostics, but it does correct the semantic behavior.

Fixes #70068


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

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/lib/Parse/ParseDecl.cpp (+9-2)
  • (added) clang/test/CodeGen/thread_local.c (+27)
  • (added) clang/test/Sema/thread_local.c (+16)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cf3f0c343b4d014..b104c5da97ab124 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -493,6 +493,9 @@ Bug Fixes in This Version
   Fixes (`#65143 <https://github.com/llvm/llvm-project/issues/65143>`_)
 - Fix crash in formatting the real/imaginary part of a complex lvalue.
   Fixes (`#69218 <https://github.com/llvm/llvm-project/issues/69218>`_)
+- No longer use C++ ``thread_local`` semantics in C23 when using
+  ``thread_local`` instead of ``_Thread_local``.
+  Fixes (`#70068 <https://github.com/llvm/llvm-project/issues/70068>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 14a28e5a31c57db..43c18090d6ef79c 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -4076,8 +4076,15 @@ void Parser::ParseDeclarationSpecifiers(
     case tok::kw_thread_local:
       if (getLangOpts().C23)
         Diag(Tok, diag::warn_c23_compat_keyword) << Tok.getName();
-      isInvalid = DS.SetStorageClassSpecThread(DeclSpec::TSCS_thread_local, Loc,
-                                               PrevSpec, DiagID);
+      // We map thread_local to _Thread_local in C23 mode so it retains the C
+      // semantics rather than getting the C++ semantics.
+      // FIXME: diagnostics will now show _Thread_local when the user wrote
+      // thread_local in source in C23 mode; we need some general way to
+      // identify which way the user spelled the keyword in source.
+      isInvalid = DS.SetStorageClassSpecThread(
+          getLangOpts().C23 ? DeclSpec::TSCS__Thread_local
+                            : DeclSpec::TSCS_thread_local,
+          Loc, PrevSpec, DiagID);
       isStorageClass = true;
       break;
     case tok::kw__Thread_local:
diff --git a/clang/test/CodeGen/thread_local.c b/clang/test/CodeGen/thread_local.c
new file mode 100644
index 000000000000000..2daa43492cbf271
--- /dev/null
+++ b/clang/test/CodeGen/thread_local.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -std=c23 -emit-llvm -o - %s | FileCheck %s
+
+// Ensure that thread_local and _Thread_local emit the same codegen. See
+// https://github.com/llvm/llvm-project/issues/70068 for details.
+
+void func(void) {
+  static thread_local int i = 12;
+  static _Thread_local int j = 13;
+
+  extern thread_local int k;
+  extern thread_local int l;
+
+  (void)k;
+  (void)l;
+}
+
+// CHECK:      @func.i = internal thread_local global i32 12, align 4
+// CHECK-NEXT: @func.j = internal thread_local global i32 13, align 4
+// CHECK-NEXT: @k = external thread_local global i32, align 4
+// CHECK-NEXT: @l = external thread_local global i32, align 4
+
+// CHECK:      define dso_local void @func()
+// CHECK-NEXT: entry:
+// CHECK-NEXT: %0 = call align 4 ptr @llvm.threadlocal.address.p0(ptr align 4 @k)
+// CHECK-NEXT: %1 = load i32, ptr %0, align 4
+// CHECK-NEXT: %2 = call align 4 ptr @llvm.threadlocal.address.p0(ptr align 4 @l)
+// CHECK-NEXT: %3 = load i32, ptr %2, align 4
diff --git a/clang/test/Sema/thread_local.c b/clang/test/Sema/thread_local.c
new file mode 100644
index 000000000000000..c4a4dd4ceb1cd0d
--- /dev/null
+++ b/clang/test/Sema/thread_local.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c23 %s -verify
+
+// Ensure that thread_local and _Thread_local are synonyms in C23 and both
+// restrict local variables to be explicitly static or extern.
+void func(void) {
+  // FIXME: it would be nice if the diagnostic said 'thread_local' in this case.
+  thread_local int i = 12;  // expected-error {{'_Thread_local' variables must have global storage}}
+  _Thread_local int j = 13; // expected-error {{'_Thread_local' variables must have global storage}}
+
+  static thread_local int k = 14;
+  static _Thread_local int l = 15;
+
+  extern thread_local int m;
+  extern thread_local int n;
+}
+

Copy link
Copy Markdown
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM but I did not review the codegen tests.

// restrict local variables to be explicitly static or extern.
void func(void) {
// FIXME: it would be nice if the diagnostic said 'thread_local' in this case.
thread_local int i = 12; // expected-error {{'_Thread_local' variables must have global storage}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another solution would be to say 'thread local variables' which is spelling agnostic.

@AaronBallman AaronBallman merged commit 66f4a13 into llvm:main Oct 25, 2023
AaronBallman added a commit to AaronBallman/llvm-project that referenced this pull request Oct 25, 2023
When implementing thread_local as a keyword in C23, we accidentally
started using C++11 thread_local semantics when using that keyword
instead of using C11 _Thread_local semantics.

This oversight is fixed by pretending the user wrote _Thread_local
instead. This doesn't have the best behavior in terms of diagnostics,
but it does correct the semantic behavior.

Fixes llvm#70068
Fixes llvm#69167
tru pushed a commit that referenced this pull request Oct 27, 2023
When implementing thread_local as a keyword in C23, we accidentally
started using C++11 thread_local semantics when using that keyword
instead of using C11 _Thread_local semantics.

This oversight is fixed by pretending the user wrote _Thread_local
instead. This doesn't have the best behavior in terms of diagnostics,
but it does correct the semantic behavior.

Fixes #70068
Fixes #69167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c23 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C23 thread_local is mapping to C++ thread_local, not C11 _Thread_local

5 participants