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

threads_win: fix build error with VS2010 x86 #24405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

httpstorm
Copy link
Contributor

InterlockedAnd64 and InterlockedAdd64 are not available on VS2010 x86. We already have implemented replacements for other functions, such as InterlockedOr64. Apply the same approach to fix the errors. A CRYPTO_RWLOCK rw_lock is added to rcu_lock_st.

Note that the return values of the replacement atomic functions for VS2010 x86 are not checked, because I did not know how to respond to these errors in each location where they are used. This might happen if the lock is not initialised yet. Though the lock is initialised in the beginning when each object is constructed, so this should not occur.

Replace InterlockedOr64 and InterlockedOr with CRYPTO_atomic_load and CRYPTO_atomic_load_int, using the existing design pattern.

Add documentation and tests for the new atomic functions CRYPTO_atomic_add64, CRYPTO_atomic_and

Note:
An alternative or addition to this PR would be to implement InterlockedAdd64, InterlockedOr64, and InterlockedAnd64 in x86 assembly as described here #24109 . This will make them lockless and improve performance on x86 code compiled with VS2010. Please let me know which path is preferred?

  1. If we apply only this PR as is, all code will be using the wrappers: we get consistency, but code compiled with VS2010 for x86 will use locks in atomic functions, which is way slower than lockless atomics.
  2. If we apply this PR and then add the assembly functions, all code will be using the wrappers: we get consistency at the small cost of an additional function call and higher complexity of the changes here. Lockless atomics.
  3. If we implement only the assembly functions we get slightly better performance than (2), and there will be less changes. Lockless atomics.

Fixes:

libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedAdd64 referenced in function _get_hold_current_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedOr referenced in function _get_hold_current_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedAnd64 referenced in function _update_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedOr64 referenced in function _ossl_synchronize_rcu

Fixes: #24326 the original PR that was eventually split in 3 parts.

Build and run tested: Windows 2003 x64, Windows 2003 x86, Visual Studio 2010.
@t8m @tom-cosgrove-arm @paulidale @nhorman @shahsb @levitte @mattcaswell @hlandau @viruscamp @yjh-styx

Checklist
  • documentation is added or updated
  • tests are added or updated

@github-actions github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels May 15, 2024
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels May 15, 2024
@httpstorm httpstorm force-pushed the threads_win-vs2010.x86.2024-05-15 branch from 615012f to c0acf80 Compare May 27, 2024 20:46
@httpstorm
Copy link
Contributor Author

@t8m
Is there anything else you want me to do or change?
I rebased the PR on master and now all tests pass. Any idea why there was a failing test two weeks ago? Also I keep getting many mails about failing tests from the CI each day, and I don't understand the reason or purpose, since they are unrelated to my PR.

InterlockedAnd64 and InterlockedAdd64 are not available on VS2010 x86.
We already have implemented replacements for other functions, such as
InterlockedOr64. Apply the same approach to fix the errors.
A CRYPTO_RWLOCK rw_lock is added to rcu_lock_st.

Replace InterlockedOr64 and InterlockedOr with CRYPTO_atomic_load and
CRYPTO_atomic_load_int, using the existing design pattern.

Add documentation and tests for the new atomic functions
CRYPTO_atomic_add64, CRYPTO_atomic_and

Fixes:
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedAdd64 referenced in function _get_hold_current_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedOr referenced in function _get_hold_current_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedAnd64 referenced in function _update_qp
libcrypto.lib(libcrypto-lib-threads_win.obj) : error LNK2019: unresolved external symbol _InterlockedOr64 referenced in function _ossl_synchronize_rcu

Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
@httpstorm httpstorm force-pushed the threads_win-vs2010.x86.2024-05-15 branch from c0acf80 to 1f03752 Compare May 28, 2024 08:47
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer branch: master Merge to master branch severity: ABI change This pull request contains ABI changes severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants