Opened 3 years ago

Closed 8 months ago

#13116 closed defect (fixed)

Thread Local Storage does not work with DLLs loaded using LoadLibrary

Reported by: disc Owned by:
Priority: normal Milestone: 3.0.1
Component: wxMSW Version: stable-latest
Keywords: WinXP TLS DLL Cc:
Blocked By: Blocking:
Patch: no

Description

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.

Change History (18)

comment:1 follow-up: Changed 3 years ago by vadz

  • Status changed from new to confirmed

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.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by disc

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

Replying to 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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by vadz

Replying to 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?

comment:4 in reply to: ↑ 3 Changed 3 years ago by disc

Replying to vadz:

Replying to 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).

comment:5 follow-up: Changed 3 years ago by 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.

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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by disc

Replying to 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.

comment:7 Changed 13 months ago by vadz

  • Keywords WinXP added
  • Milestone 3.0 deleted

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:

  • src/msw/main.cpp

    diff --git a/src/msw/main.cpp b/src/msw/main.cpp
    index 65b4fc1..9d83265 100644
    a b  
    6969 
    7070#if wxUSE_BASE 
    7171 
     72// http://trac.wxwidgets.org/ticket/13116 
     73#ifdef __VISUALC__ 
     74extern "C" DWORD _tls_index; 
     75 
     76static struct TlsCheck 
     77{ 
     78    TlsCheck() 
     79    { 
     80        if ( !_tls_index ) 
     81        { 
     82            wxLogDebug("WARNING: you may be loading wxWidgets dynamically, " 
     83                       "using ::LoadLibrary(). Compiler TLS support does not " 
     84                       "work reliably in this case, rebuild with " 
     85                       "wxUSE_COMPILER_TLS=0 to avoid crashes."); 
     86        } 
     87    } 
     88} gs_tlsCheck; 
     89#endif // __VISUALC__ 
     90 
    7291// ---------------------------------------------------------------------------- 
    7392// wrapper wxEntry catching all Win32 exceptions occurring in a wx program 
    7493// ---------------------------------------------------------------------------- 

comment:8 Changed 10 months ago by Trigve

  • Keywords TLS DLL added

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

comment:9 Changed 10 months ago by vadz

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.

comment:10 Changed 9 months ago by afalkenhahn

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.

comment:11 Changed 9 months ago by vadz

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 :-(

comment:12 Changed 9 months ago by vadz

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)

comment:14 Changed 9 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(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.

comment:15 Changed 9 months ago by vadz

  • Milestone set to 3.0.1
  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

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

comment:16 Changed 9 months ago by bpetty

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

comment:17 Changed 9 months ago by vadz

No, thanks for noticing this! Fixed in r75571.

comment:18 Changed 8 months ago by VZ

  • Resolution changed from port to stable to fixed
  • Status changed from portneeded to closed

(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.

Note: See TracTickets for help on using tickets.