diff mbox

Fix DW_AT_data_member_location/DW_AT_bit_offset handling (PR debug/78839)

Message ID 20170111201930.GQ21933@tucnak
State New
Headers show

Commit Message

Jakub Jelinek Jan. 11, 2017, 8:19 p.m. UTC 
Hi!

In GCC 5 and earlier, field_byte_offset had code for
PCC_BITFIELD_TYPE_MATTERS target that figured out what
DW_AT_data_member_location value to use vs. DW_AT_bit_offset.

That code is still in there, but due to several bugs added in r231762
it never triggers anymore.
One is that the is_{bit,byte}_offset_cst variables are set to the opposite
of how they are named (they are true if the corresponding offset is not
an INTEGER_CST).  Another one is that the PCC_BITFIELD_TYPE_MATTERS block,
that has been in GCC 5 and earlier guarded just with
if (PCC_BITFIELD_TYPE_MATTERS),
is guarded with what the var names imply, but the earlier bail-out is
according to what the var is set to, so for variable bit size we return
early, and the PCC_BITFIELD_TYPE_MATTERS is then only invoked for variable
size while it was meant to be used only for constant size.
Even with that fixed, we in a large computation compute something that is
completely ignored.

The following patch fixes all this and restores the GCC 5 behavior.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

For -gdwarf-5 (and after 2 years maybe also for -gdwarf-4) we should use
DW_AT_data_bit_offset instead, which gdb just gained support for recently.
I'll work on that tomorrow.

2017-01-11  Jakub Jelinek  <jakub@redhat.com>

	PR debug/78839
	* dwarf2out.c (field_byte_offset): Restore the
	PCC_BITFIELD_TYPE_MATTERS behavior for INTEGER_CST DECL_FIELD_OFFSET
	and DECL_FIELD_BIT_OFFSET.  Use fold_build2 instead of build2 + fold.
	(analyze_variants_discr, gen_variant_part): Use fold_build2 instead
	of build2 + fold.


	Jakub

Comments

Pierre-Marie de Rodat Jan. 12, 2017, 4:42 p.m. UTC | #1 
Jakub,

On 01/11/2017 09:19 PM, Jakub Jelinek wrote:
> In GCC 5 and earlier, field_byte_offset had code for
> PCC_BITFIELD_TYPE_MATTERS target that figured out what
> DW_AT_data_member_location value to use vs. DW_AT_bit_offset.
>
> That code is still in there, but due to several bugs added in r231762
> it never triggers anymore.

I totally plead guilty for this! When I originally patched this 
function, something like 3 years ago, I had a hard time trying to 
understand it (in retrospect, I probably failed that), tried various 
things, had headaches while thinking about variable bit offsets, and 
then IIRC saw no regression in GCC or GDB’s testsuite, so stopped 
worrying and submitted my patch for review.

… anyway that’s it for the context. So thank you very much for working 
on this!

> The following patch fixes all this and restores the GCC 5 behavior.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

FWIW, this patch triggers no regression in my DWARF/Ada testsuite.

> +      object_offset_in_bytes
> +	= wi::lrshift (object_offset_in_bits, LOG2_BITS_PER_UNIT);
> +      if (ctx->variant_part_offset == NULL_TREE)
> +	{
> +	  *cst_offset = object_offset_in_bytes.to_shwi ();
> +	  return NULL;
> +	}
> +      tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);

I don’t understand this special case: IIUC, if this IF block was 
missing, the flow would make this function return the same result thanks 
to the similar IF block several lines below. Is it to avoid the 
conversions to tree/wide int/HOST_WIDE_INT (i.e. an optimization)?
Jakub Jelinek Jan. 12, 2017, 4:44 p.m. UTC | #2 
On Thu, Jan 12, 2017 at 05:42:55PM +0100, Pierre-Marie de Rodat wrote:
> > +      object_offset_in_bytes
> > +	= wi::lrshift (object_offset_in_bits, LOG2_BITS_PER_UNIT);
> > +      if (ctx->variant_part_offset == NULL_TREE)
> > +	{
> > +	  *cst_offset = object_offset_in_bytes.to_shwi ();
> > +	  return NULL;
> > +	}
> > +      tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);
> 
> I don’t understand this special case: IIUC, if this IF block was missing,
> the flow would make this function return the same result thanks to the
> similar IF block several lines below. Is it to avoid the conversions to
> tree/wide int/HOST_WIDE_INT (i.e. an optimization)?

Yes, the if (ctx->variant_part_offset == NULL_TREE) block is optimization
for the most common case.

	Jakub
Jason Merrill Jan. 17, 2017, 6:20 p.m. UTC | #3 
On Wed, Jan 11, 2017 at 3:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> +  else
>  #endif /* PCC_BITFIELD_TYPE_MATTERS */
> -
> -  tree_result = byte_position (decl);
> +    tree_result = byte_position (decl);

Let's add a blank line after this assignment.  OK with that change.

Jason
diff mbox

Patch

--- gcc/dwarf2out.c.jj	2017-01-11 17:45:47.000000000 +0100
+++ gcc/dwarf2out.c	2017-01-11 18:49:37.184605527 +0100
@@ -17980,10 +17980,6 @@  static dw_loc_descr_ref
 field_byte_offset (const_tree decl, struct vlr_context *ctx,
 		   HOST_WIDE_INT *cst_offset)
 {
-  offset_int object_offset_in_bits;
-  offset_int object_offset_in_bytes;
-  offset_int bitpos_int;
-  bool is_byte_offset_cst, is_bit_offset_cst;
   tree tree_result;
   dw_loc_list_ref loc_result;
 
@@ -17994,20 +17990,21 @@  field_byte_offset (const_tree decl, stru
   else
     gcc_assert (TREE_CODE (decl) == FIELD_DECL);
 
-  is_bit_offset_cst = TREE_CODE (DECL_FIELD_BIT_OFFSET (decl)) != INTEGER_CST;
-  is_byte_offset_cst = TREE_CODE (DECL_FIELD_OFFSET (decl)) != INTEGER_CST;
-
   /* We cannot handle variable bit offsets at the moment, so abort if it's the
      case.  */
-  if (is_bit_offset_cst)
+  if (TREE_CODE (DECL_FIELD_BIT_OFFSET (decl)) != INTEGER_CST)
     return NULL;
 
 #ifdef PCC_BITFIELD_TYPE_MATTERS
   /* We used to handle only constant offsets in all cases.  Now, we handle
      properly dynamic byte offsets only when PCC bitfield type doesn't
      matter.  */
-  if (PCC_BITFIELD_TYPE_MATTERS && is_byte_offset_cst && is_bit_offset_cst)
+  if (PCC_BITFIELD_TYPE_MATTERS
+      && TREE_CODE (DECL_FIELD_OFFSET (decl)) == INTEGER_CST)
     {
+      offset_int object_offset_in_bits;
+      offset_int object_offset_in_bytes;
+      offset_int bitpos_int;
       tree type;
       tree field_size_tree;
       offset_int deepest_bitpos;
@@ -18102,13 +18099,22 @@  field_byte_offset (const_tree decl, stru
 	  object_offset_in_bits
 	    = round_up_to_align (object_offset_in_bits, decl_align_in_bits);
 	}
+
+      object_offset_in_bytes
+	= wi::lrshift (object_offset_in_bits, LOG2_BITS_PER_UNIT);
+      if (ctx->variant_part_offset == NULL_TREE)
+	{
+	  *cst_offset = object_offset_in_bytes.to_shwi ();
+	  return NULL;
+	}
+      tree_result = wide_int_to_tree (sizetype, object_offset_in_bytes);
     }
+  else
 #endif /* PCC_BITFIELD_TYPE_MATTERS */
-
-  tree_result = byte_position (decl);
+    tree_result = byte_position (decl);
   if (ctx->variant_part_offset != NULL_TREE)
-    tree_result = fold (build2 (PLUS_EXPR, TREE_TYPE (tree_result),
-				ctx->variant_part_offset, tree_result));
+    tree_result = fold_build2 (PLUS_EXPR, TREE_TYPE (tree_result),
+			       ctx->variant_part_offset, tree_result);
 
   /* If the byte offset is a constant, it's simplier to handle a native
      constant rather than a DWARF expression.  */
@@ -23727,14 +23733,12 @@  analyze_variants_discr (tree variant_par
 
 	      if (!lower_cst_included)
 		lower_cst
-		  = fold (build2 (PLUS_EXPR, TREE_TYPE (lower_cst),
-				  lower_cst,
-				  build_int_cst (TREE_TYPE (lower_cst), 1)));
+		  = fold_build2 (PLUS_EXPR, TREE_TYPE (lower_cst), lower_cst,
+				 build_int_cst (TREE_TYPE (lower_cst), 1));
 	      if (!upper_cst_included)
 		upper_cst
-		  = fold (build2 (MINUS_EXPR, TREE_TYPE (upper_cst),
-				  upper_cst,
-				  build_int_cst (TREE_TYPE (upper_cst), 1)));
+		  = fold_build2 (MINUS_EXPR, TREE_TYPE (upper_cst), upper_cst,
+				 build_int_cst (TREE_TYPE (upper_cst), 1));
 
 	      if (!get_discr_value (lower_cst,
 				    &new_node->dw_discr_lower_bound)
@@ -23905,8 +23909,8 @@  gen_variant_part (tree variant_part_decl
 		 we recurse.  */
 
 	      vlr_sub_ctx.variant_part_offset
-	        = fold (build2 (PLUS_EXPR, TREE_TYPE (variant_part_offset),
-				variant_part_offset, byte_position (member)));
+		= fold_build2 (PLUS_EXPR, TREE_TYPE (variant_part_offset),
+			       variant_part_offset, byte_position (member));
 	      gen_variant_part (member, &vlr_sub_ctx, variant_die);
 	    }
 	  else