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

add support for illumos #1046

Closed
wants to merge 1 commit into from
Closed

add support for illumos #1046

wants to merge 1 commit into from

Conversation

jclulow
Copy link

@jclulow jclulow commented Aug 23, 2020

We recently added support for an illumos host triple to the Rust toolchain, and would like to be able to build ring there. This patch adds support for that. We are sufficiently similar to Solaris that we just needed some extra conditional compilation directives.

The tests don't currently pass:

$ cargo test --all -- --test-threads=1
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
     Running target/debug/deps/ring-2e180370d30c697d

running 88 tests
test aead::aes::tests::test_aes ... ok
test aead::aes_gcm::tests::max_input_len_test ... ok
test aead::block::tests::test_bitxor_assign ... ok
test aead::chacha20_poly1305::tests::max_input_len_test ... ok
test aead::chacha::tests::chacha20_tests ... ok
test aead::poly1305::tests::test_poly1305 ... ok
test aead::poly1305::tests::test_state_layout ... ok
test arithmetic::bigint::tests::test_elem_exp_consttime ... ok
test arithmetic::bigint::tests::test_elem_mul ... ok
test arithmetic::bigint::tests::test_elem_reduced ... ok
test arithmetic::bigint::tests::test_elem_reduced_once ... ok
test arithmetic::bigint::tests::test_elem_squared ... ok
test arithmetic::bigint::tests::test_modulus_debug ... ok
test arithmetic::bigint::tests::test_mul_add_words ... ok
test arithmetic::bigint::tests::test_public_exponent_debug ... ok
test bssl::tests::result::semantics ... ok
test bssl::tests::result::size_and_alignment ... ok
test c::tests::test_libc_compatible ... ok
test constant_time::tests::test_constant_time ... ok
test cpu::intel::x86_64_tests::test_avx_movbe_mask ... ok
test digest::tests::max_input::SHA1_FOR_LEGACY_USE_ONLY::max_input_test ... ok
test digest::tests::max_input::SHA1_FOR_LEGACY_USE_ONLY::too_long_input_test_block ... fatal runtime error: failed to initiate panic, error 5
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/ws/safari/ring/target/debug/deps/ring-2e180370d30c697d --test-threads=1` (signal: 6, SIGABRT: process abort signal)

The reason for this is a somewhat complicated artefact of the unwinding machinery and dynamic linker dependencies, and my plan is to go and fix this directly in the Rust toolchain. The issue is in the order of NEEDED entries in the resultant executable:

$ elfdump -d /ws/safari/ring/target/debug/deps/ring-2e180370d30c697d  | awk '/index.*tag/ || /Section/ || /NEEDED/'
Dynamic Section:  .dynamic
     index  tag                value
       [0]  NEEDED            0x86d36             libc.so.1
       [1]  NEEDED            0x86daf             libm.so.2
       [2]  NEEDED            0x86db9             libsocket.so.1
       [3]  NEEDED            0x86e19             libumem.so.1
       [4]  NEEDED            0x86dda             libgcc_s.so.1
       [5]  NEEDED            0x86e02             libssp.so.0

Because libc.so.1 is before libgcc_s.so.1 in the list, we will use the unwinding symbols that are provided as part of the system ABI. Unfortunately for our purposes, there are some additional symbols in the GCC unwinder that are not in the base platform library -- these symbols fall through and are bound to the GCC version, and thus we have two totally separate implementations trying to work on one another's data structures and the result is a crash (or worse). I intend to fix this in the Rust toolchain, to make sure the correct ordering of NEEDED entries is always present in built binaries. Once that's fixed there I expect better behaviour here, as we see with unwinding in binaries that don't have as much custom/FFI stuff.

I expect the same issue afflicts the Solaris stuff today, though they might have added the additional symbols to their base libc unwinder.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@briansmith
Copy link
Owner

Is there a convenient way to test in Illumos? I checked the https://github.com/rust-embedded/cross support matrix and I didn't see it there. What would be the easiest way to run the tests, starting on either a Linux x86-64 host or Windows x86-64 host?

@jclulow
Copy link
Author

jclulow commented Aug 26, 2020

Hi Brian! The easiest way is generally in a virtual machine, if your Linux/Windows hosts have that available? I know that, e.g., GitHub Actions doesn't currently seem to expose, say, KVM. We are looking at more readily available (e.g., via an API) options for driving tests from CI/CD systems, but I don't have a great answer just yet!

I hadn't seen the cross stuff before, but I will take a look at that too!

Do you think it would be alright to at least get this merged without fixing the broader testing situation? One I have the Rust build issue (with the library order) sorted out I intend to come back and see how many tests actually pass at that point. In the meantime it'd be good to be able to build other things that use ring.

@francescocarzaniga
Copy link
Contributor

@briansmith you can take a look at this pull for a Dockerfile to cross-compile to Illumos. Since it will enter the stable channel with the release of 1.47 I guess cross will add it then.

@briansmith
Copy link
Owner

Hi, I'm going to close this because we did merge a different small change to get things working for people on illumos, even though we don't have a CI/CD solution. Thanks for the PR and thanks for the pointers about how to get the ball rolling towards a CI/CD solution. I guess for now we can't really afford to add illumos to the GitHub Actions build matrix.

@briansmith briansmith closed this Nov 24, 2020
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