Page MenuHomePhabricator

[MS] Implement on-demand TLS initialization for Microsoft CXX ABI
ClosedPublic

Authored by momo5502 on Dec 9 2021, 10:29 AM.

Details

Summary

TLS initializers, for example constructors of thread-local variables, don't necessarily get called. If a thread was created before a module is loaded, the module's TLS initializers are not executed for this particular thread.

This is why Microsoft added support for dynamic TLS initialization. Before every use of thread-local variables, a check is added that runs the module's TLS initializers on-demand.

To do this, the method __dyn_tls_on_demand_init gets called. Internally, it simply calls __dyn_tls_init.

No additional TLS initializer that sets the guard needs to be emitted, as the guard always gets set by __dyn_tls_init.
The guard is also checked again within __dyn_tls_init. This makes our check redundant, however, as Microsoft's compiler also emits this check, the behaviour is adopted here.

Diff Detail

Event Timeline

momo5502 requested review of this revision.Dec 9 2021, 10:29 AM
momo5502 created this revision.
momo5502 edited the summary of this revision. (Show Details)Dec 9 2021, 10:35 AM
momo5502 updated this revision to Diff 393247.Dec 9 2021, 12:00 PM
rnk edited reviewers, added: hans; removed: rnk.Dec 16 2021, 10:30 AM
rnk added a subscriber: rnk.Dec 16 2021, 10:33 AM

I probably don't have time to review this, so let me redirect to @hans.

clang/lib/CodeGen/MicrosoftCXXABI.cpp
2402

My understanding is that every DLL has exactly one __tls_guard variable. All TLS variables in a DLL are initialized as soon as a thread accesses one TLS variable for a DLL. Is that accurate? Can you add comments about the way this is intended to work here?

Is this a new feature in MSVC? Seems like it might be. If so, should it be predicated on isCompatibleWithMSVC(1925) or something?

momo5502 updated this revision to Diff 395856.Dec 22 2021, 6:11 AM
momo5502 edited the summary of this revision. (Show Details)

A call to isCompatibleWithMSVC was added with the proper version that introduced the change (1925) and comments describing the intention of the change were added.

Additionally, a bug was fixed. Setting the guard variable to true before doing the initialization breaks the feature, as the guard needs to be false for __dyn_tls_init to work.
Not emitting the store to the guard should be fine, as it will be set by __dyn_tls_init anyways. The behaviour is now identical to the way Microsoft's compiler does it.

momo5502 marked an inline comment as done.Dec 22 2021, 6:13 AM
majnemer added inline comments.Dec 24 2021, 8:51 PM
clang/lib/CodeGen/MicrosoftCXXABI.cpp
404–406

Should this depend on the MSVC compatibility version?

aganea added a subscriber: aganea.Dec 29 2021, 11:50 AM
aganea added inline comments.
clang/lib/CodeGen/MicrosoftCXXABI.cpp
2472

s/varibale/variable/

2479

Missing period at the end of the sentence please.

clang/test/CodeGenCXX/ms-thread_local.cpp
1–3

Also please check with -fms-compatibility-version=1920 to ensure that the dynamic initialization isn't generated.

momo5502 updated this revision to Diff 396783.Dec 31 2021, 6:12 AM
momo5502 marked 4 inline comments as done.

Comments were addressed

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with constructors/destructors, just like it is currently done here for the Itanium ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases when triggering dynamic TLS initialization even for constant variables is useful.
For example there might be custom TLS callbacks that can affect the value of this variable.

If desired, I can change it to match the behaviour of MS, but I thought it could be beneficial to diverge in this case.

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with constructors/destructors, just like it is currently done here for the Itanium ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases when triggering dynamic TLS initialization even for constant variables is useful.
For example there might be custom TLS callbacks that can affect the value of this variable.

If desired, I can change it to match the behaviour of MS, but I thought it could be beneficial to diverge in this case.

IMO, it is probably best to match behavior here within reason (ignoring bugs latent in MSVC) here. My thinking is that in the face of COMDATs, we cannot ensure which copy of an inline function will make its way to the binary and different link orders would provide different behavior.

momo5502 updated this revision to Diff 397107.Jan 3 2022, 11:04 AM

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with constructors/destructors, just like it is currently done here for the Itanium ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases when triggering dynamic TLS initialization even for constant variables is useful.
For example there might be custom TLS callbacks that can affect the value of this variable.

If desired, I can change it to match the behaviour of MS, but I thought it could be beneficial to diverge in this case.

IMO, it is probably best to match behavior here within reason (ignoring bugs latent in MSVC) here. My thinking is that in the face of COMDATs, we cannot ensure which copy of an inline function will make its way to the binary and different link orders would provide different behavior.

Fair enough. Logic has now moved to the generic C++ ABI implementation and the test was adjusted.

majnemer accepted this revision.Jan 3 2022, 3:13 PM

Looks great! Please give others some time to review it as it is a holiday season...

This revision is now accepted and ready to land.Jan 3 2022, 3:13 PM
majnemer retitled this revision from Implement on-demand TLS initialization for Microsoft CXX ABI to [MS] Implement on-demand TLS initialization for Microsoft CXX ABI.Jan 12 2022, 6:24 PM
This revision was landed with ongoing or failed builds.Jan 13 2022, 9:24 PM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Jan 17 2022, 6:35 AM

Looks great, thanks! Maurice, do you want to add a note about this to docs/ReleaseNotes.rst?

Looks great, thanks! Maurice, do you want to add a note about this to docs/ReleaseNotes.rst?

Good point. I created a new change: https://reviews.llvm.org/D117500