Re: [ros-dev] [ros-diffs] [jgardou] 57171: [MINGWEX] - mark DllMain as a weak symbol for GCC. - supply a stubbed DllMain for MSVC. - DllMain is optional, and some DLLs don't implement it. That doesn't mean that they have no...

[Timo Kreuzer]

Hi,

I don't agree with these changes.

If we need a DllEntry in the lib, then we should use the one that is 
provided by mingw-w64 (all this code is 100% mingw-w64). It simply 
returns true (thats exactly what the one in MS CRT does) and works for 
gcc and MSVC.
Also what's the point of DisableThreadLibraryCalls() here? As you wrote 
yourself, no DllMain only means "I have nothing more to initialize than 
the CRT". But this disables the CRT thread initializion, too.
There is also no need for weak symbols, we can simply call the stub 
DllMain, since it doesn't (shouldn't) do anything.

Regards,
Timo

Am 27.08.2012 01:31, schrieb [e-mail]:
> Author: jgardou
> Date: Sun Aug 26 23:31:49 2012
> New Revision: 57171
>
> URL: http://svn.reactos.org/svn/reactos?rev=57171&view=rev
> Log:
> [MINGWEX]
> mark DllMain as a weak symbol for GCC.
> supply a stubbed DllMain for MSVC.
> DllMain is optional, and some DLLs don't implement it. That doesn't mean that they have no entry point, it means "I have nothing more to initialize than the CRT".
>
> Added:
>      trunk/reactos/lib/sdk/crt/startup/mscdllmain.c   (with props)
> Modified:
>      trunk/reactos/lib/sdk/crt/msvcrtex.cmake
>      trunk/reactos/lib/sdk/crt/startup/crtdll.c
>
> Modified: trunk/reactos/lib/sdk/crt/msvcrtex.cmake
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/msvcrtex.cmake?...
> ==============================================================================
> --- trunk/reactos/lib/sdk/crt/msvcrtex.cmake [iso-8859-1] (original)
> +++ trunk/reactos/lib/sdk/crt/msvcrtex.cmake [iso-8859-1] Sun Aug 26 23:31:49 2012
> @@ -66,7 +66,9 @@
>   endif()
>
> if(MSVC)
> list(APPEND MSVCRTEX_SOURCE startup/mscmain.c)
> list(APPEND MSVCRTEX_SOURCE
>    startup/mscmain.c
>    startup/mscdllmain.c)
> else()
>     list(APPEND MSVCRTEX_SOURCE startup/gccmain.c)
> endif()
>
> Modified: trunk/reactos/lib/sdk/crt/startup/crtdll.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/startup/crtdll....
> ==============================================================================
> --- trunk/reactos/lib/sdk/crt/startup/crtdll.c [iso-8859-1] (original)
> +++ trunk/reactos/lib/sdk/crt/startup/crtdll.c [iso-8859-1] Sun Aug 26 23:31:49 2012
> @@ -50,7 +50,19 @@
>
> extern int mingw_app_type;
>
> +/*
> It is possible that a DLL provides no DllMain entry point.
> Mark it as a weak symbol for GCC.
> Tests show that at link time, MSVC looks for a function first in the object files provided, and then
> in the libraries. This means that we must provide a basic implementation in msvcrtex, which will be used
> if none is found in the object files provided to link.exe.
> This also means that we can't rely on a DllMain function implemented in a static library when linking a DLL.
> */
>
> +#ifdef __GNUC__
> +extern WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason, LPVOID lpreserved) __attribute__((weak));
> +#else
>   extern WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason, LPVOID lpreserved);
> +#endif
>
> extern WINBOOL WINAPI DllEntryPoint (HANDLE, DWORD, LPVOID);
>
> @@ -198,10 +210,12 @@
>       }
>     if (dwReason == DLL_PROCESS_ATTACH)
>       __main ();
> retcode = DllMain(hDllHandle,dwReason,lpreserved);
> if(DllMain)
> retcode = DllMain(hDllHandle,dwReason,lpreserved);
> if (dwReason == DLL_PROCESS_ATTACH && ! retcode)
>  {
> DllMain (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> if(DllMain)
>  DllMain (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> DllEntryPoint (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
>  _CRT_INIT (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
>     }
>
> Added: trunk/reactos/lib/sdk/crt/startup/mscdllmain.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/startup/mscdllm...
> ==============================================================================
> --- trunk/reactos/lib/sdk/crt/startup/mscdllmain.c (added)
> +++ trunk/reactos/lib/sdk/crt/startup/mscdllmain.c [iso-8859-1] Sun Aug 26 23:31:49 2012
> @@ -1,0 +1,11 @@
> +#include <oscalls.h>
> +#define _DECL_DLLMAIN
> +#include <process.h>
>
> +WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason, LPVOID lpreserved)
> +{
> /* If the DLL provides no DllMain, then chances are that it doesn't bother with thread initialization */
> if(dwReason == DLL_PROCESS_ATTACH)
>    DisableThreadLibraryCalls(hDllHandle);
> return TRUE;
>
> +}
> Propchange: trunk/reactos/lib/sdk/crt/startup/mscdllmain.c
>  svn:eol-style = native

[Jérôme Gardou]

Hi.

Please see the inlined answers.

Timo Kreuzer a écrit :
> Hi,
>
> I don't agree with these changes.
>
> If we need a DllEntry in the lib, then we should use the one that is
> provided by mingw-w64 (all this code is 100% mingw-w64). It simply
> returns true (thats exactly what the one in MS CRT does) and works for
> gcc and MSVC.

There is DllEntryPoint in the code base, but no DllMain. Is it what you
are talking about?
> Also what's the point of DisableThreadLibraryCalls() here? As you wrote
> yourself, no DllMain only means "I have nothing more to initialize than
> the CRT". But this disables the CRT thread initializion, too.

You have a point. If nothing is given, then the programmer could assume 
that nothing happens.
Regarding CRT thread initialization: DisableThreadLibraryCalls means 
that the DLLs ahs nothing to initialize on thread creation, and that the 
loader can gain some performance in skipping this DLL. We have some Dlls 
that call it, and if this is a problem with our CRT, then this is bad.
Quoting MSDN:
"Do not call this function from a DLL that is linked to the static C 
run-time library (CRT). The static CRT requires DLL_THREAD_ATTACH and 
DLL_THREAD_DETATCH notifications to function properly"
Reading between the lines, that means that every CRT thread 
initialization is done in msvcrt.dll, and nowhere else. What we link 
statically to our DLLs shouldn't have anything to do on thread 
initializations.
> There is also no need for weak symbols, we can simply call the stub
> DllMain, since it doesn't (shouldn't) do anything.

As said in the comment, link.exe choses symbols defined in the object 
files it is given, and then in the libraries. So adding a stubbed 
version of dllmain in our library is harmless (unless someone puts his 
DllMain in a static library and links to that library.) I don't really 
know about ld behaviour regarding this case, and the 
"--allow-multiple-definition" option of ld (see 
http://sourceware.org/binutils/docs-2.22/ld/Options.html#Options ) 
scares me.
> Regards,
> Timo

Regards.
Jérôme
> Am 27.08.2012 01:31, schrieb [e-mail]:
> > Author: jgardou
> > Date: Sun Aug 26 23:31:49 2012
> > New Revision: 57171
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=57171&view=rev
> > Log:
> > [MINGWEX]
> > mark DllMain as a weak symbol for GCC.
> > supply a stubbed DllMain for MSVC.
> > DllMain is optional, and some DLLs don't implement it. That doesn't
> >
> > mean that they have no entry point, it means "I have nothing more to
> > initialize than the CRT".
> >
> > Added:
> >      trunk/reactos/lib/sdk/crt/startup/mscdllmain.c   (with props)
> > Modified:
> >      trunk/reactos/lib/sdk/crt/msvcrtex.cmake
> >      trunk/reactos/lib/sdk/crt/startup/crtdll.c
> >
> > Modified: trunk/reactos/lib/sdk/crt/msvcrtex.cmake
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/msvcrtex.cmake?...
> >
> > ==============================================================================
> >
> > --- trunk/reactos/lib/sdk/crt/msvcrtex.cmake [iso-8859-1] (original)
> > +++ trunk/reactos/lib/sdk/crt/msvcrtex.cmake [iso-8859-1] Sun Aug 26
> > 23:31:49 2012
> > @@ -66,7 +66,9 @@
> >   endif()
> >   if(MSVC)
> > list(APPEND MSVCRTEX_SOURCE startup/mscmain.c)
> > list(APPEND MSVCRTEX_SOURCE
> >    startup/mscmain.c
> >    startup/mscdllmain.c)
> > else()
> >     list(APPEND MSVCRTEX_SOURCE startup/gccmain.c)
> > endif()
> >
> > Modified: trunk/reactos/lib/sdk/crt/startup/crtdll.c
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/startup/crtdll....
> >
> > ==============================================================================
> >
> > --- trunk/reactos/lib/sdk/crt/startup/crtdll.c [iso-8859-1] (original)
> > +++ trunk/reactos/lib/sdk/crt/startup/crtdll.c [iso-8859-1] Sun Aug 26
> > 23:31:49 2012
> > @@ -50,7 +50,19 @@
> >   extern int mingw_app_type;
> > +/*
> > It is possible that a DLL provides no DllMain entry point.
> > Mark it as a weak symbol for GCC.
> > Tests show that at link time, MSVC looks for a function first in
> >
> > the object files provided, and then
> > in the libraries. This means that we must provide a basic
> >
> > implementation in msvcrtex, which will be used
> > if none is found in the object files provided to link.exe.
> > This also means that we can't rely on a DllMain function
> >
> > implemented in a static library when linking a DLL.
> > */
> >
> > +#ifdef __GNUC__
> > +extern WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason,
> > LPVOID lpreserved) __attribute__((weak));
> > +#else
> >   extern WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason,
> > LPVOID lpreserved);
> > +#endif
> >   extern WINBOOL WINAPI DllEntryPoint (HANDLE, DWORD, LPVOID);
> > @@ -198,10 +210,12 @@
> >       }
> >     if (dwReason == DLL_PROCESS_ATTACH)
> >       __main ();
> > retcode = DllMain(hDllHandle,dwReason,lpreserved);
> > if(DllMain)
> > retcode = DllMain(hDllHandle,dwReason,lpreserved);
> > if (dwReason == DLL_PROCESS_ATTACH && ! retcode)
> >  {
> > DllMain (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> > if(DllMain)
> >  DllMain (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> > DllEntryPoint (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> > _CRT_INIT (hDllHandle, DLL_PROCESS_DETACH, lpreserved);
> > }
> >
> > Added: trunk/reactos/lib/sdk/crt/startup/mscdllmain.c
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/startup/mscdllm...
> >
> > ==============================================================================
> >
> > --- trunk/reactos/lib/sdk/crt/startup/mscdllmain.c (added)
> > +++ trunk/reactos/lib/sdk/crt/startup/mscdllmain.c [iso-8859-1] Sun
> > Aug 26 23:31:49 2012
> > @@ -1,0 +1,11 @@
> > +#include <oscalls.h>
> > +#define _DECL_DLLMAIN
> > +#include <process.h>
> >
> > +WINBOOL WINAPI DllMain (HANDLE hDllHandle, DWORD dwReason, LPVOID
> > lpreserved)
> > +{
> > /* If the DLL provides no DllMain, then chances are that it
> >
> > doesn't bother with thread initialization */
> > if(dwReason == DLL_PROCESS_ATTACH)
> >    DisableThreadLibraryCalls(hDllHandle);
> > return TRUE;
> >
> > +}
> > Propchange: trunk/reactos/lib/sdk/crt/startup/mscdllmain.c
> >  svn:eol-style = native
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Timo Kreuzer]

Am 27.08.2012 15:45, schrieb Jérôme Gardou:
> Hi.
>
> Please see the inlined answers.
>
> Timo Kreuzer a écrit :
> > Hi,
> >
> > I don't agree with these changes.
> >
> > If we need a DllEntry in the lib, then we should use the one that is
> > provided by mingw-w64 (all this code is 100% mingw-w64). It simply
> > returns true (thats exactly what the one in MS CRT does) and works for
> > gcc and MSVC.
>
> There is DllEntryPoint in the code base, but no DllMain. Is it what you
> are talking about?

No, I was talking about DllMain. It is not in our codebase, but in 
mingw-w64. I even have the file in my working copy, probably from the 
last sync, but I didn't commit it for whatever reason.
> > Also what's the point of DisableThreadLibraryCalls() here? As you wrote
> > yourself, no DllMain only means "I have nothing more to initialize than
> > the CRT". But this disables the CRT thread initializion, too.
>
> You have a point. If nothing is given, then the programmer could 
> assume that nothing happens.
> Regarding CRT thread initialization: DisableThreadLibraryCalls means 
> that the DLLs ahs nothing to initialize on thread creation, and that 
> the loader can gain some performance in skipping this DLL. We have 
> some Dlls that call it, and if this is a problem with our CRT, then 
> this is bad.
> Quoting MSDN:
> "Do not call this function from a DLL that is linked to the static C 
> run-time library (CRT). The static CRT requires DLL_THREAD_ATTACH and 
> DLL_THREAD_DETATCH notifications to function properly"
> Reading between the lines, that means that every CRT thread 
> initialization is done in msvcrt.dll, and nowhere else. What we link 
> statically to our DLLs shouldn't have anything to do on thread 
> initializations.

In fact, MS uses 2 versions of DllMain. One in the static crt and one in 
msvcrt.lib. And the latter does in fact call DisableThreadLibraryCalls. 
So you are right.
The DllMainCRTStartup from mingw-w64 handles them, but the code is so 
***** that I didn't see, that this runs into nowhere and it does not do 
anything with them.
We just need to make sure this DllMain is not included in our static 
CRT, even though that one is probably not usable for compiling a proper 
dll anyway.
> > There is also no need for weak symbols, we can simply call the stub
> > DllMain, since it doesn't (shouldn't) do anything.
>
> As said in the comment, link.exe choses symbols defined in the object 
> files it is given, and then in the libraries. So adding a stubbed 
> version of dllmain in our library is harmless (unless someone puts his 
> DllMain in a static library and links to that library.) I don't really 
> know about ld behaviour regarding this case, and the 
> "--allow-multiple-definition" option of ld (see 
> http://sourceware.org/binutils/docs-2.22/ld/Options.html#Options ) 
> scares me.

That is normal and expectable linker behaviour. LD works the same way 
(unless it's totally fubar). The function from the lib does not get 
included, if it's present in the main object files. (This only works on 
a per file basis. So when the function was in the same compilation unit 
as a second function that was linked into the dll, then the function 
would also be forced in and it would result in an error about multiple 
definitons.)
So under the circumstance that the programmer is sane and doesn't put 
his DllMain into a static library, you can happily add a stubbed version.

Therefore I suggest:
- Add the DllMain for both GCC and MSVC
- Don't use a weak symbol and do not check "if (DllMain)" and instead 
keep the original code unmodified

Please don't make unneccessary modifications to 3rd party code, based on 
the fear, that LD *might* be broken.

Thanks,
Timo