search

rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
532 stars 245 forks source link

Switch from `winapi` to `windows-sys` #634

Closed CraftSpider closed 2 months ago

CraftSpider commented 3 months ago

This is a rebase of #494, as it was mentioned in #537 that there was potential interest in such a thing.

CraftSpider commented 3 months ago

Currently, this avoids actually depending on windows-bindgen at build-time by simply using a file generated with the arguments in bindings.txt. This is similar to how std does it, and avoids issues with as-if-std. While similar to the previous method, this way the windows system APIs are generated entirely by the same system as windows-sys, and are based on the official API metadata.

ChrisDenton commented 3 months ago

If we're using generated bindings then I don't think we need verify-windows-sys. There's no need to hand-write the bindings when we can use the generated bindings directly.

EDIT: never mind, that won't work because windows-bindgen doesn't have a way to generate delay-loaded imports.

CraftSpider commented 3 months ago

~If we're using generated bindings then I don't think we need verify-windows-sys. There's no need to hand-write the bindings when we can use the generated bindings directly.~

EDIT: never mind, that won't work because windows-bindgen doesn't have a way to generate delay-loaded imports.

It may be a good idea to remove the feature and make the assertion unconditional - it's purely compile-time, and since we are comparing to the auto-generated bindings, we always have access to the original definitions. Either a feature in windows-bindgen to generate type for each function, or access to typeof in Rust, would let us just use the binding types directly, but alas.

workingjubilee commented 2 months ago

Thanks for rebasing! Finally figured out what busted CI so I'm gonna merge this through finally.

kleisauke commented 1 month ago

FWIW, this breaks the tier-3 thumbv7a-pc-windows-msvc and thumbv7a-uwp-windows-msvc targets. For example, IMAGE_FILE_MACHINE_ARMNT is gone now. https://github.com/rust-lang/backtrace-rs/blob/38d49aa0395c50047ab26e418a0cb30d9955fb64/src/backtrace/dbghelp32.rs#L222

PR https://github.com/rust-lang/rust/pull/112527 could be relevant here.

workingjubilee commented 1 month ago

Tier 3 targets are allowed to be broken. If they weren't, I wouldn't be able to even land this PR. If someone wants them to be supported, they need to start maintaining them. https://doc.rust-lang.org/rustc/target-tier-policy.html

workingjubilee commented 1 month ago

And the appropriate venue for this is not in the tail end of a PR, but a new issue.

kleisauke commented 1 month ago

Sorry, the last time I dealt with a similar issue (see #572) the fix was straightforward, but this is somewhat non-trivial as windows-sys lacks platform-specific type definitions for ARM32.

That said, this is a good opportunity to remove this unsupported Microsoft target from my toolchain. If anyone fancies it, they can fix this, but it won't be me this time.