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

Add llvm debuginfo configure option #37742

Merged
merged 3 commits into from Nov 15, 2016

Conversation

Projects
None yet
7 participants
@mrhota
Contributor

mrhota commented Nov 13, 2016

CC @nnethercote @Mark-Simulacrum

We add a new configure option, --enable-llvm-debuginfo, to do exactly what you'd think.

Re: #31033

Fixes #37738

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 13, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Nov 13, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Nov 13, 2016

Contributor

There’s at least two things missing:

  1. Corresponding changes to rustbuild (src/bootstrap) and build scripts (src/librustc_llvm/build.rs);
  2. Possibly debug info generation for llvm wrapers (in src/rustllvm)
Contributor

nagisa commented Nov 13, 2016

There’s at least two things missing:

  1. Corresponding changes to rustbuild (src/bootstrap) and build scripts (src/librustc_llvm/build.rs);
  2. Possibly debug info generation for llvm wrapers (in src/rustllvm)
@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Nov 13, 2016

Contributor

Note that rustbuild already includes support for RelWithDebInfo (since a few days ago, actually).

So the only thing to be done for rustbuild (I think) is to add a line like

("LLVM_DEBUGINFO", self.llvm_release_debuginfo),

here: https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L342

Contributor

TimNN commented Nov 13, 2016

Note that rustbuild already includes support for RelWithDebInfo (since a few days ago, actually).

So the only thing to be done for rustbuild (I think) is to add a line like

("LLVM_DEBUGINFO", self.llvm_release_debuginfo),

here: https://github.com/rust-lang/rust/blob/master/src/bootstrap/config.rs#L342

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Nov 13, 2016

Contributor

@TimNN hmm, should I change the name of the --enable-llvm-debuginfo option? I didn't know anything about rustbuild until just now!

Contributor

mrhota commented Nov 13, 2016

@TimNN hmm, should I change the name of the --enable-llvm-debuginfo option? I didn't know anything about rustbuild until just now!

Let rustbuild parse llvm debuginfo option
update_with_config_mk() needs to read the new llvm debuginfo config
option from config.mk. Other than that, rustbuild already supports
LLVM's RelWithDebInfo build type.
@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Nov 13, 2016

Contributor

@mrhota: I'm honestly not sure. When choosing the name for rustbuild, I went with llvm_release_debuginfo because it seemed more accurate: We already had llvm-optimize, which, if not enabled, always included debuginfo, so the new setting is really only applicable whenllvm-optimize is enabled.

So one could argue to use --enable-llvm-release-debuginfo for consistency & accuracy or for --enable-llvm-debuginfo since it's shorter / easier to remember. (Note that for rustbuild, we have a config.toml file and the option is present in the [llvm] section so it's only called release-debuginfo).

I think I would just stay with --enable-llvm-debuginfo.

Contributor

TimNN commented Nov 13, 2016

@mrhota: I'm honestly not sure. When choosing the name for rustbuild, I went with llvm_release_debuginfo because it seemed more accurate: We already had llvm-optimize, which, if not enabled, always included debuginfo, so the new setting is really only applicable whenllvm-optimize is enabled.

So one could argue to use --enable-llvm-release-debuginfo for consistency & accuracy or for --enable-llvm-debuginfo since it's shorter / easier to remember. (Note that for rustbuild, we have a config.toml file and the option is present in the [llvm] section so it's only called release-debuginfo).

I think I would just stay with --enable-llvm-debuginfo.

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Nov 13, 2016

Contributor

@nagisa After glancing through the code, I don't think rustllvm needs any changes. I wouldn't know how or where to make those changes, though, so maybe someone with more expertise should weigh in.

Contributor

mrhota commented Nov 13, 2016

@nagisa After glancing through the code, I don't think rustllvm needs any changes. I wouldn't know how or where to make those changes, though, so maybe someone with more expertise should weigh in.

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Nov 13, 2016

Contributor

@TimNN hehe too late! 😄 I decided to go with your name: I agree it's more accurate. Moreover, it seems like maybe we're moving toward deprecating the configure system, so option names there might be less important. /me shrugs.

Contributor

mrhota commented Nov 13, 2016

@TimNN hehe too late! 😄 I decided to go with your name: I agree it's more accurate. Moreover, it seems like maybe we're moving toward deprecating the configure system, so option names there might be less important. /me shrugs.

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Nov 14, 2016

Contributor

cc @alexcrichton I think this is complete, but I might have missed something (esp. in rustbuild). Thoughts?

Contributor

mrhota commented Nov 14, 2016

cc @alexcrichton I think this is complete, but I might have missed something (esp. in rustbuild). Thoughts?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 14, 2016

Member

@bors: r+

Looks great to me, thanks @mrhota!

Member

alexcrichton commented Nov 14, 2016

@bors: r+

Looks great to me, thanks @mrhota!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 14, 2016

Contributor

📌 Commit d3574b8 has been approved by alexcrichton

Contributor

bors commented Nov 14, 2016

📌 Commit d3574b8 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 15, 2016

Contributor

⌛️ Testing commit d3574b8 with merge 43006fc...

Contributor

bors commented Nov 15, 2016

⌛️ Testing commit d3574b8 with merge 43006fc...

bors added a commit that referenced this pull request Nov 15, 2016

Auto merge of #37742 - mrhota:llvm_debuginfo, r=alexcrichton
Add llvm debuginfo configure option

CC @nnethercote @Mark-Simulacrum

We add a new configure option, `--enable-llvm-debuginfo`, to do exactly what you'd think.

Re: #31033

Fixes #37738

@bors bors merged commit d3574b8 into rust-lang:master Nov 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Nov 15, 2016

Contributor

Note to anyone following along: the final name chosen for the configure option was --enable-llvm-release-debuginfo.

Contributor

nnethercote commented Nov 15, 2016

Note to anyone following along: the final name chosen for the configure option was --enable-llvm-release-debuginfo.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Nov 15, 2016

Contributor

I just tried this option. I'm using configure/make to build, not rustbuild. It's not working, as far as I can tell. I did "make clean", configured with --enable-optimize --enable-debuginfo --enable--llvm-release-debuginfo, rebuilt the compiler. But my profiles of the compiler still lack filenames and line numbers for LLVM functions.

Is there some way I can check the resulting rustc binary for the debuginfo? I'm on Linux.

Contributor

nnethercote commented Nov 15, 2016

I just tried this option. I'm using configure/make to build, not rustbuild. It's not working, as far as I can tell. I did "make clean", configured with --enable-optimize --enable-debuginfo --enable--llvm-release-debuginfo, rebuilt the compiler. But my profiles of the compiler still lack filenames and line numbers for LLVM functions.

Is there some way I can check the resulting rustc binary for the debuginfo? I'm on Linux.

@TimNN

This comment has been minimized.

Show comment
Hide comment
@TimNN

TimNN Nov 15, 2016

Contributor

@nnethercote: I haven't used the make based system in a long time, however I think that make clean may not clean llvm, so you may want to try and manually remove the llvm build dir.

Contributor

TimNN commented Nov 15, 2016

@nnethercote: I haven't used the make based system in a long time, however I think that make clean may not clean llvm, so you may want to try and manually remove the llvm build dir.

@mrhota

This comment has been minimized.

Show comment
Hide comment
@mrhota

mrhota Nov 16, 2016

Contributor

@TimNN @nnethercote there is a make clean-llvm command, too. But I don't know if it does what you'd think it does. I just deleted the build dir.

Contributor

mrhota commented Nov 16, 2016

@TimNN @nnethercote there is a make clean-llvm command, too. But I don't know if it does what you'd think it does. I just deleted the build dir.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Nov 16, 2016

Contributor

Deleting the build dir fixes the problem -- I'm now getting files and line numbers for LLVM in my profiling output. Thanks!

Contributor

nnethercote commented Nov 16, 2016

Deleting the build dir fixes the problem -- I'm now getting files and line numbers for LLVM in my profiling output. Thanks!

@mrhota mrhota deleted the mrhota:llvm_debuginfo branch Nov 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment