rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
537 stars 246 forks source link

gimli: Enable backtrace for FreeBSD #402

Closed cemeyer closed 3 years ago

cemeyer commented 3 years ago

384 attempt number 2.

Closes #325.

cemeyer commented 3 years ago

It is just @lzutao's earlier PR, rebased over the uclibc merge conflict.

alexcrichton commented 3 years ago

Thanks! Is this ready to land with libc changes published?

cemeyer commented 3 years ago

Rust newbie — how would I find out? On #384 lzutao mentioned:

This doesn't block on libc anymore.

Edit: dl_iterate_phdr was merged Oct 27; libc 0.2.81 was published Dec 6. That contains the dl_iterate_phdr addition:

https://github.com/rust-lang/libc/releases/tag/0.2.81 => 70962e3943d22d94141954db18eb7295b5edec5d

https://github.com/rust-lang/libc/blob/70962e3943d22d94141954db18eb7295b5edec5d/src/unix/bsd/freebsdlike/mod.rs#L1588-L1598

So I think we're good!

alexcrichton commented 3 years ago

To confirm, have you tested this on the platforms enabled here? If so I think the libc dependency's version should be bumped in this crate's version requirement because we'll require a newer version with the bindings.

cemeyer commented 3 years ago

I have not. I can test on FreeBSD but I don’t have any of the other systems. Would that just be cargo test, or any additional manual testing? I can bump the libc requirement in cargo.toml. Thanks!

alexcrichton commented 3 years ago

Yes a cargo test wouold be great. I'm hesitant to enable platforms here unless they've been tested since this switch likely pulls in a big chunk of code.

cemeyer commented 3 years ago

Yes a cargo test wouold be great.

Ok, that was fast. I think I must have already run it. It passes. However, I think the tests may need platform adjustment as well to actually cover this change. Give me a few minutes... Edit: well, I don't know how to tell if we are actually testing gimli-symbolize-ation on FreeBSD.

I'm hesitant to enable platforms here unless they've been tested since this switch likely pulls in a big chunk of code.

They're tier 2+ platforms, right? I'm worried we might be applying higher scrutiny to restoring functionality that was lost in rust 1.47 than we were in losing it in the first place. At the same time, I don't feel strongly about Open/NetBSD support and am ok dropping them if you insist.

alexcrichton commented 3 years ago

My main goal is that if you're explicitly affecting new targets in this PR there should at least be confidence that the code compiles. It doesn't sound like you're checking that for all the platforms listed here? I would ideally like confirmation that the tests work with this PR. If there are other bugs then they should be filed as new issues.

cemeyer commented 3 years ago

Ok, I’ll drop the non-FreeBSD diff. Cargo test passes.

alexcrichton commented 3 years ago

Thanks!

cemeyer commented 3 years ago

Thank you! :-)