code review 1676048: Add support for reading in the ELF program headers (issue1676048)

[rmin...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com,
rminnich),

I'd like you to review this change.

Description:
Add support for reading in the ELF program headers
This fairly simple change adds in support for reading
ELF program headers. I tried to follow the rules of the
section headers, e.g. promoting 32 to 64 int in the
struct, and so on. I've tested this against ELF files
and it seems to be right.

Please review this at http://codereview.appspot.com/1676048/show

Affected files:
M src/pkg/debug/elf/file.go

Index: src/pkg/debug/elf/file.go
===================================================================
--- a/src/pkg/debug/elf/file.go
+++ b/src/pkg/debug/elf/file.go
@@ -87,6 +87,7 @@
Filesz uint64
Memsz uint64
Align uint64
+ Off uint64
}

// A Prog represents a single ELF program header in an ELF binary.
@@ -204,6 +205,8 @@
// Read ELF file header
var shoff int64
var shentsize, shnum, shstrndx int
+ var phoff int64
+ var phentsize, phnum int
shstrndx = -1
switch f.Class {
case ELFCLASS32:
@@ -221,6 +224,9 @@
shentsize = int(hdr.Shentsize)
shnum = int(hdr.Shnum)
shstrndx = int(hdr.Shstrndx)
+ phentsize = int(hdr.Phentsize)
+ phoff = int64(hdr.Phoff)
+ phnum = int(hdr.Phnum)
case ELFCLASS64:
hdr := new(Header64)
sr.Seek(0, 0)
@@ -236,13 +242,58 @@
shentsize = int(hdr.Shentsize)
shnum = int(hdr.Shnum)
shstrndx = int(hdr.Shstrndx)
+ phentsize = int(hdr.Phentsize)
+ phoff = int64(hdr.Phoff)
+ phnum = int(hdr.Phnum)
}
if shstrndx < 0 || shstrndx >= shnum {
return nil, &FormatError{0, "invalid ELF shstrndx", shstrndx}
}

// Read program headers
- // TODO
+ fmt.Printf("phentsize %d phnum %d phoff %d\n", phentsize, phnum, phoff)
+ f.Progs = make([]*Prog, phnum)
+ for i := 0; i < phnum; i++ {
+ off := phoff + int64(i)*int64(phentsize)
+ sr.Seek(off, 0)
+ s := new(Prog)
+ switch f.Class {
+ case ELFCLASS32:
+ ph := new(Prog32)
+ if err := binary.Read(sr, f.ByteOrder, ph); err != nil {
+ return nil, err
+ }
+ s.ProgHeader = ProgHeader{
+ Type: ProgType(ph.Type),
+ Off: uint64(ph.Off),
+ Vaddr: uint64(ph.Vaddr),
+ Paddr: uint64(ph.Paddr),
+ Filesz: uint64(ph.Filesz),
+ Memsz: uint64(ph.Memsz),
+ Flags: ProgFlag(ph.Flags),
+ Align: uint64(ph.Align),
+ }
+ case ELFCLASS64:
+ ph := new(Prog64)
+ if err := binary.Read(sr, f.ByteOrder, ph); err != nil {
+ return nil, err
+ }
+ s.ProgHeader = ProgHeader{
+ Type: ProgType(ph.Type),
+ Off: uint64(ph.Off),
+ Vaddr: uint64(ph.Vaddr),
+ Paddr: uint64(ph.Paddr),
+ Filesz: uint64(ph.Filesz),
+ Memsz: uint64(ph.Memsz),
+ Flags: ProgFlag(ph.Flags),
+ Align: uint64(ph.Align),
+ }
+ }
+ s.sr = io.NewSectionReader(r, int64(s.Off), int64(s.Filesz))
+ s.ReaderAt = s.sr
+ f.Progs[i] = s
+ }
+

// Read section headers
f.Sections = make([]*Section, shnum)

[ia...@golang.org]

TestOpen in file_test.go should be expanded to test that program headers
are read correctly.

http://codereview.appspot.com/1676048/diff/2001/3001
File src/pkg/debug/elf/file.go (right):

http://codereview.appspot.com/1676048/diff/2001/3001#newcode90
src/pkg/debug/elf/file.go:90: Off uint64
I think this should move up to after Flags to correspond to the ordering
of the ELF struct.

http://codereview.appspot.com/1676048/diff/2001/3001#newcode254
src/pkg/debug/elf/file.go:254: fmt.Printf("phentsize %d phnum %d phoff

%d\n", phentsize, phnum, phoff)

Remove Printf.

http://codereview.appspot.com/1676048/show

[ron minnich]

On Mon, Jun 28, 2010 at 11:43 AM, <ia...@golang.org> wrote:
> TestOpen in file_test.go should be expanded to test that program headers
> are read correctly.

Question: I can fix this test for linux, but don't have access to the
other systems that it can test on.

Will it be impossible to take my patch until the test is expanded?

Thanks for the review.

Ron

[Russ Cox]

> Question: I can fix this test for linux, but don't have access to the
> other systems that it can test on.
>
> Will it be impossible to take my patch until the test is expanded?

No, because there's nothing operating system-specific
in this package.

Russ

[ia...@golang.org]

> Question: I can fix this test for linux, but don't have access to the
> other systems that it can test on.

Look at the test code. It runs on files which are provided in the
testdata directory.

Ian

http://codereview.appspot.com/1676048/show

[rmin...@gmail.com]

Hello golan...@googlegroups.com, iant (cc:
golan...@googlegroups.com, rminnich),

Please take another look.

http://codereview.appspot.com/1676048/show