Regression: `error: this directive must appear between .cfi_startproc and .cfi_endproc directives` #39

polarathene · 2024-10-15T22:51:35Z

I recently started getting failures when doing builds with basic projects using eyra which internally brings in unwinding.

The errors initially were lacking information about the crate producing the error but I've identified it's a recent change to unwinding.

When using nightly to build with -Z build-std=std,panic_abort paired with -C panic=abort I am getting this failure:

cargo init /tmp/example && cd /tmp/example
cargo add unwinding

# Add `extern crate unwinding;` to `src/main.rs`, then build:
RUSTFLAGS='-C panic=abort -C target-feature=+crt-static' cargo +nightly build \
  --release \
  --target x86_64-unknown-linux-gnu \
  -Z build-std=std,panic_abort

   Compiling unwinding v0.2.3
error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:66:13
   |
66 |             .cfi_def_cfa_offset 0xA0
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:4:13
   |
4  |             .cfi_def_cfa_offset 0xA0
   |             ^

error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:90:13
   |
90 |             .cfi_def_cfa_offset 8
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:28:13
   |
28 |             .cfi_def_cfa_offset 8
   |             ^

The failure cause when used indirectly via eyra seemed to not trigger provided I didn't have -C panic=abort or did not do -Z build-std (which requires =std,panic_abort if abort is set).

However with unwinding directly I noticed that it would fail like above with only -C panic=abort present:

# Failure as above:
RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target x86_64-unknown-linux-gnu
RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target x86_64-unknown-linux-gnu -Z build-std=std,panic_abort

# Without `-C panic=abort`, this works as expected:
cargo +nightly build --release --target x86_64-unknown-linux-gnu
cargo +nightly build --release --target x86_64-unknown-linux-gnu -Z build-std=std,panic_abort

# With the `panic-handler` feature enabled, same error as above:
cargo add unwinding --features panic-handler
# Previously this would fail for duplicate lang item (panic_impl):
RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target x86_64-unknown-linux-gnu
RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target x86_64-unknown-linux-gnu -Z build-std=std,panic_abort
# Failure duplicate lang item (panic_impl), as expected due to std use:
cargo +nightly build --release --target x86_64-unknown-linux-gnu

Previously it working with unwinding=0.2.2 on nightly before the naked_asm failure was introduced which 0.2.3 fixed and presumably introduced this regression?:

[toolchain]
profile = "minimal"
channel = "nightly-2024-10-04"
components = ["rust-src"]
targets = ["x86_64-unknown-linux-gnu"]

The prior cargo commands for the 0.2.2 release on that nightly all avoid the reported error.

The text was updated successfully, but these errors were encountered:

nbdd0121 · 2024-10-17T03:24:18Z

I cannot reproduce this

polarathene · 2024-10-17T04:30:08Z

UPDATE: A follow-up comment of mine details a working fix that's probably appropriate? Hopefully this reproduction works for you to verify?

I cannot reproduce this

This should provide a fairly good reproduction:

docker run --rm -it --workdir /example fedora:41
dnf install -y nano gcc rustup-init
rustup-init -y --profile minimal --default-toolchain nightly && . "$HOME/.cargo/env"

cargo init
cargo add unwinding
# Add `extern crate unwinding;` at the top:
nano src/main.rs

# Build:
$ RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target x86_64-unknown-linux-gnu

  Downloaded unwinding v0.2.3
  Downloaded gimli v0.31.1
  Downloaded libc v0.2.159
  Downloaded 3 crates (1.1 MB) in 0.35s
   Compiling libc v0.2.159
   Compiling gimli v0.31.1
   Compiling unwinding v0.2.3
error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:66:13
   |
66 |             .cfi_def_cfa_offset 0xA0
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:4:13
   |
4  |             .cfi_def_cfa_offset 0xA0
   |             ^

error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:90:13
   |
90 |             .cfi_def_cfa_offset 8
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:28:13
   |
28 |             .cfi_def_cfa_offset 8
   |             ^

[package]
name = "example"
version = "0.1.0"
edition = "2021"

[dependencies]
unwinding = "0.2.3"

extern crate unwinding;

fn main() {
    println!("Hello, world!");
}

If you cannot reproduce with the above environment, then I suppose it could be an issue with my kernel? Otherwise everything else should be the fairly well isolated within that Docker container?

$ uname -a
Linux 4065fe3fc76f 5.15.123.1-microsoft-standard-WSL2 #1 SMP Mon Aug 7 19:01:48 UTC 2023 x86_64 GNU/Linux

$ rustc --version --verbose

rustc 1.84.0-nightly (798fb83f7 2024-10-16)
binary: rustc
commit-hash: 798fb83f7d24e31b16acca113496f39ff168c143
commit-date: 2024-10-16
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1

polarathene · 2024-10-17T04:33:40Z

The errors do point to two lines from here:

unwinding/src/unwinder/arch/x86_64.rs

Lines 59 to 95 in 41a5258

    
           #[naked] 
        
           pub extern "C-unwind" fn save_context(f: extern "C" fn(&mut Context, *mut ()), ptr: *mut ()) { 
        
               // No need to save caller-saved registers here. 
        
               unsafe { 
        
                   core::arch::naked_asm!( 
        
                       " 
        
                       sub rsp, 0x98 
        
                       .cfi_def_cfa_offset 0xA0 
        
                       mov [rsp + 0x18], rbx 
        
                       mov [rsp + 0x30], rbp 
        
                       /* Adjust the stack to account for the return address */ 
        
                       lea rax, [rsp + 0xA0] 
        
                       mov [rsp + 0x38], rax 
        
                       mov [rsp + 0x60], r12 
        
                       mov [rsp + 0x68], r13 
        
                       mov [rsp + 0x70], r14 
        
                       mov [rsp + 0x78], r15 
        
                       /* Return address */ 
        
                       mov rax, [rsp + 0x98] 
        
                       mov [rsp + 0x80], rax 
        
                       stmxcsr [rsp + 0x88] 
        
                       fnstcw [rsp + 0x90] 
        
                       mov rax, rdi 
        
                       mov rdi, rsp 
        
                       call rax 
        
                       add rsp, 0x98 
        
                       .cfi_def_cfa_offset 8 
        
                       ret 
        
                       " 
        
                   ); 
        
               } 
        
           }

That function was modified for the 0.2.3 release where the regression was introduced.

polarathene · 2024-10-17T05:11:02Z

Working fix?

@nbdd0121 I cloned this repo and followed the error output advice to include .cfi_startproc and .cfi_endproc and it was able to compile successfully.

These lines were inserted after and before the respective opening/closing " in the referenced function core::arch::naked_asm!() call above.

Actually I see that a PR isn't available for the 0.2.3 change that introduced the naked_asm! switch, but these .cfi lines were introduced via this PR which was also part of the 0.2.3 release.

Side-note: For some reason you haven't been tagging commits for releases after 0.2.1 btw, so it required scanning over the commit history to realize which release that change belonged to. It's possible that it would have also failed builds before the switch to naked_asm! as a result.

Affects other archs too

I don't know how to easily/properly test for the other archs, but I assume they'd likewise raise that failure if I did have.

I was able to produce the same failure for aarch64 via:

rustup target add aarch64-unknown-linux-gnu
dnf install -y zig
cargo install cargo-zigbuild
RUSTFLAGS='-C panic=abort' cargo +nightly build --release --target aarch64-unknown-linux-gnu

And applying the same change to get a successful build.

I did the same for riscv64gc but Zig failed to compile that for a different reason, something about .cfi_label, but that could be a Zig specific failure (it's not always reliable for builds).

Unsure if correct approach

I also have no idea if that's the right fix for this, as assembly isn't something I have any experience in, so the placement may be incorrect.

No clue why my specific build environment fails to build if you're able to reproduce it with that Docker reproduction, but not in a different build environment 🤷‍♂️ (any context as to what your build environment is?)

I did find this reference from rust-lang/rust tests where they seem to do the same approach? Might be correct after all?

nbdd0121 · 2024-10-19T00:42:21Z

No, cfi_startproc must not be added. It will break all builds other than your setup.

I've managed to reproduce the issue, all of the following must happen:

-Cpanic=abort is used
Debug info is disabled
-Cforce-unwind-tables is disabled

In which case, unwinding is useless because unwinding is disabled and unwinding info is not existent. Why would you need unwinding crate in this case?

polarathene · 2024-10-19T02:21:22Z

No, cfi_startproc must not be added. It will break all builds other than your setup.

Oh alright, thanks for pointing that out then :)

In which case, unwinding is useless because unwinding is disabled and unwinding info is not existent. Why would you need unwinding crate in this case?

I was doing some gnu target builds with eyra as mentioned. This would bring in the unwinding crate, and perhaps it's something I need to report there if it shouldn't be building unwinding for some builds?

Until this 0.2.3 release, my nightly builds did build and work successfully with eyra prior. But even on the same eyra release, it's dependency on unwinding would consider 0.2.3 semver compatible with 0.2.2 so Cargo resolves the newer release and I need to pin 0.2.2 in Cargo.lock (or add an explicit dependency to my own Cargo.toml) as a workaround.

Also I thought in this scenario it might also resolve the implicit -C link-arg=-lunwind? If it's not needed why is it trying to link the unwind lib? 🤔

$ dnf install -y musl-libc-static
$ RUSTFLAGS='-L /usr/x86_64-linux-musl/lib64 -C target-feature=+crt-static -C link-self-contained=no -C panic=abort' \
  cargo +nightly build --release --target x86_64-unknown-linux-musl \
  -Z build-std=std,panic_abort

error: linking with `cc` failed: exit status: 1
  = note: /usr/bin/ld: cannot find -lunwind: No such file or directory

The cfi_startproc approach does prevent failure for building the unwinding crate, but it didn't resolve that link issue. The README implies the crate would provide such and could link to it, so I'm not sure why that isn't working:

unwinding/README.md

Lines 8 to 9 in 41a5258

    
           This library serves two purposes: 
        
           1. Provide a pure Rust alternative to libgcc_eh or libunwind.

unwinding/README.md

Lines 38 to 41 in 41a5258

    
           If you want to link to the unwinder in a Rust binary, simply add 
        
           ```rust 
        
           extern crate unwinding; 
        
           ```

polarathene · 2024-10-19T02:34:58Z

For the Eyra example, here is a minimal reproduction.

RUSTFLAGS='-C link-arg=-nostartfiles -C target-feature=+crt-static -C panic=abort' \
  cargo build --release --target x86_64-unknown-linux-gnu \
  -Z build-std=std,panic_abort

error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:66:13
   |
66 |             .cfi_def_cfa_offset 0xA0
   |             ^

extern crate eyra;

fn main() {
    println!("Hello, world!");
}

[package]
name = "example"
version = "0.1.0"
edition = "2021"

[dependencies]
eyra = "0.19.0"

The unwinding dependency comes from here in c-scape dep:

# Enable "libc" and don't depend on "spin".
# TODO: Eventually, we should propose a `fde-phdr-rustix` backend option to
# upstream `unwinding` so that it doesn't need to go through `dl_iterate_phdr`,
# but `fde-phdr-dl` works for now.
[target.'cfg(not(target_arch = "arm"))'.dependencies.unwinding]
version = "0.2.0"
default-features = false
features = [
    "unwinder",
    "dwarf-expr",
    "hide-trace",
    "fde-phdr-dl",
    "fde-registry",
    "libc",
]

nbdd0121 · 2024-10-19T07:02:09Z

A workaround is to enable debug information. I need to figure out if it's possible to have conditional CFI directives, but it could be difficult.

@bjorn3 do you have any suggestions that would make this setup work without having to remove the CFI instructions?

bjorn3 · 2024-10-19T07:28:42Z

I guess one workaround would be to move from naked asm to global asm and explicitly add cfi_startproc and cfi_endproc directives. Also IMO this is arguably a bug in rustc. Rust's inline asm rules state that the cfi directives are allowed in inline asm. There is no explicit restriction against using them with panic=abort, so it would be reasonable to assume that the compiler will always either insert cfi_startproc as necessary even with panic=abort or remove all cfi directives when panic=abort is used.

Remove "unwinding" as a dependency of "nightly", and make it a dependency of "panic-handler" and "eh-personality" instead. This makes it possible to avoid depending on "unwinding" when it isn't needed, which helps with nbdd0121/unwinding#39.

…#149) * Make depending on "unwinding" conditional on `cfg(panic = "unwind)`. Using `cfg(panic = "unwind")` in Cargo.toml means we can avoid depending on the "unwinding" crate when unwinding isn't enabled. This avoids nbdd0121/unwinding#39. * Use a unique crate name in origin-start-dynamic-linker. * Add a panic=abort example.

sunfishcode · 2024-10-20T20:28:22Z

I can reproduce this bug in a simple example without eyra or any other dependency other than unwinding, using current Rust nightly (2024-10-19).

Cargo.toml:

[package]
name = "repro"
version = "0.1.0"
edition = "2021"

[profile.release]
panic = "abort"
[profile.dev]
panic = "abort"

[dependencies]
unwinding = { version = "0.2.3", default-features = false, features = [ "unwinder" ] }

src/main.rs:

fn main() {
    println!("Hello, world!");
}

Build command:

 cargo +nightly run --release 
   Compiling gimli v0.31.1
   Compiling unwinding v0.2.3
error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /home/me/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:66:13
   |
66 |             .cfi_def_cfa_offset 0xA0
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:4:13
   |
4  |             .cfi_def_cfa_offset 0xA0
   |             ^

error: this directive must appear between .cfi_startproc and .cfi_endproc directives
  --> /home/me/.cargo/registry/src/index.crates.io-6f17d22bba15001f/unwinding-0.2.3/src/unwinder/arch/x86_64.rs:90:13
   |
90 |             .cfi_def_cfa_offset 8
   |             ^
   |
note: instantiated into assembly here
  --> <inline asm>:28:13
   |
28 |             .cfi_def_cfa_offset 8
   |             ^

error: could not compile `unwinding` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

sunfishcode · 2024-10-20T23:48:03Z

The interesting thing from origin's perspective is that changing the [dependencies] to [target.'cfg(panic = "unwind")'.dependencies] in that example doesn't fix it. (I previously thought it did, but I was mistaken.) If there's another way to disable a dependency when panic = "abort" is used, I'd be interested to learn about it, however otherwise I'll look into making unwinding optional and just asking users to figure out when it needs to be enabled.

nbdd0121 · 2024-10-21T00:59:45Z

I'll make the debug CFI instructions optional. ~~Will take a few days before I had time to work on this~~

polarathene · 2024-10-21T05:47:39Z

This is mostly notes for myself (and others that land here), summarizing what they should do 👍

I can reproduce this bug in a simple example without eyra or any other dependency other than unwinding

I demonstrated that with my reproduction comment, except I didn't check if the extern crate was actually required, nor did I minimize unwinding default features.

eyra was brought up as an example of where unwinding was implicitly a dependency, even when the build would apparently not require unwinding. Which I see you've since addressed to allow it to be opt-out 👍

Proper opt-out of `unwinding` when it would be redundant

Resolving the issue properly is beyond my experience, but the discussion seems to be progressing for how to resolve it going forward 😎

I have no clue what was happening in prior builds where this wasn't an issue, but if unwinding was redundant then I assume it'd be optimized out from the build, it was just the initial compilation stage where Rust couldn't determine that context yet, so the build failure was hit.

Like @sunfishcode detailed here, the relevance for unwinding was getting confusing for me too 😓 but thankfully from that discussion the apparent solution for anyone that does bring in unwinding is:

[target.'cfg(all(panic = "unwind", not(target_arch = "arm")))'.dependencies.unwinding]
version = "0.2.3"

So that avoids the assembly errors for when the crate shouldn't be relevant right?

The interesting thing from origin's perspective is that changing the [dependencies] to [target.'cfg(panic = "unwind")'.dependencies] in that example doesn't fix it. (I previously thought it did, but I was mistaken.)

It seems to be only respected by RUSTFLAGS='-C panic=abort' but not panic = "abort" in Cargo.toml profile. In rust code the condition will be respected from either source however (demonstrated here).

musl + libstd failure with `-lunwind`

There's the related -musl cross compile target failure example I shared earlier, where unwinding shouldn't be relevant either, but as this comment by @bjorn3 states, -C link-arg=-lunwind is implicitly enforced when building libstd.

Providing that library is meant to be a goal of unwinding crate, but perhaps it's unable to do so as seamlessly in that scenario. Should I raise a separate issue here about it, or is it something the README should document as a caveat? Or should I file an issue upstream somewhere about that? (if it would be possible for unwinding to become compatible)

Looking at the README, I think it's hinted for dynamic linking to build a shared library / cdylib, but a static lib could also built separately to satisfy -C link-arg=-lunwind?

polarathene mentioned this issue Oct 19, 2024

unwinding dep features beneficial for no_std sunfishcode/eyra#51

Open

sunfishcode mentioned this issue Oct 19, 2024

Make depending on "unwinding" conditional on cfg(panic = "unwind"). sunfishcode/origin#149

Merged

sunfishcode mentioned this issue Oct 20, 2024

Update versions of things in CI. sunfishcode/eyra#54

Merged

polarathene mentioned this issue Oct 21, 2024

fix: Ensure mod unwind_unimplemented works without nightly feature enabled sunfishcode/origin#150

Merged

nbdd0121 mentioned this issue Oct 21, 2024

Only add CFI instructions for panic=unwind build #40

Merged

nbdd0121 closed this as completed in #40 Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression: `error: this directive must appear between .cfi_startproc and .cfi_endproc directives` #39

Regression: `error: this directive must appear between .cfi_startproc and .cfi_endproc directives` #39

polarathene commented Oct 15, 2024

nbdd0121 commented Oct 17, 2024

polarathene commented Oct 17, 2024 •

edited

Loading

polarathene commented Oct 17, 2024 •

edited

Loading

polarathene commented Oct 17, 2024 •

edited

Loading

nbdd0121 commented Oct 19, 2024 •

edited

Loading

polarathene commented Oct 19, 2024 •

edited

Loading

polarathene commented Oct 19, 2024

nbdd0121 commented Oct 19, 2024

bjorn3 commented Oct 19, 2024

sunfishcode commented Oct 20, 2024

sunfishcode commented Oct 20, 2024

nbdd0121 commented Oct 21, 2024 •

edited

Loading

polarathene commented Oct 21, 2024 •

edited

Loading

Regression: error: this directive must appear between .cfi_startproc and .cfi_endproc directives #39

Regression: error: this directive must appear between .cfi_startproc and .cfi_endproc directives #39

Comments

polarathene commented Oct 15, 2024

nbdd0121 commented Oct 17, 2024

polarathene commented Oct 17, 2024 • edited Loading

polarathene commented Oct 17, 2024 • edited Loading

polarathene commented Oct 17, 2024 • edited Loading

Working fix?

Affects other archs too

Unsure if correct approach

nbdd0121 commented Oct 19, 2024 • edited Loading

polarathene commented Oct 19, 2024 • edited Loading

polarathene commented Oct 19, 2024

nbdd0121 commented Oct 19, 2024

bjorn3 commented Oct 19, 2024

sunfishcode commented Oct 20, 2024

sunfishcode commented Oct 20, 2024

nbdd0121 commented Oct 21, 2024 • edited Loading

polarathene commented Oct 21, 2024 • edited Loading

Proper opt-out of unwinding when it would be redundant

musl + libstd failure with -lunwind

Regression: `error: this directive must appear between .cfi_startproc and .cfi_endproc directives` #39

Regression: `error: this directive must appear between .cfi_startproc and .cfi_endproc directives` #39

polarathene commented Oct 17, 2024 •

edited

Loading

polarathene commented Oct 17, 2024 •

edited

Loading

polarathene commented Oct 17, 2024 •

edited

Loading

nbdd0121 commented Oct 19, 2024 •

edited

Loading

polarathene commented Oct 19, 2024 •

edited

Loading

nbdd0121 commented Oct 21, 2024 •

edited

Loading

polarathene commented Oct 21, 2024 •

edited

Loading

Proper opt-out of `unwinding` when it would be redundant

musl + libstd failure with `-lunwind`