Re: [PATCH] Fix DW_AT_data_member_location/DW_AT_bit_offset handling (PR debug/78839)

2017-01-17 Thread Jason Merrill 
On Wed, Jan 11, 2017 at 3:19 PM, Jakub Jelinek  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

Re: [PATCH] Fix DW_AT_data_member_location/DW_AT_bit_offset handling (PR debug/78839)

2017-01-12 Thread Jakub Jelinek 
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

Re: [PATCH] Fix DW_AT_data_member_location/DW_AT_bit_offset handling (PR debug/78839)

2017-01-12 Thread Pierre-Marie de Rodat 

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)?


--
Pierre-Marie de Rodat

[PATCH] Fix DW_AT_data_member_location/DW_AT_bit_offset handling (PR debug/78839)

2017-01-11 Thread Jakub Jelinek 
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  

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.

--- gcc/dwarf2out.c.jj  2017-01-11 17:45:47.0 +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,
-