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

added inline assembly to patch the problem #24221

Closed
wants to merge 1 commit into from

Conversation

lamosekm
Copy link

This pull request addresses a build issue with OpenSSL on Visual Studio versions prior to 1929 (VS2019). The problem was that older MSVC versions lacked inline assembly implementations for certain interlocked functions, namely InterlockedAdd64, InterlockedOr64, and InterlockedAnd64.

The proposed changes introduce:

  • Inline assembly code to provide InterlockedAdd64, InterlockedOr64, and InterlockedAnd64 functions.
  • A conditional preprocessor directive to check for MSVC versions prior to 1929 and define the inline keyword as __inline for compatibility.
  • A definition for InterlockedOr to be _InterlockedOr, ensuring consistent function naming across different versions of MSVC.

These changes ensure that OpenSSL can be built using older versions of the MSVC compiler, thus maintaining backward compatibility, especially for systems that still rely on older development environments.

Testing:

The changes have been compiled and tested on Visual Studio 2017, confirming that the build succeeds without errors and the functions perform as expected. The inline assembly has been written to be compatible with the calling conventions and requirements of the older compilers.

By merging this PR, OpenSSL will gain improved compatibility with projects that must maintain legacy systems.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Apr 21, 2024
@lamosekm
Copy link
Author

@bbbrumley hey professor, here is my first Software Engineering Assignment. I still need to do one more to make up for those 3 missed quizzes from that one week. I will begin working on that shortly. This was a first for me, so apologies if some of it isn't 100% correct. Thanks, Mitch.

mov ecx, edx
add ebx, [esp+8+4] ; Add low part of _Val
adc ecx, [esp+8+8] ; Add high part of _Val with carry
lock cmpxchg8b qword ptr [esi] ; Compare and exchange if unchanged
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to work for 32 bit windows platforms? I feel like it might be safer for older platforms to use the fallback approach that pthreads uses (i.e. using an internal mutex)

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Apr 24, 2024
@t8m
Copy link
Member

t8m commented Apr 24, 2024

We will need CLA submitted to be able to take this contribution. https://www.openssl.org/policies/cla.html

@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 Apr 24, 2024
@bbbrumley
Copy link
Contributor

This is somehow related to #24209 right?

@tom-cosgrove-arm
Copy link
Contributor

Actually, I don't think we should accept this PR. It's not just that the commit message "added inline assembly to patch the problem" needs re-writing, but as @bbbrumley notes, the body of the commit (the assembly) is copied directly from #24209 without attribution to the original author @yjh-styx. Only whitespace and comments have been changed. Unless @lamosekm and @yjh-styx are the same person, this is not acceptable to my mind.

(Once a PR has been merged, the original author's name is associated with that code in git, and changes to comments and whitespace can be accepted separately. Offering up someone else's work ostensibly as one's own is definitely not acceptable, and the author of this PR makes no reference to the original PR where the assembly comes from.)

@t8m
Copy link
Member

t8m commented Apr 25, 2024

Closing this.

@t8m t8m closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch hold: cla required The contributor needs to submit a license agreement severity: ABI change This pull request contains ABI changes severity: fips change The pull request changes FIPS provider sources triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants