diff mbox

[ping] Re: Fix PR debug/66728

Message ID 87h9l4v2pi.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Nov. 2, 2015, 3:30 p.m. UTC 
Bernd Schmidt <bschmidt@redhat.com> writes:
> On 10/28/2015 02:06 PM, Richard Sandiford wrote:
>> Ulrich Weigand <uweigand@de.ibm.com> writes:
>>> seems this still hasn't gone upstream ...  Any news?
>>
>> Ah, sorry, I should have been pinging it.  I think it's still waiting
>> for review.
>
> Hmm, unfortunately I have a hard time making sense of this patch. Some 
> extra explanation would be appreciated.

CONST_WIDE_INTs are like CONST_INTs in that their mode is always VOIDmode.
The loc_descriptor code:

    case CONST_WIDE_INT:
      if (mode == VOIDmode)
        mode = GET_MODE (rtl);

was simply bogus, but a harmless no-op.  Much more serious was the
add_const_value_attribute code:

    case CONST_WIDE_INT:
      add_AT_wide (die, DW_AT_const_value,
                  std::make_pair (rtl, GET_MODE (rtl)));
      return true;

which, because GET_MODE (rtl) returns VOIDmode, would always try to
create a zero-precision integer.

The problem is that, unlike loc_descriptor, add_const_value_attribute
had no separate mode parameter.  The patch therefore adds one and tries
its best to find the appropriate value, given the TYPE_MODE/DECL_MODE
split.  I used DECL_MODE in add_location_or_const_value_attribute for
consistency with the {int_,}loc_descriptor calls in dw_loc_list_1 and
loc_list_from_tree.  I used TYPE_MODE in tree_add_const_value_attribute
because it doesn't deal exclusively with decls.  But like I say, the two
should be the same in practice, since I don't think anything would
promote to a value that's big enough to need a CONST_WIDE_INT.

>>>>   static bool
>>>> -add_const_value_attribute (dw_die_ref die, rtx rtl)
>>>> +add_const_value_attribute (dw_die_ref die, machine_mode mode, rtx rtl)
>
> Need to document the extra parameter, especially in light of the 
> TYPE_MODE/DECL_MODE/rtx mode confusion you pointed out.

I've reattached it below FWIW.

>>>>   {
>>>>     switch (GET_CODE (rtl))
>>>>       {
>>>>       case CONST_INT:
>>>> -      {
>>>> -	HOST_WIDE_INT val =3D INTVAL (rtl);
>>>> +      if (mode !=3D BLKmode)
>>>> +	{
>>>> +	  gcc_checking_assert (SCALAR_INT_MODE_P (mode));
>>>> +	  HOST_WIDE_INT val =3D INTVAL (rtl);
>>>> =20
>>>> -	if (val < 0)
>>>> -	  add_AT_int (die, DW_AT_const_value, val);
>>>> -	else
>>>> -	  add_AT_unsigned (die, DW_AT_const_value, (unsigned HOST_WIDE_INT) val);
>>>> -      }
>>>> -      return true;
>>>> +	  if (val < 0)
>>>> +	    add_AT_int (die, DW_AT_const_value, val);
>>>> +	  else
>>>> +	    add_AT_unsigned (die, DW_AT_const_value,
>>>> +			     (unsigned HOST_WIDE_INT) val);
>>>> +	  return true;
>>>> +	}
>
> This is all a bit html mangled, but this and the other change in 
> loc_descriptor seem to have the effect of doing nothing when called with 
> BLKmode. What is the desired effect of this on the debug information, 
> and how is it achieved?

Well, I'm not familiar with this code, so I'm not really the best person
to say.  But for CONST_INT loc_descriptor currently just refuses to
generate debug information for the value, which is achieved by returning
null from that function.  The patch extends this to CONST_WIDE_INT.
The patch also takes the same approach in add_const_value_attribute,
where the desired effect of not generating debug info for the value
is achieved by returning false.  This fixes the bug because previously
we tried to generate debug info and return true, but did so using a
zero-bit value.

Thanks,
Richard


gcc/
	PR debug/66728
	* dwarf2out.c (loc_descriptor): Remove redundant GET_MODE of
	CONST_WIDE_INTs.  Handle BLKmode for CONST_WIDE_INT too.
	(add_const_value_attribute): Add a mode parameter.
	Check it for CONST_INT and CONST_WIDE_INT.  Use it to build
	wide_int values.
	(add_location_or_const_value_attribute): Update call.
	(tree_add_const_value_attribute): Likewise.
    
gcc/testsuite/
	PR debug/66728
	* gcc.dg/debug/dwarf2/pr66728.c: New test.
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 9ce3f09..cd84af6 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3252,7 +3252,7 @@  static HOST_WIDE_INT field_byte_offset (const_tree);
 static void add_AT_location_description	(dw_die_ref, enum dwarf_attribute,
 					 dw_loc_list_ref);
 static void add_data_member_location_attribute (dw_die_ref, tree);
-static bool add_const_value_attribute (dw_die_ref, rtx);
+static bool add_const_value_attribute (dw_die_ref, machine_mode, rtx);
 static void insert_int (HOST_WIDE_INT, unsigned, unsigned char *);
 static void insert_wide_int (const wide_int &, unsigned char *, int);
 static void insert_float (const_rtx, unsigned char *);
@@ -13799,10 +13799,9 @@  loc_descriptor (rtx rtl, machine_mode mode,
       break;
 
     case CONST_WIDE_INT:
-      if (mode == VOIDmode)
-	mode = GET_MODE (rtl);
-
-      if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict))
+      if (mode != VOIDmode
+	  && mode != BLKmode
+	  && (dwarf_version >= 4 || !dwarf_strict))
 	{
 	  loc_result = new_loc_descr (DW_OP_implicit_value,
 				      GET_MODE_SIZE (mode), 0);
@@ -15577,25 +15576,33 @@  insert_float (const_rtx rtl, unsigned char *array)
    constants do not necessarily get memory "homes".  */
 
 static bool
-add_const_value_attribute (dw_die_ref die, rtx rtl)
+add_const_value_attribute (dw_die_ref die, machine_mode mode, rtx rtl)
 {
   switch (GET_CODE (rtl))
     {
     case CONST_INT:
-      {
-	HOST_WIDE_INT val = INTVAL (rtl);
+      if (mode != BLKmode)
+	{
+	  gcc_checking_assert (SCALAR_INT_MODE_P (mode));
+	  HOST_WIDE_INT val = INTVAL (rtl);
 
-	if (val < 0)
-	  add_AT_int (die, DW_AT_const_value, val);
-	else
-	  add_AT_unsigned (die, DW_AT_const_value, (unsigned HOST_WIDE_INT) val);
-      }
-      return true;
+	  if (val < 0)
+	    add_AT_int (die, DW_AT_const_value, val);
+	  else
+	    add_AT_unsigned (die, DW_AT_const_value,
+			     (unsigned HOST_WIDE_INT) val);
+	  return true;
+	}
+      return false;
 
     case CONST_WIDE_INT:
-      add_AT_wide (die, DW_AT_const_value,
-		   std::make_pair (rtl, GET_MODE (rtl)));
-      return true;
+      if (mode != BLKmode)
+	{
+	  gcc_checking_assert (SCALAR_INT_MODE_P (mode));
+	  add_AT_wide (die, DW_AT_const_value, std::make_pair (rtl, mode));
+	  return true;
+	}
+      return false;
 
     case CONST_DOUBLE:
       /* Note that a CONST_DOUBLE rtx could represent either an integer or a
@@ -15672,7 +15679,7 @@  add_const_value_attribute (dw_die_ref die, rtx rtl)
 
     case CONST:
       if (CONSTANT_P (XEXP (rtl, 0)))
-	return add_const_value_attribute (die, XEXP (rtl, 0));
+	return add_const_value_attribute (die, mode, XEXP (rtl, 0));
       /* FALLTHROUGH */
     case SYMBOL_REF:
       if (!const_ok_for_output (rtl))
@@ -16175,7 +16182,7 @@  add_location_or_const_value_attribute (dw_die_ref die, tree decl, bool cache_p)
 
   rtl = rtl_for_decl_location (decl);
   if (rtl && (CONSTANT_P (rtl) || GET_CODE (rtl) == CONST_STRING)
-      && add_const_value_attribute (die, rtl))
+      && add_const_value_attribute (die, DECL_MODE (decl), rtl))
     return true;
 
   /* See if we have single element location list that is equivalent to
@@ -16196,7 +16203,7 @@  add_location_or_const_value_attribute (dw_die_ref die, tree decl, bool cache_p)
       if (GET_CODE (rtl) == EXPR_LIST)
 	rtl = XEXP (rtl, 0);
       if ((CONSTANT_P (rtl) || GET_CODE (rtl) == CONST_STRING)
-	  && add_const_value_attribute (die, rtl))
+	  && add_const_value_attribute (die, DECL_MODE (decl), rtl))
 	 return true;
     }
   /* If this decl is from BLOCK_NONLOCALIZED_VARS, we might need its
@@ -16399,7 +16406,7 @@  tree_add_const_value_attribute (dw_die_ref die, tree t)
 
   rtl = rtl_for_decl_init (init, type);
   if (rtl)
-    return add_const_value_attribute (die, rtl);
+    return add_const_value_attribute (die, TYPE_MODE (type), rtl);
   /* If the host and target are sane, try harder.  */
   else if (CHAR_BIT == 8 && BITS_PER_UNIT == 8
 	   && initializer_constant_valid_p (init, type))
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c
new file mode 100644
index 0000000..ba41e97
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile { target { x86_64-*-* && lp64 } } } */
+/* { dg-options "-O -gdwarf -dA" } */
+
+__uint128_t
+test (void)
+{
+  static const __uint128_t foo = ((((__uint128_t) 0x22334455) << 96)
+				  | 0x99aabb);
+
+  return foo;
+}
+
+/* { dg-final { scan-assembler {\.quad\t0x99aabb\t# DW_AT_const_value} } } */
+/* { dg-final { scan-assembler {\.quad\t0x2233445500000000\t} } } */