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

Implement line debuginfo #291

Merged
merged 9 commits into from
Jan 26, 2019
Merged

Implement line debuginfo #291

merged 9 commits into from
Jan 26, 2019

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 17, 2019

cc @philipc because you are the author of a big part of this
cc #166

blocked on gimli-rs/gimli#362

@bjorn3 bjorn3 added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 17, 2019
@bjorn3 bjorn3 added this to the MVP milestone Jan 17, 2019
@philipc
Copy link
Contributor

philipc commented Jan 18, 2019

Unfortunately I had done a couple more commits and forgot to push them :( So a bit of duplicated work there. Not sure if you want to go through my changes and see if there is anything you need, or if you want me to just review what you've done.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 18, 2019

There are a few little changes from your branch I want to incorporate in this PR.

@bjorn3 bjorn3 force-pushed the debuginfo_line branch 2 times, most recently from 929a338 to 3f247cc Compare January 19, 2019 10:56
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 19, 2019

I am working on macOS support, but you can already review this @philipc.

Edit: moved the rest of this comment to #303

@philipc
Copy link
Contributor

philipc commented Jan 20, 2019

I think there's two issues here.

First, yes, .debug_ relocations are not needed for MachO, but that is already handled in faerie, as discussed before. Of course, it doesn't hurt to omit them here too.

Second, MachO relocations use implicit addends, so you need to write the addend instead of the 0. This will apply to other targets too (eg ELF EM_386). faerie doesn't handle this case for us (and I think this assert in cranelift is the same problem).

src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
@bjorn3 bjorn3 mentioned this pull request Jan 26, 2019
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 26, 2019

I am going to merge. I will try to fix the macOS problems later.

@bjorn3 bjorn3 merged commit 3e8ed85 into master Jan 26, 2019
@bjorn3 bjorn3 deleted the debuginfo_line branch January 26, 2019 12:10
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 26, 2019

Thanks for all the great work @philipc!

@bjorn3 bjorn3 mentioned this pull request Jan 27, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants