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

sample test case is broken with 1.75 #230

Closed
osiewicz opened this issue Dec 28, 2023 · 12 comments
Closed

sample test case is broken with 1.75 #230

osiewicz opened this issue Dec 28, 2023 · 12 comments

Comments

@osiewicz
Copy link
Contributor

In PR #229 we started seeing weird CI failures for

- name: Native version of cargo-show-asm (Intel ASM) native CPU no default features
. I suspect that an upgrade from 1.74 to 1.75 is what caused this, as it looks like the CI grabs the newest Rust version automatically.
Attached is the old output of the failing test and a new one. Note that the base is 1.73 and not 1.74, though it doesn't seem to make much of a difference.
cargo-show-asm-1-75-ci-failure.zip

@osiewicz
Copy link
Contributor Author

It looks like main is now stripped? That seems somewhat plausible as it's defined in lib.rs, so who knows..

@pacak
Copy link
Owner

pacak commented Dec 28, 2023

Interestingly enough it works on my machine with 1.75.0

% cargo run --release --  --manifest-path sample/Cargo.toml --native --intel
    Finished release [optimized] target(s) in 0.02s
     Running `target/release/cargo-asm --manifest-path sample/Cargo.toml --native --intel`
    Finished release [optimized   debuginfo] target(s) in 0.00s

Try one of those by name or a sequence number
0 "<&T as core::fmt::Display>::fmt" [17]
1 "<sample::MyRngCore as rand_core::block::BlockRngCore>::generate" [18]
2 "core::ptr::drop_in_place<ahash::random_state::DefaultRandomSource>" [7]
3 "hashbrown::raw::RawTable<T,A>::reserve_rehash" [1238]
4 "hashbrown::raw::RawTable<T,A>::reserve_rehash::{{closure}}" [299]
5 "hashbrown::set::HashSet<T,S,A>::insert" [382]
6 "hashbrown::set::HashSet<T>::new" [241]
7 "make_bar" [8]
8 "sample::main" [751]
% rustc --version
rustc 1.75.0 (82e1608df 2023-12-21)

Can you try passing --everything to check if sample::main is there under some other name? Or any ideas how to reproduce it on my end?

@pacak
Copy link
Owner

pacak commented Dec 28, 2023

Stripping shouldn't affect anything - c-s-a doesn't run on compiled code, it asks rustc to dump generated .s file.

@osiewicz
Copy link
Contributor Author

osiewicz commented Dec 28, 2023

Can you try building it with --no-default-features on sample? E.g:
cargo run --release -- --manifest-path sample/Cargo.toml --native --intel sample::main --rust --no-default-features
It looks like that makes a difference, as we also test the same scenario with default features on sample in CI and that passes even on 1.75.

By stripping I've meant dead code elimination, maybe there's now a MIR optimization that removes main from libs (so that it won't even have code generated for it) - though I don't see why it would only happen with default features turned off.
I'll post results with --everything in the morning.

@osiewicz
Copy link
Contributor Author

osiewicz commented Dec 28, 2023

I have a hunch, will try that in the morning though;
With default features turned off, sample::main has a signature of fn() -> u32 which IIRC isn't a valid function signature for an entry point. With default features turned on, its signature is fn() -> () and that is a valid entry point signature. Maybe it is being preserved with default features (as it's an entry point) and being stripped away without any default features (as it's dead code?)

@pacak
Copy link
Owner

pacak commented Dec 28, 2023

cargo run --release -- --manifest-path sample/Cargo.toml --native --intel sample::main --rust --no-default-features

works for me...

CI seem to detect it when it's there if I run with no argument... But fails to find it when it's called.

fn() -> () vs -> u32 shouldn't really affect anything, in both cases it should be a label like this: _ZN6sample4main17hecd2c58e95d09ed1E:.

I'll poke around, will let you know if I manage to reproduce it.

@osiewicz
Copy link
Contributor Author

Well, IMHO the return value should make a difference as to whether the code is stripped altogether or not.

@pacak
Copy link
Owner

pacak commented Dec 28, 2023

Can you attach the raw .s file?

@osiewicz
Copy link
Contributor Author

It is in original post in a .zip, together with .s for 1.73.
I am sorry for not providing one for 1.74, but this is what I had at hand.

@osiewicz
Copy link
Contributor Author

osiewicz commented Dec 29, 2023

Okay, got the jackpot. The return value was indeed not an issue.
The culprit lied in rust-lang/rust#116505
In short, with default features turned off, main was trivial enough to be marked as inline function automatically which then made the symbol weak. Since nobody was referencing it, it got stripped away.
Marking main in default-features=false config as #[inline(never)] or replacing 1 1 in it's body with a simple println!("foo") call (to make the main function sophisticated enough not to be subject to the new automatic marking as inline) makes the test pass again.

@pacak
Copy link
Owner

pacak commented Dec 29, 2023

Good catch!

@osiewicz
Copy link
Contributor Author

Closing as it's fixed with e7e31ac71fdc8d350226a39ac2be5a4a47c6bac0

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

No branches or pull requests

2 participants