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 building with latest BoringSSL #2230

Merged
merged 3 commits into from
May 3, 2024
Merged

Conversation

davidben
Copy link
Contributor

@davidben davidben commented May 1, 2024

This required two fixes. First, test_verify_param_set_depth_fails_verification needed to be updated. The history here is that OpenSSL 1.1.0 made a backwards-incompatible change to the semantics of the depth limit. Ideally, rust-openssl would have documented the semantics of its own APIs and normalized the behavior, but it instead silently picked up the semantics change.

BoringSSL aligned with OpenSSL's new behavior in
b251d813ec615e7ef01d82073f94960eb13b1e0a, but since then rust-openssl has codified the old behavior in tests. We need to update some cfgs to reflect this.

Second, BoringSSL requires a C++ runtime (we have required a C++ compiler for a long time). This reveals a problem in Cargo's dependency management strategy for externally-built static libraries. Work around this by making some guesses about what library to link in, but see the comment for why this is unsafe. This unsafety appears to be inherent to Cargo and the choice of having Cargo drive cross-language builds, rather than providing hooks for an integrated build system.

@alex
Copy link
Collaborator

alex commented May 2, 2024

Looks like the i686 build needs to be updated with -msse2 in the right place.

@davidben
Copy link
Contributor Author

davidben commented May 2, 2024

Ah fun. @botovq was trying to convince me earlier that BoringSSL's cmake build should automatically set that. I liked that making the caller do it helped surface the issue and make them need to opt in that "yes, we are okay assuming sse2 support". But now that it's been a while and come up twice, yeah maybe we should just do it automatically now.

@alex
Copy link
Collaborator

alex commented May 2, 2024 via email

@davidben
Copy link
Contributor Author

davidben commented May 2, 2024

Ended up setting it in CI because it (looked like) the place to update was very easy. Also given rust-lang/rust#82435, you all probably should be setting it across all OpenSSLs anyway. If your Rust code believes in SSE2, no sense denying your CI the extra registers on the C half!

@davidben
Copy link
Contributor Author

davidben commented May 2, 2024

Hmm, CI seems to now be upset about OpensslCallbacks, which seems unrelated to this PR. Did Rust change and get fussier?

@alex
Copy link
Collaborator

alex commented May 2, 2024

latest rust got more aggressive about dead code warnings. #2234 should fix

@alex
Copy link
Collaborator

alex commented May 2, 2024

Ok, you should be good to rebase.

This required two fixes. First,
test_verify_param_set_depth_fails_verification needed to be updated. The
history here is that OpenSSL 1.1.0 made a backwards-incompatible change
to the semantics of the depth limit. Ideally, rust-openssl would have
documented the semantics of its own APIs and normalized the behavior,
but it instead silently picked up the semantics change.

BoringSSL aligned with OpenSSL's new behavior in
b251d813ec615e7ef01d82073f94960eb13b1e0a, but since then rust-openssl
has codified the old behavior in tests. We need to update some cfgs to
reflect this.

Second, BoringSSL requires a C++ runtime (we have required a C++
compiler for a long time). This reveals a problem in Cargo's
dependency management strategy for externally-built static libraries.
Work around this by making some guesses about what library to link in,
but see the comment for why this is unsafe. This unsafety appears to be
inherent to Cargo and the choice of having Cargo drive cross-language
builds, rather than providing hooks for an integrated build system.
Newer BoringSSL requires SSE2 on 32-bit x86, but I opted to apply it to
all the OpenSSLs that CI builds. This allows the compiler to vectorize
random bits of the library, so hopefully it'll run faster on CI.
Especially with 32-bit x86 being so register-poor.

Additionally, Rust's definition of i686 already assumes SSE2 (see
rust-lang/rust#82435), so there's no sense in
pretending to test pre-SSE2 targets.
@davidben
Copy link
Contributor Author

davidben commented May 3, 2024

Rebased!

@alex alex merged commit 121df8c into sfackler:master May 3, 2024
61 checks passed
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.

2 participants