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: Ensure mod unwind_unimplemented works without nightly feature enabled #150

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented Oct 21, 2024

TL;DR: The PR now only reverts a recent change, so that origin will have mod unwind_unimplemented when the nightly / unwinding feature is not enabled. Details here.

Originally this PR was intended to fix building unwinding with nightly feature for panic = "abort" but I was mistaken as the update below notes. That build issue will be resolved with release of unwinding after 0.2.3 via: nbdd0121/unwinding#40


Original PR message (with update)

This restores the Eyra hello-world-small crate example to build with panic = "abort".

References:


This PR isn't quite ready to merge, when built there is many warnings now emitted about the feature = "unwinding" no longer being valid as it is no longer created implicitly from the nightly feature requirements.

UPDATE: I was mistaken about nightly = ["dep:unwinding"] vs nightly = ["unwinding"]. Apart from the difference in implicit feature being dropped, the difference did not actually affect the cfg condition on the unwinding dependency itself, or at least I cannot reproduce such.

I did notice that panic = "abort" in Cargo.toml has no affect on the dependency condition (only -C panic=abort does):

# This is only prevented by `-C panic=abort`, not by equivalent `Cargo.toml` setting
[target.'cfg(panic = "unwind")'.dependencies.some-crate]
version = "0.1.0"

[profile.release]
panic = "abort"
// Either approach for setting abort works here though
fn main() {
    #[cfg(panic = "unwind")]
    println!("unwind");

    #[cfg(panic = "abort")]
    println!("abort");
}
RUSTFLAGS='-C panic=abort' cargo run --release

@@ -37,8 +37,12 @@ pub(crate) mod naked;
#[cfg(all(feature = "unwinding", not(target_arch = "arm")))]
#[allow(unused_extern_crates)]
extern crate unwinding;
#[cfg(all(feature = "unwinding", target_arch = "arm"))]
#[cfg(any(not(feature = "unwinding"), target_arch = "arm"))]
Copy link
Contributor Author

@polarathene polarathene Oct 21, 2024

Choose a reason for hiding this comment

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

I reverted this back to what it was, as I think it's valid to enable for either condition vs only unwinding enabled on arm?


Errors without this change

For x86_64 this change allowed the hello-world-small Eyra example to build successfully (if the nightly feature of origin was not enabled by c-scape):

  • This was with eyra => c-gull => c-scape => origin crates all cloned locally where I removed the nightly feature from c-scape's origin feature list.
  • I tried to reproduce with just origin but no luck. Presumably something eyra/c-scape was doing with origin and it's unwind modules here?

Without the change, these are the two kinds of failures:

With -C target-feature=+crt-static:

  = note: rust-lld: error: undefined symbol: _dl_find_object
          >>> referenced by unwind-dw2-fde-dip.o:(_Unwind_Find_FDE) in archive /usr/lib/gcc/x86_64-redhat-linux/14/libgcc_eh.a
          >>> referenced by unwind-dw2-fde-dip.o:(_Unwind_Find_FDE) in archive /usr/lib/gcc/x86_64-redhat-linux/14/libgcc_eh.a
          collect2: error: ld returned 1 exit status

Without -C target-feature=+crt-static, all the symbols that the module provides are listed as errors:

  = note: rust-lld: error: undefined symbol: _Unwind_Resume

...

RUSTFLAGS='-C panic=abort' vs panic = "abort" (Cargo.toml)

If building with -C panic=abort (regardless of panic setting in Cargo.toml profile), with the nightly feature you'll get:

error[E0463]: can't find crate for `unwinding`
  --> /origin/src/lib.rs:39:1
   |
39 | extern crate unwinding;
   | ^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

That could be prevented by matching the unwinding dep cfg requirement for panic = "unwind", but you'd then also need the opposite for the mod unwind_unimplemented condition.

Not possible to use cfg with features yet: rust-lang/cargo#1197

Comment on lines +42 to +45
// If we don't have "unwinding", provide stub functions for unwinding and
// panicking.
#[cfg(not(feature = "unwinding"))]
mod stubs;
Copy link
Contributor Author

@polarathene polarathene Oct 21, 2024

Choose a reason for hiding this comment

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

I figured given the context it was better to co-locate with the other unwinding related logic?

mod stubs was previously toggled by the nightly feature but has since lost that context with the switch to unwinding feature. Thus technically unwind_unimplemented and stubs are similar beyond the arm conditional.

Not sure how you want to approach this.

@polarathene

This comment was marked as outdated.

Cargo.toml Outdated Show resolved Hide resolved
@polarathene polarathene changed the title fix: Ensure nightly feature doesn't force unwinding fix: Ensure mod unwind_unimplemented works without nightly feature enabled Oct 24, 2024
@sunfishcode
Copy link
Owner

Thanks!

@sunfishcode sunfishcode merged commit 29bbfab into sunfishcode:main Nov 23, 2024
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