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

Make a module out of debuginfo #24762

Merged
merged 12 commits into from
Apr 30, 2015
Merged

Make a module out of debuginfo #24762

merged 12 commits into from
Apr 30, 2015

Conversation

nrc
Copy link
Member

@nrc nrc commented Apr 24, 2015

Closes #20780

r? @michaelwoerister

I'm sure this could be done better with deeper knowledge of debuginfo, but this seems like a good start.

@michaelwoerister
Copy link
Member

Cool! I'll take a look at this over the weekend.

@michaelwoerister
Copy link
Member

I've taken a look at this and overall I think this is the right direction to go in. However, I would factor some things a little differently:

Metadata creation is the most complicated thing in this module. Especially for type metadata many things interconnect and coupling is rather tight. However, only the type_metadata() function really needs to be exposed to the rest of the debuginfo module, while the specialized implementations called by type_metadata() are more or less implementation details, including the TypeMap and handling of recursive types. This PR splits these things up into various files though (mod.rs, adt.rs, and metadata.rs). I would merge adt.rs into metadata.rs (maybe make adt.rs a private sub-module of metadata.rs) and move the TypeMap, UniqueTypeId, RecursiveTypeDescription, and create_and_register_recursive_type_forward_declaration into this new metadata.rs file. This way, we should be able to keep most things in metadata.rs private. Only type_metadata(), file_metadata(), scope_metadata(), and compile_unit_metadata() would be public.

Some nits:

  • I think create.rs doesn't quite carry its weight. I'd get rid of it and move is_node_local_to_unit() and create_DIArray() to utils.rs, and declare_local() to mod.rs.
  • I'd rename types.rs to naming.rs or type_naming.rs or something to that effect because it only deals with naming.
  • I'd move create_scope_map() from utils.rs to its own file since this is a rather important part of creating debuginfo (although it is just one big function).
  • Source location handling could also be moved from mod.rs into its own module but that's not a high priority.

Apart from the above, this looks good to me. I like that the GDB stuff get's its own module and that mod.rs gets condensed down to the few things that are actually needed by other parts of the compiler.

@bors
Copy link
Contributor

bors commented Apr 27, 2015

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

@nrc
Copy link
Member Author

nrc commented Apr 29, 2015

@michaelwoerister ping for r? All changes made as suggested.

@michaelwoerister
Copy link
Member

@bors r+ 7bfb5ed

@michaelwoerister
Copy link
Member

Thanks!

@bors
Copy link
Contributor

bors commented Apr 29, 2015

⌛ Testing commit 7bfb5ed with merge b2998a7...

@bors
Copy link
Contributor

bors commented Apr 29, 2015

💔 Test failed - auto-mac-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Apr 29, 2015 at 2:52 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/4760


Reply to this email directly or view it on GitHub
#24762 (comment).

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 29, 2015
Closes rust-lang#20780

r? @michaelwoerister

I'm sure this could be done better with deeper knowledge of debuginfo, but this seems like a good start.
@bors bors merged commit 7bfb5ed into rust-lang:master Apr 30, 2015
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.

Split debuginfo into its own module
4 participants