Re: [AD] dll_tls for MSVC

[Trent Gamblin <nooskewl@xxxxxxxxxx>]

Thanks, I applied all 3 of your patches.

Thanks,
Trent

-----Original Message-----
From: Markus Henschel [mailto:markus.henschel@xxxxxxxxxx
Sent: January 7, 2014 9:29 AM
To: alleg-developers@xxxxxxxxxx
Subject: Re: [AD] dll_tls for MSVC

> -----Original Message-----
> From: Trent Gamblin <nooskewl@xxxxxxxxxx>
> Sent: Mo 06.01.2014 20:28
> To: alleg-developers@xxxxxxxxxx
> Subject: Re: [AD] dll_tls for MSVC
> 
> > -----Original Message-----
> > From: Markus Henschel <markus.henschel@xxxxxxxxxx>
> > Sent: Mo 06.01.2014 18:43 
> > To: alleg-developers@xxxxxxxxxx
> > Subject: dll_tls for MSVC
> >
> > Happy New Year!
> >
> > I tried to use the dll-tls implementation of Allegro with MSVC. If I
enable
> > WANT_DLL_TLS in the CMAKE build it simply gets disabled again by this
> code
> > in CMakeLists.txt:
> >
> > if(MSVC)
> >     # MSVC never needs DLL TLS and it just confuses
> >     set(ALLEGRO_CFG_DLL_TLS 0)
> > .
> >
> > I think I have a case where I need dll-tls because I have to support
> > WindowsXP. In WindowsXP compiler supported TLS doesn't work for dlls
> that
> > are dynamically loaded at runtime using LoadLibrary/LoadLibraryEx. I get
> > chrashes as soon as the thread local storage is accessed.
> > http://support.microsoft.com/kb/118816/en-us
> >
> > Windows Vista and above don't have this problem.
> >
> > I'm using allegro with .NET so I cannot load the allegro dll implicitly
by the
> > import table of the executable. The only choice I have is using dll-tls.
Are
> > there any objections to removing the line mentioned above from
> > CMakeLists.txt so the dll-tls implementation can be used again with
MSVC?
> I
> > attached a patch for that. It also disables dll-tls by default so it is
only used if
> > explicitely requested or if there is no other choice (older mingw
versions).
> >
> > I noticed that the dll-tls implementation creates a thread_local_state
> object
> > for every thread created by the process after the allegro dll has been
> loaded.
> > This is inefficient because most threads probably won't need the object.
I
> > also observed memory leaks because some threads in my process are
> being
> > created after the allegro dll has been loaded but are still running
after the
> > allegro dll is unloaded. In this case the thread_local_state object
leaks. I
> fixed
> > this by doing lazy initialization of thread_local_state. In this case
only
> threads
> > that access the thread local object actually get one. There is still
potential
> for
> > memory leaks if one of these threads lives longer than the allegro dll
is still
> > loaded but this is much less likely. I also attached a patch for that.
> >
> > It would be nice if some of you could review/submit these patches.
> >
> > Thanks
> >
> > Markus Henschel
> 
> Are you sure non-DLL TLS works on MinGW dynamic builds? Can you point to
> some documentation that says so or test it? I may be being paranoid but it
> bothers me that it's not going to get tested.
> 
> The patch looks fine otherwise. I'll look at the other one and apply if
you can
> confirm the question about MinGW.
> 
> Thanks,
> Trent

I checked the source code in tls.c. There is this code:
***************
#if defined(ALLEGRO_MINGW32) && !defined(ALLEGRO_CFG_DLL_TLS)
   /*
    * MinGW < 4.2.1 doesn't have builtin thread local storage, so we
    * must use the Windows API.
    */
   #if __GNUC__ < 4
      #define ALLEGRO_CFG_DLL_TLS
   #elif __GNUC__ == 4 && __GNUC_MINOR__ < 2
      #define ALLEGRO_CFG_DLL_TLS
   #elif __GNUC__ == 4 && __GNUC_MINOR__ == 2 && __GNUC_PATCHLEVEL__ < 1
      #define ALLEGRO_CFG_DLL_TLS
   #endif
#endif
***************

The author of this seemed to believe that starting with Mingw 4.2.1 native
tls would work.

So I made a quick test with the current Mingw gcc version 4.8.1 and it seems
to work fine. I could run several examples including ex_threads.exe and
ex_threads2.exe without problems. I also ran the tests. There have been two
failures:

FAIL test png interlaced [sw] - hash=a0c383ef
FAIL test projection [hw] - RMS error is 47.1281

But I doubt this has anything to do with my changes. It's kinda hard to run
test tests and examples though because the executables are in a different
path than the dlls. 

I had another issue when compiling allegro with mingw though. The direct
sound backend didn't compile out of the box. I had compile errors about
various direct sound 8 types not being defined. The reason seems to be that
my DirectX 9 SDK (June 2010) dsound.h contains compile time checks for a
define called NTDDI_VERSION that specifies the minimum supported windows
version. This seems to be windows 2000 by default for Mingw. But for direct
sound 8 you need at least WindowsXP. So I copied some code from wwindow.c
that cures the issue by defining _WIN32_WINNT accordingly. I think some
windows header defines NTDDI_VERSION to the right value if _WIN32_WINNT is
defined beforehand. Patch is attached.

Regards,
Markus

P.S: Sorry for posting HTML to the list last time. I didn't realize that
before I read your reply. I reformatted the email to contain a clean
history.