code review 3470044: debug/elf, debug/macho: add ImportedLibraries, Imported...

debug/elf, debug/macho: add ImportedLibraries, ImportedSymbols

[rsc, 2010-12-07 21:44:43]

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

I'd like you to review this change.

[rsc1, 2010-12-07 21:49:02]

R+=iant

[iant, 2010-12-07 22:19:20]

LGTM

Various comments below.

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

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:79: func (f *File) StringTable(s *Section) ([]byte,
os.Error) {
This method sort of suggests that every section has a string table, but actually
only a few do.  I think it might be cleaner to change the parameter to a uint32
section index and change the callers to pass the Link field.

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:357: return nil, os.ErrorString("length of symbol
section is not a multiple of Sym64Size")
s/Sym64Size/Sym32Size/

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:365: // The first entry is all zeros.
Skipping the first entry means that you are going to have to remember to
subtract 1 from every symbol index you deal with.  That's fine if that is what
you want to do.

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:379: symbols[i].Section = SectionIndex(sym.Shndx)
Doesn't have to happen now, but at some point this code is going to have to
change to deal with object files with more than 0xff00 sections.

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:557: if ST_BIND(s.Info) == STB_GLOBAL {
Should also check that s.Section == SHN_UNDEF.  Otherwise you are going to
return globally visible symbols defined by the binary.  The function doc should
indicate that it does not return weak symbols.

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/elf/file.go#new...
src/pkg/debug/elf/file.go:564: // ImportedLibraries returns the paths of all
libraries
Comment should probably s/paths/names/.  In general this will just return
library names.

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/macho/file.go
File src/pkg/debug/macho/file.go (right):

http://codereview.appspot.com/3470044/diff/2001/src/pkg/debug/macho/file.go#n...
src/pkg/debug/macho/file.go:263: var hdr SymtabCmd
Maybe call a subroutine here?

[rsc, 2010-12-07 22:32:30]

Done.  I unexported stringTable too.
I left the Symbol list alone (1-indexed)
because more accesses are iterating over
the whole thing than indexing.

Russ

[rsc, 2010-12-08 18:53:21]

*** Submitted as 4bccc7a30cc5 ***

debug/elf, debug/macho: add ImportedLibraries, ImportedSymbols

R=r, iant
CC=golang-dev
http://codereview.appspot.com/3470044