From 8314544bd6b3c5f0bee89a6bd411ced0aeba1a8c Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Tue, 25 Jan 2022 09:34:35 -0500 Subject: [PATCH] debug/dwarf: fix problems with handling of bit offsets for bitfields This patch reworks the handling of the DWARF DW_AT_bit_offset and DW_AT_data_bit_offset attributes to resolve problems arising from a previous related change (CL 328709). In CL 328709 the DWARF type reader was updated to look for and use the DW_AT_data_bit_offset attribute for structure fields, handling the value of the attribute in the same way as for DW_AT_bit_offset. This caused problems for clients, since the two attributes have very different semantics. This CL effectively reverts CL 328709 and moves to a scheme in which we detect and report the two attributes separately/independently. This patch also corrects a problem in the DWARF type reader in the code that detects and fixes up the type of struct fields corresponding to zero-length arrays; the code in question was testing the DW_AT_bit_offset attribute value but assuming DW_AT_data_bit_offset semantics, meaning that it would fail to fix up cases such as typedef struct another_struct { unsigned short quix; int xyz[0]; unsigned x:1; long long array[40]; } t; The code in question has been changed to avoid using BitOffset and instead consider only ByteOffset and BitSize. Fixes #50685. Updates #46784. Change-Id: Ic15ce01c851af38ebd81af827973ec49badcab6f Reviewed-on: https://go-review.googlesource.com/c/go/+/380714 Trust: Than McIntosh Run-TryBot: Than McIntosh TryBot-Result: Gopher Robot Reviewed-by: Ian Lance Taylor --- api/go1.18.txt | 2 + src/debug/dwarf/testdata/bitfields.c | 17 +++ src/debug/dwarf/testdata/bitfields.elf4 | Bin 0 -> 2464 bytes src/debug/dwarf/testdata/typedef.elf5 | Bin 0 -> 6016 bytes src/debug/dwarf/type.go | 144 ++++++++++++++++++++---- src/debug/dwarf/type_test.go | 85 +++++++++++--- 6 files changed, 209 insertions(+), 39 deletions(-) create mode 100644 src/debug/dwarf/testdata/bitfields.c create mode 100644 src/debug/dwarf/testdata/bitfields.elf4 create mode 100644 src/debug/dwarf/testdata/typedef.elf5 diff --git a/api/go1.18.txt b/api/go1.18.txt index afcb31c638..7805d29eb7 100644 --- a/api/go1.18.txt +++ b/api/go1.18.txt @@ -13,6 +13,8 @@ pkg debug/buildinfo, func ReadFile(string) (*debug.BuildInfo, error) pkg debug/buildinfo, type BuildInfo = debug.BuildInfo pkg debug/elf, const R_PPC64_RELATIVE = 22 pkg debug/elf, const R_PPC64_RELATIVE R_PPC64 +pkg debug/dwarf, type BasicType struct, DataBitOffset int64 +pkg debug/dwarf, type StructField struct, DataBitOffset int64 pkg debug/plan9obj, var ErrNoSymbols error pkg go/ast, method (*IndexListExpr) End() token.Pos pkg go/ast, method (*IndexListExpr) Pos() token.Pos diff --git a/src/debug/dwarf/testdata/bitfields.c b/src/debug/dwarf/testdata/bitfields.c new file mode 100644 index 0000000000..05833336c9 --- /dev/null +++ b/src/debug/dwarf/testdata/bitfields.c @@ -0,0 +1,17 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +/* +Linux ELF: +gcc -gdwarf-4 -m64 -c bitfields.c -o bitfields.elf4 +*/ + +typedef struct another_struct { + unsigned short quix; + int xyz[0]; + unsigned x:1; + long long array[40]; +} t_another_struct; +t_another_struct q2; + diff --git a/src/debug/dwarf/testdata/bitfields.elf4 b/src/debug/dwarf/testdata/bitfields.elf4 new file mode 100644 index 0000000000000000000000000000000000000000..2e06e68ce9dded2509e1dae216322d9409cbd070 GIT binary patch literal 2464 zcmbtWON$dh5U$S4?9L`;3BK?(ilU;KY?gIdl-0ocS`_i(Ng+&<-t1s@qB9*$_M+fH zJb6?EAK=A5;6>1*zra7CC-o!NKCD;hBQO`&_FVm)^u2_oXImRZhqD+&)EK(se7_I}GYLS{904v-fjqT1v zLB)G$<{_8bfD$$gkp+YhgIr#s6Pc{%3&>@nn1|@0&m_ehz#!SLg$A&VJCRS_m<3jP zirR!+ggn5e+5TeLN*A{mOIrX^WtJ{l*<;pmiSj)4Q~cM|452Rs|h!KX~>z7 zw9_#02Ym83ynx6weEDH3&pCbh;wOlst5>25RtdvJV?L4!ERC22T4UYD8kx&lP3&4g9gP?;5x=LHqT--YLg3BmTa! zUnxn)s?U%hty7)oiV=$Bgu^xAHc^TolB-%1d?3J)bLcd~5S)N_T}RGo!}C{q=%bl! zxXot3Z~o$T1K00xOo<}8p3k+8)xl}?*48*qw1anrJ9sB6p>SK*qL%)LZ^vYF?^Lf& zEP?d+Y1bZ92#e1g`Rc?H$jm>0G4y!-Ykm!L&6>1qofy*FNQ}4{djvf+&-GD6q`XuO zT~Y}&rxd3jT78ce5i@I2h3~|W^xQ?(@3C|rlJcJ@(AJ4cnz82*PwM}xx}>^K>YsiI z&Hm}$nKcoDHwvWliIW*iBS_dxJP7pwGP5qCYABO)5&P{3ZXk$~VH`qsljN zy{^W|P73{`z?Uizf3W47V4fenOm}*R5Sl4?e@uM literal 0 HcmV?d00001 diff --git a/src/debug/dwarf/testdata/typedef.elf5 b/src/debug/dwarf/testdata/typedef.elf5 new file mode 100644 index 0000000000000000000000000000000000000000..aec48f64522f651a2b5c341c4b6c0badf92c2801 GIT binary patch literal 6016 zcmbuCc|6tG8^^!*TCy(@ku2HCt(V~=QDK#;ghTl2YbNs%YF@OEe>vcWnd7sboobx@~_iNpP03R-g z!;r#ZJemJ_gffiRWL8~T^V$T&%9cwJDS(ikn+M?95eXdx`b~#ZVhL}W;d4KweN1{h&42u4wMI2{+bavI;^B; zyWGci|JCo}Ubp=1+wOjUR@bS(!?`|MCYxMcvO1$ZD^upH+sr=TY_<2(yoHOhloJb{ zCiYez%kZ7w;WV-_?vs-@UI!E=4&N<)ReE;a@RX<9K3sk5y0NX`Z<8A}wT`lfe#nV& zdEc*EdEVY2$svO((cw0y_pMVm$+}!^uMVkwZTCWFp^b%pYWp3%)D^pz&ok)yBjiq8 z{jiFR;-JTmp_Ml+Th38#m>{^|v!hDg4Zvu9QXuY$3<&%OHAq}?GzE9FOnxsxLO`omYQq$+vf2i-P&M-S>&dBS`zpI{__g%N& z^jOE>v_ZqV`l))s@ip7mIJEGd-}4{H^{?6gT>JZ)2j-Qmf^YRd+o@|9YQ6UZQOdPj-u7)nn{=njbBC1z>dtPzI7xiYpqE>*X=F=(nEV3Q z>D8Aa);B9u^uMvqvsCeSxKVn|?0HGERlrcJaEXp=TZ%K^GS6K8R#Xaam9NEa-o=)H zq0<3h)SFE@)~8(FZujww!tVt?Prs4(vS z_lBaQo?qA#8@|0gM2xj&bx@~xO>#U-Lzv7siIX(0B!*4EZEjd`K``%9Oi?r24 zM@O7OgF4^)xzIhdLmbSlIb$Ug^K4-61GnrxxepAj%s1HVSX}q!+0)*ny4B`Qi5ZWU ziKjO?j%?IA^Go4cUnMu+-=orGu4`5{wb~AjR49MmSREd}{@rihm`)oz*5mWwK>3o2 z4nagjL?pvelaFq?oHLzalo`f55Xs~i1;z$h4u|uXnkqJU^3V@ejHL6(RUU@?$#G2Ezj)2Xs{9_&FXbjv zyMBx)xnFYOZm2?6TK7e{D8(Zm)Qg-goIRrhOn&)(YeS{+@%*w%!}puKcFw(2ed5R; z)7O^I>VC6zt4{DmH}x|KYdt;k6nf8Bp)KaxWYH;(qp9*sAOouBF*4=d6&+$`M-Y?0URD=D0}M*48;A z+dSn}pn2h^>+Eb!=tW=N&=9^|t0b2{U};!#WlqU?8~yY%_S#3L$@CSk{Ceg$M$8_q z=J9oorsi8xVOuqG;bOnV9gAYx?RkOxYqbGC{cO+k?+o%=Y_s_1AnTx+{vGyyc0517 znSN$Ieu{VI$SN~>V$(~E@4R{L##Zx%k@12gV?Lkf$aApcJ6c0^tRq=ujL{FjMtlR8 zGHA)0Fe8NnK3Q*VXy9Z9GaQKqXPU|+g)OMj0CD`o1#LuznX9I;fjduC-UkV=|1SmS zm1PD-Cu)`%pOv>;Wl+RKMTU`|2s1K_;=gd|NdM6$!>Ig&VWlR@L<)H(HQ>Z27Qe=^ zUIPLMp8|X;#j%g|#uUf*0OnQ{Hv~@Z1(M$cxGSYM1MWp}oEPU`Nb#A#mrxw{hxN-S zJ{x!h#od9&QQQ-F3dOyFODOIGJcr``zzZmjFG4)dEffz%{$$p^8s7opj1*zCFoqY! z2pk!~ECC~MVg$1pfioj;VFa#>z>N{`9ng=Den`Q0L=wJZB>FQ8{g9ZC)O;l8BR$`h zNfg8;3x^zFarWEv9E;;;CZ&hP$M0CwIK<-Y zx2y(Sj<{cPH{x8@ERNr`qOvBWuaxQ(1g2i|euYN){*Y z4%{3MoILMc*bics9*{H}zVmUxdRS7aPv#ttldYd2Z5`9&_nIB}_&Wm^oCotI*g=Iq zX2$sn@IAoN`Hg|&KH2>;ywr?Du^Nw?h)*lY}G$bwR2ahfDpa 0 { s += " : " + strconv.FormatInt(f.BitSize, 10) - s += "@" + strconv.FormatInt(f.BitOffset, 10) + s += "@" + strconv.FormatInt(f.bitOffset(), 10) } } s += "}" @@ -469,8 +554,12 @@ func (d *Data) readType(name string, r typeReader, off Offset, typeCache map[Off // AttrName: name of base type in programming language of the compilation unit [required] // AttrEncoding: encoding value for type (encFloat etc) [required] // AttrByteSize: size of type in bytes [required] - // AttrBitOffset: for sub-byte types, size in bits - // AttrBitSize: for sub-byte types, bit offset of high order bit in the AttrByteSize bytes + // AttrBitOffset: bit offset of value within containing storage unit + // AttrDataBitOffset: bit offset of value within containing storage unit + // AttrBitSize: size in bits + // + // For most languages BitOffset/DataBitOffset/BitSize will not be present + // for base types. name, _ := e.Val(AttrName).(string) enc, ok := e.Val(AttrEncoding).(int64) if !ok { @@ -517,8 +606,12 @@ func (d *Data) readType(name string, r typeReader, off Offset, typeCache map[Off t.Name = name t.BitSize, _ = e.Val(AttrBitSize).(int64) haveBitOffset := false - if t.BitOffset, haveBitOffset = e.Val(AttrBitOffset).(int64); !haveBitOffset { - t.BitOffset, _ = e.Val(AttrDataBitOffset).(int64) + haveDataBitOffset := false + t.BitOffset, haveBitOffset = e.Val(AttrBitOffset).(int64) + t.DataBitOffset, haveDataBitOffset = e.Val(AttrDataBitOffset).(int64) + if haveBitOffset && haveDataBitOffset { + err = DecodeError{name, e.Offset, "duplicate bit offset attributes"} + goto Error } case TagClassType, TagStructType, TagUnionType: @@ -533,6 +626,7 @@ func (d *Data) readType(name string, r typeReader, off Offset, typeCache map[Off // AttrType: type of member [required] // AttrByteSize: size in bytes // AttrBitOffset: bit offset within bytes for bit fields + // AttrDataBitOffset: field bit offset relative to struct start // AttrBitSize: bit size for bit fields // AttrDataMemberLoc: location within struct [required for struct, class] // There is much more to handle C++, all ignored for now. @@ -551,7 +645,8 @@ func (d *Data) readType(name string, r typeReader, off Offset, typeCache map[Off t.Incomplete = e.Val(AttrDeclaration) != nil t.Field = make([]*StructField, 0, 8) var lastFieldType *Type - var lastFieldBitOffset int64 + var lastFieldBitSize int64 + var lastFieldByteOffset int64 for kid := next(); kid != nil; kid = next() { if kid.Tag != TagMember { continue @@ -578,30 +673,31 @@ func (d *Data) readType(name string, r typeReader, off Offset, typeCache map[Off f.ByteOffset = loc } - haveBitOffset := false f.Name, _ = kid.Val(AttrName).(string) f.ByteSize, _ = kid.Val(AttrByteSize).(int64) - if f.BitOffset, haveBitOffset = kid.Val(AttrBitOffset).(int64); !haveBitOffset { - f.BitOffset, haveBitOffset = kid.Val(AttrDataBitOffset).(int64) + haveBitOffset := false + haveDataBitOffset := false + f.BitOffset, haveBitOffset = kid.Val(AttrBitOffset).(int64) + f.DataBitOffset, haveDataBitOffset = kid.Val(AttrDataBitOffset).(int64) + if haveBitOffset && haveDataBitOffset { + err = DecodeError{name, e.Offset, "duplicate bit offset attributes"} + goto Error } f.BitSize, _ = kid.Val(AttrBitSize).(int64) t.Field = append(t.Field, f) - bito := f.BitOffset - if !haveBitOffset { - bito = f.ByteOffset * 8 - } - if bito == lastFieldBitOffset && t.Kind != "union" { + if lastFieldBitSize == 0 && lastFieldByteOffset == f.ByteOffset && t.Kind != "union" { // Last field was zero width. Fix array length. // (DWARF writes out 0-length arrays as if they were 1-length arrays.) fixups.recordArrayType(lastFieldType) } lastFieldType = &f.Type - lastFieldBitOffset = bito + lastFieldByteOffset = f.ByteOffset + lastFieldBitSize = f.BitSize } if t.Kind != "union" { b, ok := e.Val(AttrByteSize).(int64) - if ok && b*8 == lastFieldBitOffset { + if ok && b == lastFieldByteOffset { // Final field must be zero width. Fix array length. fixups.recordArrayType(lastFieldType) } diff --git a/src/debug/dwarf/type_test.go b/src/debug/dwarf/type_test.go index 431d0853e0..0acc606df7 100644 --- a/src/debug/dwarf/type_test.go +++ b/src/debug/dwarf/type_test.go @@ -83,15 +83,19 @@ func peData(t *testing.T, name string) *Data { return d } -func TestTypedefsELF(t *testing.T) { testTypedefs(t, elfData(t, "testdata/typedef.elf"), "elf") } +func TestTypedefsELF(t *testing.T) { + testTypedefs(t, elfData(t, "testdata/typedef.elf"), "elf", typedefTests) +} func TestTypedefsMachO(t *testing.T) { - testTypedefs(t, machoData(t, "testdata/typedef.macho"), "macho") + testTypedefs(t, machoData(t, "testdata/typedef.macho"), "macho", typedefTests) } -func TestTypedefsELFDwarf4(t *testing.T) { testTypedefs(t, elfData(t, "testdata/typedef.elf4"), "elf") } +func TestTypedefsELFDwarf4(t *testing.T) { + testTypedefs(t, elfData(t, "testdata/typedef.elf4"), "elf", typedefTests) +} -func testTypedefs(t *testing.T, d *Data, kind string) { +func testTypedefs(t *testing.T, d *Data, kind string, testcases map[string]string) { r := d.Reader() seen := make(map[string]bool) for { @@ -115,7 +119,7 @@ func testTypedefs(t *testing.T, d *Data, kind string) { typstr = t1.Type.String() } - if want, ok := typedefTests[t1.Name]; ok { + if want, ok := testcases[t1.Name]; ok { if seen[t1.Name] { t.Errorf("multiple definitions for %s", t1.Name) } @@ -130,7 +134,7 @@ func testTypedefs(t *testing.T, d *Data, kind string) { } } - for k := range typedefTests { + for k := range testcases { if !seen[k] { t.Errorf("missing %s", k) } @@ -229,21 +233,42 @@ func TestUnsupportedTypes(t *testing.T) { } } -func TestBitOffsetsELF(t *testing.T) { testBitOffsets(t, elfData(t, "testdata/typedef.elf")) } +var expectedBitOffsets1 = map[string]string{ + "x": "S:1 DBO:32", + "y": "S:4 DBO:33", +} + +var expectedBitOffsets2 = map[string]string{ + "x": "S:1 BO:7", + "y": "S:4 BO:27", +} + +func TestBitOffsetsELF(t *testing.T) { + f := "testdata/typedef.elf" + testBitOffsets(t, elfData(t, f), f, expectedBitOffsets2) +} func TestBitOffsetsMachO(t *testing.T) { - testBitOffsets(t, machoData(t, "testdata/typedef.macho")) + f := "testdata/typedef.macho" + testBitOffsets(t, machoData(t, f), f, expectedBitOffsets2) } func TestBitOffsetsMachO4(t *testing.T) { - testBitOffsets(t, machoData(t, "testdata/typedef.macho4")) + f := "testdata/typedef.macho4" + testBitOffsets(t, machoData(t, f), f, expectedBitOffsets1) } func TestBitOffsetsELFDwarf4(t *testing.T) { - testBitOffsets(t, elfData(t, "testdata/typedef.elf4")) + f := "testdata/typedef.elf4" + testBitOffsets(t, elfData(t, f), f, expectedBitOffsets1) +} + +func TestBitOffsetsELFDwarf5(t *testing.T) { + f := "testdata/typedef.elf5" + testBitOffsets(t, elfData(t, f), f, expectedBitOffsets1) } -func testBitOffsets(t *testing.T, d *Data) { +func testBitOffsets(t *testing.T, d *Data, tag string, expectedBitOffsets map[string]string) { r := d.Reader() for { e, err := r.Next() @@ -262,15 +287,26 @@ func testBitOffsets(t *testing.T, d *Data) { t1 := typ.(*StructType) + bitInfoDump := func(f *StructField) string { + res := fmt.Sprintf("S:%d", f.BitSize) + if f.BitOffset != 0 { + res += fmt.Sprintf(" BO:%d", f.BitOffset) + } + if f.DataBitOffset != 0 { + res += fmt.Sprintf(" DBO:%d", f.DataBitOffset) + } + return res + } + for _, field := range t1.Field { // We're only testing for bitfields if field.BitSize == 0 { continue } - - // Ensure BitOffset is not zero - if field.BitOffset == 0 { - t.Errorf("bit offset of field %s in %s %s is not set", field.Name, t1.Kind, t1.StructName) + got := bitInfoDump(field) + want := expectedBitOffsets[field.Name] + if got != want { + t.Errorf("%s: field %s in %s: got info %q want %q", tag, field.Name, t1.StructName, got, want) } } } @@ -279,3 +315,22 @@ func testBitOffsets(t *testing.T, d *Data) { } } } + +var bitfieldTests = map[string]string{ + "t_another_struct": "struct another_struct {quix short unsigned int@0; xyz [0]int@4; x unsigned int@4 : 1@31; array [40]long long int@8}", +} + +// TestBitFieldZeroArrayIssue50685 checks to make sure that the DWARF +// type reading code doesn't get confused by the presence of a +// specifically-sized bitfield member immediately following a field +// whose type is a zero-length array. Prior to the fix for issue +// 50685, we would get this type for the case in testdata/bitfields.c: +// +// another_struct {quix short unsigned int@0; xyz [-1]int@4; x unsigned int@4 : 1@31; array [40]long long int@8} +// +// Note the "-1" for the xyz field, which should be zero. +// +func TestBitFieldZeroArrayIssue50685(t *testing.T) { + f := "testdata/bitfields.elf4" + testTypedefs(t, elfData(t, f), "elf", bitfieldTests) +} -- 2.50.0