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 <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

6 files changed, 209 insertions, 39 deletions

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 0000000000..2e06e68ce9
--- /dev/null
+++ b/src/debug/dwarf/testdata/bitfields.elf4
diff --git a/src/debug/dwarf/testdata/typedef.elf5 b/src/debug/dwarf/testdata/typedef.elf5
new file mode 100644
index 0000000000..aec48f6452
--- /dev/null
+++ b/src/debug/dwarf/testdata/typedef.elf5
diff --git a/src/debug/dwarf/type.go b/src/debug/dwarf/type.go
index 2e5a605174..9c15cfb920 100644
--- a/src/debug/dwarf/type.go
+++ b/src/debug/dwarf/type.go
@@ -33,10 +33,14 @@ func (c *CommonType) Size() int64 { return c.ByteSize }
 // Basic types
 
 // A BasicType holds fields common to all basic types.
+//
+// See the documentation for StructField for more info on the interpretation of
+// the BitSize/BitOffset/DataBitOffset fields.
 type BasicType struct {
 	CommonType
-	BitSize   int64
-	BitOffset int64
+	BitSize       int64
+	BitOffset     int64
+	DataBitOffset int64
 }
 
 func (b *BasicType) Basic() *BasicType { return b }
@@ -150,13 +154,87 @@ type StructType struct {
 }
 
 // A StructField represents a field in a struct, union, or C++ class type.
+//
+// Bit Fields
+//
+// The BitSize, BitOffset, and DataBitOffset fields describe the bit
+// size and offset of data members declared as bit fields in C/C++
+// struct/union/class types.
+//
+// BitSize is the number of bits in the bit field.
+//
+// DataBitOffset, if non-zero, is the number of bits from the start of
+// the enclosing entity (e.g. containing struct/class/union) to the
+// start of the bit field. This corresponds to the DW_AT_data_bit_offset
+// DWARF attribute that was introduced in DWARF 4.
+//
+// BitOffset, if non-zero, is the number of bits between the most
+// significant bit of the storage unit holding the bit field to the
+// most significant bit of the bit field. Here "storage unit" is the
+// type name before the bit field (for a field "unsigned x:17", the
+// storage unit is "unsigned"). BitOffset values can vary depending on
+// the endianness of the system. BitOffset corresponds to the
+// DW_AT_bit_offset DWARF attribute that was deprecated in DWARF 4 and
+// removed in DWARF 5.
+//
+// At most one of DataBitOffset and BitOffset will be non-zero;
+// DataBitOffset/BitOffset will only be non-zero if BitSize is
+// non-zero. Whether a C compiler uses one or the other
+// will depend on compiler vintage and command line options.
+//
+// Here is an example of C/C++ bit field use, along with what to
+// expect in terms of DWARF bit offset info. Consider this code:
+//
+// struct S {
+//   int q;
+//   int j:5;
+//   int k:6;
+//   int m:5;
+//   int n:8;
+// } s;
+//
+// For the code above, one would expect to see the following for
+// DW_AT_bit_offset values (using GCC 8):
+//
+//          Little   |     Big
+//          Endian   |    Endian
+//                   |
+//   "j":     27     |     0
+//   "k":     21     |     5
+//   "m":     16     |     11
+//   "n":     8      |     16
+//
+// Note that in the above the offsets are purely with respect to the
+// containing storage unit for j/k/m/n -- these values won't vary based
+// on the size of prior data members in the containing struct.
+//
+// If the compiler emits DW_AT_data_bit_offset, the expected values
+// would be:
+//
+//   "j":     32
+//   "k":     37
+//   "m":     43
+//   "n":     48
+//
+// Here the value 32 for "j" reflects the fact that the bit field is
+// preceded by other data members (recall that DW_AT_data_bit_offset
+// values are relative to the start of the containing struct). Hence
+// DW_AT_data_bit_offset values can be quite large for structs with
+// many fields.
+//
+// DWARF also allow for the possibility of base types that have
+// non-zero bit size and bit offset, so this information is also
+// captured for base types, but it is worth noting that it is not
+// possible to trigger this behavior using mainstream languages.
+//
 type StructField struct {
-	Name       string
-	Type       Type
-	ByteOffset int64
-	ByteSize   int64 // usually zero; use Type.Size() for normal fields
-	BitOffset  int64 // within the ByteSize bytes at ByteOffset
-	BitSize    int64 // zero if not a bit field
+	Name          string
+	Type          Type
+	ByteOffset    int64
+	ByteSize      int64 // usually zero; use Type.Size() for normal fields
+	BitOffset     int64
+	DataBitOffset int64
+	BitSize       int64 // zero if not a bit field
 }
 
 func (t *StructType) String() string {
@@ -166,6 +244,13 @@ func (t *StructType) String() string {
 	return t.Defn()
 }
 
+func (f *StructField) bitOffset() int64 {
+	if f.BitOffset != 0 {
+		return f.BitOffset
+	}
+	return f.DataBitOffset
+}
+
 func (t *StructType) Defn() string {
 	s := t.Kind
 	if t.StructName != "" {
@@ -184,7 +269,7 @@ func (t *StructType) Defn() string {
 		s += "@" + strconv.FormatInt(f.ByteOffset, 10)
 		if f.BitSize > 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)
+}