code review 133130043: cmd/{5l,6l,8l}: stop copying pclntab to plan9 pclntab

cmd/{5l,6l,8l}: stop copying pclntab to plan9 pclntab For Go 1.2, pclntab was changed from Plan 9's pclntab format to a new Go-specific format, so it no longer makes sense to copy it to the Plan 9 executable's PC/line number table section.

[mdempsky, 2014-08-28 01:23:06]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[bradfitz, 2014-08-28 14:54:52]

[+rsc, iant] On Wed, Aug 27, 2014 at 6:23 PM, mdempsky via golang-codereviews < golang-codereviews@googlegroups.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > cmd/{5l,6l,8l}: stop copying pclntab to plan9 pclntab > > For Go 1.2, pclntab was changed from Plan 9's pclntab format to a new > Go-specific format, so it no longer makes sense to copy it to the > Plan 9 executable's PC/line number table section. > > Please review this at https://codereview.appspot.com/133130043/ > > Affected files (+6, -50 lines): > M src/cmd/5l/asm.c > M src/cmd/5l/l.h > M src/cmd/6l/asm.c > M src/cmd/6l/l.h > M src/cmd/8l/asm.c > M src/cmd/8l/l.h > > > Index: src/cmd/5l/asm.c > =================================================================== > --- a/src/cmd/5l/asm.c > +++ b/src/cmd/5l/asm.c > @@ -551,8 +551,6 @@ > { > uint32 symo; > Section *sect; > - LSym *sym; > - int i; > > if(debug['v']) > Bprint(&bso, "%5.2f asmb\n", cputime()); > @@ -587,7 +585,6 @@ > > /* output symbol table */ > symsize = 0; > - lcsize = 0; > symo = 0; > if(!debug['s']) { > // TODO: rationalize > @@ -627,15 +624,6 @@ > case Hplan9: > asmplan9sym(); > cflush(); > - > - sym = linklookup(ctxt, "pclntab", 0); > - if(sym != nil) { > - lcsize = sym->np; > - for(i=0; i < lcsize; i++) > - cput(sym->p[i]); > - > - cflush(); > - } > break; > } > } > @@ -655,7 +643,7 @@ > LPUT(symsize); /* nsyms */ > LPUT(entryvalue()); /* va of entry */ > LPUT(0L); > - LPUT(lcsize); > + LPUT(0L); > break; > case Hlinux: > case Hfreebsd: > @@ -671,8 +659,7 @@ > print("datsize=%ulld\n", segdata.filelen); > print("bsssize=%ulld\n", segdata.len - segdata.filelen); > print("symsize=%d\n", symsize); > - print("lcsize=%d\n", lcsize); > - print("total=%lld\n", segtext.filelen+segdata.len+ > symsize+lcsize); > + print("total=%lld\n", segtext.filelen+segdata.len+ > symsize); > } > } > > Index: src/cmd/5l/l.h > =================================================================== > --- a/src/cmd/5l/l.h > +++ b/src/cmd/5l/l.h > @@ -66,7 +66,6 @@ > EXTERN int debug[128]; > EXTERN char* noname; > EXTERN Prog* lastp; > -EXTERN int32 lcsize; > EXTERN char literal[32]; > EXTERN int nerrors; > EXTERN int32 instoffset; > Index: src/cmd/6l/asm.c > =================================================================== > --- a/src/cmd/6l/asm.c > +++ b/src/cmd/6l/asm.c > @@ -608,10 +608,8 @@ > asmb(void) > { > int32 magic; > - int i; > vlong vl, symo, dwarfoff, machlink; > Section *sect; > - LSym *sym; > > if(debug['v']) > Bprint(&bso, "%5.2f asmb\n", cputime()); > @@ -686,8 +684,6 @@ > } > > symsize = 0; > - spsize = 0; > - lcsize = 0; > symo = 0; > if(!debug['s']) { > if(debug['v']) > @@ -739,15 +735,6 @@ > case Hplan9: > asmplan9sym(); > cflush(); > - > - sym = linklookup(ctxt, "pclntab", 0); > - if(sym != nil) { > - lcsize = sym->np; > - for(i=0; i < lcsize; i++) > - cput(sym->p[i]); > - > - cflush(); > - } > break; > case Hwindows: > if(debug['v']) > @@ -778,8 +765,8 @@ > lputb(symsize); /* nsyms */ > vl = entryvalue(); > lputb(PADDR(vl)); /* va of entry */ > - lputb(spsize); /* sp offsets */ > - lputb(lcsize); /* line offsets */ > + lputb(0); /* sp offsets */ > + lputb(0); /* line offsets */ > vputb(vl); /* va of entry */ > break; > case Hdarwin: > Index: src/cmd/6l/l.h > =================================================================== > --- a/src/cmd/6l/l.h > +++ b/src/cmd/6l/l.h > @@ -78,9 +78,7 @@ > EXTERN LSym* datap; > EXTERN int debug[128]; > EXTERN char literal[32]; > -EXTERN int32 lcsize; > EXTERN char* rpath; > -EXTERN int32 spsize; > EXTERN LSym* symlist; > EXTERN int32 symsize; > > Index: src/cmd/8l/asm.c > =================================================================== > --- a/src/cmd/8l/asm.c > +++ b/src/cmd/8l/asm.c > @@ -556,8 +556,6 @@ > int32 magic; > uint32 symo, dwarfoff, machlink; > Section *sect; > - LSym *sym; > - int i; > > if(debug['v']) > Bprint(&bso, "%5.2f asmb\n", cputime()); > @@ -606,8 +604,6 @@ > } > > symsize = 0; > - spsize = 0; > - lcsize = 0; > symo = 0; > if(!debug['s']) { > // TODO: rationalize > @@ -654,15 +650,6 @@ > case Hplan9: > asmplan9sym(); > cflush(); > - > - sym = linklookup(ctxt, "pclntab", 0); > - if(sym != nil) { > - lcsize = sym->np; > - for(i=0; i < lcsize; i++) > - cput(sym->p[i]); > - > - cflush(); > - } > break; > case Hwindows: > if(debug['v']) > @@ -689,8 +676,8 @@ > lputb(segdata.len - segdata.filelen); > lputb(symsize); /* nsyms */ > lputb(entryvalue()); /* va of entry */ > - lputb(spsize); /* sp offsets */ > - lputb(lcsize); /* line offsets */ > + lputb(0L); /* sp offsets */ > + lputb(0L); /* line offsets */ > break; > case Hdarwin: > asmbmacho(); > Index: src/cmd/8l/l.h > =================================================================== > --- a/src/cmd/8l/l.h > +++ b/src/cmd/8l/l.h > @@ -62,9 +62,7 @@ > EXTERN int debug[128]; > EXTERN char literal[32]; > EXTERN Prog* firstp; > -EXTERN int32 lcsize; > EXTERN char* rpath; > -EXTERN int32 spsize; > EXTERN LSym* symlist; > EXTERN int32 symsize; > > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > 

[iant, 2014-08-28 15:18:21]

Looks right to me but it's Plan 9 specific, so I'll leave it to the Plan 9 folks to approve. Ian On Thu, Aug 28, 2014 at 7:54 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > [+rsc, iant] > > > > > On Wed, Aug 27, 2014 at 6:23 PM, mdempsky via golang-codereviews > <golang-codereviews@googlegroups.com> wrote: >> >> Reviewers: golang-codereviews, >> >> Message: >> Hello golang-codereviews@googlegroups.com, >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> cmd/{5l,6l,8l}: stop copying pclntab to plan9 pclntab >> >> For Go 1.2, pclntab was changed from Plan 9's pclntab format to a new >> Go-specific format, so it no longer makes sense to copy it to the >> Plan 9 executable's PC/line number table section. >> >> Please review this at https://codereview.appspot.com/133130043/ >> >> Affected files (+6, -50 lines): >> M src/cmd/5l/asm.c >> M src/cmd/5l/l.h >> M src/cmd/6l/asm.c >> M src/cmd/6l/l.h >> M src/cmd/8l/asm.c >> M src/cmd/8l/l.h >> >> >> Index: src/cmd/5l/asm.c >> =================================================================== >> --- a/src/cmd/5l/asm.c >> +++ b/src/cmd/5l/asm.c >> @@ -551,8 +551,6 @@ >> { >> uint32 symo; >> Section *sect; >> - LSym *sym; >> - int i; >> >> if(debug['v']) >> Bprint(&bso, "%5.2f asmb\n", cputime()); >> @@ -587,7 +585,6 @@ >> >> /* output symbol table */ >> symsize = 0; >> - lcsize = 0; >> symo = 0; >> if(!debug['s']) { >> // TODO: rationalize >> @@ -627,15 +624,6 @@ >> case Hplan9: >> asmplan9sym(); >> cflush(); >> - >> - sym = linklookup(ctxt, "pclntab", 0); >> - if(sym != nil) { >> - lcsize = sym->np; >> - for(i=0; i < lcsize; i++) >> - cput(sym->p[i]); >> - >> - cflush(); >> - } >> break; >> } >> } >> @@ -655,7 +643,7 @@ >> LPUT(symsize); /* nsyms */ >> LPUT(entryvalue()); /* va of entry */ >> LPUT(0L); >> - LPUT(lcsize); >> + LPUT(0L); >> break; >> case Hlinux: >> case Hfreebsd: >> @@ -671,8 +659,7 @@ >> print("datsize=%ulld\n", segdata.filelen); >> print("bsssize=%ulld\n", segdata.len - segdata.filelen); >> print("symsize=%d\n", symsize); >> - print("lcsize=%d\n", lcsize); >> - print("total=%lld\n", >> segtext.filelen+segdata.len+symsize+lcsize); >> + print("total=%lld\n", >> segtext.filelen+segdata.len+symsize); >> } >> } >> >> Index: src/cmd/5l/l.h >> =================================================================== >> --- a/src/cmd/5l/l.h >> +++ b/src/cmd/5l/l.h >> @@ -66,7 +66,6 @@ >> EXTERN int debug[128]; >> EXTERN char* noname; >> EXTERN Prog* lastp; >> -EXTERN int32 lcsize; >> EXTERN char literal[32]; >> EXTERN int nerrors; >> EXTERN int32 instoffset; >> Index: src/cmd/6l/asm.c >> =================================================================== >> --- a/src/cmd/6l/asm.c >> +++ b/src/cmd/6l/asm.c >> @@ -608,10 +608,8 @@ >> asmb(void) >> { >> int32 magic; >> - int i; >> vlong vl, symo, dwarfoff, machlink; >> Section *sect; >> - LSym *sym; >> >> if(debug['v']) >> Bprint(&bso, "%5.2f asmb\n", cputime()); >> @@ -686,8 +684,6 @@ >> } >> >> symsize = 0; >> - spsize = 0; >> - lcsize = 0; >> symo = 0; >> if(!debug['s']) { >> if(debug['v']) >> @@ -739,15 +735,6 @@ >> case Hplan9: >> asmplan9sym(); >> cflush(); >> - >> - sym = linklookup(ctxt, "pclntab", 0); >> - if(sym != nil) { >> - lcsize = sym->np; >> - for(i=0; i < lcsize; i++) >> - cput(sym->p[i]); >> - >> - cflush(); >> - } >> break; >> case Hwindows: >> if(debug['v']) >> @@ -778,8 +765,8 @@ >> lputb(symsize); /* nsyms */ >> vl = entryvalue(); >> lputb(PADDR(vl)); /* va of entry */ >> - lputb(spsize); /* sp offsets */ >> - lputb(lcsize); /* line offsets */ >> + lputb(0); /* sp offsets */ >> + lputb(0); /* line offsets */ >> vputb(vl); /* va of entry */ >> break; >> case Hdarwin: >> Index: src/cmd/6l/l.h >> =================================================================== >> --- a/src/cmd/6l/l.h >> +++ b/src/cmd/6l/l.h >> @@ -78,9 +78,7 @@ >> EXTERN LSym* datap; >> EXTERN int debug[128]; >> EXTERN char literal[32]; >> -EXTERN int32 lcsize; >> EXTERN char* rpath; >> -EXTERN int32 spsize; >> EXTERN LSym* symlist; >> EXTERN int32 symsize; >> >> Index: src/cmd/8l/asm.c >> =================================================================== >> --- a/src/cmd/8l/asm.c >> +++ b/src/cmd/8l/asm.c >> @@ -556,8 +556,6 @@ >> int32 magic; >> uint32 symo, dwarfoff, machlink; >> Section *sect; >> - LSym *sym; >> - int i; >> >> if(debug['v']) >> Bprint(&bso, "%5.2f asmb\n", cputime()); >> @@ -606,8 +604,6 @@ >> } >> >> symsize = 0; >> - spsize = 0; >> - lcsize = 0; >> symo = 0; >> if(!debug['s']) { >> // TODO: rationalize >> @@ -654,15 +650,6 @@ >> case Hplan9: >> asmplan9sym(); >> cflush(); >> - >> - sym = linklookup(ctxt, "pclntab", 0); >> - if(sym != nil) { >> - lcsize = sym->np; >> - for(i=0; i < lcsize; i++) >> - cput(sym->p[i]); >> - >> - cflush(); >> - } >> break; >> case Hwindows: >> if(debug['v']) >> @@ -689,8 +676,8 @@ >> lputb(segdata.len - segdata.filelen); >> lputb(symsize); /* nsyms */ >> lputb(entryvalue()); /* va of entry */ >> - lputb(spsize); /* sp offsets */ >> - lputb(lcsize); /* line offsets */ >> + lputb(0L); /* sp offsets */ >> + lputb(0L); /* line offsets */ >> break; >> case Hdarwin: >> asmbmacho(); >> Index: src/cmd/8l/l.h >> =================================================================== >> --- a/src/cmd/8l/l.h >> +++ b/src/cmd/8l/l.h >> @@ -62,9 +62,7 @@ >> EXTERN int debug[128]; >> EXTERN char literal[32]; >> EXTERN Prog* firstp; >> -EXTERN int32 lcsize; >> EXTERN char* rpath; >> -EXTERN int32 spsize; >> EXTERN LSym* symlist; >> EXTERN int32 symsize; >> >> >> >> -- >> You received this message because you are subscribed to the Google Groups >> "golang-codereviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-codereviews+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. > > 

[0intro, 2014-08-28 15:42:20]

This pclntab section was already zero-sized, so it will not be much different. -- David du Colombier 

[rsc, 2014-08-28 15:46:40]

Is this in your way somehow? If not, let's not do it. This code is all going to die soon. It doesn't need cleanups. 

[0intro, 2014-08-28 15:48:35]

> Is this in your way somehow? No, leaving cleanup aside, I don't think this change is needed. -- David du Colombier 

[mdempsky, 2014-08-28 15:54:47]

On Thu, Aug 28, 2014 at 8:46 AM, Russ Cox <rsc@golang.org> wrote: > Is this in your way somehow? > No, not in my way at all. Just while fixing the PE/Plan9 issues from my previous CL, I noticed this code was technically broken too because "pclntab" should now be "runtime.pclntab". I was initially going to fix that too, but then looking into it further it seemed like the code is already wrong anyway since the Go pclntab is no longer in Plan 9 format. 

[gobot, 2014-09-29 19:28:08]

R=rsc@golang.org (assigned by bradfitz@golang.org)

[rsc, 2014-10-20 18:56:57]

R=close let's leave this alone - it will be replaced by cmd/link