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

__CxxFrameHandler3 still referenced even though panic=abort #101134

Closed
ChrisDenton opened this issue Aug 28, 2022 · 27 comments
Closed

__CxxFrameHandler3 still referenced even though panic=abort #101134

ChrisDenton opened this issue Aug 28, 2022 · 27 comments
Labels
C-bug Category: This is a bug. O-windows-msvc Toolchain: MSVC, Operating system: Windows regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ChrisDenton
Copy link
Contributor

Code

I tried this code:

fn main() {}

With this section in Cargo.toml:

[profile.release]
panic = 'abort'
lto = true

And also disabled msvcrt.lib. Unfortunately rust does not have a good way to do this so I used a build script to add a libs directory that contains an empty msvcrt.lib:

use std::{env, path::Path};

fn main() {
    // Disable msvcrt.lib
    let dir = env::var_os("CARGO_MANIFEST_DIR").unwrap();
    let dir = Path::new(&dir).join("libs");
    println!("cargo:rustc-link-search=native={}", dir.display());
}

For a full example see: https://github.com/ChrisDenton/panic-abort

I expected to see this happen: When running cargo build --release, the symbol __CxxFrameHandler3 is not referenced at all. Because with panic=abort there should be no panic code.

Instead, this happened: __CxxFrameHandler3 is referenced. This causes the following linker error because I disabled msvcrt.lib:

error LNK2001: unresolved external symbol __CxxFrameHandler3

Version it worked on

It most recently worked on: 1.59

rustc +1.59 --version --verbose

rustc 1.59.0 (9d1b2106e 2022-02-23)
binary: rustc
commit-hash: 9d1b2106e23b1abd32fce1f17267604a5102f57a
commit-date: 2022-02-23
host: x86_64-pc-windows-msvc
release: 1.59.0
LLVM version: 13.0.0

Version with regression

rustc +1.60 --version --verbose:

rustc 1.60.0 (7737e0b5c 2022-04-04)
binary: rustc
commit-hash: 7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c
commit-date: 2022-04-04
host: x86_64-pc-windows-msvc
release: 1.60.0
LLVM version: 14.0.0

@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged

@ChrisDenton ChrisDenton added O-windows-msvc Toolchain: MSVC, Operating system: Windows C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 28, 2022
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Aug 28, 2022
@ChrisDenton
Copy link
Contributor Author

@BatmanAoD I think this suggests that panic's unwind structs are being included even when panic=abort? Whereas before it wasn't?

@Urgau
Copy link
Contributor

Urgau commented Aug 29, 2022

Because with panic=abort there should be no panic code.

I don't think this is true, the C-unwind ABI still needs it even in panic=abort, otherwise we would not be able to catch the panic and turn it into an abort.

@ChrisDenton
Copy link
Contributor Author

Hm, then why did it only start needing it with 1.60?

@apiraino
Copy link
Contributor

apiraino commented Aug 31, 2022

probably a bisection could help understanding when this started

@rustbot label e-needs-bisection t-compiler

@rustbot rustbot added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2022
@Urgau
Copy link
Contributor

Urgau commented Aug 31, 2022

Hm, then why did it only start needing it with 1.60?

Probably because there was a fix in 1.60. There are currently some effort that are being done to fix the unsoundness around the C ABI when unwinding. Or maybe there is just something that now requires it.

@ChrisDenton
Copy link
Contributor Author

probably a bisection could help understanding when this started

Regression was in nightly-2022-01-19. I'm still attempting to narrow it down:

found 6 bors merge commits in the specified range
  commit[0] 2022-01-17UTC: Auto merge of #90986 - camsteffen:nested-filter, r=cjgillot
  commit[1] 2022-01-17UTC: Auto merge of #93009 - matthiaskrgr:rollup-3fkxg6i, r=matthiaskrgr
  commit[2] 2022-01-18UTC: Auto merge of #93001 - flip1995:clippyup, r=Manishearth
  commit[3] 2022-01-18UTC: Auto merge of #93021 - matthiaskrgr:rollup-o7z8zoe, r=matthiaskrgr
  commit[4] 2022-01-18UTC: Auto merge of #87648 - JulianKnodt:const_eq_constrain, r=oli-obk
  commit[5] 2022-01-18UTC: Auto merge of #92731 - bjorn3:asm_support_changes, r=nagisa
ERROR: no CI builds available between ee5d8d37baaf5b5a81a98396952839c73ae41c68 and 9ad5d82f822b3cb67637f11be2e65c5662b66ec0 within last 167 days

@ChrisDenton
Copy link
Contributor Author

Most likely candidate: #92877

@ChrisDenton
Copy link
Contributor Author

Hm, then why did it only start needing it with 1.60?

Probably because it there a fix for 1.60. There are currently some effort that are being done to fix the unsoundness around the C when unwinding. Or maybe there is just something that now required it.

I can understand wanting to add exception handlers in some cases. But I don't understand using __CxxFrameHandler3 instead of a handler function that immediately aborts. After all, it always sound to abort, no?

@Urgau
Copy link
Contributor

Urgau commented Aug 31, 2022

Most likely candidate: #92877

Yes, that must be the one. It even say in the PR description:

[..] In fact this is incorrect in the presence of extern "C-unwind" which must create a landing pad when compiled with -C panic=abort so that foreign exceptions are caught and properly turned into aborts.


I can understand wanting to add exception handlers in some cases. But I don't understand using __CxxFrameHandler3 instead of a handler function that immediately aborts. After all, it always sound to abort, no?

This is unfortunately outside my knowledge, sorry.
But from what I understand __CxxFrameHandler3 is the CRT provided function on Windows for dealing with unwind, so it doesn't surprise me that it is used.

@apiraino apiraino removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Sep 1, 2022
@ChrisDenton
Copy link
Contributor Author

Ok, it does surprise me. For platform details see here. This handler can choose not to handle the exception (which may mean the program continues to unwind) or it may explicitly continue unwinding.

I would (perhaps naively?) expect Rust to always abort instead of maybe abort or maybe unwind. And it just seems like an unnecessary dependency. I could implement __CxxFrameHandler3 myself as an immediate abort I don't think that would be unsound?

@BatmanAoD
Copy link
Member

@Amanieu Does #92845 change this? I'm not really able to answer any of @ChrisDenton's questions above, but it seems like we should now be linking a personality function in std rather than __CxxFrameHandler3.

@Amanieu
Copy link
Member

Amanieu commented Sep 2, 2022

I can understand wanting to add exception handlers in some cases. But I don't understand using __CxxFrameHandler3 instead of a handler function that immediately aborts. After all, it always sound to abort, no?

This is due to an LLVM limitation. LLVM selects the unwinding metadata format (DWARF, SJLJ, SEH, etc) based on the name of the personality function. So we have to use __CxxFrameHandler3 to generate SEH unwinding metadata.

@Amanieu Does #92845 change this? I'm not really able to answer any of @ChrisDenton's questions above, but it seems like we should now be linking a personality function in std rather than __CxxFrameHandler3.

No, that PR only affects platforms using DWARF unwinding since we provide our own personality function there. For SEH we link to __CxxFrameHandler3 from msvcrt.


@ChrisDenton Does -Zbuild-std fix this issue? It may just be because we build the standard library with panic=unwind so that it can be used with both panic=unwind and panic=abort.

@ChrisDenton
Copy link
Contributor Author

Ah, I tried:

cargo +nightly build -Z build-std=core,panic_abort,alloc,std --target x86_64-pc-windows-msvc

And __CxxFrameHandler3 was not linked. So I think you're right.

@BatmanAoD
Copy link
Member

@ChrisDenton Does that solve your issue, or do you still consider this a bug?

@ChrisDenton
Copy link
Contributor Author

Yes, I have a workflow that works and I don't think this issue is tracking anything useful. So I'll close this now.

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 15, 2022
@wwylele
Copy link
Contributor

wwylele commented Mar 24, 2023

Hi, I encountered this issue today. While I understand the cause and the available workaround, it is still a nightly-only solution (you can't have rebuild core flag on stable yet). Should we reopen this issue till a better solution is created?

I also want to note that

  • a simple arithmetic expression can cause linking to the handler due to overflow check (yes this can be turned off but it is not obvious at first glance)
  • Some projects need, or default to, C++-less linking. In my case, it is a Windows kernel driver.

@BatmanAoD
Copy link
Member

@wwylele Sorry, I'm a bit out of my depth here, but:

  • I assume you're not using no_std? I would expect that to be one way to avoid this issue in stable.
  • I believe that despite the name, __CxxFrameHandler3 is still linkable even without C++. msvcrt.lib is the C runtime. Are you sure you need your driver not to link msvcrt?

@BatmanAoD
Copy link
Member

I modified the example code from this issue to have a simple no_std app (copied from a related PR), and indeed it builds and runs without issue.

I'm not sure what would be needed to fix this issue for apps using std.

@wwylele
Copy link
Contributor

wwylele commented Mar 25, 2023

@BatmanAoD I am using no_std. Here is my full example setup:

rust project:

[package]
name = "panicbug"
version = "0.0.0"
edition = "2021"

[lib]
name = "panicbug"
crate-type = ["staticlib"]

[profile.dev]
panic = "abort"

[profile.release]
panic = "abort"

[dependencies]
#![no_std]

#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    loop {}
}

#[export_name = "foo"]
pub extern "C" fn foo(a: i32, b: i32) -> i32 {
    a + b // has overflow check in debug build
}

Then in Visual Studio 2019, with WDK installed, create a default kernel mode driver (KMDF) project. Switch to x64 (same as the rust library). First build the solution to make sure the vanilla default code builds. Then add the following code in Driver.c:

int foo(int, int);

...
DriverEntry(
    _In_ PDRIVER_OBJECT  DriverObject,
    _In_ PUNICODE_STRING RegistryPath
    )
{
...
foo(1,1);
...
}

And also add library output from rust to the dependency in the visual studio project (Project/Properties/Linker/General/Additional Library Diretories, then ...Linker/Input/Additional Dependencies), then build. The error is:

Build started...
1>------ Build started: Project: KMDF Driver1, Configuration: Debug x64 ------
1>Building 'KMDF Driver1' with toolset 'WindowsKernelModeDriver10.0' and the 'Universal' target platform.
1>Stamping x64\Debug\KMDFDriver1.inf
1>Stamping [Version] section with DriverVer=03/24/2023,22.29.49.386
1>G:\KMDF Driver1\KMDF Driver1\KMDFDriver1.inf(5-5): warning 1324: [Version] section should specify PnpLockdown=1.
1>Device.c
1>Driver.c
1>Queue.c
1>Generating Code...
1>panicbug.lib(core-33d812eb184c6410.core.7c40f712-cgu.0.rcgu.o) : error LNK2001: unresolved external symbol __CxxFrameHandler3
1>panicbug.lib(core-33d812eb184c6410.core.7c40f712-cgu.0.rcgu.o) : error LNK2001: unresolved external symbol _fltused
1>G:\KMDF Driver1\x64\Debug\KMDFDriver1.sys : fatal error LNK1120: 2 unresolved externals
1>Done building project "KMDF Driver1.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

(bonus: the floating point dependency _fltused is also linked, even though there is no float in sight. I assume this is also part of the panic unwind code)
If you change the dependency to the release build from rust, the error goes away

@wwylele
Copy link
Contributor

wwylele commented Mar 25, 2023

It won't surprise me that the default Windows driver project doesn't link to the normal msvcrt.lib

@ChrisDenton
Copy link
Contributor Author

I believe core is pre-built with panic-unwind (thus using that exception handler) which is why they need to be rebuilt with build-std. I agree that this is frustrating.

Maybe the stable solution is to have a separate target?

@bjorn3
Copy link
Member

bjorn3 commented Mar 25, 2023

Based on https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-exceptions I believe for Windows kernel code -Cpanic=abort is not correct. Many functions can throw SEH exceptions. It is just that C++ exceptions are not allowed and thus the C++ handler __CxxFrameHandler3 isn't provided. Instead another handler function should be used. I don't know which one though.

Maybe the stable solution is to have a separate target?

Definitely.

@Amanieu
Copy link
Member

Amanieu commented Mar 25, 2023

-Cpanic=abort already does the right thing for SEH exceptions: we force generation of unwind tables, which means that SEH exceptions are properly propagated up the call stack. However we don't run destructors for SEH exceptions (which is the same behavior as C++ destructors).

@bjorn3
Copy link
Member

bjorn3 commented Mar 25, 2023

However we don't run destructors for SEH exceptions (which is the same behavior as C++ destructors).

And that is not the correct behavior. In Rust it is UB to unwind past stack frames without running destructors. It should either abort when crossing the ABI boundary (which happens for C++ exceptions in userspace, but I don't know if this happens for SEH exceptions) or it should run destructors anyway. Aborting is not an option inside a kernel driver, (especially as I think it is possible to cause a kernel SEH exception from userspace) so we have to run destructors on SEJ exceptions anyway. This is effectively the same as wrapping every function call with a __try + __finally block in MSVC C, right? Which is fully supported in the kernel.

@BatmanAoD
Copy link
Member

@bjorn3 I think that depends on whether the SEH exception is forced, i.e. a longjmp. In this case it's incorrect to run destructors, and it's the programmer's responsibility to make sure that there aren't any destructors that could run in the stack frames that are at risk of being unwound.

But if we're talking about a context where __try and __finally are permitted, then I'm not clear on why panic=abort is being used in the first place, or why __CxxFrameHandler3 isn't available.

@bjorn3
Copy link
Member

bjorn3 commented Mar 25, 2023

In windows kernel drivers C++ exceptions aren't allowed and as such __CxxFrameHandler3 is not available. __try and __finally are allowed however (and as such it is not forced unwinding). In fact seem to be required to correctly handle userspace memory accesses as those may refer to unmapped memory. At least according to https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/handling-exceptions

@BatmanAoD
Copy link
Member

Huh...that's definitely not what I would have expected! So unwinding (with user-defined cleanup operations) is permissible, but it's incompatible with C++ exceptions. That does sound like it warrants being defined as a separate platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows-msvc Toolchain: MSVC, Operating system: Windows regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants