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

Update BoringSSL to 6a2ccdcc2ed1d37a43a2183658d2ae61fd5ce208 #255

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

Lukasa
Copy link
Collaborator

@Lukasa Lukasa commented Sep 13, 2024

There have been a few changes in the BoringSSL build, so those are present in this commit too. Otherwise, this updates BoringSSL to 6a2ccdcc2ed1d37a43a2183658d2ae61fd5ce208.

@Lukasa Lukasa added the semver/patch No public API change. label Sep 13, 2024
Copy link
Contributor

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Changes to the vendor script look good. Didn't review every line of vendored code of course. Looks like the API break pipeline is just because the branch is outdated with main.


if(CMAKE_SYSTEM_NAME STREQUAL Darwin AND CMAKE_SYSTEM_PROCESSOR MATCHES "amd64|x86_64")
target_sources(CCryptoBoringSSL PRIVATE
crypto/chacha/chacha-x86_64-mac.mac.x86_64.S
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to all of the asm? And is this big if needed anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. It's still there, but I suspect the script is looking in the wrong place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah, so these are now generated by BoringSSL, and it expects the names to match.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

OOI what happens if they are missing? Presumably there's a lower-perf fallback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I think the compile would fail. You need NO_ASM set to fallback. We don't build the cmake version in CI here which is why we didn't notice.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a cmake CI job which did pass:
https://ci.swiftserver.group/job/swift-crypto-cmake-prb/306/

It builds the project with ninja:

command: /bin/bash -xcl "cmake -G Ninja -D CMAKE_BUILD_TYPE=Release -B out -S . && ninja -C out"

and it looks like we only set NO_ASM when building was WASI:

.define("OPENSSL_NO_ASM", .when(platforms: [Platform.wasi])),

This is a little surprising given the above...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's very curious.

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM

@glbrntt glbrntt merged commit 81bee98 into apple:main Sep 16, 2024
9 checks passed
@Lukasa Lukasa deleted the cb-update-boringssl-20240903 branch September 16, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants