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

Thread Local Storage does not work with DLLs loaded using LoadLibrary #13116

Closed
wxtrac opened this issue Apr 4, 2011 · 20 comments
Closed

Thread Local Storage does not work with DLLs loaded using LoadLibrary #13116

wxtrac opened this issue Apr 4, 2011 · 20 comments

Comments

@wxtrac
Copy link
Collaborator

@wxtrac wxtrac commented Apr 4, 2011

Issue migrated from trac ticket # 13116

component: wxMSW | priority: normal | resolution: fixed | keywords: WinXP TLS DLL

2011-04-04 11:09:43: @discnl created the issue


See PRB: Calling LoadLibrary() to Load a DLL That Has Static TLS and optionally this series on TLS for more in-depth information.
In the best case LoadLibrary fails (Windows 95) but with later systems you will get an access violation at some point (if you're lucky when first accessing a !__declspec(thread) variable).
It would be unfortunate to, by default, disable TLS compiler support for MSW but I don't see an alternative currently.

This applies to XP/Windows Server 2003 and earlier systems and for example occurs with Photoshop plug-ins.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 4, 2011

2011-04-04 12:39:01: @vadz changed status from new to confirmed

2011-04-04 12:39:01: @vadz commented

Yes, I'm aware of this problem but unfortunately I don't know what to do about it neither. Disabling TLS compiler support really seems too drastic but OTOH we probably can't decide which TLW implementation we use during run-time.

So IMO the best we can do is to document it and provide a way to disable it simply, e.g. by predefining wxNO_COMPILER_TLS when building.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 4, 2011

2011-04-04 13:35:56: @discnl commented


[hesitated to move this to wx-dev but keeping it in trac for now for tracking reasons]

Replying to [comment:1 vadz]:

Yes, I'm aware of this problem but unfortunately I don't know what to do about it neither. Disabling TLS compiler support really seems too drastic but OTOH we probably can't decide which TLW implementation we use during run-time.

Too drastic in what way? Does it introduce other bugs (that can't be fixed using the TLS functions?) or a (measured) performance penalty?
I now see the note at include/wx/string.h:60 mentioning performance improvements for string position caching but even there it's disabled for MSW because of these DLL build reasons (though maybe it was also disabled because wxUSE_UNICODE_UTF8=1 is not the default under MSW so the caching usually isn't needed anyway).

So IMO the best we can do is to document it and provide a way to disable it simply, e.g. by predefining wxNO_COMPILER_TLS when building.

'No one' will read it and from my recollection the crashes took some time to figure out and to point the finger at the 2.8 -> 2.9 transition and TLS. Of course the usage of a DLL through LoadLibrary is a lot smaller than an executable but with my current knowledge I would still prefer the opposite and have an opt-in for MSW TLS compiler support.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2011

2011-04-06 00:18:11: @vadz commented


Replying to [comment:2 disc]:

Too drastic in what way? Does it introduce other bugs (that can't be fixed using the TLS functions?) or a (measured) performance penalty?

The latter. I don't remember the numbers any more but if you run tests/benchmarks/tls.cpp (well, actually run the benchmark passing it the TLS tests on the command line) you should be able to see that the difference is very significant.

So IMO the best we can do is to document it and provide a way to disable it simply, e.g. by predefining wxNO_COMPILER_TLS when building.

'No one' will read it and from my recollection the crashes took some time to figure out

This is IMO the worst thing. If we could detect that we had been dynamically loaded we could at least assert if wxNO_COMPILER_TLS is not defined. Can we do this?

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2011

2011-04-06 02:29:02: @discnl commented


Replying to [comment:3 vadz]:

Replying to [comment:2 disc]:

Too drastic in what way? Does it introduce other bugs (that can't be fixed using the TLS functions?) or a (measured) performance penalty?

The latter. I don't remember the numbers any more but if you run tests/benchmarks/tls.cpp (well, actually run the benchmark passing it the TLS tests on the command line) you should be able to see that the difference is very significant.

Unfortunately using MSVC9 and a default setup.h it doesn't work (while doing command-line stuff there's some STL assert about not being sorted). Using wxUSE_STD_DEFAULT==0 fixes it. I can elaborate on that later if needed. Anyway indeed it's almost three times as slow here, maybe that drowns among other things going on though such as string conversions.

So IMO the best we can do is to document it and provide a way to disable it simply, e.g. by predefining wxNO_COMPILER_TLS when building.

'No one' will read it and from my recollection the crashes took some time to figure out

This is IMO the worst thing. If we could detect that we had been dynamically loaded we could at least assert if wxNO_COMPILER_TLS is not defined. Can we do this?

This would be a nice solution! I was thinking it could be done using (module) handles somehow but the best I've been able to come up with so far is designate in wx' WinMain that we are an exe (and thus by default a DLL). That might give false positives though in case someone is using for example wxIMPLEMENT_APP_NO_MAIN, but the assert message could take this probability into account.
I had another poor man's solution which, come to think more about it, may actually be better than the above and involves checking the extension of the module's name: anything but .exe (any others?) is considered a DLL and triggers the assert (preferably only when LoadLibrary was used but I don't know how to check for this).

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2011

2011-04-06 17:54:26: @vadz commented


The trouble with both of these methods is that they will certainly result in a lot of false positives so at the very least we'd need to provide some simple way of disabling this check if you're sure it's ok to use compiler TLS in your case.

I thought about checking "something" from DllMain() but this has 2 problems: first I don't know what this "something" should be and second we don't even have DllMain(). And in fact we could be compiled as a static library and linked inside a DLL so this is not a good check at all finally.

My last idea is to check _tls_index: as mentioned in Nynaeve's blog, it would be 0 in the problematic case but in vast majority of cases it is not actually 0 as any main program non trivial enough to load wx DLLs is almost certainly using TLS on its own. The only question is whether we can access it in a portable way, I'm not quite sure about this.

P.S. I'd definitely be interested in fixing the benchmarks with STL (I guess this was never tested before it became the default) but it's indeed a different issue.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 7, 2011

2011-04-07 12:40:51: @discnl commented


Replying to [comment:5 vadz]:

The trouble with both of these methods is that they will certainly result in a lot of false positives so at the very least we'd need to provide some simple way of disabling this check if you're sure it's ok to use compiler TLS in your case.

Yes, I was thinking of something that does not require a re-compile of the wx libraries so maybe a new function call (wx[MSW]DisableTLSAssert?) or, dare I say it, a wx system option.
What about the location of the assert, after OnInit has been called?

I thought about checking "something" from DllMain() but this has 2 problems: first I don't know what this "something" should be and second we don't even have DllMain(). And in fact we could be compiled as a static library and linked inside a DLL so this is not a good check at all finally.

I would have preferred DllMain too of course but since it was not there I opted for WinMain.

My last idea is to check _tls_index: as mentioned in Nynaeve's blog, it would be 0 in the problematic case but in vast majority of cases it is not actually 0 as any main program non trivial enough to load wx DLLs is almost certainly using TLS on its own. The only question is whether we can access it in a portable way, I'm not quite sure about this.

I tested for _tls_index at OnInit() and this works in the two cases I tried: With XP I have a _tls_index of 0 (crashes later on), Windows 7 has it set to 4 (no crash). It's probably not portable but will at least work for some versions of MSVC (tried with 9 only now). Also IIUC the assert will then work correctly in cases where the DLL was not loaded using LoadLibrary.

P.S. I'd definitely be interested in fixing the benchmarks with STL (I guess this was never tested before it became the default) but it's indeed a different issue.

Submitted as ticket #13134.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 2, 2013

2013-09-02 16:14:01: @vadz commented


I'm afraid I still have no idea about how to fix this :-( I tried adding the checks for _tls_index but this doesn't work for simple programs (like the minimal sample that I tested this with) that really don't use any TLS slots on their own, resulting in false positives. And we risk scaring newbies, who would start with something simple, with such warnings if we leave it like this. If we only give warnings when our own main() or WinMain() is not used, it should reduce their number, but it still seems likely that people will be surprised/scared by it.

OTOH this does create problems in practice, I just recently had a client whose DLL (linking with wx) was crashing under XP because of this. But the only solution is probably to wait for XP to die out...

If I or anybody else ever returns to work on this, another note: it looks like only VC8+ have _tls_index, at least I couldn't find any mention of it in VC7 CRT sources. So any checks would need to be restricted for these MSVC versions.

Also, here is the minimal check I added for testing:

#!diff
diff --git a/src/msw/main.cpp b/src/msw/main.cpp
index 65b4fc1..9d83265 100644
--- a/src/msw/main.cpp
+++ b/src/msw/main.cpp
@@ -69,6 +69,25 @@

 #if wxUSE_BASE

+// http://trac.wxwidgets.org/ticket/13116
+#ifdef __VISUALC__
+extern "C" DWORD _tls_index;
+
+static struct TlsCheck
+{
+    TlsCheck()
+    {
+        if ( !_tls_index )
+        {
+            wxLogDebug("WARNING: you may be loading wxWidgets dynamically, "
+                       "using ::LoadLibrary(). Compiler TLS support does not "
+                       "work reliably in this case, rebuild with "
+                       "wxUSE_COMPILER_TLS=0 to avoid crashes.");
+        }
+    }
+} gs_tlsCheck;
+#endif // __VISUALC__
+
 // ----------------------------------------------------------------------------
 // wrapper wxEntry catching all Win32 exceptions occurring in a wx program
 // ----------------------------------------------------------------------------

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Nov 22, 2013

2013-11-22 19:46:22: Trigve (Trigve) commented


I just run into this bug on the one machine using XP SP3. I'm dynamically loading the DLL. The problem is only with XPs. In the vista and newer it was fixed.
I've found this link [1] which talks about the problem and the solution. It looks like the problem couldn't be solved dynamically at link-time, I mean do something when we detect we're on xp and something other if we're at something new but use macro for defining TLS. Maybe with some inline functions? What do you think?

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms686997%28v=vs.85%29.aspx

Trigve

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Nov 23, 2013

2013-11-23 01:17:33: @vadz commented


Unfortunately I still don't know how to fix it and can't add much to what I wrote in the comment:7. If you have some idea of a solution, please post to wx-dev. I don't.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 1, 2014

2014-01-01 23:14:00: @softwarefailure commented


May I suggest that this problem is mentioned somewhere more prominently instead of just in wx/setup.h with 60kb of finetuning definitions which most people will only skim? I've just had this problem as well and it took me several hours to finally work out why the heck it crashed on Windows XP while working fine on newer Windowses.

I'd suggest to mention it in docs/msw/winxp.txt and also in the main docs/msw/install.txt as a special caution for people who plan to build DLLs containing wxWidgets. Mentioning this nasty issue in a more prominent place could really spare people a lot of hassle.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 2, 2014

2014-01-02 20:41:33: @vadz commented


Do people really read docs/msw/winxp.txt though? I guess we should indeed mention this there, just in case they do, but I think we mostly really need to find some way of detecting this problem at run-time.

Or maybe just sacrifice the runtime performance and disable compiler TLS support for as long as we support XP. Although this would be really a pity, especially for wxString code using TLS :-(

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 3, 2014

2014-01-03 16:23:40: @vadz commented


For further reference, here are the benchmark results with MSVC 2013:

% ./vc120_mswu/bench.exe /a 100 {wx,Win32,Compiler,Dummy}TLS
wxWidgets benchmarking program
Build: 3.1.0 (wchar_t,Visual C++ 1800,wx containers,compatible with 2.8)
Benchmarking wxTLS: 942ms total, 9.41 avg (min=6, max=14)
Benchmarking Win32TLS: 4771ms total, 47.69 avg (min=47, max=50)
Benchmarking CompilerTLS: 621ms total, 6.20 avg (min=6, max=7)
Benchmarking DummyTLS: 601ms total, 6.00 avg (min=6, max=7)

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 4, 2014

2014-01-04 21:41:21: @RobinD42 commented


See also this discussion on wx-dev: https://groups.google.com/d/msg/wx-dev/hIlUK1D3Hiw/qIuVYZ1RQJ4J

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 7, 2014

2014-01-07 22:54:43: @vadz changed status from confirmed to closed

2014-01-07 22:54:43: @vadz set resolution to fixed

2014-01-07 22:54:43: @vadz commented

(In [75568]) Disable the use of compiler TLS by default under Windows.

While compiler TLS support is simpler to use and much faster than using our
own Win32 API-based TLS implementation, it results in difficult to debug
crashes when used inside a dynamically loaded DLL under Windows XP, so disable
it by default to be safe.

Closes #13116.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 7, 2014

2014-01-07 22:57:35: @vadz changed status from closed to portneeded

2014-01-07 22:57:35: @vadz changed resolution from fixed to port to stable

2014-01-07 22:57:35: @vadz commented

Strangely enough, this seems ABI-compatible as we don't actually expose thread-specific variables anywhere, so it should be backported.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 7, 2014

2014-01-07 23:13:20: @tierra commented


Did you mean to remove the check for __WXOSX_IPHONE__ for wxUSE_PREFERENCES_EDITOR in 75568?

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jan 8, 2014

2014-01-08 01:39:09: @vadz commented


No, thanks for noticing this! Fixed in 75571.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Feb 2, 2014

2014-02-02 02:15:22: @vadz changed status from portneeded to closed

2014-02-02 02:15:22: @vadz changed resolution from port to stable to fixed

2014-02-02 02:15:22: @vadz commented

(In [75764]) Disable the use of compiler TLS by default under Windows.

While compiler TLS support is simpler to use and much faster than using our
own Win32 API-based TLS implementation, it results in difficult to debug
crashes when used inside a dynamically loaded DLL under Windows XP, so disable
it by default to be safe.

Closes #13116.

@wxtrac wxtrac closed this Feb 2, 2014
@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jul 2, 2016

2016-07-02 14:32:31: @vadz commented


In 26a58e9:
Don't use thread-safe statics with MSVS when targeting XP

Disable thread-safe initialization for static local variables in Visual Studio
2015 when XP toolset is used as this results in crashes when using DLLs under
XP, see #13116.

Closes #17403.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 29, 2016

2016-09-29 01:37:46: @vadz commented


In 2a21297:
Don't use thread-safe statics with MSVS when targeting XP

Disable thread-safe initialization for static local variables in Visual Studio
2015 when XP toolset is used as this results in crashes when using DLLs under
XP, see #13116.

See #17403.

See #305

(cherry picked from commit 26a58e9)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant