[PATCH] fix debug information for bitfields

[PATCH] fix debug information for bitfields

[Nathan Froyd]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

The attached patch fixes an issue with DWARF2's treatment of bitfields.
The existing field_byte_offset assumes PCC_BITFIELD_TYPE_MATTERS and so
does a fair amount of gymnastics to discover actual byte offsets of
fields by aligning bit offsets according to a field's type, etc.
However, if you're using m68k-linux (or h8300-coff or a very few other
ports), PCC_BITFIELD_TYPE_MATTERS == 0 and so type-based alignment
doesn't apply.

For a concrete example of how this patch changes things, when given the
following code:

struct bits
{
  short a : 10;
  long b : 10;
  char c;
  short d : 10;
  short e : 10;
};

mainline (and previous versions) generate the following for 'e':

 <2><9b>: Abbrev Number: 3 (DW_TAG_member)
     DW_AT_name        : e      
     DW_AT_decl_file   : 1      
     DW_AT_decl_line   : 7      
     DW_AT_type        : <ab>   
     DW_AT_byte_size   : 2      
     DW_AT_bit_size    : 10     
     DW_AT_bit_offset  : 10     
     DW_AT_data_member_location: 2 byte block: 23 4     (DW_OP_plus_uconst: 4)

whereas with the patch, we get:

 <2><9b>: Abbrev Number: 3 (DW_TAG_member)
     DW_AT_name        : e      
     DW_AT_decl_file   : 1      
     DW_AT_decl_line   : 7      
     DW_AT_type        : <ab>   
     DW_AT_byte_size   : 2      
     DW_AT_bit_size    : 10     
     DW_AT_bit_offset  : 2      
     DW_AT_data_member_location: 2 byte block: 23 5     (DW_OP_plus_uconst: 5)

The DWARF that mainline currently emits is inconsistent and causes one
commercial debugger internal heartburn.

Tested by bootstrapping a cross-compiler to m68k-linux and a native
x86_64-linux (which is a PCC_BITFIELD_TYPE_MATTERS == 1 target) compiler
and running the GCC testsuite with no regressions.  I also verified that
the patch does not change the debug output on x86_64 for the above
example.

OK to commit?

:ADDPATCH dwarf:

-Nathan

[-- Attachment #2: dw2-bitfields.diff --]
[-- Type: text/plain, Size: 10133 bytes --]

2007-03-22  Nathan Froyd  <froydnj@codesourcery.com>

	* dwarf2out.c (field_byte_offset): Move the existing logic
	under the control of PCC_BITFIELD_TYPE_MATTERS and just use
	the bit offset of the field if !PCC_BITFIELD_TYPE_MATTERS.

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 123125)
+++ dwarf2out.c	(working copy)
@@ -9692,29 +9692,14 @@ round_up_to_align (HOST_WIDE_INT t, unsi
 static HOST_WIDE_INT
 field_byte_offset (tree decl)
 {
-  unsigned int type_align_in_bits;
-  unsigned int decl_align_in_bits;
-  unsigned HOST_WIDE_INT type_size_in_bits;
   HOST_WIDE_INT object_offset_in_bits;
-  tree type;
-  tree field_size_tree;
   HOST_WIDE_INT bitpos_int;
-  HOST_WIDE_INT deepest_bitpos;
-  unsigned HOST_WIDE_INT field_size_in_bits;
 
   if (TREE_CODE (decl) == ERROR_MARK)
     return 0;
 
   gcc_assert (TREE_CODE (decl) == FIELD_DECL);
 
-  type = field_type (decl);
-  field_size_tree = DECL_SIZE (decl);
-
-  /* The size could be unspecified if there was an error, or for
-     a flexible array member.  */
-  if (! field_size_tree)
-    field_size_tree = bitsize_zero_node;
-
   /* We cannot yet cope with fields whose positions are variable, so
      for now, when we see such things, we simply return 0.  Someday, we may
      be able to handle such cases, but it will be damn difficult.  */
@@ -9723,75 +9708,108 @@ field_byte_offset (tree decl)
 
   bitpos_int = int_bit_position (decl);
 
-  /* If we don't know the size of the field, pretend it's a full word.  */
-  if (host_integerp (field_size_tree, 1))
-    field_size_in_bits = tree_low_cst (field_size_tree, 1);
-  else
-    field_size_in_bits = BITS_PER_WORD;
+#ifdef PCC_BITFIELD_TYPE_MATTERS
+  if (PCC_BITFIELD_TYPE_MATTERS)
+    {
+      tree type;
+      tree field_size_tree;
+      HOST_WIDE_INT deepest_bitpos;
+      unsigned HOST_WIDE_INT field_size_in_bits;
+      unsigned int type_align_in_bits;
+      unsigned int decl_align_in_bits;
+      unsigned HOST_WIDE_INT type_size_in_bits;
+
+      type = field_type (decl);
+      field_size_tree = DECL_SIZE (decl);
+
+      /* The size could be unspecified if there was an error, or for
+         a flexible array member.  */
+      if (! field_size_tree)
+        field_size_tree = bitsize_zero_node;
+
+      /* If we don't know the size of the field, pretend it's a full word.  */
+      if (host_integerp (field_size_tree, 1))
+        field_size_in_bits = tree_low_cst (field_size_tree, 1);
+      else
+        field_size_in_bits = BITS_PER_WORD;
 
-  type_size_in_bits = simple_type_size_in_bits (type);
-  type_align_in_bits = simple_type_align_in_bits (type);
-  decl_align_in_bits = simple_decl_align_in_bits (decl);
-
-  /* The GCC front-end doesn't make any attempt to keep track of the starting
-     bit offset (relative to the start of the containing structure type) of the
-     hypothetical "containing object" for a bit-field.  Thus, when computing
-     the byte offset value for the start of the "containing object" of a
-     bit-field, we must deduce this information on our own. This can be rather
-     tricky to do in some cases.  For example, handling the following structure
-     type definition when compiling for an i386/i486 target (which only aligns
-     long long's to 32-bit boundaries) can be very tricky:
+      type_size_in_bits = simple_type_size_in_bits (type);
+      type_align_in_bits = simple_type_align_in_bits (type);
+      decl_align_in_bits = simple_decl_align_in_bits (decl);
+
+      /* The GCC front-end doesn't make any attempt to keep track of the
+         starting bit offset (relative to the start of the containing
+         structure type) of the hypothetical "containing object" for a
+         bit-field.  Thus, when computing the byte offset value for the
+         start of the "containing object" of a bit-field, we must deduce
+         this information on our own. This can be rather tricky to do in
+         some cases.  For example, handling the following structure type
+         definition when compiling for an i386/i486 target (which only
+         aligns long long's to 32-bit boundaries) can be very tricky:
 
 	 struct S { int field1; long long field2:31; };
 
-     Fortunately, there is a simple rule-of-thumb which can be used in such
-     cases.  When compiling for an i386/i486, GCC will allocate 8 bytes for the
-     structure shown above.  It decides to do this based upon one simple rule
-     for bit-field allocation.  GCC allocates each "containing object" for each
-     bit-field at the first (i.e. lowest addressed) legitimate alignment
-     boundary (based upon the required minimum alignment for the declared type
-     of the field) which it can possibly use, subject to the condition that
-     there is still enough available space remaining in the containing object
-     (when allocated at the selected point) to fully accommodate all of the
-     bits of the bit-field itself.
-
-     This simple rule makes it obvious why GCC allocates 8 bytes for each
-     object of the structure type shown above.  When looking for a place to
-     allocate the "containing object" for `field2', the compiler simply tries
-     to allocate a 64-bit "containing object" at each successive 32-bit
-     boundary (starting at zero) until it finds a place to allocate that 64-
-     bit field such that at least 31 contiguous (and previously unallocated)
-     bits remain within that selected 64 bit field.  (As it turns out, for the
-     example above, the compiler finds it is OK to allocate the "containing
-     object" 64-bit field at bit-offset zero within the structure type.)
-
-     Here we attempt to work backwards from the limited set of facts we're
-     given, and we try to deduce from those facts, where GCC must have believed
-     that the containing object started (within the structure type). The value
-     we deduce is then used (by the callers of this routine) to generate
-     DW_AT_location and DW_AT_bit_offset attributes for fields (both bit-fields
-     and, in the case of DW_AT_location, regular fields as well).  */
-
-  /* Figure out the bit-distance from the start of the structure to the
-     "deepest" bit of the bit-field.  */
-  deepest_bitpos = bitpos_int + field_size_in_bits;
-
-  /* This is the tricky part.  Use some fancy footwork to deduce where the
-     lowest addressed bit of the containing object must be.  */
-  object_offset_in_bits = deepest_bitpos - type_size_in_bits;
-
-  /* Round up to type_align by default.  This works best for bitfields.  */
-  object_offset_in_bits
-    = round_up_to_align (object_offset_in_bits, type_align_in_bits);
-
-  if (object_offset_in_bits > bitpos_int)
-    {
-      /* Sigh, the decl must be packed.  */
+         Fortunately, there is a simple rule-of-thumb which can be used
+         in such cases.  When compiling for an i386/i486, GCC will
+         allocate 8 bytes for the structure shown above.  It decides to
+         do this based upon one simple rule for bit-field allocation.
+         GCC allocates each "containing object" for each bit-field at
+         the first (i.e. lowest addressed) legitimate alignment boundary
+         (based upon the required minimum alignment for the declared
+         type of the field) which it can possibly use, subject to the
+         condition that there is still enough available space remaining
+         in the containing object (when allocated at the selected point)
+         to fully accommodate all of the bits of the bit-field itself.
+
+         This simple rule makes it obvious why GCC allocates 8 bytes for
+         each object of the structure type shown above.  When looking
+         for a place to allocate the "containing object" for `field2',
+         the compiler simply tries to allocate a 64-bit "containing
+         object" at each successive 32-bit boundary (starting at zero)
+         until it finds a place to allocate that 64- bit field such that
+         at least 31 contiguous (and previously unallocated) bits remain
+         within that selected 64 bit field.  (As it turns out, for the
+         example above, the compiler finds it is OK to allocate the
+         "containing object" 64-bit field at bit-offset zero within the
+         structure type.)
+
+         Here we attempt to work backwards from the limited set of facts
+         we're given, and we try to deduce from those facts, where GCC
+         must have believed that the containing object started (within
+         the structure type). The value we deduce is then used (by the
+         callers of this routine) to generate DW_AT_location and
+         DW_AT_bit_offset attributes for fields (both bit-fields and, in
+         the case of DW_AT_location, regular fields as well).  */
+
+      /* Figure out the bit-distance from the start of the structure to
+         the "deepest" bit of the bit-field.  */
+      deepest_bitpos = bitpos_int + field_size_in_bits;
+
+      /* This is the tricky part.  Use some fancy footwork to deduce
+         where the lowest addressed bit of the containing object must
+         be.  */
       object_offset_in_bits = deepest_bitpos - type_size_in_bits;
 
-      /* Round up to decl_align instead.  */
-      object_offset_in_bits
-	= round_up_to_align (object_offset_in_bits, decl_align_in_bits);
+      /* Round up to type_align by default.  This works best for
+         bitfields.  */
+      object_offset_in_bits += type_align_in_bits - 1;
+      object_offset_in_bits /= type_align_in_bits;
+      object_offset_in_bits *= type_align_in_bits;
+
+      if (object_offset_in_bits > bitpos_int)
+        {
+          object_offset_in_bits = deepest_bitpos - type_size_in_bits;
+
+          /* Round up to decl_align instead.  */
+          object_offset_in_bits += decl_align_in_bits - 1;
+          object_offset_in_bits /= decl_align_in_bits;
+          object_offset_in_bits *= decl_align_in_bits;
+        }
+    }
+  else
+#endif
+    {
+      object_offset_in_bits = bitpos_int;
     }
 
   return object_offset_in_bits / BITS_PER_UNIT;

Re: [PATCH] fix debug information for bitfields

[Eric Botcazou]

> OK to commit?

No, you're reverting

2007-02-12  Eric Botcazou  <ebotcazou@adacore.com>

	* dwarf2out.c (round_up_to_align): New static function.
	(field_byte_offset): Use it to round the offset.

-- 
Eric Botcazou

Re: [PATCH] fix debug information for bitfields

[Nathan Froyd]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

On Thu, Mar 22, 2007 at 05:30:51PM +0100, Eric Botcazou wrote:
> > OK to commit?
> 
> No, you're reverting
> 
> 2007-02-12  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* dwarf2out.c (round_up_to_align): New static function.
> 	(field_byte_offset): Use it to round the offset.

Oops.  Sorry about that.  Revised patch attached.

-Nathan

[-- Attachment #2: dw2-bitfields.diff --]
[-- Type: text/plain, Size: 9986 bytes --]

2007-03-22  Nathan Froyd  <froydnj@codesourcery.com>

	* dwarf2out.c (field_byte_offset): Move the existing logic
	under the control of PCC_BITFIELD_TYPE_MATTERS and just use
	the bit offset of the field if !PCC_BITFIELD_TYPE_MATTERS.

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 123125)
+++ dwarf2out.c	(working copy)
@@ -9692,29 +9692,14 @@ round_up_to_align (HOST_WIDE_INT t, unsi
 static HOST_WIDE_INT
 field_byte_offset (tree decl)
 {
-  unsigned int type_align_in_bits;
-  unsigned int decl_align_in_bits;
-  unsigned HOST_WIDE_INT type_size_in_bits;
   HOST_WIDE_INT object_offset_in_bits;
-  tree type;
-  tree field_size_tree;
   HOST_WIDE_INT bitpos_int;
-  HOST_WIDE_INT deepest_bitpos;
-  unsigned HOST_WIDE_INT field_size_in_bits;
 
   if (TREE_CODE (decl) == ERROR_MARK)
     return 0;
 
   gcc_assert (TREE_CODE (decl) == FIELD_DECL);
 
-  type = field_type (decl);
-  field_size_tree = DECL_SIZE (decl);
-
-  /* The size could be unspecified if there was an error, or for
-     a flexible array member.  */
-  if (! field_size_tree)
-    field_size_tree = bitsize_zero_node;
-
   /* We cannot yet cope with fields whose positions are variable, so
      for now, when we see such things, we simply return 0.  Someday, we may
      be able to handle such cases, but it will be damn difficult.  */
@@ -9723,75 +9708,106 @@ field_byte_offset (tree decl)
 
   bitpos_int = int_bit_position (decl);
 
-  /* If we don't know the size of the field, pretend it's a full word.  */
-  if (host_integerp (field_size_tree, 1))
-    field_size_in_bits = tree_low_cst (field_size_tree, 1);
-  else
-    field_size_in_bits = BITS_PER_WORD;
+#ifdef PCC_BITFIELD_TYPE_MATTERS
+  if (PCC_BITFIELD_TYPE_MATTERS)
+    {
+      tree type;
+      tree field_size_tree;
+      HOST_WIDE_INT deepest_bitpos;
+      unsigned HOST_WIDE_INT field_size_in_bits;
+      unsigned int type_align_in_bits;
+      unsigned int decl_align_in_bits;
+      unsigned HOST_WIDE_INT type_size_in_bits;
+
+      type = field_type (decl);
+      field_size_tree = DECL_SIZE (decl);
+
+      /* The size could be unspecified if there was an error, or for
+         a flexible array member.  */
+      if (! field_size_tree)
+        field_size_tree = bitsize_zero_node;
+
+      /* If we don't know the size of the field, pretend it's a full word.  */
+      if (host_integerp (field_size_tree, 1))
+        field_size_in_bits = tree_low_cst (field_size_tree, 1);
+      else
+        field_size_in_bits = BITS_PER_WORD;
 
-  type_size_in_bits = simple_type_size_in_bits (type);
-  type_align_in_bits = simple_type_align_in_bits (type);
-  decl_align_in_bits = simple_decl_align_in_bits (decl);
-
-  /* The GCC front-end doesn't make any attempt to keep track of the starting
-     bit offset (relative to the start of the containing structure type) of the
-     hypothetical "containing object" for a bit-field.  Thus, when computing
-     the byte offset value for the start of the "containing object" of a
-     bit-field, we must deduce this information on our own. This can be rather
-     tricky to do in some cases.  For example, handling the following structure
-     type definition when compiling for an i386/i486 target (which only aligns
-     long long's to 32-bit boundaries) can be very tricky:
+      type_size_in_bits = simple_type_size_in_bits (type);
+      type_align_in_bits = simple_type_align_in_bits (type);
+      decl_align_in_bits = simple_decl_align_in_bits (decl);
+
+      /* The GCC front-end doesn't make any attempt to keep track of the
+         starting bit offset (relative to the start of the containing
+         structure type) of the hypothetical "containing object" for a
+         bit-field.  Thus, when computing the byte offset value for the
+         start of the "containing object" of a bit-field, we must deduce
+         this information on our own. This can be rather tricky to do in
+         some cases.  For example, handling the following structure type
+         definition when compiling for an i386/i486 target (which only
+         aligns long long's to 32-bit boundaries) can be very tricky:
 
 	 struct S { int field1; long long field2:31; };
 
-     Fortunately, there is a simple rule-of-thumb which can be used in such
-     cases.  When compiling for an i386/i486, GCC will allocate 8 bytes for the
-     structure shown above.  It decides to do this based upon one simple rule
-     for bit-field allocation.  GCC allocates each "containing object" for each
-     bit-field at the first (i.e. lowest addressed) legitimate alignment
-     boundary (based upon the required minimum alignment for the declared type
-     of the field) which it can possibly use, subject to the condition that
-     there is still enough available space remaining in the containing object
-     (when allocated at the selected point) to fully accommodate all of the
-     bits of the bit-field itself.
-
-     This simple rule makes it obvious why GCC allocates 8 bytes for each
-     object of the structure type shown above.  When looking for a place to
-     allocate the "containing object" for `field2', the compiler simply tries
-     to allocate a 64-bit "containing object" at each successive 32-bit
-     boundary (starting at zero) until it finds a place to allocate that 64-
-     bit field such that at least 31 contiguous (and previously unallocated)
-     bits remain within that selected 64 bit field.  (As it turns out, for the
-     example above, the compiler finds it is OK to allocate the "containing
-     object" 64-bit field at bit-offset zero within the structure type.)
-
-     Here we attempt to work backwards from the limited set of facts we're
-     given, and we try to deduce from those facts, where GCC must have believed
-     that the containing object started (within the structure type). The value
-     we deduce is then used (by the callers of this routine) to generate
-     DW_AT_location and DW_AT_bit_offset attributes for fields (both bit-fields
-     and, in the case of DW_AT_location, regular fields as well).  */
-
-  /* Figure out the bit-distance from the start of the structure to the
-     "deepest" bit of the bit-field.  */
-  deepest_bitpos = bitpos_int + field_size_in_bits;
-
-  /* This is the tricky part.  Use some fancy footwork to deduce where the
-     lowest addressed bit of the containing object must be.  */
-  object_offset_in_bits = deepest_bitpos - type_size_in_bits;
-
-  /* Round up to type_align by default.  This works best for bitfields.  */
-  object_offset_in_bits
-    = round_up_to_align (object_offset_in_bits, type_align_in_bits);
-
-  if (object_offset_in_bits > bitpos_int)
-    {
-      /* Sigh, the decl must be packed.  */
+         Fortunately, there is a simple rule-of-thumb which can be used
+         in such cases.  When compiling for an i386/i486, GCC will
+         allocate 8 bytes for the structure shown above.  It decides to
+         do this based upon one simple rule for bit-field allocation.
+         GCC allocates each "containing object" for each bit-field at
+         the first (i.e. lowest addressed) legitimate alignment boundary
+         (based upon the required minimum alignment for the declared
+         type of the field) which it can possibly use, subject to the
+         condition that there is still enough available space remaining
+         in the containing object (when allocated at the selected point)
+         to fully accommodate all of the bits of the bit-field itself.
+
+         This simple rule makes it obvious why GCC allocates 8 bytes for
+         each object of the structure type shown above.  When looking
+         for a place to allocate the "containing object" for `field2',
+         the compiler simply tries to allocate a 64-bit "containing
+         object" at each successive 32-bit boundary (starting at zero)
+         until it finds a place to allocate that 64- bit field such that
+         at least 31 contiguous (and previously unallocated) bits remain
+         within that selected 64 bit field.  (As it turns out, for the
+         example above, the compiler finds it is OK to allocate the
+         "containing object" 64-bit field at bit-offset zero within the
+         structure type.)
+
+         Here we attempt to work backwards from the limited set of facts
+         we're given, and we try to deduce from those facts, where GCC
+         must have believed that the containing object started (within
+         the structure type). The value we deduce is then used (by the
+         callers of this routine) to generate DW_AT_location and
+         DW_AT_bit_offset attributes for fields (both bit-fields and, in
+         the case of DW_AT_location, regular fields as well).  */
+
+      /* Figure out the bit-distance from the start of the structure to
+         the "deepest" bit of the bit-field.  */
+      deepest_bitpos = bitpos_int + field_size_in_bits;
+
+      /* This is the tricky part.  Use some fancy footwork to deduce
+         where the lowest addressed bit of the containing object must
+         be.  */
       object_offset_in_bits = deepest_bitpos - type_size_in_bits;
 
-      /* Round up to decl_align instead.  */
+      /* Round up to type_align by default.  This works best for
+         bitfields.  */
       object_offset_in_bits
-	= round_up_to_align (object_offset_in_bits, decl_align_in_bits);
+        = round_up_to_align (object_offset_in_bits, type_align_in_bits);
+
+      if (object_offset_in_bits > bitpos_int)
+        {
+          object_offset_in_bits = deepest_bitpos - type_size_in_bits;
+
+          /* Round up to decl_align instead.  */
+          object_offset_in_bits
+            = round_up_to_align (object_offset_in_bits, decl_align_in_bits);
+        }
+    }
+  else
+#endif
+    {
+      object_offset_in_bits = bitpos_int;
     }
 
   return object_offset_in_bits / BITS_PER_UNIT;

Re: [PATCH] fix debug information for bitfields

[Mark Mitchell]

Nathan Froyd wrote:
> On Thu, Mar 22, 2007 at 05:30:51PM +0100, Eric Botcazou wrote:
>>> OK to commit?
>> No, you're reverting
>>
>> 2007-02-12  Eric Botcazou  <ebotcazou@adacore.com>
>>
>> 	* dwarf2out.c (round_up_to_align): New static function.
>> 	(field_byte_offset): Use it to round the offset.
> 
> Oops.  Sorry about that.  Revised patch attached.

This patch is OK with a coding-style nit:

+  else
+#endif
+    {
+      object_offset_in_bits = bitpos_int;
     }

We're supposed to elide the braces for a one-line dependent statement.
No need to retest with that change.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713