Incorrect Microsoft mangled name for the guard variable of a static local variable

[tahonermann]

Clang and MSVC generate different mangled names for the guard variable of a static local variable as demonstrated in the following example exercised with MSVC 19.35.32216.1 for x64 from Visual Studio 2022 and Clang 17. See https://godbolt.org/z/MznoPb8bb.

> type t.cpp
struct __declspec(IMPEXP) S {
  static S& get();
  static S& singleton() {
    static S& s = get();
    return s;
  }
};
S& f() {
  return S::singleton();
}

# MSVC compilation for dllexport:
> cl /nologo /c /DIMPEXP=dllexport t.cpp
t.cpp

> llvm-objdump.exe -t t.obj | find "singleton"
[18](sec  6)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000 ?singleton@S@@SAAEAU1@XZ
...
[39](sec 11)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 ?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA
[42](sec 12)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 ?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA

# Clang compilation for dllexport:
> clang -c -DIMPEXP=dllexport t.cpp

> llvm-objdump -t t.o | find "singleton"
[13](sec  5)(fl 0x00)(ty  20)(scl   2) (nx 0) 0x00000000 ?singleton@S@@SAAEAU1@XZ
[25](sec  8)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 ?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA
[28](sec  9)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 ?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
...

The mangled names for S::singleton() and its static local variable s match, but the guard variable names differ with regard to a substitution reference ("2" vs "1"):

• MSVC: ?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA
• LLVM: ?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA

Demangling demonstrates that the MSVC generated one is correct. Note that the return type is wrong for the second name below.

# MSVC:
> llvm-undname '?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA'
?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA
int `public: static struct S & __cdecl S::singleton(void)'::`2'::$TSS0

# LLVM:
> llvm-undname '?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA'
?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
int `public: static struct singleton & __cdecl S::singleton(void)'::`2'::$TSS0

Since the guard variable is only accessible by S::singleton(), this mismatch normally isn't an issue; any TU that exports a definition of S::singleton() will provide an implementation that references the guard variable exported from the same TU. For TUs that import the definition, no references to the symbols for the local static variable or its guard variable will be emitted.

# MSVC compilation for dllimport:
> cl /nologo /c /DIMPEXP=dllimport t.cpp
t.cpp

> llvm-objdump.exe" -t t.obj | find "singleton"
[ 9](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?singleton@S@@SAAEAU1@XZ

# Clang compilation for dllimport:
> clang -c -DIMPEXP=dllimport t.cpp

> llvm-objdump -t t.o | find "singleton"
[14](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?singleton@S@@SAAEAU1@XZ

However, when S::singleton() is declared with __forceinline, problems ensue. MSVC apparently does not inline imported inline functions even when declared __forceinline. Meanwhile, Clang inlines the function definition, but emits import references for the static local variable and its guard variable. See https://godbolt.org/z/qKdazcTah.

# Same source file above with __forceinline added to the S::singleton() declaration.
> type t.cpp
struct __declspec(IMPEXP) S {
  static S& get();
  __forceinline static S& singleton() {
    static S& s = get();
    return s;
  }
};
S& f() {
  return S::singleton();
}

# MSVC compilation for dllimport with __forceinline:
> cl /nologo /c /DIMPEXP=dllimport t.cpp
t.cpp

> llvm-objdump.exe -t t.obj | find "singleton"
[ 9](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?singleton@S@@SAAEAU1@XZ

# Clang compilation for dllimport with __forceinline:
> clang -c -DIMPEXP=dllimport t.cpp

> llvm-objdump -t t.o | find "singleton"
[14](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
[19](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA

This difference in behavior results in link failures when a __forceinline function with a static local variable is:

1. compiled for export via declspec(dllexport) in a DLL using MSVC, and
2. compiled for import via declspec(dllimport) in a DLL/Executable using Clang.

The link failure occurs because Clang emits a dependency on the guard variable symbol and that dependency isn't satisfied by the MSVC built DLL (due to the difference in name mangling).

[tahonermann]

Fixing the mangled name would be a breaking change for code like that in the test case when __forceinline is present, the DLL providing the symbols is built by Clang, the DLL/executable consuming the symbols is built by Clang, and both the provider and the consumer are not both recompiled. So there is some risk in just fixing the name.

It looks to me like the best course of action is to:

1. Fix the name mangling issue for the next Clang release, but use the old behavior when -fclang-abi-compat= is used to opt-in to an older ABI.
2. Modify the behavior of the __forceinline specifier so that functions declared __declspect(dllimport) are no longer inlined so as to match the MSVC behavior. This behavior should also be tied to -fclang-abi-compat= to allow for backward compatibility.

@rnk

[tahonermann]

Alternatively, rather than unconditionally not inlining __declspec(dllimport) __forceinline functions, some minor changes to the following code might suffice:

clang/lib/CodeGen/CodeGenModule.cpp:
3980 bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
....
3998   if (F->hasAttr<DLLImportAttr>() && !F->hasAttr<AlwaysInlineAttr>()) {
3999     // Check whether it would be safe to inline this dllimport function.
4000     DLLImportFunctionVisitor Visitor;
4001     Visitor.TraverseFunctionDecl(const_cast<FunctionDecl*>(F));
4002     if (!Visitor.SafeToInline)
4003       return false;
4004
4005     if (const CXXDestructorDecl *Dtor = dyn_cast<CXXDestructorDecl>(F)) {
4006       // Implicit destructor invocations aren't captured in the AST, so the
4007       // check above can't see them. Check for them manually here.
4008       for (const Decl *Member : Dtor->getParent()->decls())
4009         if (isa<FieldDecl>(Member))
4010           if (HasNonDllImportDtor(cast<FieldDecl>(Member)->getType()))
4011             return false;
4012       for (const CXXBaseSpecifier &B : Dtor->getParent()->bases())
4013         if (HasNonDllImportDtor(B.getType()))
4014           return false;
4015     }
4016   }
....
4029 }

It looks like @zmodem has worked on this before per the following commits:

• 0479c53b6c5224c16bdc78cb014e76c0b0dbc6f9: [dllimport] Honor always_inline when deciding whether a dllimport function should be available for inlining (PR48925)
• 93f7547260f1e490fd95b68f05f0b69d356ce8be: Try harder to not inline dllimport functions referencing non-dllimport functions
• 6eaa8323a887a878f54dfcc65d4fff72c0f5c161: Allow TLS vars in dllimport/export functions; only inline dllimport functions when safe (PR24593)

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Tom Honermann (tahonermann)

Clang and MSVC generate different mangled names for the guard variable of a static local variable as demonstrated in the following example exercised with MSVC 19.35.32216.1 for x64 from Visual Studio 2022 and Clang 17. See https://godbolt.org/z/MznoPb8bb. ``` > type t.cpp struct __declspec(IMPEXP) S { static S& get(); static S& singleton() { static S& s = get(); return s; } }; S& f() { return S::singleton(); }

MSVC compilation for dllexport:

> cl /nologo /c /DIMPEXP=dllexport t.cpp
t.cpp

> llvm-objdump.exe -t t.obj | find "singleton"
[18](sec 6)(fl 0x00)(ty 20)(scl 2) (nx 0) 0x00000000 ?singleton@S@@SAAEAU1@XZ
...
[39](sec 11)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 ?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA
[42](sec 12)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 ?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA

Clang compilation for dllexport:

> clang -c -DIMPEXP=dllexport t.cpp

> llvm-objdump -t t.o | find "singleton"
[13](sec 5)(fl 0x00)(ty 20)(scl 2) (nx 0) 0x00000000 ?singleton@S@@SAAEAU1@XZ
[25](sec 8)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 ?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA
[28](sec 9)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 ?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
...

The mangled names for `S::singleton()` and its static local variable `s` match, but the guard variable names differ with regard to a substitution reference ("2" vs "1"):
- MSVC: `?$TSS0@?1??singleton@<!-- -->S@@<!-- -->SAAEAU2@<!-- -->XZ@<!-- -->4HA`
- LLVM: `?$TSS0@?1??singleton@<!-- -->S@@<!-- -->SAAEAU1@<!-- -->XZ@<!-- -->4HA`

Demangling demonstrates that the MSVC generated one is correct. Note that the return type is wrong for the second name below.

MSVC:

> llvm-undname '?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA'
?$TSS0@?1??singleton@S@@SAAEAU2@XZ@4HA
int public: static struct S & __cdecl S::singleton(void)'::2'::$TSS0

LLVM:

> llvm-undname '?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA'
?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
int public: static struct singleton & __cdecl S::singleton(void)'::2'::$TSS0

Since the guard variable is only accessible by `S::singleton()`, this mismatch normally isn't an issue; any TU that exports a definition of `S::singleton()` will provide an implementation that references the guard variable exported from the same TU. For TUs that import the definition, no references to the symbols for the local static variable or its guard variable will be emitted.

MSVC compilation for dllimport:

> cl /nologo /c /DIMPEXP=dllimport t.cpp
t.cpp

> llvm-objdump.exe" -t t.obj | find "singleton"
[ 9](sec 0)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imp?singleton@S@@SAAEAU1@XZ

Clang compilation for dllimport:

> clang -c -DIMPEXP=dllimport t.cpp

> llvm-objdump -t t.o | find "singleton"
[14](sec 0)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imp?singleton@S@@SAAEAU1@XZ

However, when `S::singleton()` is declared with `__forceinline`, problems ensue. MSVC apparently does not inline imported inline functions even when declared `__forceinline`. Meanwhile, Clang inlines the function definition, but emits import references for the static local variable and its guard variable. See https://godbolt.org/z/qKdazcTah.

Same source file above with __forceinline added to the S::singleton() declaration.

> type t.cpp
struct __declspec(IMPEXP) S {
static S& get();
__forceinline static S& singleton() {
static S& s = get();
return s;
}
};
S& f() {
return S::singleton();
}

MSVC compilation for dllimport with __forceinline:

> cl /nologo /c /DIMPEXP=dllimport t.cpp
t.cpp

> llvm-objdump.exe -t t.obj | find "singleton"
[ 9](sec 0)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imp?singleton@S@@SAAEAU1@XZ

Clang compilation for dllimport with __forceinline:

> clang -c -DIMPEXP=dllimport t.cpp

> llvm-objdump -t t.o | find "singleton"
[14](sec 0)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imp?$TSS0@?1??singleton@S@@SAAEAU1@XZ@4HA
[19](sec 0)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imp?s@?1??singleton@S@@SAAEAU2@XZ@4AEAU2@EA

This difference in behavior results in link failures when a `__forceinline` function with a static local variable is:

1. compiled for export via  `declspec(dllexport)` in a DLL using MSVC, and
2. compiled for import via `declspec(dllimport)` in a DLL/Executable using Clang.

The link failure occurs because Clang emits a dependency on the guard variable symbol and that dependency isn't satisfied by the MSVC built DLL (due to the difference in name mangling).
</details>

[zmodem]

+@mstorsjo also

Nice find!

MSVC apparently does not inline imported inline functions even when declared __forceinline

It does when optimizations are enabled: https://godbolt.org/z/WcMdKET91 so I wouldn't want to dial back the inlining.

Fixing the mangled name would be a breaking change

At least it would be a link failure rather than a silent break, right?

I don't think we've made strong stability guarantees for clang's msvc abi so far, so I'm not sure we need to do the -fclang-abi-compat= dance?

[rnk]

So, we should definitely fix the mangling bug, but why do you want to inline this? The code pattern is quite complicated, and unlikely to be profitable to inline:
https://godbolt.org/z/1frhWbnbf

At first I was concerned that Clang wasn't going to properly import the static local variable, since referring to non-exported things is a common reason we block inlining, but it looks like we've been getting it right and exporting and importing static locals since forever.

The substitution tracking portion of the mangler is unfortunately quite complicated. It suggests that we are mangling the portions of the name in the wrong order.

[tahonermann]

It does when optimizations are enabled: https://godbolt.org/z/WcMdKET91 so I wouldn't want to dial back the inlining.

Ah, so it does.

At least it would be a link failure rather than a silent break, right?

Yes. The problematic case is when a DLL that provides the symbols is built with an earlier version of Clang and can't be (easily) rebuilt with a newer version. In that case, there wouldn't be a simple workaround if there wasn't a way to opt-in to the previous behavior. This risk is probably fairly low though.

The substitution tracking portion of the mangler is unfortunately quite complicated. It suggests that we are mangling the portions of the name in the wrong order.

I think it is actually an off-by-one error. Consider the effect of adding a namespace.

> type t.cpp
namespace NS {
struct __declspec(IMPEXP) S {
  static S& get();
  __forceinline static S& singleton() {
    static S& s = get();
    return s;
  }
};
}
NS::S& f() {
  return NS::S::singleton();
}

# Clang compilation for dllimport with __forceinline:
> clang -c -DIMPEXP=dllimport t.cpp

> llvm-objdump -t t.o | find "singleton"
[14](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?$TSS0@?1??singleton@S@NS@@SAAEAU12@XZ@4HA
[19](sec  0)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 __imp_?s@?1??singleton@S@NS@@SAAEAU23@XZ@4AEAU23@EA

> llvm-undname '?$TSS0@?1??singleton@S@NS@@SAAEAU12@XZ@4HA'
?$TSS0@?1??singleton@S@NS@@SAAEAU12@XZ@4HA
int `public: static struct S::singleton & __cdecl NS::S::singleton(void)'::`2'::$TSS0

The return type is now encoded as struct S::singleton & rather than struct singleton & (it should be struct NS::S &).

[tahonermann]

I don't know if there is any way to tell for sure, but I suspect the problem is that Microsoft is treating the $TSS0 portion of the mangled name as back referenceable. If so, that would imply updating the following function to add the fragment via a call to mangleSourceName(). I see there are quite a few places where mangleSourceName() is already called with special names.

clang/lib/AST/MicrosoftMangle.cpp:
3918 void MicrosoftMangleContextImpl::mangleThreadSafeStaticGuardVariable(
3919     const VarDecl *VD, unsigned GuardNum, raw_ostream &Out) {
3920   msvc_hashing_ostream MHO(Out);
3921   MicrosoftCXXNameMangler Mangler(*this, MHO);
3922
3923   Mangler.getStream() << "?$TSS" << GuardNum << '@';
3924   Mangler.mangleNestedName(VD);
3925   Mangler.getStream() << "@4HA";
3926 }