Skip to content

Add llvm debuginfo configure option #37742

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

Merged
merged 3 commits into from
Nov 15, 2016
Merged

Conversation

mrhota
Copy link
Contributor

@mrhota 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
Copy link
Contributor

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
Copy link
Member

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
Copy link
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
Copy link
Contributor Author

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!

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

mrhota commented Nov 14, 2016

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

@alexcrichton
Copy link
Member

@bors: r+

Looks great to me, thanks @mrhota!

@bors
Copy link
Collaborator

bors commented Nov 14, 2016

📌 Commit d3574b8 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 15, 2016

⌛ Testing commit d3574b8 with merge 43006fc...

bors added a commit that referenced this pull request Nov 15, 2016
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
@nnethercote
Copy link
Contributor

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

@nnethercote
Copy link
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.

@TimNN
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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 llvm_debuginfo branch November 16, 2016 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants