code review 6848044: debug/elf: do not skip first symbol in the symbol table

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.

[jsing, 2012-11-13 13:49:24]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[mikio, 2012-11-13 14:30:57]

LGTM

Thanks! it also passes on freebsd.

[iant2, 2012-11-13 14:33:27]

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
>
>
>

[jsing, 2012-11-13 15:15:41]

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.

[iant2, 2012-11-13 16:46:32]

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

[jsing, 2012-11-14 10:35:34]

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.

[iant, 2012-11-14 14:19:36]

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|

[jsing, 2012-11-14 15:22:31]

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.

[jsing, 2012-11-14 15:27:16]

*** 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

[rsc, 2013-03-19 23:28:25]

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.

[mtj1, 2013-03-19 23:44:06]

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

[jsing, 2013-03-20 03:21:22]

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.

[rsc, 2013-03-21 20:31:31]

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.