Skip to content

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jan 26, 2018

This commit adds the ability for rustc to not run dsymutil by default
on OSX. A new codegen option, -Z run-dsymutil=no, was added to specify
that dsymutil should not run and instead the compiler should
unconditionally keep the object files around in a compilation if
necessary for debug information.

cc #47240

@rust-highfive
Copy link
Contributor

r? @estebank

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

/// Returns a boolean indicating whether we should preserve the object files on
/// the filesystem for their debug information. This is often useful with
/// split-dwarf like schemes.
fn preserve_objects_for_their_debuginfo(sess: &Session) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sort of hoping that we can use a function like this to enable split dwarf on Linux over time as well!

@alexcrichton
Copy link
Member Author

r? @michaelwoerister

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2018
@kennytm
Copy link
Member

kennytm commented Jan 26, 2018

Would -C run-dsymutil=no cause RUST_BACKTRACE=1 to not have any line numbers?

@alexcrichton
Copy link
Member Author

A good question and one I forgot to verify! Looks like the answer is "no" though which is a bummer!

@JohnColanduoni, out of curiosity, do you know if there's a way to get libbacktrace to relatively easily do the same logic that lldb is doing? (looking for object files in addition to the dSYM)

@JohnColanduoni
Copy link
Contributor

@alexchrichton Not easily, it would probably make more sense to add this feature as part of a pure-Rust rewrite of libbacktrace. I didn’t know lldb had this functionality on Mac, I’ve had issues where lldb refuses to give me line numbers if Spotlight hasn’t yet indexed tbe dSYM.

@alexcrichton
Copy link
Member Author

Ah ok, thanks for the info @JohnColanduoni! In light of that I've now switched this to a -Z flag instead of a -C flag so we can continue to tweak it. I think that we'll want this option though to play around with over time still with incremental compilation.

@@ -77,7 +77,7 @@ pub fn get_linker(sess: &Session) -> (PathBuf, Command, Vec<(OsString, OsString)
Command::new(linker)
};

if let Some(ref linker) = sess.opts.cg.linker {
if let Some(ref linker) = sess.opts.debugging_opts.linker {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've changed the wrong variable...

[00:18:57] error[E0609]: no field `linker` on type `rustc::session::config::DebuggingOptions`
[00:18:57]   --> librustc_trans/back/link.rs:80:56
[00:18:57]    |
[00:18:57] 80 |     if let Some(ref linker) = sess.opts.debugging_opts.linker {
[00:18:57]    |                                                        ^^^^^^ unknown field
[00:18:57]    |
[00:18:57]    = note: available fields are: `codegen_backend`, `verbose`, `span_free_formats`, `identify_regions`, `emit_end_regions` ... and 89 others

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er oops!

// *not* running dsymutil then the object files are the only source of truth
// for debug information, so we must preserve them.
if sess.target.target.options.is_like_osx {
match sess.opts.cg.run_dsymutil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... the cgdebugging_opts should be applied here 😄

[00:18:57] error[E0609]: no field `run_dsymutil` on type `rustc::session::config::CodegenOptions`
[00:18:57]    --> librustc_trans/back/link.rs:224:28
[00:18:57]     |
[00:18:57] 224 |         match sess.opts.cg.run_dsymutil {
[00:18:57]     |                            ^^^^^^^^^^^^ unknown field
[00:18:57]     |
[00:18:57]     = note: available fields are: `ar`, `linker`, `link_arg`, `link_args`, `link_dead_code` ... and 28 others

@BatmanAoD
Copy link
Member

@kennytm @alexcrichton Something seems odd here; GitHub isn't displaying the commits in which kennytm's review comments were resolved, if indeed they were resolved. What is the status on this? (Also, does @michaelwoerister still need to review, since kennytm already did?)

@alexcrichton
Copy link
Member Author

Ah I think we're just waiting on review! @michaelwoerister I believe is on leave right now, but this isn't a particularly pressing PR so I'd be fine waiting.

@bors
Copy link
Collaborator

bors commented Feb 7, 2018

☔ The latest upstream changes (presumably #48053) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2018
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2018
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks, @alexcrichton!

@@ -168,7 +168,9 @@ pub(crate) fn link_binary(sess: &Session,
if !sess.opts.cg.save_temps {
if sess.opts.output_types.should_trans() {
for obj in trans.modules.iter().filter_map(|m| m.object.as_ref()) {
remove(sess, obj);
if !preserve_objects_for_their_debuginfo(sess) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it makes much of a difference but this if could be pulled out of the loop, I think.

This commit adds the ability for rustc to not run `dsymutil` by default
on OSX. A new codegen option, `-Z run-dsymutil=no`, was added to specify
that `dsymutil` should *not* run and instead the compiler should
unconditionally keep the object files around in a compilation if
necessary for debug information.

cc rust-lang#47240
@alexcrichton
Copy link
Member Author

@bors: r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Feb 12, 2018

📌 Commit 0f16eee has been approved by michaelwoerister

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 12, 2018
@alexcrichton
Copy link
Member Author

@bors: rollup

kennytm added a commit to kennytm/rust that referenced this pull request Feb 13, 2018
…elwoerister

rustc: Add the ability to not run dsymutil

This commit adds the ability for rustc to not run `dsymutil` by default
on OSX. A new codegen option, `-Z run-dsymutil=no`, was added to specify
that `dsymutil` should *not* run and instead the compiler should
unconditionally keep the object files around in a compilation if
necessary for debug information.

cc rust-lang#47240
bors added a commit that referenced this pull request Feb 13, 2018
Rollup of 14 pull requests

- Successful merges: #47784, #47846, #48033, #48083, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48163, #48165, #48167
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
…elwoerister

rustc: Add the ability to not run dsymutil

This commit adds the ability for rustc to not run `dsymutil` by default
on OSX. A new codegen option, `-Z run-dsymutil=no`, was added to specify
that `dsymutil` should *not* run and instead the compiler should
unconditionally keep the object files around in a compilation if
necessary for debug information.

cc rust-lang#47240
bors added a commit that referenced this pull request Feb 14, 2018
bors added a commit that referenced this pull request Feb 15, 2018
bors added a commit that referenced this pull request Feb 15, 2018
@kennytm kennytm merged commit 0f16eee into rust-lang:master Feb 15, 2018
@alexcrichton alexcrichton deleted the less-dsymutil branch February 26, 2018 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants