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][MSVC] Correct mangling of thread-safe static initialization variables. #85300

Merged

Conversation

tahonermann
Copy link
Contributor

Static local variables with dynamic initializers depend on implicitly defined guard variables to synchronize thread-safe initialization. These guard variables may have external linkage and therefore require a stable name for linkage purposes. The Microsoft ABI assigns these variables a local name of $TSS followed by a discriminator and mangles them as a static local variable of type int. Previously, the $TSS<discriminator> portion of the name was not registered as a back reference candidate and this resulted in incorrect back references for enclosing class and/or namespace scopes that might be referenced in the signature of the enclosing function. This change adds the previously missing back reference registration. This matches the mangling performed by MSVC and resolves incompatibilities when inline functions with static local variables are inlined across DLL boundaries.

This is an ABI change and has the potential to cause backward compatibility issues with previous Clang releases.

@tahonermann tahonermann requested review from rnk and zmodem March 14, 2024 19:40
@tahonermann tahonermann self-assigned this Mar 14, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 14, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 2024

@llvm/pr-subscribers-clang

Author: Tom Honermann (tahonermann)

Changes

Static local variables with dynamic initializers depend on implicitly defined guard variables to synchronize thread-safe initialization. These guard variables may have external linkage and therefore require a stable name for linkage purposes. The Microsoft ABI assigns these variables a local name of $TSS followed by a discriminator and mangles them as a static local variable of type int. Previously, the $TSS&lt;discriminator&gt; portion of the name was not registered as a back reference candidate and this resulted in incorrect back references for enclosing class and/or namespace scopes that might be referenced in the signature of the enclosing function. This change adds the previously missing back reference registration. This matches the mangling performed by MSVC and resolves incompatibilities when inline functions with static local variables are inlined across DLL boundaries.

This is an ABI change and has the potential to cause backward compatibility issues with previous Clang releases.


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+6)
  • (modified) clang/lib/AST/MicrosoftMangle.cpp (+3-2)
  • (modified) clang/test/CodeGenCXX/mangle-ms-back-references.cpp (+17)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8935a610722a31..1d7902671a18fd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -47,6 +47,12 @@ C++ Specific Potentially Breaking Changes
 
 ABI Changes in This Version
 ---------------------------
+- Fixed Microsoft name mangling of implicitly defined variables used for thread
+  safe static initialization of static local variables. This change resolves
+  incompatibilities with code compiled by MSVC but might introduce
+  incompatibilities with code compiled by earlier versions of Clang when an
+  inline member function that contains a static local variable with a dynamic
+  initializer is declared with ``__declspec(dllimport)``. (#GH83616).
 
 AST Dumping Potentially Breaking Changes
 ----------------------------------------
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index b272a546573a31..aa26bb7ed46f48 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -390,6 +390,7 @@ class MicrosoftCXXNameMangler {
                           const FunctionDecl *D = nullptr,
                           bool ForceThisQuals = false,
                           bool MangleExceptionSpec = true);
+  void mangleSourceName(StringRef Name);
   void mangleNestedName(GlobalDecl GD);
 
 private:
@@ -408,7 +409,6 @@ class MicrosoftCXXNameMangler {
     mangleUnqualifiedName(GD, cast<NamedDecl>(GD.getDecl())->getDeclName());
   }
   void mangleUnqualifiedName(GlobalDecl GD, DeclarationName Name);
-  void mangleSourceName(StringRef Name);
   void mangleOperatorName(OverloadedOperatorKind OO, SourceLocation Loc);
   void mangleCXXDtorType(CXXDtorType T);
   void mangleQualifiers(Qualifiers Quals, bool IsMember);
@@ -3920,7 +3920,8 @@ void MicrosoftMangleContextImpl::mangleThreadSafeStaticGuardVariable(
   msvc_hashing_ostream MHO(Out);
   MicrosoftCXXNameMangler Mangler(*this, MHO);
 
-  Mangler.getStream() << "?$TSS" << GuardNum << '@';
+  Mangler.getStream() << "?";
+  Mangler.mangleSourceName("$TSS" + llvm::utostr(GuardNum));
   Mangler.mangleNestedName(VD);
   Mangler.getStream() << "@4HA";
 }
diff --git a/clang/test/CodeGenCXX/mangle-ms-back-references.cpp b/clang/test/CodeGenCXX/mangle-ms-back-references.cpp
index cb95c100b3d22e..b27a9c5acacb77 100644
--- a/clang/test/CodeGenCXX/mangle-ms-back-references.cpp
+++ b/clang/test/CodeGenCXX/mangle-ms-back-references.cpp
@@ -83,3 +83,20 @@ class H;
 
 void ManyParams(T01 &, T02 &, T03 &, T04 &, T05 &, T06 &, T07 &, T08 &, T09 &, T10 &, H<T11> &, H<T11> &) {}
 // CHECK: "?ManyParams@@YAXAAVT01@@AAVT02@@AAVT03@@AAVT04@@AAVT05@@AAVT06@@AAVT07@@AAVT08@@AAVT09@@AAVT10@@AAV?$H@VT11@@@@AAV?$H@VT11@@@@@Z"
+
+namespace NS {
+// The name "TSS0" for the name of the class below has been specifically
+// chosen to ensure that back reference lookup does not match against the
+// implicitly generated "$TSS0" name of the thread safe static initialization
+// variable.
+struct __declspec(dllexport) TSS0 {
+  static TSS0& get();
+  __forceinline static TSS0& singleton() {
+    static TSS0& lsv = get();
+    return lsv;
+  }
+};
+}
+// CHECK: "?singleton@TSS0@NS@@SAAAU12@XZ"
+// CHECK: "?lsv@?1??singleton@TSS0@NS@@SAAAU23@XZ@4AAU23@A"
+// CHECK: "?$TSS0@?1??singleton@TSS0@NS@@SAAAU23@XZ@4HA"

@tahonermann tahonermann force-pushed the thonerma-gh83616-ms-mangle-tss-guard branch from 89bb7c9 to 76c7940 Compare March 14, 2024 20:05
Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Please include a "Fixes #83616" reference in the commit message.

clang/lib/AST/MicrosoftMangle.cpp Show resolved Hide resolved
…ariables.

Static local variables with dynamic initializers depend on implicitly defined
guard variables to synchronize thread-safe initialization.  These guard
variables may have external linkage and therefore require a stable name for
linkage purposes.  The Microsoft ABI assigns these variables a local name of
'$TSS' followed by a discriminator and mangles them as a static local variable
of type 'int'.  Previously, the '$TSS<discriminator>' portion of the name was
not registered as a back reference candidate and this resulted in incorrect
back references for enclosing class and/or namespace scopes that might be
referenced in the signature of the enclosing function.  This change adds the
previously missing back reference registration.  This matches the mangling
performed by MSVC and resolves incompatibilities when inline functions with
static local variables are inlined across DLL boundaries.

This is an ABI change and has the potential to cause backward compatibility
issues with previous Clang releases.

Fixes llvm#83616
@tahonermann tahonermann force-pushed the thonerma-gh83616-ms-mangle-tss-guard branch from 76c7940 to 0547c32 Compare March 15, 2024 15:36
@tahonermann tahonermann merged commit f128607 into llvm:main Mar 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants