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

ci: add nopanic check for windows #526

Merged
merged 8 commits into from
Oct 17, 2024
Merged

ci: add nopanic check for windows #526

merged 8 commits into from
Oct 17, 2024

Conversation

newpavlov
Copy link
Member

Relevant issue: #516

@newpavlov newpavlov mentioned this pull request Oct 16, 2024
20 tasks
@newpavlov
Copy link
Member Author

newpavlov commented Oct 16, 2024

It looks like link.exe does not perform the same dead code elimination as linkers in Linux and macOS (I confirmed it with an empty getrandom_wrapper function), so we need to reconsider our approach towards nopanic checks on Windows.

@newpavlov newpavlov closed this Oct 16, 2024
@ChrisDenton
Copy link

On my machine link.exe does eliminate it but still wants it to exist in the first place. In any case, you could use lto to eliminate the symbol before the linker even sees it.

@newpavlov
Copy link
Member Author

LTO is irrelevant here. I tried to use the following program:

#![no_std]

#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    extern "C" {
        fn panic_nonexistent() -> !;
    }
    unsafe { panic_nonexistent() }
}

#[no_mangle]
pub extern "C" fn getrandom_wrapper(_: *mut u8, _: usize) -> u32 {
    0
}

And it still was failing the linking step. I will try to use a slightly different approach to this a bit later.

@ChrisDenton
Copy link

It should be highly relevant. Link time optimization happens before the linker is given anything (unless you use llvm's linker) so if panic is never called then no references to panic_nonexistent will exist by the time the linker sees it.

@newpavlov newpavlov reopened this Oct 17, 2024
@newpavlov
Copy link
Member Author

Huh, you are right. Thank you! TIL, before that I thought LTO applies only during "link time" and has not effect in cases like the snippet above.

@newpavlov newpavlov merged commit b055577 into master Oct 17, 2024
116 checks passed
@newpavlov newpavlov deleted the windows_nopanic branch October 17, 2024 19:16
newpavlov added a commit that referenced this pull request Oct 17, 2024
This check now works thanks to LTO enabled in #526.
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