|
|
Descriptiondebug/elf: do not skip first symbol in the symbol table
Do not skip the first symbol in the symbol table. Any other indexes
into the symbol table (for example, indexes in relocation entries)
will now refer to the symbol following the one that was intended.
Add an object that contains debug relocations, which debug/dwarf
failed to decode correctly. Extend the relocation tests to cover
this case.
Note that the existing tests passed since the symbol following the
symbol that required relocation is also of type STT_SECTION.
Fixes issue 4107.
Patch Set 1 #Patch Set 2 : diff -r fc4e67d82024 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r fc4e67d82024 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r d9896259d8e5 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 5 : diff -r d9896259d8e5 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r d1854dd425b2 https://go.googlecode.com/hg/ #
MessagesTotal messages: 13
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM Thanks! it also passes on freebsd.
Sign in to reply to this message.
This is going to change the behaviour of the Symbols function in debug/elf and potentially break any current users. I don't know if that is a good idea. Certainly any change would need to be documented in doc/go1.1.html. Ian On Tue, Nov 13, 2012 at 5:49 AM, <jsing@google.com> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > debug/elf: do not skip first symbol in the symbol table > > Do not skip the first symbol in the symbol table. Any other indexes > into the symbol table (for example, indexes in relocation entries) > will now refer to the symbol following the one that was intended. > > Add an object that contains debug relocations, which debug/dwarf > failed to decode correctly. Extend the relocation tests to cover > this case. > > Note that the existing tests passed since the symbol following the > symbol that required relocation is also of type STT_SECTION. > > Fixes issue 4107. > > Please review this at http://codereview.appspot.com/6848044/ > > Affected files: > M src/pkg/debug/elf/file.go > M src/pkg/debug/elf/file_test.go > A src/pkg/debug/elf/testdata/gcc-amd64-openbsd-debug-with-rela.obj > > > Index: src/pkg/debug/elf/file.go > =================================================================== > --- a/src/pkg/debug/elf/file.go > +++ b/src/pkg/debug/elf/file.go > @@ -417,10 +417,6 @@ > return nil, nil, errors.New("cannot load string table > section") > } > > - // The first entry is all zeros. > - var skip [Sym32Size]byte > - symtab.Read(skip[0:]) > - > symbols := make([]Symbol, symtab.Len()/Sym32Size) > > i := 0 > @@ -460,10 +456,6 @@ > return nil, nil, errors.New("cannot load string table > section") > } > > - // The first entry is all zeros. > - var skip [Sym64Size]byte > - symtab.Read(skip[0:]) > - > symbols := make([]Symbol, symtab.Len()/Sym64Size) > > i := 0 > Index: src/pkg/debug/elf/file_test.go > =================================================================== > --- a/src/pkg/debug/elf/file_test.go > +++ b/src/pkg/debug/elf/file_test.go > @@ -175,23 +175,41 @@ > } > } > > +type relocationTestEntry struct { > + entryNumber int > + entry *dwarf.Entry > +} > + > type relocationTest struct { > - file string > - firstEntry *dwarf.Entry > + file string > + entries []relocationTestEntry > } > > var relocationTests = []relocationTest{ > { > "testdata/go-relocation-test-gcc441-x86-64.obj", > - &dwarf.Entry{Offset: 0xb, Tag: dwarf.TagCompileUnit, > Children: true, Field: []dwarf.Field{{Attr: dwarf.AttrProducer, Val: "GNU C > 4.4.1"}, {Attr: dwarf.AttrLanguage, Val: int64(1)}, {Attr: dwarf.AttrName, > Val: "go-relocation-test.c"}, {Attr: dwarf.AttrCompDir, Val: "/tmp"}, {Attr: > dwarf.AttrLowpc, Val: uint64(0x0)}, {Attr: dwarf.AttrHighpc, Val: > uint64(0x6)}, {Attr: dwarf.AttrStmtList, Val: int64(0)}}}, > + []relocationTestEntry{ > + {0, &dwarf.Entry{Offset: 0xb, Tag: > dwarf.TagCompileUnit, Children: true, Field: []dwarf.Field{{Attr: > dwarf.AttrProducer, Val: "GNU C 4.4.1"}, {Attr: dwarf.AttrLanguage, Val: > int64(1)}, {Attr: dwarf.AttrName, Val: "go-relocation-test.c"}, {Attr: > dwarf.AttrCompDir, Val: "/tmp"}, {Attr: dwarf.AttrLowpc, Val: uint64(0x0)}, > {Attr: dwarf.AttrHighpc, Val: uint64(0x6)}, {Attr: dwarf.AttrStmtList, Val: > int64(0)}}}}, > + }, > }, > { > "testdata/go-relocation-test-gcc441-x86.obj", > - &dwarf.Entry{Offset: 0xb, Tag: dwarf.TagCompileUnit, > Children: true, Field: []dwarf.Field{{Attr: dwarf.AttrProducer, Val: "GNU C > 4.4.1"}, {Attr: dwarf.AttrLanguage, Val: int64(1)}, {Attr: dwarf.AttrName, > Val: "t.c"}, {Attr: dwarf.AttrCompDir, Val: "/tmp"}, {Attr: dwarf.AttrLowpc, > Val: uint64(0x0)}, {Attr: dwarf.AttrHighpc, Val: uint64(0x5)}, {Attr: > dwarf.AttrStmtList, Val: int64(0)}}}, > + []relocationTestEntry{ > + {0, &dwarf.Entry{Offset: 0xb, Tag: > dwarf.TagCompileUnit, Children: true, Field: []dwarf.Field{{Attr: > dwarf.AttrProducer, Val: "GNU C 4.4.1"}, {Attr: dwarf.AttrLanguage, Val: > int64(1)}, {Attr: dwarf.AttrName, Val: "t.c"}, {Attr: dwarf.AttrCompDir, > Val: "/tmp"}, {Attr: dwarf.AttrLowpc, Val: uint64(0x0)}, {Attr: > dwarf.AttrHighpc, Val: uint64(0x5)}, {Attr: dwarf.AttrStmtList, Val: > int64(0)}}}}, > + }, > }, > { > "testdata/go-relocation-test-gcc424-x86-64.obj", > - &dwarf.Entry{Offset: 0xb, Tag: dwarf.TagCompileUnit, > Children: true, Field: []dwarf.Field{{Attr: dwarf.AttrProducer, Val: "GNU C > 4.2.4 (Ubuntu 4.2.4-1ubuntu4)"}, {Attr: dwarf.AttrLanguage, Val: int64(1)}, > {Attr: dwarf.AttrName, Val: "go-relocation-test-gcc424.c"}, {Attr: > dwarf.AttrCompDir, Val: "/tmp"}, {Attr: dwarf.AttrLowpc, Val: uint64(0x0)}, > {Attr: dwarf.AttrHighpc, Val: uint64(0x6)}, {Attr: dwarf.AttrStmtList, Val: > int64(0)}}}, > + []relocationTestEntry{ > + {0, &dwarf.Entry{Offset: 0xb, Tag: > dwarf.TagCompileUnit, Children: true, Field: []dwarf.Field{{Attr: > dwarf.AttrProducer, Val: "GNU C 4.2.4 (Ubuntu 4.2.4-1ubuntu4)"}, {Attr: > dwarf.AttrLanguage, Val: int64(1)}, {Attr: dwarf.AttrName, Val: > "go-relocation-test-gcc424.c"}, {Attr: dwarf.AttrCompDir, Val: "/tmp"}, > {Attr: dwarf.AttrLowpc, Val: uint64(0x0)}, {Attr: dwarf.AttrHighpc, Val: > uint64(0x6)}, {Attr: dwarf.AttrStmtList, Val: int64(0)}}}}, > + }, > + }, > + { > + "testdata/gcc-amd64-openbsd-debug-with-rela.obj", > + []relocationTestEntry{ > + {203, &dwarf.Entry{Offset: 0xc62, Tag: > dwarf.TagMember, Children: false, Field: []dwarf.Field{{Attr: > dwarf.AttrName, Val: "it_interval"}, {Attr: dwarf.AttrDeclFile, Val: > int64(7)}, {Attr: dwarf.AttrDeclLine, Val: int64(236)}, {Attr: > dwarf.AttrType, Val: dwarf.Offset(0xb7f)}, {Attr: dwarf.AttrDataMemberLoc, > Val: []byte{0x23, 0x0}}}}}, > + {204, &dwarf.Entry{Offset: 0xc70, Tag: > dwarf.TagMember, Children: false, Field: []dwarf.Field{{Attr: > dwarf.AttrName, Val: "it_value"}, {Attr: dwarf.AttrDeclFile, Val: int64(7)}, > {Attr: dwarf.AttrDeclLine, Val: int64(237)}, {Attr: dwarf.AttrType, Val: > dwarf.Offset(0xb7f)}, {Attr: dwarf.AttrDataMemberLoc, Val: []byte{0x23, > 0x10}}}}}, > + }, > }, > } > > @@ -207,20 +225,24 @@ > t.Error(err) > continue > } > - reader := dwarf.Reader() > - // Checking only the first entry is sufficient since it has > - // many different strings. If the relocation had failed, all > - // the string offsets would be zero and all the strings > would > - // end up being the same. > - firstEntry, err := reader.Next() > - if err != nil { > - t.Error(err) > - continue > - } > - > - if !reflect.DeepEqual(test.firstEntry, firstEntry) { > - t.Errorf("#%d: mismatch: got:%#v want:%#v", i, > firstEntry, test.firstEntry) > - continue > + for _, testEntry := range test.entries { > + reader := dwarf.Reader() > + for j := 0; j < testEntry.entryNumber; j++ { > + entry, err := reader.Next() > + if entry == nil || err != nil { > + t.Errorf("Failed to skip to entry > %d: %v", testEntry.entryNumber, err) > + continue > + } > + } > + entry, err := reader.Next() > + if err != nil { > + t.Error(err) > + continue > + } > + if !reflect.DeepEqual(testEntry.entry, entry) { > + t.Errorf("#%d/%d: mismatch: got:%#v > want:%#v", i, testEntry.entryNumber, entry, testEntry.entry) > + continue > + } > } > } > } > Index: src/pkg/debug/elf/testdata/gcc-amd64-openbsd-debug-with-rela.obj > =================================================================== > new file mode 100644 > index > 0000000000000000000000000000000000000000..f62b1ea1cada83530e45320cc58394933753dddb > GIT binary patch > literal 6544 > zc$}44YmD4h6}}$ZV~=Mav)NDzjl`CKT4=M6Bu%#2ZIY&sJeo#*kfuVFEYFN*##xU& > zu|2cd^pUhlq0OTYAeHz50xDIAKdn%t2!2or_)+?!LPA2I{-~(<)fTCRQcw#G=iGCz > z$Lq`xu(IQOzVp22o_p?{$?-e(-&s%;CLe`uXML*w?3wJ3cZ$z3R=!M5>M9e;m(9uY > zHRgn}MyV7TD@-co^0x9qSpiGxC{s-&_D;@QjA`Sn3`55BFTr4YSv46eUX_Y4W+=+u > zNmMkiWqT)Qz-@^n8M7u8)$Cg?qbbHlKAZ72I+`&x2E<t3^?0hTqi98l>cpMcoNDeE > zDFDFsRbVQ7hNiOnz`?28rZ!J+oSvK7I6J#_cI({DQ%sp@g>84{aOnCoaa5aWhWu4) > z*Jo<Ya6oFoU_~kP;H`ET4GQa-8o>AzADc`wAAyms6l*OfV!Eb#LBfjm?(qI3@`9!f > zq6f*2`ns^AX6&fKj#^8{BV2vgO5(<#t$q){qHTlOc{^c+g?Gta>@@?oKE=HPyDRyN > zWd-?WH^@nVz}yy;xMc%M6!f?iM#&U~%k_!9eLrU##7@l!Ie2Z_h+;cVQC!^=+4&?Y > zYQ_MSEt^qb*Gnj1>V6nBr}#(4If>l<8!lq>MFv>2eb<R;w!7c#N0EW|e4HkYP@3A_ > zbxKZ}@O|5q)#5avyQ+C{!uQ8Haf<!!^LcT?4_xevH)MqOW^JeFeer5h3h(~BEWuyP > z2~%(f^TGr_kQ1iR4xP^l6Z~L5;!YAF(sb1R(q&dnuv;h8{;~~Nv9@y*#g;Pe1mp`> > zk=yj*#O2mPbOu<@sn<CWogknifmi^&*gNJTkVpv$bOliJ!`c!8M@7AIz^fy)BA|NM > zt@>Q+*d7h%p%H?-h95c!vL3D>OVc=joUqYA{E?Sv0+5NiN6@1pei_SSB9>SJ$Av)R > zwK;u41cEf5-{S)4IkpdJVTF@_0`JC$@bLlsnfL*X)-<eC#!j3jyIN&U9hywDgW<() > z!c>O45}QrqGE7s$o*Ti0mTf0$!k_CfGJp*UR>Y2-p1T^au(An(En`=~NKcDMm$8C? > zJ}c;`8}O-}ll)o|IYErjGiZsRl@1qrRv@)bm#>U=oyb_-?IC%+sOg4xw1Zx>uZU`H > zlK59;adYa1wA5`;FWQZLr~_RMp$+Mt50RV}5=-t1k~3n3+F>0d)Mf=c@Vx-bNt+Wu > zhkR}n5k|h(MtYMd>)3&9)%JXDbhGG6vD=J~A#jt>ilHQZH$Z5Mup2K|eeP&$x}UkD > zdBN|-ZY>Pzm{9Fzsgnh_$(+StsKysZTev8(hUW`x5wVdI+OC!X$1%*f8YfQT&NJ<{ > z)yRIP-H!X08F=6wHAER6=V>Alw0Fk>aEe`<j>B0zjez2Z3Df8H6N#Nc-}odE*ff~F > zi7$CKAibMNPpEAnHLkUlv`A^5E6K9ax4nvWlxZdU!p~7aoQ?V(ep<+*Qooz8hdlW7 > z`_@S>L?7Z6B9Axyp{isP?Zkr3d#2tvFWE#dT}-FSorT<4H$8^+&bCy53?7M7fJ{W( > z#F4JUC)XthwxoN+6(z1Ot0lOtnZL*TnQWZ;!*Ag9W!uyrc?(I|H1$W{=UhoXrf8T< > zDf|T9Qi^pXDZZ0D0d>hgbv`92+$Ra(TUP(ZZ*eK)hSk6M7Lv4G^{3uPg2qo@;$yxE > z_0yZ+Hd>@DsGr$|gxG-k*?W<a+fP5|AW54~e<nggY(4$i$B~kIOn>erPKxcMKmU4> > zFB4b%)|;H@?;8Ev?{MiX^qo%;%4{S3{G`E=bQ9?>TxalE$cCf;V6;epXgB%~r?}xv > zJJ5ghJcrV~ufKQ^0eNESKUt$mlEQs?JpyvME<D32N&f8nNQx6h|HaFa;^F-2RV3-e > z(qHp6>H>AU?Ziv0xb+|ujRhng4kwBW4&2h!2F$+dB{3^5A`K@9Gd95BNLg(BFF*=N > zrFaX!?W4Dz<FiWf>#qPkhDVgK5gREUKS_`GEM{yX#3Y5d16ajacstctRLiMGz%_1J > zrEx3_TxJ^kzBmF=Cdcfy7j)sOyzIg`Y20;6T&;moj}w$<#(n}dW>sNtAyhHW+=J@e > z{8{O+5jjAfJ824U9?u_6Y07x__=pVfz2hWL)L)3IFpE|yXoPI7@wd0peCrsT*=a;x > zZHXCw7qM{CsUK#>NAep0wougghxi(xcA~I>z&{D7$cT8(rpz1HjEa!V4X<QEGN)gW > zAth}#Uuyk%6f@PfakeEdji-&NW2M7sL~Xkjb|Y3=k`bXu-C1EJUxtK82uf`c5&|nO > zbnH?<$XY~}xl%~T)m(-n9YWS2;>EB&taOB+DTJEs1WJ^ZB3ke35#y^}is>8EEGt$! > z4eFT8n#Qu_KvzOD7qF5Mg4{JDr_CzX_FEw<YPA7Z@_<=8er(Y6t7s<<VrK1pYD_FR > z8IJ>I5~$Xo`!MU17fsSRoG|NOXSjiadbq)?e-ojqOU(N8f=N}Tndp%l!zBe{cmA4( > z5W_9E)??Q0G=|u&M`4FqdkE1;3od1`2U*sv2QG2N9&FUZVZ^LM-<6s@Ts(vxB&a~D > zau1r;<D17tby!baFjwzE>!gg8E4AtoTWdZ2!Z`YB*$G75hM-Pg8WYZ1cFpPV^qnC^ > z6S2CJI2=4n;0S_Xi(}^~XB9S%9_oI=@39q2Q+@|-D%FNh>!B+m83dt_Rpc`C<r=vY > za5HR%wmr+rl@EcVlvMfkTr;uOrNA63Pkg456Ttmx;3fTris)Pgp-^|L-6o#~E3dg` > zwH|vPD~`Q`zn3VVj9Ku>e~M9Nx^Xn)1vS50cW2s8t>r<1t^(tgcondk8$iRHo}Ph! > zouwwq)*RnwY%xyiUa%cK+juRGr(5`g&0@9d`N@Vnr@&Zd7`wh(O9TuBKqI?96eE}% > zECviTp>BsAH>k$->1N2_7-ox1{(^D<vhhbqb(;B)`@|g2WIluVcnNMqYMD@LSZV+P > z)pZ%0j#t`=Q-yaD(Yqx_iF*{1!p%%q;~0kAu2Zc>ZjVhzuI~tlw^uRtG4okwr&|pi > zI8vklm>>9?&j0yb!|1oOVh-OqL|)6$*9v_#hfDpdb9g03Uk88Y0R0F5NB=i6Ht=^t > zssD!@o|oM*M1NL*2XX=DWw#B837_l5#P1ObaANW*GzDMg<*p(AADf`}hdDei`_K^m > zXbru0UP1p58yx@9&#jIV<2Tvm@%uyk|5W&2cLo1A&HUQq^M=r0zJfl|g#r4nT}AJC > zp#Zl8hQFic<1Y>IF9p_L!T*1IH1@%zLAW}^iaKy6h!2bq&_OGQxIP3ohT!H9+`0_I > z$R{#!$~;c+zrS9RmgM{ML-5O5Y}$;|M$8}CmoSp-tZvQ3lg--RA^5!+JJ~EeSMK9j > z3nK#m<q*6!vKp7qksL1n+v1}k^4jQN`L*MN{k}KepI7#OKSnEkd?bV4k-@X8n9XOl > G=l=(aF1s24 > > >
Sign in to reply to this message.
On 14 November 2012 01:33, Ian Lance Taylor <iant@google.com> wrote: > This is going to change the behaviour of the Symbols function in > debug/elf and potentially break any current users. I don't know if > that is a good idea. Certainly any change would need to be documented > in doc/go1.1.html. Indeed. As I've noted in the issue, there are two obvious fixes - stop skipping the first symbol, or adjust the symbol index in the relocation code. While the second of these avoids a change in existing behaviour, I think the first option is more "correct" since we expose the ELF and DWARF data. Anyone using these packages is currently expected to know that an index into the symbol table needs to be adjusted to account for the fact that debug/elf is discarding the first symbol.
Sign in to reply to this message.
On Tue, Nov 13, 2012 at 7:15 AM, Joel Sing <jsing@google.com> wrote: > On 14 November 2012 01:33, Ian Lance Taylor <iant@google.com> wrote: >> This is going to change the behaviour of the Symbols function in >> debug/elf and potentially break any current users. I don't know if >> that is a good idea. Certainly any change would need to be documented >> in doc/go1.1.html. > > Indeed. As I've noted in the issue, there are two obvious fixes - stop > skipping the first symbol, or adjust the symbol index in the > relocation code. While the second of these avoids a change in existing > behaviour, I think the first option is more "correct" since we expose > the ELF and DWARF data. Anyone using these packages is currently > expected to know that an index into the symbol table needs to be > adjusted to account for the fact that debug/elf is discarding the > first symbol. Makes sense to me. Can you please add a change to go1.1.html to this CL? Thanks. Ian
Sign in to reply to this message.
On 2012/11/13 16:46:32, iant2 wrote: > On Tue, Nov 13, 2012 at 7:15 AM, Joel Sing <mailto:jsing@google.com> wrote: > > On 14 November 2012 01:33, Ian Lance Taylor <mailto:iant@google.com> wrote: > >> This is going to change the behaviour of the Symbols function in > >> debug/elf and potentially break any current users. I don't know if > >> that is a good idea. Certainly any change would need to be documented > >> in doc/go1.1.html. > > > > Indeed. As I've noted in the issue, there are two obvious fixes - stop > > skipping the first symbol, or adjust the symbol index in the > > relocation code. While the second of these avoids a change in existing > > behaviour, I think the first option is more "correct" since we expose > > the ELF and DWARF data. Anyone using these packages is currently > > expected to know that an index into the symbol table needs to be > > adjusted to account for the fact that debug/elf is discarding the > > first symbol. > > Makes sense to me. Can you please add a change to go1.1.html to this > CL? Thanks. Done. PTAL.
Sign in to reply to this message.
LGTM with one change. https://codereview.appspot.com/6848044/diff/10001/doc/go1.1.html File doc/go1.1.html (right): https://codereview.appspot.com/6848044/diff/10001/doc/go1.1.html#newcode71 doc/go1.1.html:71: processes the symbol table returned from debug/elf may need to be adjusted to s|processes the symbol table returned from debug/elf|calls the debug/elf functions Symbols or ImportedSymbols|
Sign in to reply to this message.
On 2012/11/14 14:19:36, iant wrote: > LGTM with one change. > > https://codereview.appspot.com/6848044/diff/10001/doc/go1.1.html > File doc/go1.1.html (right): > > https://codereview.appspot.com/6848044/diff/10001/doc/go1.1.html#newcode71 > doc/go1.1.html:71: processes the symbol table returned from debug/elf may need > to be adjusted to > s|processes the symbol table returned from debug/elf|calls the debug/elf > functions Symbols or ImportedSymbols| Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=9ea9e7e6e0c8 *** debug/elf: do not skip first symbol in the symbol table Do not skip the first symbol in the symbol table. Any other indexes into the symbol table (for example, indexes in relocation entries) will now refer to the symbol following the one that was intended. Add an object that contains debug relocations, which debug/dwarf failed to decode correctly. Extend the relocation tests to cover this case. Note that the existing tests passed since the symbol following the symbol that required relocation is also of type STT_SECTION. Fixes issue 4107. R=golang-dev, mikioh.mikioh, iant, iant CC=golang-dev http://codereview.appspot.com/6848044
Sign in to reply to this message.
Message was sent while issue was closed.
Old CL, but I found this via the Go 1.1 release notes. I am concerned about the API change here, even though it does not manifest in a Go signature change. Is there something wrong with requiring people to write -1 when indexing into the symbol list? I am concerned that people who have already written the -1 in Go 1.0 will now have to take it out in Go 1.1. This seems to me a violation of the Go 1 compatibility promise.
Sign in to reply to this message.
why not replicate the first element and give an address of array[1] On Tue, Mar 19, 2013 at 4:28 PM, <rsc@golang.org> wrote: > Old CL, but I found this via the Go 1.1 release notes. I am concerned > about the API change here, even though it does not manifest in a Go > signature change. Is there something wrong with requiring people to > write -1 when indexing into the symbol list? I am concerned that people > who have already written the -1 in Go 1.0 will now have to take it out > in Go 1.1. This seems to me a violation of the Go 1 compatibility > promise. > > > https://codereview.appspot.**com/6848044/<https://codereview.appspot.com/6848... > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > > -- Michael T. Jones | Chief Technology Advocate | mtj@google.com | +1 650-335-5765
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/03/19 23:28:25, rsc wrote: > Old CL, but I found this via the Go 1.1 release notes. I am concerned about the > API change here, even though it does not manifest in a Go signature change. Is > there something wrong with requiring people to write -1 when indexing into the > symbol list? I am concerned that people who have already written the -1 in Go > 1.0 will now have to take it out in Go 1.1. This seems to me a violation of the > Go 1 compatibility promise. As discussed at the time of the CL, we already expose the ELF and DWARF data, some of which specify indexes into the symbol table. All of these indexes are now incorrect since we've silently dropped a symbol. I agree that this changes the behaviour of debug/elf between Go 1.0 and Go 1.1, however the counter argument is that previously you had to know that you needed to subtract one when indexing into the slice returned by Symbols. I do not believe this was documented anywhere, so if people are using this code for Go 1.0 they have already read well beyond the documentation. Given the amount of time and effort that it took me to debug the problem that this was causing (a missing -1 in our own standard library), I am strongly in favour of returning the full and correct symbol table. Another option would be to add a new function that returns all of the symbols, and leave Symbols with the broken Go 1.0 behaviour.
Sign in to reply to this message.
Message was sent while issue was closed.
Rereading the review, Ian had the same reaction I did. The fact is, it is possible to write correct code with the Go 1.0 interface, so the Go 1.1 interface should not change. Yes, it means that people have to subtract 1 to index into the symbols list, and yes, if we were starting from scratch we probably wouldn't make that decision, but we did promise not to change the semantics of existing code unnecessarily, and this seems unnecessary. I will send a CL to revert to the old behavior and fix the relocation indices.
Sign in to reply to this message.
|