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

Release build is crashing on startup #8

Closed
JPHutchins opened this issue Oct 28, 2024 · 13 comments · Fixed by #11
Closed

Release build is crashing on startup #8

JPHutchins opened this issue Oct 28, 2024 · 13 comments · Fixed by #11

Comments

@JPHutchins
Copy link
Contributor

JPHutchins commented Oct 28, 2024

I was working on the MSI. It's working OK, but I've discovered that --release builds are crashing on startup, whether installed via MSI or not. Debug builds work fine both with and without the installer.

cargo run --release
warning: field `stub_instance_id` is never read
  --> src\usbipd.rs:71:9
   |
51 | pub struct UsbDevice {
   |            --------- field in this struct
...
71 |     pub stub_instance_id: Option<String>,
   |         ^^^^^^^^^^^^^^^^
   |
   = note: `UsbDevice` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: `wsl-usb-manager` (bin "wsl-usb-manager") generated 1 warning
    Finished `release` profile [optimized] target(s) in 0.15s
     Running `target\release\wsl-usb-manager.exe`
error: process didn't exit successfully: `target\release\wsl-usb-manager.exe` (exit code: 0xc000001d, STATUS_ILLEGAL_INSTRUCTION)
@JPHutchins
Copy link
Contributor Author

I've add this .cargo/config.toml

[target.x86_64-pc-windows-msvc]
rustflags = ["-C", "target-feature=+crt-static"]

[profile.release]
codegen-units = 1
lto = true
opt-level = 0
debug = false
rpath = false
panic = 'unwind'
incremental = false
overflow-checks = true

It crashes on anything but opt-level 0.

@JPHutchins
Copy link
Contributor Author

JPHutchins commented Oct 28, 2024

I think it makes sense to leave optimizations at 0 for now. It's not like the code is huge or taking up a bunch of resources.

@JPHutchins JPHutchins mentioned this issue Oct 28, 2024
3 tasks
@nickbeth
Copy link
Owner

nickbeth commented Nov 7, 2024

Looks like some sort of mis-compilation issue, but I'd like to avoid disabling optimizations for release builds. I'm not too familiar with the flags you added to the cargo config, what are they for?

@JPHutchins
Copy link
Contributor Author

Agreed. This issue is reproducible on main without any of the changes listed. I do see that the binary you'd released is 756kB in size whereas the one I am producing is 752kB.

I wonder if there is anything in your environment? A .cargo folder?

@nickbeth
Copy link
Owner

nickbeth commented Nov 7, 2024

I've done a bit of debugging and I'm able to reproduce it. It seems like the count_children function is wrongly optimized to always call the panic_const_add_overflow panic handler somehow. As you said, this only happens with opt-level > 0. I'm still digging as this is very interesting, and potentially a Rust issue.

@JPHutchins
Copy link
Contributor Author

Possibly hard for the compiler to see that count is anything other than 0 here? Because count is first transferred to count_ptr then transmuted? Could it be passed count_ptr as LPARAM?

https://github.com/gabdube/native-windows-gui/blob/a6c96e8de5d01fe7bb566d737622dfead3cd1aed/native-windows-gui/src/controls/tabs.rs#L610-L619

@nickbeth
Copy link
Owner

nickbeth commented Nov 8, 2024

Not quite that, but the fact that the compiler gets very angry when you materialize references from thin air, you basically run into instant undefined behavior when you do that. I have a patch that fixes the issue, sadly native-windows-gui looks abandoned at this point but we can always fork and use the patch feature of Cargo to deploy it.

@nickbeth
Copy link
Owner

nickbeth commented Nov 8, 2024

The patch consists of replacing this transmute and this transmute with pointer casts.

@JPHutchins
Copy link
Contributor Author

compiler gets very angry when you materialize references from thin air

Would have been nice if the compiler said anything about it. It seems like working with the Windows APIs is filled with unsafe and UB.

NWG last release is 4 years ago. I will consider port to Tauri but realistically IDK when I'll have time.

@nickbeth
Copy link
Owner

I opened a PR to native-windows-gui fixing this issue. I don't expect it to ever get merged, but in the meantime I forked the repo and created a branch that works for this app. I'll create a PR that retrieves the crate from git with the fix, release builds should work again!

@JPHutchins
Copy link
Contributor Author

@nickbeth yay! BTW, what do you recommend for debug (in Windows)? Something like GDB, or VSCode or VS would be OK.

@nickbeth
Copy link
Owner

I've used the CodeLLDB extension in VSCode without many issues, very easy to setup. I guess you need LLDB installed though? I had it already and it didn't complain.

@JPHutchins
Copy link
Contributor Author

Fix confirmed!

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 a pull request may close this issue.

2 participants