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

gimli: Enable backtrace for FreeBSD #402

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

cemeyer
Copy link
Contributor

@cemeyer cemeyer commented Jan 1, 2021

#384 attempt number 2.

Closes #325.

@cemeyer
Copy link
Contributor Author

cemeyer commented Jan 1, 2021

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

@alexcrichton
Copy link
Member

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

@cemeyer
Copy link
Contributor Author

cemeyer commented Jan 4, 2021

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
Copy link
Member

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
Copy link
Contributor Author

cemeyer commented Jan 4, 2021

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
Copy link
Member

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 cemeyer force-pushed the unbreak_backtrace_bsds branch from 277e2d9 to eca94be Compare January 4, 2021 18:44
@cemeyer
Copy link
Contributor Author

cemeyer commented Jan 4, 2021

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
Copy link
Member

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
Copy link
Contributor Author

cemeyer commented Jan 4, 2021

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

@cemeyer cemeyer force-pushed the unbreak_backtrace_bsds branch from eca94be to 4c7f391 Compare January 4, 2021 19:59
@cemeyer cemeyer changed the title gimli: Enable backtrace for BSDs gimli: Enable backtrace for FreeBSD Jan 4, 2021
@alexcrichton alexcrichton merged commit 6f4992c into rust-lang:master Jan 4, 2021
@alexcrichton
Copy link
Member

Thanks!

@cemeyer
Copy link
Contributor Author

cemeyer commented Jan 4, 2021

Thank you! :-)

@cemeyer cemeyer deleted the unbreak_backtrace_bsds branch January 4, 2021 21:37
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.

Gimli doesn't support FreeBSD
2 participants