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

Improve conditional compilation around PerlAsm #1937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nspin
Copy link

@nspin nspin commented Jan 27, 2024

build.rs determines whether the target platform is supported by PerlAsm using both target_arch and target_os. Instances of conditional compilation in both src/ and crypto/ were using just target_arch to determine whether PerlAsm symbols are present, resulting in link-time build failures for certain targets, including, for example, aarch64-unknown-none.

This commit fixes those instances of conditional compilation to align with the build script.

I've made the most conservative possible modifications to these cfg conditions, so some of them may contain redundancy. In particular, those that were any(target_arch = "aarch64", target_arch = "arm", target_arch = "x86_64", target_arch = "x86") are now all(have_perlasm, any(target_arch = "aarch64", target_arch = "arm", target_arch = "x86_64", target_arch = "x86")), but it may be the case that some of these ought to just be have_perlasm.

This PR addresses #1793. As discussed in that issue, the conditions in build.rs for PerlAsm support may be overly conservative, but expanding the set of targets which are deemed to be supported by PerlAsm will be trickier. This PR is just a bug fix that enables platforms such as aarch64-unknown-none to use the fallbacks rather than failing to build at link-time.

@nspin nspin force-pushed the pr/improve-conditional-compilation-around-perlasm branch from 5412e5f to f57c2d7 Compare January 29, 2024 12:35
Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I am sorry about all the changes that were merged recently that created conflicts for this PR.

I think this is a good step towards what we need.

@@ -24,7 +24,7 @@
#include "../bn/internal.h"

#if !defined(OPENSSL_NO_ASM) && \
(defined(OPENSSL_X86_64) || defined(OPENSSL_AARCH64)) && \
defined(RING_HAVE_PERLASM) && \
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest we set OPENSSL_NO_ASM instead of RING_HAVE_PERLASM to better align with upstream BoringSSL and OpenSSL.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

build.rs Outdated
@@ -433,7 +435,9 @@ fn build_c_code(

generate_prefix_symbols_asm_headers(out_dir, ring_core_prefix).unwrap();

let (asm_srcs, obj_srcs) = if let Some(asm_target) = asm_target {
let (asm_srcs, obj_srcs) = if let Some(asm_target) = find_asm_target(target) {
println!("cargo:rustc-cfg=have_perlasm");
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest we name it just perlasm.

I am worried about the risk for users of non-Cargo build systems. I suggest when we do not set perlasm, we set no_perlasm. Then we should add a check like this to lib.rs to ensure that the such users have addressed this:

const PERLASM_CONFIGURED: () = assert!((cfg!(perlasm) && !cfg!(no_perlasm)) || (!cfg!(perlasm) && cfg!(no_perlasm)));

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I've implemented this.

@nspin nspin force-pushed the pr/improve-conditional-compilation-around-perlasm branch from f57c2d7 to e949d34 Compare March 30, 2024 08:13
src/lib.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.30%. Comparing base (7bc7672) to head (e949d34).

❗ Current head e949d34 differs from pull request most recent head 0e644b7. Consider uploading reports for the commit 0e644b7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1937      +/-   ##
==========================================
+ Coverage   96.26%   96.30%   +0.03%     
==========================================
  Files         140      140              
  Lines       20401    20855     +454     
  Branches      226      226              
==========================================
+ Hits        19639    20084     +445     
- Misses        728      737       +9     
  Partials       34       34              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

The deny job failure is fixed in PR #2012, so just ignore that.

When building without perlasm, there will be a lot of dead code. We should probably do something like:
#![cfg_attr(all(no_perlasm, any(target_arch = "aarch64", ....)),allow(dead_code)] in lib.rs to deal with this, at least temporarily.

We should also add a build-only job to CI for target aarch64-unknown-none or similar that ensures the build succeeds.

src/lib.rs Outdated
@@ -142,3 +142,6 @@ mod sealed {
// ```
pub trait Sealed {}
}

const PERLASM_CONFIGURED: () =
Copy link
Owner

Choose a reason for hiding this comment

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

This must be named _PERLASM_CONFIGURED to pass the clippy job.

Copy link
Owner

Choose a reason for hiding this comment

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

This assertion fails when running the package job.

Copy link
Author

Choose a reason for hiding this comment

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

This must be named _PERLASM_CONFIGURED to pass the clippy job.

Done.

Copy link
Author

Choose a reason for hiding this comment

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

This assertion fails when running the package job.

See below.

build.rs Outdated
@@ -419,6 +423,8 @@ fn build_c_code(
core_name_and_version: &str,
) {
let (asm_srcs, obj_srcs) = if let Some(asm_target) = asm_target {
println!("cargo:rustc-cfg=perlasm");
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like the cargo:rustc-cfg directives need to be emitted from generate_sources_and_preassemble instead so that the package job (mk/package.sh) succeeds.

Copy link
Author

Choose a reason for hiding this comment

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

I believe that wouldn't cover the case where RING_PREGENERATE_ASM != 1 && !is_git. I've moved these println!() statements into main() itself, which covers all cases.

@nspin nspin force-pushed the pr/improve-conditional-compilation-around-perlasm branch 6 times, most recently from 60a2951 to 0e644b7 Compare April 16, 2024 22:14
build.rs determines whether the target platform is supported by PerlAsm
using both target_arch and target_os. Instances of conditional
compilation in both src/ and crypto/ were using just target_arch to
determine whether PerlAsm symbols are present, resulting in link-time
build failures for certain targets, including, for example,
aarch64-unknown-none.

This commit fixes those instances of conditional compilation to align
with the build script.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@nspin nspin force-pushed the pr/improve-conditional-compilation-around-perlasm branch from 0e644b7 to 31b9789 Compare November 15, 2024 02:03
@nspin
Copy link
Author

nspin commented Nov 15, 2024

@briansmith I just now rebased again. Are there any more changes you'd like me to make to this PR?

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