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

Fix processor detection in CMakeLists.txt #909

Merged
merged 4 commits into from
Oct 29, 2023
Merged

Conversation

jiegec
Copy link
Contributor

@jiegec jiegec commented Sep 4, 2023

CMAKE_SYSTEM_NAME is set to the OS name, e.g. Linux. It should check CMAKE_SYSTEM_PROCESSOR instead.

The regex is wrong: [i?86|x86] will matching anything containing 6, so it will take the wrong branch for many ISAs, e.g. riscv64 and even ARM64 when it is uppercased. It eventually leads to wrong DES encryption due to wrong sizeof(DES_LONG).

Handle cases when CMAKE_SYSTEM_PROCESSOR is upper-cased, e.g. in windows.

@jiegec jiegec force-pushed the master branch 3 times, most recently from 2501f04 to d349eb9 Compare September 4, 2023 01:34
@jiegec
Copy link
Contributor Author

jiegec commented Sep 4, 2023

CI failed in windows:

Building Custom Rule D:/a/portable/portable/crypto/CMakeLists.txt
  Assembling D:\a\portable\portable\crypto\aes\aes-masm-x86_64.S...
D:\a\portable\portable\crypto\aes\aes-masm-x86_64.S(1840): error A2189: invalid combination with segment alignment : 64 [D:\a\portable\portable\build\crypto\crypto_obj.vcxproj]
D:\a\portable\portable\crypto\aes\aes-masm-x86_64.S(2230): error A[21](https://github.com/libressl/portable/actions/runs/6068067965/job/16460548254?pr=909#step:6:22)89: invalid combination with segment alignment : 64 [D:\a\portable\portable\build\crypto\crypto_obj.vcxproj]
D:\a\portable\portable\crypto\aes\aes-masm-x86_64.S(2624): error A2189: invalid combination with segment alignment : 64 [D:\a\portable\portable\build\crypto\crypto_obj.vcxproj]

I don't have Windows environment to test.

@vszakats
Copy link
Contributor

vszakats commented Sep 6, 2023

Another case is Universal macOS builds, setup with e.g.
CMAKE_OSX_ARCHITECTURES=arm64;x86_64.

I don't think the current build layout can handle that, and matching one
of the platforms seems wrong as well. Possibly ASM should be disabled
with multiple values there. I haven't made tests though.

It'd be useful to make that work out of the box. For that, the CPU
selection should probably be delegated to the C preprocessor by
#including the necessary ASM code into C at compile-time, while
keeping the set of source files the same, regardless of target CPU.

busterb added a commit to busterb/portable that referenced this pull request Oct 2, 2023
@busterb busterb self-assigned this Oct 29, 2023
@busterb
Copy link
Contributor

busterb commented Oct 29, 2023

Hi, thanks for the PR. I think what you are doing here is correct; the Windows failure is due to it actually fixing the detection of processor and enabling MASM assembly in the build, which is coincidentally broken. I'll add a patch to this to work around that and fix the MASM in another patch set so this applies cleanly on its own.

@busterb busterb merged commit 8048941 into libressl:master Oct 29, 2023
33 checks passed
busterb added a commit that referenced this pull request Nov 2, 2023
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