[C] Fixes for Windows #789
Conversation
These functions are expected to return a value between 0 and 1 but were returning a value between 0 and RAND_MAX. RAND_MAX + 1 is known not to overflow on MSVC.
The callback does not have the correct signature which is expected by the InitOnceExecuteOnce function.
strdup and free need to be called from the same binary in order to ensure the same heap is used on malloc and free.
CMake v3.13 no longer includes the /FC option which makes __FILE__ lower case and absolute. This adjusts the unit test.test See https://gitlab.kitware.com/cmake/cmake/issues/18261
It manages some data which is only used from the DllMain function itself, therefore it serves no purpose.
|
|
||
| // DllMain() is the entry-point function for this DLL. | ||
|
|
||
| BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved) |
There was a problem hiding this comment.
It does nothing useful, and wastes a TLS slot.
This code is basically taken from the docs example but unlike the example it does not use dwTlsIndex anywhere outside of DllMain. So basically DllMain manages a TLS slot and some per-thread memory just for itself. The other TLS code does not need this.
There was a problem hiding this comment.
Actually, I think DllMain will be necessary. Currently, the error handling is broken in tests due to the TLS state not working. And that is partially due to it not being wired in correctly with DllMain or handling static libs correctly.
There was a problem hiding this comment.
A correct DllMain should use the existing slot and make the checks work. The changes to the TLS handling may work for static libs. Should write a test to make sure error handling is actually correct for threading, etc.
There was a problem hiding this comment.
I actually fixed the error handling in this PR, it was previously causing one of the unit tests to fail because aeron_thread_once was not working properly.
While we could make the TLS slot be managed by DllMain, it would break TLS if the media driver is used as a static library, since those don't use DllMain, and it would create the need to maintain different code paths for Windows and Linux.
I think the TLS code from this PR is fine, and it doesn't need a DllMain function to work properly, unless I'm misunderstanding something.
There was a problem hiding this comment.
And as you point out it is needed for static libs.
Not exactly, I just said that DllMain is not called in static libs, which would require a different initialization path if you want to support them.
But since you have plans to require DllMain soon, I suppose that using the media driver as a static library will not be possible anymore in the future, right?
I could move the TLS slot handling to DllMain but my understanding is that it will make retrieving the error messages impossible in a static library on Windows as a consequence, unless we add the currently existing code in an #if STATIC_LIBRARY or something similar.
There was a problem hiding this comment.
This can be done, I think, quite easily. A static set to indicate that TlsAlloc has been called by DllMain means that aeron_thread_key_create doesn't need to call it. If not, then it can call TlsAlloc. All of this can be hidden in there. So, this shouldn't require anything weird. The use of TlsSetValue can still use the existing paths and created on demand.
If you are not comfortable about this or do not quite understand what I am suggesting. Just keep the DllMain and remove the TLS calls from it and keep the rest as you fixed it. I'll fix DllMain up when I have time.
In terms of requiring DllMain. Only the use of specific options may (or may not). Eventually, we would like to do DPDK, RIO, and other specific support. Some of that will be easier to integrate with DllMain than without.
There was a problem hiding this comment.
BTW, in a static lib, won't the error state be a leak when the thread goes away? Meaning to avoid the leak, DLL would be a better idea? (I've not looked for a hook to catch thread death for a while)...
There was a problem hiding this comment.
Ok I think I understand what you're suggesting now. I'll implement that tomorrow.
Eventually, we would like to do DPDK, RIO, and other specific support.
That's great! 👍
Some of that will be easier to integrate with DllMain than without.
I figured an empty DllMain wouldn't be useful and you could add it back at a later time when needed. Anyway, I'll add it back with specific support for error messages.
BTW, in a static lib, won't the error state be a leak when the thread goes away?
Yes, it will leak. That can be mitigated from DllMain. But if I understand correctly, it also means there's currently a leak on Linux.
There was a problem hiding this comment.
On Linux it doesn't leak because pthread_key_create takes free as the destructor arg. Which should be aeron_free, but sigh...
|
BTW would you accept a PR which adds a Windows CI, using something like GitHub Actions for instance (so it won't require you to setup any external account)? If yes, would you like to also migrate the Linux CI from Travis to GitHub Actions for consistency? |
|
Migrating to GitHub Actions is on my list of things to try. Travis has been very frustrating. |
|
I've reverted so it can done in parts which are easily agreed. |
|
Everything but the DllMain and the removal of aeron_free from the test are OK. |
This fixes various things on Windows, and makes the CMake build and tests pass with VS 2019.
Every commit message explains the introduced changes in more detail.