Issue 91400044: code review 91400044: debug/plan9obj: cleanup api (Closed)
Descriptiondebug/plan9obj: cleanup api
- Don't export Prog structure.
- Remove ProgHeader and ExecTable structures.
- Add Magic, Bss and Entry fields in FileHeader.
- Replace ?_MAGIC variables with constants.
- Ignore final EOF from ReadAt.
- Improve documentation.
Fixes issue 7989.
Patch Set 1 #Patch Set 2 : diff -r b9c40afbd66c https://code.google.com/p/go #
Total comments: 10
Patch Set 3 : diff -r b9c40afbd66c https://code.google.com/p/go #Patch Set 4 : diff -r d419520d0d2e https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r d419520d0d2e https://code.google.com/p/go #Patch Set 6 : diff -r d419520d0d2e https://code.google.com/p/go #
Total comments: 14
Patch Set 7 : diff -r 4a839bf01b58 https://code.google.com/p/go #
Total comments: 2
MessagesTotal messages: 19
Hello rsc (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
drive-by https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:19: Ptrsz int Maybe a better name before this is locked in stone? PointerSize? Is this from an on-disk format? If so, is it really an int, or is it uint32 or something? https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:29: type SectionHeader struct { no docs. https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:99: type FormatError struct { docs https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/pla... File src/pkg/debug/plan9obj/plan9obj.go (right): https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/pla... src/pkg/debug/plan9obj/plan9obj.go:48: A_MAGIC = magic(0, 8) /* 68020 (retired) */ do these need to be ALL CAPS? What about Magic_A, Magic_I, Magic_J, etc? We generally try to avoid all caps style these days, even though there's a lot in pkg/syscall and some legacy accidents in pkg/os.
Sign in to reply to this message.
None of the other debug packages have separate ProgHeader and FileHeader. I don't understand the difference here. Why is there not just FileHeader? https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:19: Ptrsz int On 2014/05/13 22:25:05, bradfitz wrote: > Maybe a better name before this is locked in stone? PointerSize? > > Is this from an on-disk format? If so, is it really an int, or is it uint32 or > something? PtrSize int
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:19: Ptrsz int > Is this from an on-disk format? If so, is it really an int, or is it uint32 or > something? No, this is the size of the Entry pointer, which is deduced from the architecture. https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:19: Ptrsz int > PtrSize int Done. https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:29: type SectionHeader struct { On 2014/05/13 22:25:05, bradfitz wrote: > no docs. Done. https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/fil... src/pkg/debug/plan9obj/file.go:99: type FormatError struct { On 2014/05/13 22:25:05, bradfitz wrote: > docs Done. https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/pla... File src/pkg/debug/plan9obj/plan9obj.go (right): https://codereview.appspot.com/91400044/diff/30001/src/pkg/debug/plan9obj/pla... src/pkg/debug/plan9obj/plan9obj.go:48: A_MAGIC = magic(0, 8) /* 68020 (retired) */ > do these need to be ALL CAPS? > > What about Magic_A, Magic_I, Magic_J, etc? > > We generally try to avoid all caps style these days, even though there's a lot > in pkg/syscall and some legacy accidents in pkg/os. I agree with the style, but I initially chose to conserve the original names because I find them more coherent with existing documentation, like: http://plan9.bell-labs.com/magic/man2html/6/a.out What do you prefer, Russ?
Sign in to reply to this message.
Hello rsc@golang.org, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:32: // This structure doesn't exist on-disk, but ease navigation eases https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:78: // FormatError is returned by some operations if the data does Why is this exported if there are no exported fields on it, and no special methods outside of the normal error interface?
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:32: // This structure doesn't exist on-disk, but ease navigation On 2014/05/19 21:15:59, bradfitz wrote: > eases Done. https://codereview.appspot.com/91400044/diff/150001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:78: // FormatError is returned by some operations if the data does > Why is this exported if there are no exported fields on it, and no special > methods outside of the normal error interface? This is probably unneeded. I wanted to match the API from the other debug packages.
Sign in to reply to this message.
On Mon, May 19, 2014 at 2:36 PM, <0intro@gmail.com> wrote: > > https://codereview.appspot.com/91400044/diff/150001/src/ > pkg/debug/plan9obj/file.go > File src/pkg/debug/plan9obj/file.go (right): > > https://codereview.appspot.com/91400044/diff/150001/src/ > pkg/debug/plan9obj/file.go#newcode32 > src/pkg/debug/plan9obj/file.go:32: // This structure doesn't exist > on-disk, but ease navigation > On 2014/05/19 21:15:59, bradfitz wrote: > >> eases >> > > Done. > > > https://codereview.appspot.com/91400044/diff/150001/src/ > pkg/debug/plan9obj/file.go#newcode78 > src/pkg/debug/plan9obj/file.go:78: // FormatError is returned by some > operations if the data does > >> Why is this exported if there are no exported fields on it, and no >> > special > >> methods outside of the normal error interface? >> > > This is probably unneeded. I wanted to match the API from the other > debug packages. It's easy to add later if it's ever needed. But as it is now, it's useless noise.
Sign in to reply to this message.
> Why is this exported if there are no exported fields on it, and no > special methods outside of the normal error interface? Done.
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:18: Magic string s/string/uint32/ https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:123: for _, e := range exectab { m := binary.BigEndian.Uint32(magic) https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:124: if string(magic[:]) == e.Magic { if m == e.Magic ... https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... File src/pkg/debug/plan9obj/plan9obj.go (right): https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:40: func magic(f, b int) string { delete https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:48: A_MAGIC = magic(0, 8) /* 68020 (retired) */ it is bizarre that these are typed as string but are binary data. make them real numbers, and let's only bother with the Go targets: const ( Magic64 = 0x8000 // 64-bit expanded header Magic386 = (4*11+0)*11 + 7 MagicAMD64 = (4*26+0)*26 + 7 + Magic64 MagicARM = (4*20+0)*20 + 7 ) https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:67: type execTable struct { this can go away https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:73: var exectab = []execTable{ this table can go away. you can use magic & Magic64 != 0 to decide PtrSize and Hsize.
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:18: Magic string On 2014/05/20 02:26:03, rsc wrote: > s/string/uint32/ Done. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:123: for _, e := range exectab { On 2014/05/20 02:26:03, rsc wrote: > m := binary.BigEndian.Uint32(magic) Done. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:124: if string(magic[:]) == e.Magic { > if m == e.Magic ... Replaced by a switch. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... File src/pkg/debug/plan9obj/plan9obj.go (right): https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:40: func magic(f, b int) string { On 2014/05/20 02:26:04, rsc wrote: > delete Done. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:48: A_MAGIC = magic(0, 8) /* 68020 (retired) */ On 2014/05/20 02:26:04, rsc wrote: > it is bizarre that these are typed as string but are binary data. > make them real numbers, and let's only bother with the Go targets: > > const ( > Magic64 = 0x8000 // 64-bit expanded header > > Magic386 = (4*11+0)*11 + 7 > MagicAMD64 = (4*26+0)*26 + 7 + Magic64 > MagicARM = (4*20+0)*20 + 7 > ) > Done. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:67: type execTable struct { On 2014/05/20 02:26:03, rsc wrote: > this can go away Done. https://codereview.appspot.com/91400044/diff/190001/src/pkg/debug/plan9obj/pl... src/pkg/debug/plan9obj/plan9obj.go:73: var exectab = []execTable{ On 2014/05/20 02:26:04, rsc wrote: > this table can go away. you can use magic & Magic64 != 0 to decide PtrSize and > Hsize. Done.
Sign in to reply to this message.
LGTM for api https://codereview.appspot.com/91400044/diff/280001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/280001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:164: hdrSize += 8 what are these extra 8 bytes and why does nothing read them?
Sign in to reply to this message.
Submitting to kick the NaCl builder and see if it works, per Russ's okay. On Tue, May 20, 2014 at 10:55 AM, <rsc@golang.org> wrote: > LGTM for api > > > > https://codereview.appspot.com/91400044/diff/280001/src/ > pkg/debug/plan9obj/file.go > File src/pkg/debug/plan9obj/file.go (right): > > https://codereview.appspot.com/91400044/diff/280001/src/ > pkg/debug/plan9obj/file.go#newcode164 > src/pkg/debug/plan9obj/file.go:164: hdrSize += 8 > what are these extra 8 bytes and why does nothing read them? > > https://codereview.appspot.com/91400044/ >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=d335e30dc8cb *** debug/plan9obj: cleanup api - Don't export Prog structure. - Remove ProgHeader and ExecTable structures. - Add Magic, Bss and Entry fields in FileHeader. - Replace ?_MAGIC variables with constants. - Ignore final EOF from ReadAt. - Improve documentation. Fixes issue 7989. LGTM=rsc R=rsc, bradfitz CC=golang-codereviews https://codereview.appspot.com/91400044 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
> https://codereview.appspot.com/91400044/diff/280001/src/pkg/debug/plan9obj/fi... > src/pkg/debug/plan9obj/file.go:164: hdrSize += 8 > what are these extra 8 bytes and why does nothing read them? This is the 64-bit entry address. It is read four lines earlier.
Sign in to reply to this message.
This CL appears to have broken the linux-386 builder. See http://build.golang.org/log/6079d1020f95dd659d28c2604fe9c36f72d84cd8
Sign in to reply to this message.
https://codereview.appspot.com/91400044/diff/280001/src/pkg/debug/plan9obj/fi... File src/pkg/debug/plan9obj/file.go (right): https://codereview.appspot.com/91400044/diff/280001/src/pkg/debug/plan9obj/fi... src/pkg/debug/plan9obj/file.go:164: hdrSize += 8 On 2014/05/20 17:55:14, rsc wrote: > what are these extra 8 bytes and why does nothing read them? aha
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
