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

debug/pe: extend package so it can be used by cmd/link #15345

Open
alexbrainman opened this issue Apr 18, 2016 · 19 comments
Labels
Milestone

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 18, 2016

CC: @minux @ianlancetaylor @crawshaw

cmd/link and debug/pe share no common code, but they should - they do the same thing. If package debug/pe is worth its weight, it should be used by cmd/link. I accept that this was impossible to do when cmd/link was a C program, but now cmd/link is written in Go.

I also tried to use debug/pe in github.com/alexbrainman/goissue10776/pedump, and it is not very useful. I endup coping and changing some debug/pe code.

I also think there is some luck of PE format knowledge between us. So improving debug/pe structure and documentation should help with that.

I tried to rewrite src/cmd/link/internal/ld/ldpe.go by using debug/pe (see CL 14289). I had to extend debug/pe for that task. Here is the list of externally visible changes I had to do:

  • PE relocations have SymbolTableIndex field that is an index into symbols table. But File.Symbols slice has Aux lines removed as it is built, so SymbolTableIndex cannot be used to index into File.Symbols. We cannot change File.Symbols behavior. So I propose we introduce File.COFFSymbols slice that is like File.Symbols, but with Aux lines left untouched.
  • I have introduced StringTable that was used to convert long names in PE symbol table and PE section table into Go strings.
  • I have also introduced Section.Relocs to access PE relocations.

I propose we add the above things to debug/pe and use new debug/pe to rewrite ldpe.go and pe.go in cmd/link/internal/ld.

Alex

PS: You can google for pecoff.doc for PE detials.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Apr 18, 2016

@bradfitz bradfitz added this to the Unplanned milestone Apr 18, 2016
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 18, 2016

Sounds basically fine to me. Thanks.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 18, 2016

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 18, 2016

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

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Apr 18, 2016

gopherbot pushed a commit that referenced this issue Apr 20, 2016
No code changes. Just moved ImportDirectory next to ImportedSymbols.
And moved useless FormatError to the bottom of file.go.

Updates #15345

Change-Id: I91ff243cefd18008b1c5ee9ec4326583deee431b
Reviewed-on: https://go-review.googlesource.com/22182
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 20, 2016
PE specification requires that long section and symbol names
are stored in PE string table. Introduce StringTable that
implements this functionality. Only string table reading is
implemented.

Updates #15345

Change-Id: Ib9638617f2ab1881ad707111d96fc68b0e47340e
Reviewed-on: https://go-review.googlesource.com/22181
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
gopherbot pushed a commit that referenced this issue Apr 20, 2016
Just moving code. No code changes.

Updates #15345

Change-Id: I89c257b7aae4fbd78ce59a42909ecb3ff493659d
Reviewed-on: https://go-review.googlesource.com/22300
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 20, 2016

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 21, 2016

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

gopherbot pushed a commit that referenced this issue Apr 21, 2016
Introduce (*SectionHeader32).fullName and add documentation comments.

Updates #15345

Change-Id: I8f3b8ab9492642d62e7aad010c91c68daea3f14b
Reviewed-on: https://go-review.googlesource.com/22301
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 21, 2016
Updates #15345

Change-Id: If1fca1f6042571cb0ac689bbb3c294309dd6e7b4
Reviewed-on: https://go-review.googlesource.com/22331
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 21, 2016

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

gopherbot pushed a commit that referenced this issue Apr 21, 2016
cmd/link reads PE object files when building programs with cgo.
cmd/link accesses object relocations. Add new Section.Relocs that
provides similar functionality in debug/pe.

Updates #15345

Change-Id: I34de91b7f18cf1c9e4cdb3aedd685486a625ac92
Reviewed-on: https://go-review.googlesource.com/22332
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 21, 2016

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

gopherbot pushed a commit that referenced this issue Apr 24, 2016
Reloc.SymbolTableIndex is an index into symbol table. But
Reloc.SymbolTableIndex cannot be used as index into File.Symbols,
because File.Symbols slice has Aux lines removed as it is built.

We cannot change the way File.Symbols works, so I propose we
introduce new File.COFFSymbols that does not have that limitation.

Also unlike File.Symbols, File.COFFSymbols will consist of
COFFSymbol. COFFSymbol matches PE COFF specification exactly,
and it is simpler to use.

Updates #15345

Change-Id: Icbc265853a472529cd6d64a76427b27e5459e373
Reviewed-on: https://go-review.googlesource.com/22336
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 24, 2016

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

gopherbot pushed a commit that referenced this issue Apr 25, 2016
Updates #15345

Change-Id: Iae35d3e378cbc8157ba1ff91e4971ed4515a5e5c
Reviewed-on: https://go-review.googlesource.com/22394
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 28, 2016

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

gopherbot pushed a commit that referenced this issue Apr 29, 2016
.bss section has no data stored in PE file. But when .bss section data
is used by the linker it is assumed that its every byte is set to zero.
(*Section).Data returns garbage at this moment. Change (*Section).Data
so it returns slice filled with 0s.

Updates #15345

Change-Id: I1fa5138244a9447e1d59dec24178b1dd0fd4c5d7
Reviewed-on: https://go-review.googlesource.com/22544
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 3, 2016

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 4, 2016

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

gopherbot pushed a commit that referenced this issue May 4, 2016
See CL 22720 for details.

Updates #15345

Change-Id: If93ddbb8137d57da9846b671160b4cebe1992570
Reviewed-on: https://go-review.googlesource.com/22752
Reviewed-by: David Crawshaw <crawshaw@golang.org>
gopherbot pushed a commit that referenced this issue May 5, 2016
CLs 22181, 22332 and 22336 intorduced new functionality to be used
in cmd/link (see issue #15345 for details). But we didn't have chance
to use new functionality yet. Unexport newly introduced identifiers,
so we don't have to commit to the API until we actually tried it.

Rename File.COFFSymbols into File._COFFSymbols,
COFFSymbol.FullName into COFFSymbol._FullName,
Section.Relocs into Section._Relocs,
Reloc into _Relocs,
File.StringTable into File._StringTable and
StringTable into _StringTable.

Updates #15345

Change-Id: I770eeb61f855de85e0c175225d5d1c006869b9ec
Reviewed-on: https://go-review.googlesource.com/22720
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 17, 2016

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

gopherbot pushed a commit that referenced this issue Aug 18, 2016
CL 22720 hid all recently added functionality for go1.7.
Make everything exported again, so we could use it now.

Updates #15345

Change-Id: Id8ccba7199422b554407ec14c343d2c28fbb8f72
Reviewed-on: https://go-review.googlesource.com/27212
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Aug 18, 2016

Please help. I have created https://go-review.googlesource.com/#/c/22603/. I am running try-bots, and all try-bots succeed, except nacl. Why? Thank you.

Alex

@crawshaw

This comment has been minimized.

Copy link
Contributor

@crawshaw crawshaw commented Aug 18, 2016

Any testdata directories used on nacl need to be mentioned in go/misc/nacl/testzip.proto.

@alexbrainman

This comment has been minimized.

Copy link
Member Author

@alexbrainman alexbrainman commented Aug 18, 2016

need to be mentioned in go/misc/nacl/testzip.proto

Ahhhhhha! Thank you.

Alex

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 19, 2016

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

gopherbot pushed a commit that referenced this issue Oct 13, 2016
This CL also includes vendored copy of debug/pe,
otherwise bootstrapping fails.

Updates #15345

Change-Id: I3a8ac990e3cb12cb4d24ec11b618b68190397fd1
Reviewed-on: https://go-review.googlesource.com/22603
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 24, 2016

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

gopherbot pushed a commit that referenced this issue Oct 25, 2016
Updates #15345

Change-Id: I447d133512e99a9900928a910e161a85db6e8b75
Reviewed-on: https://go-review.googlesource.com/31792
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.