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

std: Update the backtrace crate submodule #79237

Merged
merged 1 commit into from
Nov 21, 2020

Conversation

alexcrichton
Copy link
Member

This commit updates the library/backtrace submodule which primarily
pulls in support for split-debuginfo on macOS, avoiding the need for
dsymutil to get run to get line numbers and filenames in backtraces.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2020
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :
[command]/usr/bin/git config --local http.https://github.com/.extraheader AUTHORIZATION: basic ***
##[endgroup]
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=2 origin +d868d2c3cc3c076705b9f0ffe66bcbf0a50283d1:refs/remotes/pull/79237/merge
---
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking object v0.22.0
    Checking miniz_oxide v0.4.0
    Checking hashbrown v0.9.0
    Checking addr2line v0.14.0
error: hidden lifetime parameters in types are deprecated
   --> library/std/src/../../backtrace/src/symbolize/gimli/coff.rs:104:68
    |
104 |     pub(super) fn search_object_map(&self, _addr: u64) -> Option<(&Context, u64)> {
    |                                                                    ^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
    |
    = note: `-D elided-lifetimes-in-paths` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `std`


To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:32
== clock drift check ==
  local time: Fri Nov 20 17:21:01 UTC 2020

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@Mark-Simulacrum
Copy link
Member

r=me with CI fixed

@alexcrichton
Copy link
Member Author

@bors: r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 20, 2020

📌 Commit c8bdd52743c2e33af41cdd26dde41f4bd07d2fcb has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2020
This commit updates the `library/backtrace` submodule which primarily
pulls in support for split-debuginfo on macOS, avoiding the need for
`dsymutil` to get run to get line numbers and filenames in backtraces.
@alexcrichton
Copy link
Member Author

@bors: r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 20, 2020

📌 Commit f99410b has been approved by Mark-Simulacrum

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 21, 2020
…rk-Simulacrum

std: Update the bactrace crate submodule

This commit updates the `library/backtrace` submodule which primarily
pulls in support for split-debuginfo on macOS, avoiding the need for
`dsymutil` to get run to get line numbers and filenames in backtraces.
@bors
Copy link
Contributor

bors commented Nov 21, 2020

⌛ Testing commit f99410b with merge 2b9c608fb8061377dcadeff137c5676051077eba...

@bors
Copy link
Contributor

bors commented Nov 21, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 21, 2020
@sfackler
Copy link
Member

@bors retry

CI flake

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2020
@bors
Copy link
Contributor

bors commented Nov 21, 2020

⌛ Testing commit f99410b with merge 3adedb8...

@bors
Copy link
Contributor

bors commented Nov 21, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 3adedb8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 21, 2020
@bors bors merged commit 3adedb8 into rust-lang:master Nov 21, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 21, 2020
@ehuss
Copy link
Contributor

ehuss commented Nov 23, 2020

OMG, this is amazing! I don't know what wizardry it takes for it to find the debug info without the dSYM directory (if there is extra debug information located somewhere, what's the point of dSYM?).

For me, an incremental build of cargo goes from 65s to 11s!

Can we talk about stabilizing -Zrun-dsymutil (and maybe even making run-dsymutil=no the default?)? That would be a huge productivity boost for me.

@alexcrichton alexcrichton deleted the update-backtrace branch November 23, 2020 20:10
@alexcrichton
Copy link
Member Author

I've opened #79361 to start the discussion on that.

@bjorn3
Copy link
Member

bjorn3 commented Nov 23, 2020

and maybe even making run-dsymutil=no the default?

That would be a breaking change as it would require cargo and other tools to either run dsymutil itself or uplift (copy from target/$CHANNEL/deps to target/$CHANNEL) the temporary object files for debuginfo to work I think. At the very least it would hard break tools that expect to be able to copy the .dSYM dir.

@ehuss
Copy link
Contributor

ehuss commented Nov 23, 2020

Yea, changing the default is wishful thinking on my part, just because I don't want to have to override it, and the benefits are huge for the majority of people working on macOS that don't need dSYM for anything else. I guess a good question is, are there any projects or tools that expect the .dSYM directory to exist?

I'm wondering if it would be better to add it is a profile setting? Then I could set ~/.cargo/config.toml to have profile.dev.run-dsymutil = false, and then it should be overridden in all of my projects. The downside is that everyone working on macOS who don't need dSYM (which I assume is almost everyone) would also need to add such an override. Maybe if we add a workspace-wide edition setting, we could change the default in a future edition?

I generally hate making breaking changes like this, but the hassle of doing all the above seems like a lot to accommodate a use case that I'm uncertain anyone actually needs.

for debuginfo to work I think

I'm not sure what you mean specifically here. As long as you don't move the binary out of target/debug, then tools like lldb seem to be fine finding the .o files in target/debug/deps. AFAIK, the dSYM directory is only really useful if you move the executable somewhere else (like shipping it to a user) and you want debug symbols to work in that remote location.

@alexcrichton
Copy link
Member Author

I won't disagree that turning off dsymutil by default would be a breaking change, but as @ehuss mentioned I think this is so high-value that it's worth pursuing. Obviously we'll mitigate the impact as much as possible and won't "just do this", but ignoring this massive win for compile-times on macOS I think is not a great idea in this case.

@jyn514
Copy link
Member

jyn514 commented Nov 24, 2020

This was a moderate regression in instruction counts (up to 1.4% on incr-unchanged builds of unify-linearly-debug), mostly on debug and opt builds. Do you know what could have caused that?

The 600% decrease in incremental times is amazing 🎉 🎉 congrats to everyone making that happen :) Unfortunately it's not tracked by rustc-perf since -Z run-dsymutil=no is off by default, but it definitely counts just as much!

@jyn514 jyn514 changed the title std: Update the bactrace crate submodule std: Update the backtrace crate submodule Nov 24, 2020
@bjorn3
Copy link
Member

bjorn3 commented Nov 24, 2020

rustc-perf runs Linux, not macOS, so -Zrun-dsymutil=no wouldn't have any affect anyway.

@alexcrichton
Copy link
Member Author

It's probably due to the fact that there's just more code in libstd because there's a little bit more code in the backtrace crate to parse archives and find them on the filesystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants