Fix windows mt thread resize bug with CI - draft#3370
Closed
yoniko wants to merge 9 commits intofacebook:devfrom
Closed
Fix windows mt thread resize bug with CI - draft#3370yoniko wants to merge 9 commits intofacebook:devfrom
yoniko wants to merge 9 commits intofacebook:devfrom
Conversation
In commit 031de3c some code was added that returned a boolean, but was treated as if it returned a dependency object. This wasn't tested and could not work. Moreover, zstd no longer built at all unless the entire programs directory was disabled and not even evaluated. Fix the return type checking.
In commit 031de3c a feature of Meson 0.50.0 was added, but the minimum specified version of Meson is 0.48.0. Meson therefore emitted a warning: WARNING: Project targets '>=0.48.0' but uses feature introduced in '0.50.0': required arg in compiler.has_header. And if anyone actually used Meson 0.48.0 to build with, it would error out with mysterious claims that the build file itself is invalid, rather than telling the user to install a newer version of Meson. Solve this by bumping the minimum version to align with reality. This e.g. drops support for Debian oldstable (buster)'s packaged version of Meson, but still works if backports are enabled, or if the user can `pip install` a newer version.
It's entirely possible some people don't have valgrind installed, but still want to run the tests. If they don't have it installed, then they probably don't intend to run those precise test targets anyway. Also, this solves an error when running the tests in an automated environment. The valgrind tests have a hard dependency on behavior such as `./zstd` erroring out with the message "stdin is a console, aborting" which does not work if the automated environment doesn't have a console. As a rough heuristic, automated environments lacking a console will *probably* also not have valgrind, so avoiding that test definition neatly sidesteps the issue. Also, valgrind is not easily installable on macOS, at least homebrew says it isn't available there. This makes it needlessly hard to enable the testsuite on macOS.
playTests.sh has an option to run really slow tests. This is enabled by default in Meson, but what we really want is to do like the Makefile, and run the fast ones by default, but with an option to run the slow ones instead.
Travis is no longer run, but this wasn't ported to something else.
There are a couple of oddities here. We don't attempt to build e.g. contrib, because that doesn't seem to work at the moment. Also notice that each command is its own step. This happens because github actions runs in powershell, which doesn't seem to let you abort on the first failure.
1. If threads are resized the threads' `ZSTD_pthread_t` might move while the worker still holds a pointer into it (see more details in facebook#3120). 2. The join operation was waiting for a thread and then return its `thread.arg` as a return value, but since the `ZSTD_pthread_t thread` was passed by value it would have a stale `arg` that wouldn't match the thread's actual return value. This fix changes the `ZSTD_pthread_join` API and removes support for returning a value. This means that we are diverging from the `pthread_join` API and this is no longer just an alias. In the future, if needed, we could return a Windows thread's return value using `GetExitCodeThread`, but as this path wouldn't be excised in any case, it's preferable to not add it right now.
Contributor
Author
|
This is just a draft PR that merges the added Meson CI PR with the Windows MT fixes PR. |
When spawning a Windows thread we have small worker wrapper function that translates between the interfaces of Windows and POSIX threads. This wrapper is given a pointer that might get stale before the worker starts running, resulting in UB and crashes. This commit adds synchronization so that we know the wrapper has finished reading the data it needs before we allow the main thread to resume execution.
Contributor
Author
|
No longer needed, closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.