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

Enable gimli-symbolize to be used by libstd #328

Closed
alexcrichton opened this issue May 12, 2020 · 8 comments
Closed

Enable gimli-symbolize to be used by libstd #328

alexcrichton opened this issue May 12, 2020 · 8 comments
Labels
gimli Related to the gimli implementation

Comments

@alexcrichton
Copy link
Member

alexcrichton commented May 12, 2020

Currently the gimli-symbolize feature also enables the std feature. This means that it's not suitable for inclusion into the standard library. This also means that now that we've switched this crate to using gimli by default the standard library may languish! All-in-all, let's put the final nail in the coffin of libbacktrace and switch to gimli by enabling libstd to use gimli.

Some initial words have been said about this on #189, but the general state of the world is that the gimli implementation in this crate only uses a bunch of functions from std. This ranges from:

  • std::env::current_exe() - used to parse own debuginfo
  • std::fs::* - used to find, locate, and open debuginfo. Often the current executable, macOS is much more involved though
  • std::{path,ffi}::* - manipulating paths to get passed around in various places, along with reading C strings on Linux.

I don't think it is currently clear at this time how we're going to implement this. There's a few possible strategies that have been proposed so far:

  1. Vendor an implementation of everything into this crate. This would involve duplicating code from libstd into this crate, calling all the raw syscalls ourselves. One major downside of this is that as the debuginfo-finding process gets more complicated this syscall layer is undoubtedly going to get more complicated. Additionally std::env::current_exe() is already significantly complicated to run on all platforms.
  2. Have some sort of trait which libstd implements. The standard library would then pass down its implementation into this crate. This trait would be a bit of a pain to maintain over time, however.
  3. Include this library as a submodule into libstd. This is actually much easier after the recent rejiggering of features, so this may be a a somewhat plausible route. It would involve sync'ing dependencies with libstd, however, and that may not be trivial over time. Another downside of this approach is that it's somewhat difficult to maintain this style of coding over time. It's pretty nonstandard and would likely turn away possible contributors.
  4. Split the sys module out of libstd into a separate crate. The backtrace crate would then un-stably depend on this sys module. This is a huge change for libstd, however, and highly unlikely to happen.

I'm definitely open to more ideas if folks have them :)

@alexcrichton alexcrichton added the gimli Related to the gimli implementation label May 12, 2020
@alexcrichton
Copy link
Member Author

One thing I should also mention though is that we likely want to let the gimli support bake on crates.io for some time before actually moving on this. There's undoubtedly various porting issues and/or bugs that will arise, and it'd be great to weed those out before we move into libstd.

@fitzgen
Copy link
Member

fitzgen commented May 12, 2020

FWIW, option (2) seems like the best course of action to me, based on gut feeling.

@alexcrichton
Copy link
Member Author

Taking the submodule route is actually surprisingly easy to get working.

This is the change to this crate which can be summarized with:

  • Remove usage of use crate:: to use relative super paths instead
  • Add a #[cfg] to define a local mod std { pub use crate::* } which "acts like libstd"
  • Route all use std through that module

This is the change to libstd which looks like:

  • Add some [patch] for object/addr2line/gimli, but that's just temporary. Those patches are easily upstreamed.
  • Inline dependencies of the backtrace crate directly into libstd's Cargo.toml
  • Feign the "gimli-symbolize" feature in libstd's Cargo.toml
  • Create a backtrace_rs module which points to the source of the crate. Adjust all paths in libstd referencing the previous backtrace_rs crate to reference this submodule of libstd instead.
  • Allow dead code/unused attributes in the backtrace_rs module to avoid dealing with warnings we don't want to fix.

With those two applied I was able to get everything working locally.

I was thinking again about the impact of developers on this crate, and I'm actually less worried than I was before. You'll still have access to all of core and alloc and such, it's just the few use std:: paths in gimli.rs which are a bit wonky. In that sense it's not really all that bad. The lack of use crate:: is pretty bad, but a CI lint could enforce that here in this crate, and it's easy enough to work around.

Given the level of ease to get this working, I'm pretty hesitant to use traits/duplication/vendoring code. This seems like it might be the way to go. It means updates are a bit more of a pain because it's a git submodule rather than a crates.io dependency, but code-wise it's quite light...

@est31
Copy link
Member

est31 commented May 13, 2020

The submodule method is also future-compatible with a possible (future) std/core unification.

@alexcrichton
Copy link
Member Author

With the merge of #344 I've additionally worked to get two more crates working as part of a dependency of libstd:

My assumption is that we'll want to enable zlib decompression by default like libstd does today, so I think we'll need to merge these before including into rust-lang/rust.

alexcrichton added a commit that referenced this issue Jun 16, 2020
This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.
alexcrichton added a commit that referenced this issue Jun 16, 2020
This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.
alexcrichton added a commit that referenced this issue Jun 16, 2020
This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.
alexcrichton added a commit that referenced this issue Jun 29, 2020
This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.
alexcrichton added a commit that referenced this issue Jul 7, 2020
This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.
alexcrichton added a commit that referenced this issue Jul 13, 2020
* Prepare for this crate to go into libstd

This commit is a preparation of this crate to be included as a submodule
into the standard library. I'm not 100% sold on this yet but I'm
somewhat convinced that this is going to happen this way. This is
progress on #328 and a preview of what it might look like to implement
this strategy.

Currently I don't plan to merge this to the `master` branch unless it's
decided to move forward with this integration strategy of the
gimli feature of the backtrace crate.

* Update as-if-std integration
@luser
Copy link

luser commented Jul 22, 2020

Do you think this is a viable path for this crate and any others that libstd may want to include in the future, or do you think there would be value in exploring whether some set of language/library changes could make this easier to do?

@alexcrichton
Copy link
Member Author

Personally, no, I don't think this is viable for many other crates to use. It's a real pain managing submodules and architecting the crate such that it may be included as a submodule but also builds as a standalone crate. I think that this strategy only really makes sense if it applies to a small handful of crates (e.g. 1 or 2).

I think it seems reasonable to explore changes to make this more viable, but I don't really know what the best change would be. Everything is pretty large scale compared to the maintenance burden on one crate.

@alexcrichton
Copy link
Member Author

This was done awhile back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gimli Related to the gimli implementation
Projects
None yet
Development

No branches or pull requests

4 participants