rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.63k stars 12.49k forks source link

`.eh_frame` is emitted for `-C panic=abort` on (at least) `i686-pc-windows-gnu` #128136

Closed jieyouxu closed 1 month ago

jieyouxu commented 1 month ago

https://github.com/rust-lang/rust/pull/112403 tried to prevent .eh_frame from being emitted for -Cpanic=abort and added a test (tests/run-make/panic-abort-eh_frame/Makefile) to check unnecessary landing pads related to unwinding do not appear in the object file when -Cpanic=abort.

The original Makefile test was only-linux, and when @Oneirical tried to run the (roughly) equivalent test on Windows (see https://github.com/rust-lang/rust/pull/127523), we found that this "no unnecessary landing pads" property (whichever property is checked by proxy via testing if DW_CFA exists) is not preserved at least on i686-pc-windows-gnu.

I don't understand much of this test, so I'll ask those who are more knowledgeable: for people more familiar with this test cc @nbdd0121 and @Nilstrieb, is this property also intended to be upheld on Windows and not just Linux?

nbdd0121 commented 1 month ago

We currently force uwtables to be emitted on Windows: https://github.com/rust-lang/rust/blob/d24930ceb473b7b361d108d573308e3529cb5ef7/compiler/rustc_target/src/spec/base/windows_msvc.rs#L19

jieyouxu commented 1 month ago

We currently force uwtables to be emitted on Windows:

Thank you, that's the context I'm missing 👍

jieyouxu commented 1 month ago

Actually re-opening because

the test does pass on MSVC (see rust-lang-ci/rust/actions/runs/10017286801/job/27691281649, it's not even getting ignored). It even passes on x86 gnu-windows. It is only this one specific architecture (i686-mingw) that fails.

So I'm really unsure what the intended behavior of this test is (on the i686-mingw CI job at least).

nbdd0121 commented 1 month ago

On windows-gnu we also force uwtables.

What's the issue of keeping only-linux?

jieyouxu commented 1 month ago

Hm, it was strange that only i686-mingw had the panic symbol but not other windows runners. Anyway, we can keep it only-linux. Thanks!