Avoid use of _umul128 on Windows ARM64 (VC-WIN64-ARM)#20235
Avoid use of _umul128 on Windows ARM64 (VC-WIN64-ARM)#20235jbekkema wants to merge 1 commit intoopenssl:masterfrom thesparklabs:vs-win64-arm-umul128
Conversation
_umul128 is unavailable for ARM64 when building using Visual Studio, causing a linker failure when attempting to build for VC-WIN64-ARM. This limits the use of _umul128 to AMD64 builds.
openssl-machine
added
the
hold: cla required
|
The |
paulidale
added
branch: master
paulidale
left a comment
paulidale
left a comment
There was a problem hiding this comment.
Happy to call this trivial.
github-actions
bot
added
the
severity: fips change
levitte
added
approval: done
|
CI failure seems irrelevant but needs to be fixed. Do we consider this fix as urgent? |
It is not a CI breakage, so no, not urgent. |
|
We can amend the commit message when merging. I am OK with CLA: trivial for this. |
t8m
removed
hold: cla required
|
@paulidale There is no VC-WIN64-ARM target on 1.0.2. I do not think this needs to be backported there. |
|
|
I don't believe this is the case. The BN_BYTES == 8 confirms it is a 64 bit platform (so x86 is fine), and the _MSC_VER limits it to Visual Studio builds only. I can confirm x86 builds fine without the fix. I have not tested ARM32. |
|
Yes, x86 32 bit is okay. My mistake. This only impacts AARCH64. |
tomato42
left a comment
tomato42
left a comment
There was a problem hiding this comment.
r-, the correct fix is to change the code to use __umulh instead
|
I've proposed an alternative fix in #20244 |
|
@tomato42 This pull request that specifically and narrowly includes one supported/adopted platform and thus keeping other platforms from attempting to use an unsupported intrinsic is, IMO, the better, more immediate solution to getting the build fixed and working even if it doesn't result in the most performant solution. This minor change has also already been signed off on. I think getting the build working for the target is a higher priority while performance related adjustments like your proposed change can be applied in a future release. |
|
@slproweb: I'm the original author of the buggy code. The proper fix, that handles both X64 and ARM64 is not really complex, and the difference in performance of the fallback code vs native instructions is a factor of 5 to 10 (if ARM64 is anything like x86_64 CPUs), you really don't want to use it. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
openssl/openssl#20234 are fixed and released
|
Looks like #20244 has been merged in preference. Closing this. |
10 participants
_umul128 is unavailable for ARM64 when building using Visual Studio, causing a linker failure when attempting to build for VC-WIN64-ARM. This limits the use of _umul128 to AMD64 builds.
This applies to both OpenSSL 3.0 and OpenSSL 1.1.1.
CLA: trivial