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

Avoid possible race when selecting nibble2base implementation #1786

Merged
merged 1 commit into from
May 30, 2024

Conversation

daviesrob
Copy link
Member

Selecting which version of nibble2base() to use on first call is fine in a single thread, but may lead to a race in multi-threaded code. While this is likely harmless (the value stored to the function pointer will always be the same, and the update will probably be a single store), it is best to avoid the problem by making the selection at library initialisation, before any threads have started.

As the optimised nibble2base_ssse3() already uses gcc function attributes, it seems reasonable to use attribute((constructor)) on the function that selects the version to use. If the constructor does not run, it will use the non-optimised version, which is a safe default.

Selecting which version of nibble2base() to use on first call
is fine in a single thread, but may lead to a race in
multi-threaded code.  While this is likely harmless (the value
stored to the function pointer will always be the same, and the
update will probably be a single store), it is best to avoid
the problem by making the selection at library initialisation,
before any threads have started.

As the optimised nibble2base_ssse3() already uses gcc function
attributes, it seems reasonable to use __attribute__((constructor))
on the function that selects the version to use.  If the
constructor does not run, it will use the non-optimised version,
which is a safe default.
@jkbonfield jkbonfield merged commit 5d31868 into samtools:develop May 30, 2024
9 checks passed
@daviesrob daviesrob deleted the nibble2base_resolve branch May 31, 2024 08:14
@rhpvorderman
Copy link
Contributor

Thanks for looking into this. Doing the dynamic update at load makes more sense than doing it at function call. I will use this in my code as well from now on.

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.

3 participants