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

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.

I ran gofmt on both files.

[rminnich, 2010-06-28 00:49:35]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com, rminnich),

I'd like you to review this change.

[iant, 2010-06-28 18:43:15]

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.

[rminnich, 2010-06-28 21:40:25]

On Mon, Jun 28, 2010 at 11:43 AM,  <iant@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

[rsc1, 2010-06-28 22:56:36]

> 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

[iant, 2010-06-28 23:16:58]

> 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

[rminnich, 2010-06-30 23:57:51]

Hello golang-dev@googlegroups.com, iant (cc: golang-dev@googlegroups.com,
rminnich),

Please take another look.

[rminnich, 2010-07-06 00:16:03]

Hello golang-dev@googlegroups.com, iant (cc: golang-dev@googlegroups.com,
rminnich),

Please take another look.

[iant, 2010-07-10 21:03:12]

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

http://codereview.appspot.com/1676048/diff/12002/1002#newcode305
src/pkg/debug/elf/file.go:305: f.Libs[0] = string(name)
This is the interpreter, not a dynamic library.  I'm not sure we need to gather
this information, but if we do it should go in a field by itself, not in the
f.Libs slice.

http://codereview.appspot.com/1676048/diff/12002/1002#newcode372
src/pkg/debug/elf/file.go:372: dynstrndx = i
I'm not sure we should be gathering the library information at all here.  But if
we do, this is the wrong approach.  Instead, this code should use the PT_DYNAMIC
program header to find the dynamic tags, and it should use DT_STRTAB dynamic tag
to find the string table.

It may be simplest to drop this part from the change and get this change in. 
Then we can discuss whether to pick up the library information here or whether
to provide a different call for it.

http://codereview.appspot.com/1676048/diff/12002/1003
File src/pkg/debug/elf/file_test.go (right):

http://codereview.appspot.com/1676048/diff/12002/1003#newcode119
src/pkg/debug/elf/file_test.go:119: ProgHeader{PT_LOOS + 74769744
/*PT_GNU_EH_FRAME*/ , PF_R, 0x5b8, 0x4005b8, 0x4005b8, 0x24, 0x24, 4},
Write PT_LOOS + 0x474e550 and PT_LOOS + 0x474e551.  These magic numbers are
'GNU' << 4 + {0,1}.

[rminnich, 2010-07-10 23:30:44]

On Sat, Jul 10, 2010 at 2:03 PM,  <iant@golang.org> wrote:
>
> http://codereview.appspot.com/1676048/diff/12002/1002
> File src/pkg/debug/elf/file.go (right):
>
> http://codereview.appspot.com/1676048/diff/12002/1002#newcode305
> src/pkg/debug/elf/file.go:305: f.Libs[0] = string(name)
> This is the interpreter, not a dynamic library.  I'm not sure we need to
> gather this information, but if we do it should go in a field by itself,
> not in the f.Libs slice.
>
> http://codereview.appspot.com/1676048/diff/12002/1002#newcode372
> src/pkg/debug/elf/file.go:372: dynstrndx = i
> I'm not sure we should be gathering the library information at all here.
>  But if we do, this is the wrong approach.  Instead, this code should
> use the PT_DYNAMIC program header to find the dynamic tags, and it
> should use DT_STRTAB dynamic tag to find the string table.
>
> It may be simplest to drop this part from the change and get this change
> in.  Then we can discuss whether to pick up the library information here
> or whether to provide a different call for it.



understand. I'll yank the libs stuff out. I'm using it currently but
I'll find a different way to get it -- I may temporarily fork off this
elf code.

To put it in context, I need an 'ldd function' and the way this code
works is borrowed from Lucho's xcpu implementation.

ron

[rminnich, 2010-07-12 17:28:56]

Hello golang-dev@googlegroups.com, iant (cc: golang-dev@googlegroups.com,
rminnich),

Please take another look.

[iant, 2010-07-12 20:39:46]

LGTM

Have you signed the copyright CLA?

[rminnich, 2010-07-12 21:20:54]

On Mon, Jul 12, 2010 at 1:39 PM,  <iant@golang.org> wrote:
> LGTM
>
> Have you signed the copyright CLA?

It's in the hands of our corporate legal guys, I am going to push again.

ron

[rsc1, 2010-11-01 18:45:00]

timed out