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

Fix TLS segfault in windows static build. #222

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mcmtroffaes
Copy link

I noticed a segfault when using avs+ windows static build, which I managed to reproduce with this simple sample program:

#include <iostream>
#include <avisynth_c.h>

int main(int argc, char **argv) {
    auto *env = avs_create_script_environment(3);
    if (env == nullptr) {
        std::cerr << "Failed to create AVS environment" << std::endl;
        return -1;
    }
    avs_delete_script_environment(env);
    return 0;
}

I traced the segfault to this location in the code:

https://github.com/AviSynth/AviSynthPlus/blob/master/avs_core/core/avisynth.cpp#L2224

The dwTlsIndex variable is set during DllMain:

https://github.com/AviSynth/AviSynthPlus/blob/master/avs_core/core/main.cpp#L285

which is obviously not called when the static library is built. The patch moves TlsAlloc/TlsFree to the constructor/destructor of a statically initialized object instead. A simpler solution might be to simply drop XP support and rely on __declspec(thread) instead, but I guess that might be more contentious.

@pinterf
Copy link

pinterf commented Jun 22, 2021

That XP_TLS can even be defined conditionally, triggered when v141_xp is chosen as platform, that is USING_V110_SDK71 is defined.

@mcmtroffaes
Copy link
Author

That XP_TLS can even be defined conditionally, triggered when v141_xp is chosen as platform, that is USING_V110_SDK71 is defined.

Sure, I've added a commit to that effect, although I can't test it myself.

@mcmtroffaes
Copy link
Author

Anything else needed here? Looking at the code again, I guess DllMain could be removed completely now if desired.

@pinterf
Copy link

pinterf commented Oct 13, 2021

I commited my XP_TLS usage proposal as a separate commit in 1eebe2e
Returning to the main part of the commit, its benefit would be to enable static build with XP compatible builds?

@mcmtroffaes
Copy link
Author

I commited my XP_TLS usage proposal as a separate commit in 1eebe2e

Thanks!

Returning to the main part of the commit, its benefit would be to enable static build with XP compatible builds?

Yes, that's correct (or rather, fix a segfault that currently occurs when an XP compatible build is used; static builds are technically already possible).

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

Successfully merging this pull request may close these issues.

2 participants