debug/dwarf: resolve issues with new use of DW_AT_data_bit_offset

[thanm]

I'm filing this bug to track discussion and activity related to a problem in Go's debug/dwarf package related to how DWARF stores information about bitfields (in C structs) the way in which debug/dwarf presents this info to users/clients.

Background:

During Go 1.18 development, a change was made to the debug/dwarf package to add support for the DW_AT_data_bit_offset DIE attribute. The tracking issues for that work is #46784, and the CL eventually submitted was https://go-review.googlesource.com/c/go/+/328709.

The CL in question treats DW_AT_data_bit_offset (added in DWARF 4) and DW_AT_bit_offset (present in DWARF 3 and prior) as equivalent; it changes Go's debug/dwarf package to look first for s DW_AT_data_bit_offset attr (if it is available) and assign that value to the BitOffset field of the StructField object, and if DW_AT_data_bit_offset is not present, it falls back and looks for DW_AT_bit_offset. The resulting StructField is then eventually is passed back to the client as a result of a ReadType call.

This CL was in fact problematic/buggy, due to the fact that the DW_AT_data_bit_offset attribute is not just a clone of DW_AT_bit_offset; the two have different definitions (the former records a bit offset within the enclosing struct itself, whereas the latter records an endian-sensitive offset within the containing storage unit for the bitfield). During the 1.18 release freeze Dmitry discovered this problem and pointed it out (see https://go-review.googlesource.com/c/go/+/328709/comments/edf0619d_daec236f).

Where to go from here:

A first option is just to revert CL 328709 and restore things to the state they were in prior to 1.18. The drawback of doing this is that clients using debug/dwarf to analyze code generated by compilers that are using the more modern attribute (DW_AT_data_bit_offset) will see zero values for BitOffset when making ReadType calls (this is why the bug was filed in the first place).

A second option is to leave StructField unchanged (a single BitOffset value) and then provide some sort of hook that users can call that will tell them how to interpret it (e.g. whether the compilation unit is using DW_AT_data_bit_offset or DW_AT_bit_offset). This might look something like

  // UsesV4DataBitOffset returns TRUE if the current compilation unit
  // makes use of DW_AT_data_bit_offset, FALSE if the unit does not
  // use DW_AT_data_bit_offset.
  func (r *Reader) UsesV4DataBitOffset() bool {
    ...
  }

This has the disadvantage that it requires changes to existing clients: people who use debug/dwarf and actually look at bitfields will need to modify their code. It is also a bit complicated to implement in that it's not possible to determine whether DW_AT_data_bit_offset is in use only by looking at the DWARF version (since some versions of GCC still use the older attribute even when "-gdwarf-4" is requested). This could also be attacked by adding a new field to StructField itself (perhaps a boolean indicating which interpretation to use).

A third option is to look for the new attribute and then use the new attr values to compute the old DWARF-3 values, and present the DWARF 3 values to the user. This would be the best solution in terms of backwards compatibility, but seems unpleasant given that we're effectively undoing what was intended to be a good change made in DWARF 4 (since the new attribute is endian-insensitive and is deemed to be easier for clients to use). It also seems tricky to implement, which makes it less attractive given we are fairly late in the 1.18 freeze.

A fourth option is to simply change the meaning of the field-- just assign the new attr value to BitOffset and expect that clients will interpret it correctly. For compilation units that contain the old attribute, we could just ignore the value (seems unfriendly) or possibly try to compute the new DWARF 4 value using the old attr values. This option obviously has problems in that it requires clients to update their code, and if writing code to compute new attr values from old attr values seems problematic (again given we are fairly late in the 1.18 freeze).

A related question is: how many uses of BitOffset are there out in the wild?

I did an informal audit of Go code on github (using the new/beta "cs.github.com" search feature) looking for uses of "BitOffset" in Go code (e.g. query of the form 'language:go ".BitOffset"'). There were surprising few hits.

Most of what turned up were clones of the Go and GCC repos. There is a reference to BitOffset in Delve (and all the various clones of Delve) but it turns out that they've copied "type.go" from the main Go repo's debug/dwarf package and hacked it up for their own purposes.

The only "real" references I can find are in syzcaller (hence the interest from Dmitry) and its various clones, and then a use in a package https://github.com/volatilityfoundation/dwarf2json, which does look at BitOffset and interprets it according to DWARF 3 rules.

Of course, this is only Github public repos, there may be private repos that use BitOffset and there will be code elsewhere outside of github.

[ianlancetaylor]

The fact that GCC uses DW_AT_bit_offset rather than DW_AT_data_bit_offset for -gdwarf-4 is definitely a complication. Thanks for digging that up.

Given that, I don't see what we can reasonably do other than add a new field to BasicType and StructField. We could add a bool field but I think it would simpler to add a DataBitOffset field. It's kind of awful but I don't see how else we can correctly represent the DWARF data.

CC @dvyukov

[qmuntal]

Another option is to add a new method to dwarf.Data which instructs the reader on how to fill the BitOffset, regardless of the attribute type or the dwarf version. The default notation would be to fill BitOffset using DW_AT_bit_offset notation in order to maintain backwary compatibility. Calling this method would invalidate the types cache to avoid mixing notations

[ianlancetaylor]

I don't think we want to have to transform DW_AT_data_bit_offset values to DW_AT_bit_offset values. DW_AT_bit_offset is awkward, which is why DWARF stopped using it.

[thanm]

Trivia question:

DWARF clearly allows you to put a DW_AT_bit_offset attribute on a base type, but is there a straightforward way to trigger this sort of DWARF using a C program? I can't think of any way to do it (maybe it is there for some other language).

The blurb from the DWARF 4 standard (sec 5.1, "Base Type Entries");

If the value of an object of the given type does not fully occupy the
storage described by a byte size attribute, the base type entry may
also have a DW_AT_bit_size and a DW_AT_data_bit_offset attribute, both
of whose values are integer constant values (see Section 2.19). The
bit size attribute describes the actual size in bits used to represent
values of the given type. The data bit offset attribute is the offset
in bits from the beginning of the containing storage to the beginning
of the value. Bits that are part of the offset are padding. The data
bit offset uses the bit numbering and direction conventions that are
appropriate to the current language on the target system to locate the
beginning of the storage and value. If this attribute is omitted a
default data bit offset of zero is assumed.

[ianlancetaylor]

I don't think there is any way to do it in C. I think it would require a language that permits basic types that are sub-byte integer types, such as (I think) PL/1 or Ada, and that chooses to store them in the upper bits of the memory byte.

[Cyberax]

I don't think there is any way to do it in C. I think it would require a language that permits basic types that are sub-byte integer types, such as (I think) PL/1 or Ada, and that chooses to store them in the upper bits of the memory byte.

Bitfields?

struct Test {
   unsigned int varA : 4;
   unsigned int varB : 3;
};

[thanm]

@Cyberax This is the DW_AT_bit_offset attribute of a base type that I am talking about, e.g. an integral type with DW_TAG_base_type (as described in section 5.1), not the DW_AT_bit_offset attribute of a structure field (as described in section 5.6.6). It's easy to write C example of struct fields that generate DW_AT_bit_offset.

I brought it up since it would be nice to have regression tests for the package that exercise the base type code and not just the struct field code.

[thanm]

While developing a fix for this issue, I noticed this code, from line 590-600 or so.

It tries to use AttrDataBitOffset to help with detection of zero sized arrays, but it appears that it's written intending to use DW_AT_data_bit_offset semantics (e.g. offset from start of struct) and not DW_AT_bit_offset. This means that for a struct like

typedef struct another_struct {
  unsigned short quix;
  int z[0];
  unsigned  x:1;
  long long array[40];
} t_another_struct;

From looking at the original commit that introduced it, it's pretty clear that the author was counting on the semantics of DW_AT_data_bit_offset and not DW_AT_bit_offset.This means that we don't invoke the array type fixup for "int z[0]", leaving us exposed to that. Not sure if this is a super important problem however since more modern versions of GCC don't emit the array type DWARF that this code is trying to fix up (they emit an explicit zero length now), but I think I should probably file another issue for it.

[gopherbot]

Change https://golang.org/cl/380714 mentions this issue: debug/dwarf: fix problems with handling of bit offsets for bitfields

[gopherbot]

Change https://golang.org/cl/381778 mentions this issue: doc/go1.18: mention new debug/dwarf DataBitOffset fields