-
Notifications
You must be signed in to change notification settings - Fork 52
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
tweaks, basic DWARF-based info #69
Conversation
And autogenerating the code for the |
Thank you for contributing. I will make time to review this by the end of this week. |
Any further work on this patch? I can help to merge this. |
The conflicts must be fixed. It would probably be best also to break out the moduledata and the DWARF extraction into separate PRs. |
Oh, I'd forgotten about this PR.
Very well, I will wait for #77 to be accepted and then rework this PR. |
#77 has been merged. |
6e4f070
to
62fb40e
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #69 +/- ##
===========================================
- Coverage 78.87% 78.57% -0.30%
===========================================
Files 15 16 +1
Lines 3474 3589 +115
===========================================
+ Hits 2740 2820 +80
- Misses 518 543 +25
- Partials 216 226 +10 ☔ View full report in Codecov by Sentry. |
Maybe we can add some sample with debug info to gold. |
I noticed |
Even |
Huh, you're right, @Zxilly . Somewhat surprised to see that PE is on that list. I gave the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
hope this PR get merged soon as it may conflicted with #86, blocking the further development |
Rebased to fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of places where it results in a panic. Since this is a library, I think we should try to not bring down the main application if we can. Please switch to errors instead.
It could be good to add some tests too. Either by using "golden" binaries or building some on the fly like it's done in https://github.com/goretk/gore/blob/develop/slow_test.go.
Those panics were definitely not intended to be there, whoops. Also, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor fix needed and this is ready to be merged.
The CI error was caused by go 1.22, which moduledata currently not exist in the main branch. After #88 merged it gets resolved. |
#88 has been merged. Please try again. |
Seems works fine now. |
This PR sets up the groundwork for getting DWARF-based info from ELF files, and adds fetching the built-in goroot and build version using it as examples, which currently otherwise require hacks like disassembly of code. The addresses and values of various other values (e.g
runtime.firstmoduledata
for the moduledata, or various type definitions) could be located as well.This, of course, has the drawback of only working on ELF and requiring DWARF to not be stripped, though it should be portable across all Go platforms which use ELF, both by OS and CPU architecture. And DWARF is, by default, included.
This PR also includes some housekeeping and tweaks:
typeOffset()
getSectionDataFromOffset
togetSectionDataFromAddress
for clarity (the "offset" passed as an argument is a memory address, not a file offset)io/ioutil
module with equivalent codef.Seek(x, 0)
calls withf.Seek(x, io.SeekStart)
to remove the magic valuego generate
to updategopkg_gen.go
andstdpkg_gen.go
testdata/build.go
(todo: PR to /goretk/testdata)