-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/internal/goobj: untested code #14782
Comments
There's a lot of other linker-related overlap, some of which is only now becoming clear to me. For example, cmd/internal/obj.LSym and cmd/link/internal/ld.LSym. I believe if obj.LSym had an |
The code in goobj is meant to be able to be used by cmd/objdump to enable The new format is just about replacing the ar framing with zip framing. The On Fri, Mar 11, 2016 at 4:22 PM Matthew Dempsky notifications@github.com
|
In that case I think we should plan on eventually deleting the objfile reading code cmd/link/internal/ld/objfile.go, and using goobj instead in cmd/link. |
Please do NOT merge cmd/internal/obj and cmd/internal/ld. The first is On Fri, Mar 11, 2016 at 4:26 PM David Crawshaw notifications@github.com
|
+1 on moving the shared constants and file format documentation into a separate package that both cmd/internal/obj and cmd/link can use. Also, apparently I'm mistaken. The cmd/internal/goobj code is linked into nm, objdump, etc. It looks like it should be at least manually testable by running "go tool nm" on a compiled Go .o file. We just don't seem to have tests for that functionality though. |
I think I was still a bit confused when I wrote that. What I'm interested in is merging the two pieces of objfile reading code we have: cmd/internal/goobj/read.go and cmd/link/internal/ld/objfile.go. That would mean some kind of type sharing between cmd/internal/goobj.Sym and cmd/link/internal/ld.LSym. It looks like ld.LSym could have goobj.Sym as a field inside it, and use the goobj reading code. |
I looked at that a bit a few months back. The problem I ran into is that ld.Reloc has several more fields than goobj.Reloc but goobj.Sym has a slice of goobj.Relocs... I guess it's not that many really, so they could be added, but Xadd/Xsym don't really "belong" in goobj. The other thing is that goobj's struct have SymID's everywhere where ld has *LSyms, which IIRC ends up conflicting with the idea of using a symbol table in object files. |
CL https://golang.org/cl/20104 mentions this issue. |
CL https://golang.org/cl/37797 mentions this issue. |
Updates the disassembler to support the same object file version used by the assembler and linker. Related #14782. Change-Id: I4cd7560c4e4e1350cfb27ca9cbe0fde25fe693cc Reviewed-on: https://go-review.googlesource.com/37797 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com>
I think we can consider this fixed by CL 42971, which adds a test that exercises the code (and which was written in response to a surprise goobj failure). Feel free to reopen if you disagree. |
In CL 20489, @crawshaw observed that he was able to change the cmd/internal/obj's objfile writing code and cmd/link/internal/ld's objfile reading code, but no tests failed to indicate that cmd/internal/goobj should be updated too.
cmd/internal/goobj used to be used by cmd/newlink, but that was deleted in CL 20380 (fb880b8).
It's also still used in cmd/internal/objfile/goobj.go, but that file isn't otherwise used either.
I imagine that because of #14386, we might as well just remove the dead code.
/cc @rsc
The text was updated successfully, but these errors were encountered: