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

cmd/internal/goobj: untested code #14782

Closed
mdempsky opened this issue Mar 11, 2016 · 10 comments
Closed

cmd/internal/goobj: untested code #14782

mdempsky opened this issue Mar 11, 2016 · 10 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 11, 2016

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

@crawshaw
Copy link
Member

@crawshaw crawshaw commented Mar 11, 2016

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 Extra interface{} field that cmd/link could use, then we could replace most of ld.LSym. Given I did some recent cleanup of ld.LSym, I'll attempt to do the same to obj.LSym and see how close I can get to merging those types.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2016

The code in goobj is meant to be able to be used by cmd/objdump to enable
objdump of .o files. I think that never made it into objdump, but it still
should, so please don't delete the code. I guess if I hadn't deleted
cmd/newlink there would be a test. :-(

The new format is just about replacing the ar framing with zip framing. The
goobj-parsed part current called a .o file will still exist, just inside
the zip archive.

On Fri, Mar 11, 2016 at 4:22 PM Matthew Dempsky notifications@github.com
wrote:

In CL 20489, @crawshaw https://github.com/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
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
#14386, we might as well just remove
the dead code.

/cc @rsc https://github.com/rsc


Reply to this email directly or view it on GitHub
#14782.

@crawshaw
Copy link
Member

@crawshaw crawshaw commented Mar 11, 2016

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.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2016

Please do NOT merge cmd/internal/obj and cmd/internal/ld. The first is
about writing object files and the second is about reading them. The
point of splitting the code was so that they could diverge reasonably, so
that the reading-specific parts could be dropped from one and the
writing-specific parts dropped from the other. They can both get cleaner
that way. I would like to get back to the point where cmd/link does not
import cmd/internal/obj at all. It didn't used to, but there was some
well-intentioned and reasonable cleanup that undid that separation this a
while back in order to avoid some duplicated constant definitions. It would
be good to split those constants out into a separate shared package
(cmd/internal/objfile maybe) and then add a test to go/build that cmd/link
cannot depend on cmd/internal/obj.

On Fri, Mar 11, 2016 at 4:26 PM David Crawshaw notifications@github.com
wrote:

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 Extra interface{} field that cmd/link could
use, then we could replace most of ld.LSym. Given I did some recent cleanup
of ld.LSym, I'll attempt to do the same to obj.LSym and see how close I can
get to merging those types.


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

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Mar 11, 2016

+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.

@mdempsky mdempsky changed the title cmd/internal/goobj: dead code cmd/internal/goobj: untested code Mar 11, 2016
@crawshaw
Copy link
Member

@crawshaw crawshaw commented Mar 11, 2016

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.

@mwhudson
Copy link
Contributor

@mwhudson mwhudson commented Mar 13, 2016

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.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2016

CL https://golang.org/cl/20104 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 6, 2017

CL https://golang.org/cl/37797 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 6, 2017
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>
@josharian
Copy link
Contributor

@josharian josharian commented May 16, 2017

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.

@golang golang locked and limited conversation to collaborators May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants