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

Replace libbacktrace with a native Rust library #46439

Closed
bstrie opened this issue Dec 2, 2017 · 11 comments
Closed

Replace libbacktrace with a native Rust library #46439

bstrie opened this issue Dec 2, 2017 · 11 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Dec 2, 2017

One of our last remaining C dependencies is libbacktrace. unwind-rs, from the gimli project, looks to be a promising replacement: https://github.com/gimli-rs/unwind-rs . Evaluate its suitability and continue the inexorable oxidation of the compiler.

@bstrie bstrie added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 2, 2017
@alexcrichton
Copy link
Member

Note that I don't think this is quite right in the sense that, despite the name, we don't actually use libbacktrace for unwinding. Instead we primarily use it for symbolication of a backtrace (mapping from an address to symbol name, file name, line number, etc). We also have an unwinder but that comes from a variety of sources and is often way trickier to get right on all the platforms.

A good incremental first step would be indeed to replace libbacktrace, but probably with something like addr2line instead of for unwinding functionality.

@bstrie bstrie changed the title Replace libbacktrace with unwind-rs Replace libbacktrace with a native Rust library Dec 2, 2017
@bstrie
Copy link
Contributor Author

bstrie commented Dec 2, 2017

Updated the title to be more general.

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 26, 2018
@est31
Copy link
Member

est31 commented Jun 7, 2018

Hmm so I've tried including addr2line as a crate in the Cargo.toml of libstd. Trying to compile gives me a bunch of errors like these, for a bunch of crates:

error[E0463]: can't find crate for core

Now it seems to me like that this problem is worked around for currently present crates by adding shims for them in src/rustc/*_shim. I am not sure for how many crates I'd need a shim, but the "can't find crate for core" is shown to me 6 times so it's probably at least 6 crates, but maybe more because cargo didn't compile them all. They are also probably deep in the hierarchy.

@alexcrichton what do you think we should do here? Can this be fixed by an unstable feature in cargo that we enable in libstd's Cargo.toml?

@est31 est31 mentioned this issue Jun 7, 2018
4 tasks
@fitzgen
Copy link
Member

fitzgen commented Jun 7, 2018

@est31, FYI Alex is AFK for the next month to month and a half or so.

@est31
Copy link
Member

est31 commented Jun 7, 2018

@fitzgen thanks for pointing this out.

I'm right now in contact with @Mark-Simulacrum about ways to fix the issue. Turns out that they already have a draft version of a cargo change lying around that's very similar to the one I had in mind. I only need to rebase it.

@est31
Copy link
Member

est31 commented Jun 8, 2018

So I've taken @Mark-Simulacrum 's patch for cargo ( https://github.com/Mark-Simulacrum/cargo/tree/implicit-deps ) and rebased it and changed it to use env vars. And I also changed bootstrap to generate such an env var during build (without using most of @Mark-Simulacrum 's patch on that). Then I discovered that now every single invocation of cargo that goes not through Builder::cargo in bootstrap/builder.rs causes Cargo.lock to be changed because then Cargo doesn't get the list of overrides... This already happens at least once every time during the normal build as bootstrap.py calls cargo run on bootstrap. That can be fixed by removing bootstrap from the workspace but there are far far more places that use bare cargo and I don't think we want to track down every single use of it. It makes life harder for any outside contributor who wants to e.g. cargo update a given crate or such. So I've shelved my work onto these two branches (cargo, rustc).

Changing the feature to be cargo-centric like enabling it via directives in Cargo.toml is a bit complicated and I don't know much how Cargo works internally.

@Mark-Simulacrum suggested that the easiest fix would be to run sth like cargo build -p alloc -p core etc before we compile addr2line. That's the next option I explore.

@RalfJung
Copy link
Member

Nowadays we have infrastructure to make libstd depend on crates.io, and that is working fine -- seems to me that should help with some of the issues that came up here earlier.

@est31
Copy link
Member

est31 commented May 15, 2019

@RalfJung maybe some additional work has happened since #56092, but if it reflects the most recent state of affairs, then you need to add inclusion-into-std-special invocations to the Cargo.toml of every crate you are using, recursively. See this commit as an example for the work needed.

For something like the backtrace crate which has few rust-based dependencies, this is quite easy, but this is the recursive dependency tree of the addr2line crate as of latest git commit:

addr2line v0.9.0
├── cpp_demangle v0.2.12
│   └── cfg-if v0.1.9
│   [build-dependencies]
│   └── glob v0.2.11
├── fallible-iterator v0.2.0
├── gimli v0.18.0
│   ├── arrayvec v0.4.7
│   │   └── nodrop v0.1.12
│   ├── byteorder v1.2.3
│   ├── fallible-iterator v0.2.0 (*)
│   └── stable_deref_trait v1.1.1
├── intervaltree v0.2.3
│   └── smallvec v0.4.4
├── lazycell v1.0.0
├── object v0.12.0
│   ├── goblin v0.0.22
│   │   ├── log v0.4.1
│   │   │   └── cfg-if v0.1.9 (*)
│   │   ├── plain v0.2.3
│   │   └── scroll v0.9.0
│   │       └── scroll_derive v0.9.4
│   │           ├── proc-macro2 v0.4.6
│   │           │   └── unicode-xid v0.1.0
│   │           ├── quote v0.6.3
│   │           │   └── proc-macro2 v0.4.6 (*)
│   │           └── syn v0.14.2
│   │               ├── proc-macro2 v0.4.6 (*)
│   │               ├── quote v0.6.3 (*)
│   │               └── unicode-xid v0.1.0 (*)
│   ├── scroll v0.9.0 (*)
│   └── uuid v0.7.4
├── rustc-demangle v0.1.8
└── smallvec v0.6.1

It would require quite many PRs to add that special text to all those crates. Any additional crates they rely on in future versions would need that text, too. Instead, I suggest that someone revives my PR #51440. It only requires no_std support from the crates.

@RalfJung
Copy link
Member

For something like the backtrace crate which has few rust-based dependencies, this is quite easy, but this is the recursive dependency tree of the addr2line crate as of latest git commit:

Ouch. :( Yes, that would still be quite painful today.

@est31
Copy link
Member

est31 commented Mar 21, 2020

See also rust-lang/backtrace-rs#189

@Aaron1011
Copy link
Member

This was implemented in #74682, so I believe this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. 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