Fix PR debug/66728

Fix PR debug/66728

[Richard Sandiford]

This is yet another bug caused by rtx having modeless scalar integer
constants.  We need to use context to find the actual mode of a
CONST_INT or CONST_WIDE_INT.

Getting a mode is especially awkward here.  Decls have two modes
associated with them: TYPE_MODE (TREE_TYPE (decl)) and DECL_MODE (decl).
Promotion via things like promote_decl_mode can lead to the rtl having
a mode that is different from both of them.  Sometimes structure decls
have BLKmode but are assigned an integer-mode rtl (e.g. when passing
3-byte structures by value to functions).

I think in this case we're just going to have to assume that none of
these fancy mode changes happen for something that's big enough to
need a CONST_WIDE_INT.

loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
actually integers at the source level).  That seems like the right
behaviour, so this patch does that for add_const_value_attribute too.
It asserts that the mode is otherwise sensible for both CONST_INT
and CONST_WIDE_INT.  Asserting for CONST_INT isn't strictly necessary
but means that the assumption will get much more coverage than asserting
only for CONST_WIDE_INT does.

Tested on x86_64-linux-gnu and aarch64-linux-gnu.  Also tested against
the gdb testsuite.  OK to install?

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 --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index d9d3063..383d705 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3259,7 +3259,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 *);
@@ -13795,10 +13795,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);
@@ -15576,25 +15575,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
@@ -15671,7 +15678,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))
@@ -16171,7 +16178,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
@@ -16192,7 +16199,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
@@ -16395,7 +16402,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} } } */

Re: Fix PR debug/66728

[Jeff Law]

On 08/21/2015 07:58 AM, Richard Sandiford wrote:
> This is yet another bug caused by rtx having modeless scalar integer
> constants.  We need to use context to find the actual mode of a
> CONST_INT or CONST_WIDE_INT.
We've been bumping into these as long as I can remember.  I suspect I 
still have a great essay from Jim on why it's hard to fix on a hard 
drive from one of my old HP boxes from the early 90s ;-)


> 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.
I'll have to defer to Jason & Jakub on this.  Note Jakub is on PTO right 
now.

jeff

[ping] Re: Fix PR debug/66728

[Ulrich Weigand]

Hi Richard,

seems this still hasn't gone upstream ...  Any news?

Thanks,
Ulrich

> This is yet another bug caused by rtx having modeless scalar integer
> constants.  We need to use context to find the actual mode of a
> CONST_INT or CONST_WIDE_INT.
> 
> Getting a mode is especially awkward here.  Decls have two modes
> associated with them: TYPE_MODE (TREE_TYPE (decl)) and DECL_MODE (decl).
> Promotion via things like promote_decl_mode can lead to the rtl having
> a mode that is different from both of them.  Sometimes structure decls
> have BLKmode but are assigned an integer-mode rtl (e.g. when passing
> 3-byte structures by value to functions).
> 
> I think in this case we're just going to have to assume that none of
> these fancy mode changes happen for something that's big enough to
> need a CONST_WIDE_INT.
> 
> loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
> actually integers at the source level).  That seems like the right
> behaviour, so this patch does that for add_const_value_attribute too.
> It asserts that the mode is otherwise sensible for both CONST_INT
> and CONST_WIDE_INT.  Asserting for CONST_INT isn't strictly necessary
> but means that the assumption will get much more coverage than asserting
> only for CONST_WIDE_INT does.
> 
> Tested on x86_64-linux-gnu and aarch64-linux-gnu.  Also tested against
> the gdb testsuite.  OK to install?
> 
> 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 --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index d9d3063..383d705 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -3259,7 +3259,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 *);
> @@ -13795,10 +13795,9 @@ loc_descriptor (rtx rtl, machine_mode mode,
>        break;
> =20
>      case CONST_WIDE_INT:
> -      if (mode =3D=3D VOIDmode)
> -	mode =3D GET_MODE (rtl);
> -
> -      if (mode !=3D VOIDmode && (dwarf_version >=3D 4 || !dwarf_strict))
> +      if (mode !=3D VOIDmode
> +	  && mode !=3D BLKmode
> +	  && (dwarf_version >=3D 4 || !dwarf_strict))
>  	{
>  	  loc_result =3D new_loc_descr (DW_OP_implicit_value,
>  				      GET_MODE_SIZE (mode), 0);
> @@ -15576,25 +15575,33 @@ insert_float (const_rtx rtl, unsigned char *array)
>     constants do not necessarily get memory "homes".  */
> =20
>  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 =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;
> +	}
> +      return false;
> =20
>      case CONST_WIDE_INT:
> -      add_AT_wide (die, DW_AT_const_value,
> -		   std::make_pair (rtl, GET_MODE (rtl)));
> -      return true;
> +      if (mode !=3D 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;
> =20
>      case CONST_DOUBLE:
>        /* Note that a CONST_DOUBLE rtx could represent either an integer or=
>  a
> @@ -15671,7 +15678,7 @@ add_const_value_attribute (dw_die_ref die, rtx rtl)
> =20
>      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))
> @@ -16171,7 +16178,7 @@ add_location_or_const_value_attribute (dw_die_ref d=
> ie, tree decl, bool cache_p,
> =20
>    rtl =3D rtl_for_decl_location (decl);
>    if (rtl && (CONSTANT_P (rtl) || GET_CODE (rtl) =3D=3D CONST_STRING)
> -      && add_const_value_attribute (die, rtl))
> +      && add_const_value_attribute (die, DECL_MODE (decl), rtl))
>      return true;
> =20
>    /* See if we have single element location list that is equivalent to
> @@ -16192,7 +16199,7 @@ add_location_or_const_value_attribute (dw_die_ref d=
> ie, tree decl, bool cache_p,
>        if (GET_CODE (rtl) =3D=3D EXPR_LIST)
>  	rtl =3D XEXP (rtl, 0);
>        if ((CONSTANT_P (rtl) || GET_CODE (rtl) =3D=3D 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
> @@ -16395,7 +16402,7 @@ tree_add_const_value_attribute (dw_die_ref die, tre=
> e t)
> =20
>    rtl =3D 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 =3D=3D 8 && BITS_PER_UNIT =3D=3D 8
>  	   && initializer_constant_valid_p (init, type))
> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c b/gcc/testsuite/gc=
> c.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 =3D ((((__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} } } */
> 


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [ping] Re: Fix PR debug/66728

[Richard Sandiford]

Ulrich Weigand <uweigand@de.ibm.com> writes:
> Hi Richard,
>
> 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.

Thanks,
Richard

>> This is yet another bug caused by rtx having modeless scalar integer
>> constants.  We need to use context to find the actual mode of a
>> CONST_INT or CONST_WIDE_INT.
>> 
>> Getting a mode is especially awkward here.  Decls have two modes
>> associated with them: TYPE_MODE (TREE_TYPE (decl)) and DECL_MODE (decl).
>> Promotion via things like promote_decl_mode can lead to the rtl having
>> a mode that is different from both of them.  Sometimes structure decls
>> have BLKmode but are assigned an integer-mode rtl (e.g. when passing
>> 3-byte structures by value to functions).
>> 
>> I think in this case we're just going to have to assume that none of
>> these fancy mode changes happen for something that's big enough to
>> need a CONST_WIDE_INT.
>> 
>> loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
>> actually integers at the source level).  That seems like the right
>> behaviour, so this patch does that for add_const_value_attribute too.
>> It asserts that the mode is otherwise sensible for both CONST_INT
>> and CONST_WIDE_INT.  Asserting for CONST_INT isn't strictly necessary
>> but means that the assumption will get much more coverage than asserting
>> only for CONST_WIDE_INT does.
>> 
>> Tested on x86_64-linux-gnu and aarch64-linux-gnu.  Also tested against
>> the gdb testsuite.  OK to install?
>> 
>> 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 --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
>> index d9d3063..383d705 100644
>> --- a/gcc/dwarf2out.c
>> +++ b/gcc/dwarf2out.c
>> @@ -3259,7 +3259,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 *);
>> @@ -13795,10 +13795,9 @@ loc_descriptor (rtx rtl, machine_mode mode,
>>        break;
>> =20
>>      case CONST_WIDE_INT:
>> -      if (mode =3D=3D VOIDmode)
>> -	mode =3D GET_MODE (rtl);
>> -
>> -      if (mode !=3D VOIDmode && (dwarf_version >=3D 4 || !dwarf_strict))
>> +      if (mode !=3D VOIDmode
>> +	  && mode !=3D BLKmode
>> +	  && (dwarf_version >=3D 4 || !dwarf_strict))
>>  	{
>>  	  loc_result =3D new_loc_descr (DW_OP_implicit_value,
>>  				      GET_MODE_SIZE (mode), 0);
>> @@ -15576,25 +15575,33 @@ insert_float (const_rtx rtl, unsigned char *array)
>>     constants do not necessarily get memory "homes".  */
>> =20
>>  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 =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;
>> +	}
>> +      return false;
>> =20
>>      case CONST_WIDE_INT:
>> -      add_AT_wide (die, DW_AT_const_value,
>> -		   std::make_pair (rtl, GET_MODE (rtl)));
>> -      return true;
>> +      if (mode !=3D 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;
>> =20
>>      case CONST_DOUBLE:
>>        /* Note that a CONST_DOUBLE rtx could represent either an integer or=
>>  a
>> @@ -15671,7 +15678,7 @@ add_const_value_attribute (dw_die_ref die, rtx rtl)
>> =20
>>      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))
>> @@ -16171,7 +16178,7 @@ add_location_or_const_value_attribute (dw_die_ref d=
>> ie, tree decl, bool cache_p,
>> =20
>>    rtl =3D rtl_for_decl_location (decl);
>>    if (rtl && (CONSTANT_P (rtl) || GET_CODE (rtl) =3D=3D CONST_STRING)
>> -      && add_const_value_attribute (die, rtl))
>> +      && add_const_value_attribute (die, DECL_MODE (decl), rtl))
>>      return true;
>> =20
>>    /* See if we have single element location list that is equivalent to
>> @@ -16192,7 +16199,7 @@ add_location_or_const_value_attribute (dw_die_ref d=
>> ie, tree decl, bool cache_p,
>>        if (GET_CODE (rtl) =3D=3D EXPR_LIST)
>>  	rtl =3D XEXP (rtl, 0);
>>        if ((CONSTANT_P (rtl) || GET_CODE (rtl) =3D=3D 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
>> @@ -16395,7 +16402,7 @@ tree_add_const_value_attribute (dw_die_ref die, tre=
>> e t)
>> =20
>>    rtl =3D 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 =3D=3D 8 && BITS_PER_UNIT =3D=3D 8
>>  	   && initializer_constant_valid_p (init, type))
>> diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c b/gcc/testsuite/gc=
>> c.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 =3D ((((__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} } } */
>> 

Re: [ping] Re: Fix PR debug/66728

[Bernd Schmidt]

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.

>>>   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.

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


Bernd

Re: [ping] Re: Fix PR debug/66728

[Ulrich Weigand]

Bernd Schmidt wrote:

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

Sorry for the mangling caused by my forwarding of the patch.
Richard's original patch is here:
https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01312.html

(I'll leave it to Richard to answer your other questions ...)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [ping] Re: Fix PR debug/66728

[Richard Sandiford]

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 --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} } } */

Re: [ping] Re: Fix PR debug/66728

[Richard Sandiford]

Richard Sandiford <richard.sandiford@arm.com> writes:
> 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.

And this time with the testsuite fix that Uros pointed out, sorry.

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..2c6aaf8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr66728.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { { i?86-*-* 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} } } */

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 2, 2015, at 8:29 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>   switch (GET_CODE (rtl))
>     {
>     case CONST_INT:
> -      {
> -	HOST_WIDE_INT val = INTVAL (rtl);
> +      if (mode != BLKmode)

This changes BLKmode for CONST_INT, but I didn’t see this discussed.  I didn’t see a test case?  I’d like to think that BLKmode things here would be fine.  I think they would be use for 1024 bit things that are representable in 20 bits, for example.  A value that is 1 (representable in 20 bits) can be trivially communicated the debugger.  The existing add_AT_unsigned I think can represent them, no?  Similarly for wide-int BLKmode support.  I think the real problem is simply the precision 0 part.  In the CONST_INT and CONST_DOUBLE there is no code that handled precision 0, and there is no code in the wide-int case either.  From wide-int.h:

  The precision and length of a wide_int are always greater than 0.

If is was 0, then we have failed.  When that bug is fixed, then the precision won’t be 0 and the existing code will work.  Where is the 0 first generated, and from what?

Re: [ping] Fix PR debug/66728

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> On Nov 2, 2015, at 8:29 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>>   switch (GET_CODE (rtl))
>>     {
>>     case CONST_INT:
>> -      {
>> -	HOST_WIDE_INT val = INTVAL (rtl);
>> +      if (mode != BLKmode)
>
> This changes BLKmode for CONST_INT, but I didn’t see this discussed.  I
> didn’t see a test case?  I’d like to think that BLKmode things here
> would be fine.  I think they would be use for 1024 bit things that are
> representable in 20 bits, for example.

This was:

  ... Sometimes structure decls
  have BLKmode but are assigned an integer-mode rtl (e.g. when passing
  3-byte structures by value to functions).
  [...]
  loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
  actually integers at the source level).  That seems like the right
  behaviour, so this patch does that for add_const_value_attribute too.
  It asserts that the mode is otherwise sensible for both CONST_INT
  and CONST_WIDE_INT.  Asserting for CONST_INT isn't strictly necessary
  but means that the assumption will get much more coverage than asserting
  only for CONST_WIDE_INT does.

Integer types always have an integer mode, not BLKmode.  E.g.
layout_type has:

    case BOOLEAN_TYPE:
    case INTEGER_TYPE:
    case ENUMERAL_TYPE:
      SET_TYPE_MODE (type,
		     smallest_mode_for_size (TYPE_PRECISION (type), MODE_INT));

where the smallest_mode_for_size is guaranteed to return something
that isn't BLKmode (or VOIDmode).

The BLKmodes are for non-integer things that nevertheless get passed
as integers.  I don't think debuggers would be expecting an integer
DIE to be used for them.  (loc_descriptor already punts in that case.)

> A value that is 1 (representable
> in 20 bits) can be trivially communicated the debugger.  The existing
> add_AT_unsigned I think can represent them, no?  Similarly for wide-int
> BLKmode support.  I think the real problem is simply the precision 0
> part.  In the CONST_INT and CONST_DOUBLE there is no code that handled
> precision 0, and there is no code in the wide-int case either.  From
> wide-int.h:
>
>   The precision and length of a wide_int are always greater than 0.
>
> If is was 0, then we have failed.  When that bug is fixed, then the
> precision won’t be 0 and the existing code will work.  Where is the 0
> first generated, and from what?

It's generated from:

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

where the precision is evaluated as:

  inline unsigned int
  wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
  {
    return GET_MODE_PRECISION (x.second);
  }

GET_MODE (rtl) is always VOIDmode and the precision of VOIDmode is 0,
so the precision of the wide_int passed to add_AT_wide is always 0.

Like CONST_INT, the "real" mode of a CONST_WIDE_INT is determined by
context rather than being stored in the rtx itself.  The point of the
patch is to use that mode instead of VOIDmode in the make_pair.

Thanks,
Richard

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 2, 2015, at 12:55 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> This was:
> 
>  ... Sometimes structure decls
>  have BLKmode but are assigned an integer-mode rtl (e.g. when passing
>  3-byte structures by value to functions).
>  [...]
>  loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
>  actually integers at the source level).  That seems like the right
>  behaviour

I’ll plead ignorance here, but why do you think that?  The dwarf standard says:

There are six forms of constants. There are
fixed length constant data forms for one, two,
four and eight byte values (respec
tively, DW_FORM_data1, DW_FORM_data2,
DW_FORM_data4, and DW_FORM_data8). There ar
e also variable length constant data
forms encoded using LEB128 numbers (see below). Both signed (DW_FORM_sdata) and
unsigned (DW_FORM_udata) variable
length constants are available
The data in DW_FORM_data1, DW
_FORM_data2, DW_FORM_data4 and
DW_FORM_data8 can be anything. Depending on c
ontext, it may be a signed integer, an
unsigned integer, a floating-point
constant, or anything else. A
consumer must use context to
know how to interpret the bits, wh
ich if they are target machine
data (such as an integer or
floating point constant) will be in target machine byte-order.

Certainly supplying the known byte values of a constant is preferable to throwing up our hands and saying, I know, but I’m not telling.  Given the text above, it seems like these forms can be used for content where the compiler knows the values of the bits that comprise the content.  I’d ask, is the backing of your position supported by the dwarf standard?  If yes, what part?

I think you think that this describes the type, these do not.  There is a separate system to describe the type.  For example, DW_ATE_UTF describes the bytes as forming a UTF value.  A wide int (or a CONST_INT) can be used to describe a unicode character, and it would have a DW_ATE_UTF encoding on it for the debugger to use to formulate an idea of how to display those bytes.  Further, a mythical front end could have a 3 byte unicode character, and these can be modeless as there is no 3 byte machine mode for them.  Code-gen would be BLKmode, the type would be DW_ATE_UTF, and one could form constants with CONST_INT.  In a 152 bit UTF character in that front end, CONST_INT, generally speaking, isn’t big enough, so a CONST_WIDE_INT would be formed.   The argument is the same.  That a machine has a native 3 byte type or not, is of no consequence, so _any_ decision based upon the mode in this way is flawed.  Then again, I don’t even pretend to be a language lawyer for the dwarf standard.  :-)

Re: [ping] Fix PR debug/66728

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> On Nov 2, 2015, at 12:55 PM, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>> This was:
>> 
>>  ... Sometimes structure decls
>>  have BLKmode but are assigned an integer-mode rtl (e.g. when passing
>>  3-byte structures by value to functions).
>>  [...]
>>  loc_descriptor refuses to use CONST_INT for BLKmode decls (which aren't
>>  actually integers at the source level).  That seems like the right
>>  behaviour
>
> I’ll plead ignorance here, but why do you think that?  The dwarf standard says:
>
> There are six forms of constants. There are
> fixed length constant data forms for one, two,
> four and eight byte values (respec
> tively, DW_FORM_data1, DW_FORM_data2,
> DW_FORM_data4, and DW_FORM_data8). There ar
> e also variable length constant data
> forms encoded using LEB128 numbers (see below). Both signed (DW_FORM_sdata) and
> unsigned (DW_FORM_udata) variable
> length constants are available
> The data in DW_FORM_data1, DW
> _FORM_data2, DW_FORM_data4 and
> DW_FORM_data8 can be anything. Depending on c
> ontext, it may be a signed integer, an
> unsigned integer, a floating-point
> constant, or anything else. A
> consumer must use context to
> know how to interpret the bits, wh
> ich if they are target machine
> data (such as an integer or
> floating point constant) will be in target machine byte-order.
>
> Certainly supplying the known byte values of a constant is preferable to
> throwing up our hands and saying, I know, but I’m not telling.  Given
> the text above, it seems like these forms can be used for content where
> the compiler knows the values of the bits that comprise the content.
> I’d ask, is the backing of your position supported by the dwarf
> standard?  If yes, what part?
>
> I think you think that this describes the type, these do not.  There is
> a separate system to describe the type.  For example, DW_ATE_UTF
> describes the bytes as forming a UTF value.  A wide int (or a CONST_INT)
> can be used to describe a unicode character, and it would have a
> DW_ATE_UTF encoding on it for the debugger to use to formulate an idea
> of how to display those bytes.  Further, a mythical front end could have
> a 3 byte unicode character, and these can be modeless as there is no 3
> byte machine mode for them.  Code-gen would be BLKmode, the type would
> be DW_ATE_UTF, and one could form constants with CONST_INT.  In a 152
> bit UTF character in that front end, CONST_INT, generally speaking,
> isn’t big enough, so a CONST_WIDE_INT would be formed.  The argument is
> the same.  That a machine has a native 3 byte type or not, is of no
> consequence, so _any_ decision based upon the mode in this way is
> flawed.

This isn't just an argument about the DWARF standard though.  It's an
argument about GCC internals.  Presumably these hypothetical BLKmode
types would need to support addition, but plus:BLK is not well formed,
and wouldn't distinguish between your 3-byte and 152-bit cases.  I don't
think const_int and const_wide_int are logically different.  There's the
historical decision that const_int doesn't have a stored mode, but I
don't think that was because we wanted to support const_ints that are
conceptually BLKmode.

I think from an rtl perspective the only sensible way for frontends to
cope with integers whose size doesn't match an rtl mode is to promote
to the next widest mode, which is what the stor-layout.c code I quoted
does.  Obviously if your 3 byte type is actually 3 bytes in memory rather
than 4, and no 3-byte mode is available, you can't just load and store
the value using a normal rtl move.  You have to use bitfield extraction
and insertion instead.

I picked this PR up because it was wide-int-related, even though
(as is probably all too obvious from this thread) I'm not familiar
with the dwarf2out.c code.  It's actually your commit that I'm trying
to fix here (r201707).  Would you mind taking the PR over and handling
it the way you think it should be handled?

Thanks,
Richard

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 3, 2015, at 12:46 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> This isn't just an argument about the DWARF standard though.  It's an
> argument about GCC internals.  Presumably these hypothetical BLKmode
> types would need to support addition,

I don’t recall seeing that as a requirement in the dwarf standard, nor in the language standard of the hypothetical language I posited.  Further, I don’t recall seeing a prohibition on fetching from memory, combining three byte to form a 24 bit value in a 64 bit register on x86_64, nor the prohibition on doing a 64-bit add on that architecture.  Do you have a quote to support your position?  Further, I don’t even see support that add must be supported in the language front end I posited.  Since I control it, you can’t can’t require much of anything in it.

> but plus:BLK is not well formed,

BLKmode is well formed in gcc.  I can’t image what this even means.

> and wouldn't distinguish between your 3-byte and 152-bit cases.

A 3 byte value in dwarf has a byte length of 3, and a 152 bit value has a byte length of 19.  I fail to see why in dwarf they cannot be distinguished.  A cleaver front end can track the values in that front end, and regions of memory that is uses to back objects, usually the front end does track objects, and when asked to, will generate dwarf for them.  I believe that front ends are able to track object byte sized for objects.  I believe that the C++ front end does this for example, and will generate dwarf.

> I don’t think const_int and const_wide_int are logically different.

They aren’t.  Indeed, the code for const_wide_int was copied from const_int and/or const_double and only enhanced to have an arbitrary size.

> There's the
> historical decision that const_int doesn't have a stored mode, but I
> don't think that was because we wanted to support const_ints that are
> conceptually BLKmode.

So, given that what must be, given the semantics of dwarf, and the totality of semantics available to a front end, BLKmode values are perfectly well defined.  I don’t know why you would think otherwise.  Further, arguing that we must break them, because…  is silly, without the because elaborated.  A constant value that is known at compile time to be an arbitrary value whose type is a signed n byte int can be represented in const_int when n fits, and can be expressed as a const_wide_int when n fits and can be expressed in dwarf.  Further, const_int is not unreasonable to represent constants, further, there is no prohibition on a front end having constants, nor on the size of those constants being n bytes in size, at least for values of n where n can be supported directly by const_wide_int. If there were one, you could be able to quote it.

> I think from an rtl perspective the only sensible way for frontends to
> cope with integers whose size doesn't match an rtl mode is to promote
> to the next widest mode,

No, again, to support your position, you’d have to quote the part of gcc that disclaims support.  You have not.  You can think there is a prohibition on 3 byte ints, if you want.  I can’t fix that.  What I can object to, is adding a line to the document that says that gcc cannot be used to write a front end that supports 3 byte ints, or 19 byte ints.

> which is what the stor-layout.c code I quoted
> does.  Obviously if your 3 byte type is actually 3 bytes in memory rather
> than 4, and no 3-byte mode is available, you can't just load and store
> the value using a normal rtl move.  You have to use bitfield extraction
> and insertion instead.

So?  Another technique that a front end could use is to fetch a HImode, and a QImode value, shift and or them together, but no matter.

> I picked this PR up because it was wide-int-related, even though
> (as is probably all too obvious from this thread) I'm not familiar
> with the dwarf2out.c code.  It's actually your commit that I'm trying
> to fix here (r201707).  Would you mind taking the PR over and handling
> it the way you think it should be handled?

So, I did up the patch:

Index: rtl.h
===================================================================
--- rtl.h	(revision 229720)
+++ rtl.h	(working copy)
@@ -2086,6 +2086,15 @@
 inline unsigned int
 wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
 {
+  /* VOIDmode CONST_WIDE_INT pairings are used for debug information
+     when the type of the underlying object has not been tracked and
+     is unimportant so we form a precision from the value of the
+     constant.  This is sufficient for dwarf generation as dwarf can
+     track the type separately from the value.  */
+  if (x.second == VOIDmode
+      && CONST_WIDE_INT_P (x.first))
+    return CONST_WIDE_INT_NUNITS (x.first) * HOST_BITS_PER_WIDE_INT;
+
   return GET_MODE_PRECISION (x.second);
 }
 
Index: wide-int.h
===================================================================
--- wide-int.h	(revision 229720)
+++ wide-int.h	(working copy)
@@ -156,6 +156,10 @@
 	     rtx r = ...
 	     wide_int x = std::make_pair (r, mode);
 
+   If the mode is VOIDmode, then the precision of the wide_int will be
+   the number of bits used to represent the rtl's value.  This is used for
+   untyped constants for dwarf debug information.
+
    Similarly, a wide_int can only be constructed from a host value if
    the target precision is given explicitly, such as in:
 
The intent is to exactly match what gcc did before, to have wide-int handling match what const_int does, and what const_double did in the past, merely extended to support arbitrarily large values.  I find, in this case, what was done in the past to be valid and desirable.  I don’t find any reason to deviate from what was done before.

The test case from the PR changes like this:

--- t-bad.s	2015-11-03 16:19:13.786248224 -0500
+++ t-fixed.s	2015-11-03 16:02:48.414290258 -0500
@@ -23,7 +23,7 @@ test:
 .Letext0:
 	.section	.debug_info,"",@progbits
 .Ldebug_info0:
-	.long	0x63	# Length of Compilation Unit Info
+	.long	0x74	# Length of Compilation Unit Info
 	.value	0x4	# DWARF version number
 	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
 	.byte	0x8	# Pointer Size (in bytes)
@@ -41,25 +41,28 @@ test:
 	.byte	0x1	# DW_AT_decl_file (t.c)
 	.byte	0x1	# DW_AT_decl_line
 			# DW_AT_prototyped
-	.long	0x5a	# DW_AT_type
+	.long	0x6b	# DW_AT_type
 	.quad	.LFB0	# DW_AT_low_pc
 	.quad	.LFE0-.LFB0	# DW_AT_high_pc
 	.uleb128 0x1	# DW_AT_frame_base
 	.byte	0x9c	# DW_OP_call_frame_cfa
 			# DW_AT_GNU_all_call_sites
-	.long	0x5a	# DW_AT_sibling
+	.long	0x6b	# DW_AT_sibling
 	.uleb128 0x3	# (DIE (0x4e) DW_TAG_variable)
 	.long	.LASF3	# DW_AT_name: "two127"
 	.byte	0x1	# DW_AT_decl_file (t.c)
 	.byte	0x3	# DW_AT_decl_line
-	.long	0x61	# DW_AT_type
+	.long	0x72	# DW_AT_type
+	.byte	0x10
+	.quad	0	# DW_AT_const_value
+	.quad	0x8000000000000000	# (null)
 	.byte	0	# end of children of DIE 0x2d
-	.uleb128 0x4	# (DIE (0x5a) DW_TAG_base_type)
+	.uleb128 0x4	# (DIE (0x6b) DW_TAG_base_type)
 	.byte	0x10	# DW_AT_byte_size
 	.byte	0x7	# DW_AT_encoding
 	.long	.LASF4	# DW_AT_name: "__int128 unsigned"
-	.uleb128 0x5	# (DIE (0x61) DW_TAG_const_type)
-	.long	0x5a	# DW_AT_type
+	.uleb128 0x5	# (DIE (0x72) DW_TAG_const_type)
+	.long	0x6b	# DW_AT_type
 	.byte	0	# end of children of DIE 0xb
 	.section	.debug_abbrev,"",@progbits

I’m not a dwarf expert, but, I see the addition of two quads for a const value, and that value seems to be 1 << 127 to my untrained eye.  If I had to theorizes, I’d say this is what the debug information was before.  [ checking ] I checked gcc-4.8 on x86_64, and the debug information is now virtually the same:

*** t-48.s	2015-11-03 16:24:23.286235021 -0500
--- t-fixed.s	2015-11-03 16:02:48.414290258 -0500
*************** test:
*** 28,35 ****
  	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
  	.byte	0x8	# Pointer Size (in bytes)
  	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
! 	.long	.LASF0	# DW_AT_producer: "GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector"
! 	.byte	0x1	# DW_AT_language
  	.ascii "t.c\0"	# DW_AT_name
  	.long	.LASF1	# DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc"
  	.quad	.Ltext0	# DW_AT_low_pc
--- 28,35 ----
  	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
  	.byte	0x8	# Pointer Size (in bytes)
  	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
! 	.long	.LASF0	# DW_AT_producer: "GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g"
! 	.byte	0xc	# DW_AT_language
  	.ascii "t.c\0"	# DW_AT_name
  	.long	.LASF1	# DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc"
  	.quad	.Ltext0	# DW_AT_low_pc
*************** test:
*** 55,61 ****
  	.long	0x72	# DW_AT_type
  	.byte	0x10
  	.quad	0	# DW_AT_const_value
! 	.quad	0x8000000000000000
  	.byte	0	# end of children of DIE 0x2d
  	.uleb128 0x4	# (DIE (0x6b) DW_TAG_base_type)
  	.byte	0x10	# DW_AT_byte_size
--- 55,61 ----
  	.long	0x72	# DW_AT_type
  	.byte	0x10
  	.quad	0	# DW_AT_const_value
! 	.quad	0x8000000000000000	# (null)
  	.byte	0	# end of children of DIE 0x2d
  	.uleb128 0x4	# (DIE (0x6b) DW_TAG_base_type)
  	.byte	0x10	# DW_AT_byte_size
*************** test:
*** 162,168 ****
  .Ldebug_line0:
  	.section	.debug_str,"MS",@progbits,1
  .LASF0:
! 	.string	"GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector"
  .LASF1:
  	.string	"/home/mrs/net/gcc-linuxO0/gcc"
  .LASF3:
--- 162,168 ----
  .Ldebug_line0:
  	.section	.debug_str,"MS",@progbits,1
  .LASF0:
! 	.string	"GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g"
  .LASF1:
  	.string	"/home/mrs/net/gcc-linuxO0/gcc"
  .LASF3:
*************** test:
*** 171,175 ****
  	.string	"test"
  .LASF4:
  	.string	"__int128 unsigned"
! 	.ident	"GCC: (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4"
  	.section	.note.GNU-stack,"",@progbits
--- 171,175 ----
  	.string	"test"
  .LASF4:
  	.string	"__int128 unsigned"
! 	.ident	"GCC: (GNU) 6.0.0 20151103 (experimental) [trunk revision 229720]"
  	.section	.note.GNU-stack,"",@progbits

The language seems weird, but, I bet it is inconsequential and an artifact of how I ran the test case.  The # (null) is unfortunate, I don’t think this is better than what gcc-4.8 did, but, I don’t think that was caused by my patch.

So, this would be my proposal to fix the issue.  I'd invite anyone that thinks the dwarf information was wrong in gcc-4.8 or wrong post the patch, to let us know why.  If there are reasons why my position is wrong, I’d like to hear about it, otherwise I’ll plan on checking this in with my wide-int hat on.  Since this is a serious regression in debug quality, this has to go the the release branches that contain wide-int.

Re: [ping] Fix PR debug/66728

[Richard Biener]

On Tue, Nov 3, 2015 at 10:58 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 3, 2015, at 12:46 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>> This isn't just an argument about the DWARF standard though.  It's an
>> argument about GCC internals.  Presumably these hypothetical BLKmode
>> types would need to support addition,
>
> I don’t recall seeing that as a requirement in the dwarf standard, nor in the language standard of the hypothetical language I posited.  Further, I don’t recall seeing a prohibition on fetching from memory, combining three byte to form a 24 bit value in a 64 bit register on x86_64, nor the prohibition on doing a 64-bit add on that architecture.  Do you have a quote to support your position?  Further, I don’t even see support that add must be supported in the language front end I posited.  Since I control it, you can’t can’t require much of anything in it.
>
>> but plus:BLK is not well formed,
>
> BLKmode is well formed in gcc.  I can’t image what this even means.
>
>> and wouldn't distinguish between your 3-byte and 152-bit cases.
>
> A 3 byte value in dwarf has a byte length of 3, and a 152 bit value has a byte length of 19.  I fail to see why in dwarf they cannot be distinguished.  A cleaver front end can track the values in that front end, and regions of memory that is uses to back objects, usually the front end does track objects, and when asked to, will generate dwarf for them.  I believe that front ends are able to track object byte sized for objects.  I believe that the C++ front end does this for example, and will generate dwarf.
>
>> I don’t think const_int and const_wide_int are logically different.
>
> They aren’t.  Indeed, the code for const_wide_int was copied from const_int and/or const_double and only enhanced to have an arbitrary size.
>
>> There's the
>> historical decision that const_int doesn't have a stored mode, but I
>> don't think that was because we wanted to support const_ints that are
>> conceptually BLKmode.
>
> So, given that what must be, given the semantics of dwarf, and the totality of semantics available to a front end, BLKmode values are perfectly well defined.  I don’t know why you would think otherwise.  Further, arguing that we must break them, because…  is silly, without the because elaborated.  A constant value that is known at compile time to be an arbitrary value whose type is a signed n byte int can be represented in const_int when n fits, and can be expressed as a const_wide_int when n fits and can be expressed in dwarf.  Further, const_int is not unreasonable to represent constants, further, there is no prohibition on a front end having constants, nor on the size of those constants being n bytes in size, at least for values of n where n can be supported directly by const_wide_int. If there were one, you could be able to quote it.
>
>> I think from an rtl perspective the only sensible way for frontends to
>> cope with integers whose size doesn't match an rtl mode is to promote
>> to the next widest mode,
>
> No, again, to support your position, you’d have to quote the part of gcc that disclaims support.  You have not.  You can think there is a prohibition on 3 byte ints, if you want.  I can’t fix that.  What I can object to, is adding a line to the document that says that gcc cannot be used to write a front end that supports 3 byte ints, or 19 byte ints.
>
>> which is what the stor-layout.c code I quoted
>> does.  Obviously if your 3 byte type is actually 3 bytes in memory rather
>> than 4, and no 3-byte mode is available, you can't just load and store
>> the value using a normal rtl move.  You have to use bitfield extraction
>> and insertion instead.
>
> So?  Another technique that a front end could use is to fetch a HImode, and a QImode value, shift and or them together, but no matter.
>
>> I picked this PR up because it was wide-int-related, even though
>> (as is probably all too obvious from this thread) I'm not familiar
>> with the dwarf2out.c code.  It's actually your commit that I'm trying
>> to fix here (r201707).  Would you mind taking the PR over and handling
>> it the way you think it should be handled?
>
> So, I did up the patch:
>
> Index: rtl.h
> ===================================================================
> --- rtl.h       (revision 229720)
> +++ rtl.h       (working copy)
> @@ -2086,6 +2086,15 @@
>  inline unsigned int
>  wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
>  {
> +  /* VOIDmode CONST_WIDE_INT pairings are used for debug information
> +     when the type of the underlying object has not been tracked and
> +     is unimportant so we form a precision from the value of the
> +     constant.  This is sufficient for dwarf generation as dwarf can
> +     track the type separately from the value.  */
> +  if (x.second == VOIDmode
> +      && CONST_WIDE_INT_P (x.first))
> +    return CONST_WIDE_INT_NUNITS (x.first) * HOST_BITS_PER_WIDE_INT;
> +
>    return GET_MODE_PRECISION (x.second);
>  }

I think you should limit the effect of this patch to the dwarf2out use
as the above doesn't make
sense to me.  As Richard said, a (plus:BLK ...) isn't well-formed.

Ideally we'd have an assert that you don't create a rtx_mode_t with
VOIDmode or BLKmode.

Handling the CONST_WIDE_INT in dwarf2out.c the same as we did before
(with CONST_DOUBLE)
looks sensible as far of fixing a regression (I assume the diff to the
dwarf results in essentially the
same DWARF as what was present before wide-int).

Richard.

> Index: wide-int.h
> ===================================================================
> --- wide-int.h  (revision 229720)
> +++ wide-int.h  (working copy)
> @@ -156,6 +156,10 @@
>              rtx r = ...
>              wide_int x = std::make_pair (r, mode);
>
> +   If the mode is VOIDmode, then the precision of the wide_int will be
> +   the number of bits used to represent the rtl's value.  This is used for
> +   untyped constants for dwarf debug information.
> +
>     Similarly, a wide_int can only be constructed from a host value if
>     the target precision is given explicitly, such as in:
>
> The intent is to exactly match what gcc did before, to have wide-int handling match what const_int does, and what const_double did in the past, merely extended to support arbitrarily large values.  I find, in this case, what was done in the past to be valid and desirable.  I don’t find any reason to deviate from what was done before.
>
> The test case from the PR changes like this:
>
> --- t-bad.s     2015-11-03 16:19:13.786248224 -0500
> +++ t-fixed.s   2015-11-03 16:02:48.414290258 -0500
> @@ -23,7 +23,7 @@ test:
>  .Letext0:
>         .section        .debug_info,"",@progbits
>  .Ldebug_info0:
> -       .long   0x63    # Length of Compilation Unit Info
> +       .long   0x74    # Length of Compilation Unit Info
>         .value  0x4     # DWARF version number
>         .long   .Ldebug_abbrev0 # Offset Into Abbrev. Section
>         .byte   0x8     # Pointer Size (in bytes)
> @@ -41,25 +41,28 @@ test:
>         .byte   0x1     # DW_AT_decl_file (t.c)
>         .byte   0x1     # DW_AT_decl_line
>                         # DW_AT_prototyped
> -       .long   0x5a    # DW_AT_type
> +       .long   0x6b    # DW_AT_type
>         .quad   .LFB0   # DW_AT_low_pc
>         .quad   .LFE0-.LFB0     # DW_AT_high_pc
>         .uleb128 0x1    # DW_AT_frame_base
>         .byte   0x9c    # DW_OP_call_frame_cfa
>                         # DW_AT_GNU_all_call_sites
> -       .long   0x5a    # DW_AT_sibling
> +       .long   0x6b    # DW_AT_sibling
>         .uleb128 0x3    # (DIE (0x4e) DW_TAG_variable)
>         .long   .LASF3  # DW_AT_name: "two127"
>         .byte   0x1     # DW_AT_decl_file (t.c)
>         .byte   0x3     # DW_AT_decl_line
> -       .long   0x61    # DW_AT_type
> +       .long   0x72    # DW_AT_type
> +       .byte   0x10
> +       .quad   0       # DW_AT_const_value
> +       .quad   0x8000000000000000      # (null)
>         .byte   0       # end of children of DIE 0x2d
> -       .uleb128 0x4    # (DIE (0x5a) DW_TAG_base_type)
> +       .uleb128 0x4    # (DIE (0x6b) DW_TAG_base_type)
>         .byte   0x10    # DW_AT_byte_size
>         .byte   0x7     # DW_AT_encoding
>         .long   .LASF4  # DW_AT_name: "__int128 unsigned"
> -       .uleb128 0x5    # (DIE (0x61) DW_TAG_const_type)
> -       .long   0x5a    # DW_AT_type
> +       .uleb128 0x5    # (DIE (0x72) DW_TAG_const_type)
> +       .long   0x6b    # DW_AT_type
>         .byte   0       # end of children of DIE 0xb
>         .section        .debug_abbrev,"",@progbits
>
> I’m not a dwarf expert, but, I see the addition of two quads for a const value, and that value seems to be 1 << 127 to my untrained eye.  If I had to theorizes, I’d say this is what the debug information was before.  [ checking ] I checked gcc-4.8 on x86_64, and the debug information is now virtually the same:
>
> *** t-48.s      2015-11-03 16:24:23.286235021 -0500
> --- t-fixed.s   2015-11-03 16:02:48.414290258 -0500
> *************** test:
> *** 28,35 ****
>         .long   .Ldebug_abbrev0 # Offset Into Abbrev. Section
>         .byte   0x8     # Pointer Size (in bytes)
>         .uleb128 0x1    # (DIE (0xb) DW_TAG_compile_unit)
> !       .long   .LASF0  # DW_AT_producer: "GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector"
> !       .byte   0x1     # DW_AT_language
>         .ascii "t.c\0"  # DW_AT_name
>         .long   .LASF1  # DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc"
>         .quad   .Ltext0 # DW_AT_low_pc
> --- 28,35 ----
>         .long   .Ldebug_abbrev0 # Offset Into Abbrev. Section
>         .byte   0x8     # Pointer Size (in bytes)
>         .uleb128 0x1    # (DIE (0xb) DW_TAG_compile_unit)
> !       .long   .LASF0  # DW_AT_producer: "GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g"
> !       .byte   0xc     # DW_AT_language
>         .ascii "t.c\0"  # DW_AT_name
>         .long   .LASF1  # DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc"
>         .quad   .Ltext0 # DW_AT_low_pc
> *************** test:
> *** 55,61 ****
>         .long   0x72    # DW_AT_type
>         .byte   0x10
>         .quad   0       # DW_AT_const_value
> !       .quad   0x8000000000000000
>         .byte   0       # end of children of DIE 0x2d
>         .uleb128 0x4    # (DIE (0x6b) DW_TAG_base_type)
>         .byte   0x10    # DW_AT_byte_size
> --- 55,61 ----
>         .long   0x72    # DW_AT_type
>         .byte   0x10
>         .quad   0       # DW_AT_const_value
> !       .quad   0x8000000000000000      # (null)
>         .byte   0       # end of children of DIE 0x2d
>         .uleb128 0x4    # (DIE (0x6b) DW_TAG_base_type)
>         .byte   0x10    # DW_AT_byte_size
> *************** test:
> *** 162,168 ****
>   .Ldebug_line0:
>         .section        .debug_str,"MS",@progbits,1
>   .LASF0:
> !       .string "GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector"
>   .LASF1:
>         .string "/home/mrs/net/gcc-linuxO0/gcc"
>   .LASF3:
> --- 162,168 ----
>   .Ldebug_line0:
>         .section        .debug_str,"MS",@progbits,1
>   .LASF0:
> !       .string "GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g"
>   .LASF1:
>         .string "/home/mrs/net/gcc-linuxO0/gcc"
>   .LASF3:
> *************** test:
> *** 171,175 ****
>         .string "test"
>   .LASF4:
>         .string "__int128 unsigned"
> !       .ident  "GCC: (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4"
>         .section        .note.GNU-stack,"",@progbits
> --- 171,175 ----
>         .string "test"
>   .LASF4:
>         .string "__int128 unsigned"
> !       .ident  "GCC: (GNU) 6.0.0 20151103 (experimental) [trunk revision 229720]"
>         .section        .note.GNU-stack,"",@progbits
>
> The language seems weird, but, I bet it is inconsequential and an artifact of how I ran the test case.  The # (null) is unfortunate, I don’t think this is better than what gcc-4.8 did, but, I don’t think that was caused by my patch.
>
> So, this would be my proposal to fix the issue.  I'd invite anyone that thinks the dwarf information was wrong in gcc-4.8 or wrong post the patch, to let us know why.  If there are reasons why my position is wrong, I’d like to hear about it, otherwise I’ll plan on checking this in with my wide-int hat on.  Since this is a serious regression in debug quality, this has to go the the release branches that contain wide-int.

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 4, 2015, at 1:43 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> I think you should limit the effect of this patch to the dwarf2out use
> as the above doesn't make sense to me.

Since dwarf is so special, and since other clients already do something sort of like this anyway, it isn’t unreasonable to make the client be responsible for picking a sensible mode, and asserting if they fail to.  This also transfers the cost of the special case code out to the one client that needs it, and avoids that cost for all the other clients.

The new patch is below for your consideration.

Ok?

> Ideally we'd have an assert that you don't create a rtx_mode_t with
> VOIDmode or BLKmode.

Added.

> Handling the CONST_WIDE_INT in dwarf2out.c the same as we did before
> (with CONST_DOUBLE)
> looks sensible as far of fixing a regression (I assume the diff to the
> dwarf results in essentially the
> same DWARF as what was present before wide-int).

Yes, the dwarf is the same.

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 229720)
+++ dwarf2out.c	(working copy)
@@ -15593,8 +15593,15 @@
       return true;
 
     case CONST_WIDE_INT:
-      add_AT_wide (die, DW_AT_const_value,
-		   std::make_pair (rtl, GET_MODE (rtl)));
+      {
+	machine_mode mode = GET_MODE (rtl);
+	if (mode == VOIDmode)
+	  mode = mode_for_size (CONST_WIDE_INT_NUNITS (rtl)
+				* HOST_BITS_PER_WIDE_INT,
+				MODE_INT, 0);
+	add_AT_wide (die, DW_AT_const_value,
+		     std::make_pair (rtl, mode));
+      }
       return true;
 
     case CONST_DOUBLE:
Index: rtl.h
===================================================================
--- rtl.h	(revision 229720)
+++ rtl.h	(working copy)
@@ -2086,6 +2086,7 @@
 inline unsigned int
 wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
 {
+  gcc_assert (x.second != BLKmode && x.second != VOIDmode);
   return GET_MODE_PRECISION (x.second);
 }
 

Re: [ping] Fix PR debug/66728

[Richard Biener]

On Wed, Nov 4, 2015 at 12:57 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 4, 2015, at 1:43 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> I think you should limit the effect of this patch to the dwarf2out use
>> as the above doesn't make sense to me.
>
> Since dwarf is so special, and since other clients already do something sort of like this anyway, it isn’t unreasonable to make the client be responsible for picking a sensible mode, and asserting if they fail to.  This also transfers the cost of the special case code out to the one client that needs it, and avoids that cost for all the other clients.
>
> The new patch is below for your consideration.
>
> Ok?
>
>> Ideally we'd have an assert that you don't create a rtx_mode_t with
>> VOIDmode or BLKmode.
>
> Added.
>
>> Handling the CONST_WIDE_INT in dwarf2out.c the same as we did before
>> (with CONST_DOUBLE)
>> looks sensible as far of fixing a regression (I assume the diff to the
>> dwarf results in essentially the
>> same DWARF as what was present before wide-int).
>
> Yes, the dwarf is the same.
>
> Index: dwarf2out.c
> ===================================================================
> --- dwarf2out.c (revision 229720)
> +++ dwarf2out.c (working copy)
> @@ -15593,8 +15593,15 @@
>        return true;
>
>      case CONST_WIDE_INT:
> -      add_AT_wide (die, DW_AT_const_value,
> -                  std::make_pair (rtl, GET_MODE (rtl)));
> +      {
> +       machine_mode mode = GET_MODE (rtl);
> +       if (mode == VOIDmode)
> +         mode = mode_for_size (CONST_WIDE_INT_NUNITS (rtl)
> +                               * HOST_BITS_PER_WIDE_INT,
> +                               MODE_INT, 0);
> +       add_AT_wide (die, DW_AT_const_value,
> +                    std::make_pair (rtl, mode));

I wonder if we'll manage to to get mode_for_size return BLKmode
in case of an original mode that was not of a size multiple of
HOST_BITS_PER_WIDE_INT (and that's host dependent even...).

We probably should use smallest_mode_for_size on a precision
derived from the value (ignore leading ones and zeros or so, exact
details need to be figured out).  Eventually hide this detail in a
smallest_mode_for_const_wide_int () helper.

> +      }
>        return true;
>
>      case CONST_DOUBLE:
> Index: rtl.h
> ===================================================================
> --- rtl.h       (revision 229720)
> +++ rtl.h       (working copy)
> @@ -2086,6 +2086,7 @@
>  inline unsigned int
>  wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
>  {
> +  gcc_assert (x.second != BLKmode && x.second != VOIDmode);

Please use gcc_checking_assert here.

Richard.

>    return GET_MODE_PRECISION (x.second);
>  }
>
>

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 4, 2015, at 4:15 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> I wonder if we'll manage to to get mode_for_size return BLKmode
> in case of an original mode that was not of a size multiple of
> HOST_BITS_PER_WIDE_INT (and that's host dependent even…).

> We probably should use smallest_mode_for_size on a precision
> derived from the value

Once we want to go stomping around the value, we might as well just do everything.  I prefer this version over one that has a call to assert.  I thought about creating a helper function, but since there is only 1 client for it, and since it was only 4 lines, I didn’t create one.  I’d propose deferring creation until we have more clients.  The generated dwarf remains fixed, as expected.

> Please use gcc_checking_assert here.

Done.  My test suite run with the assert did finish, and on x86_64 linux, it was never hit.

Any other issues you can spot?


Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 229720)
+++ dwarf2out.c	(working copy)
@@ -15593,8 +15593,13 @@
       return true;
 
     case CONST_WIDE_INT:
-      add_AT_wide (die, DW_AT_const_value,
-		   std::make_pair (rtl, GET_MODE (rtl)));
+      {
+	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
+	int prec = MIN (wi::min_precision (w1, UNSIGNED),
+			(unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
+	wide_int w = wide_int::from (w1, prec, UNSIGNED);
+	add_AT_wide (die, DW_AT_const_value, w);
+      }
       return true;
 
     case CONST_DOUBLE:
Index: rtl.h
===================================================================
--- rtl.h	(revision 229720)
+++ rtl.h	(working copy)
@@ -2086,6 +2086,7 @@
 inline unsigned int
 wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
 {
+  gcc_checking_assert (x.second != BLKmode && x.second != VOIDmode);
   return GET_MODE_PRECISION (x.second);
 }
 

Re: [ping] Fix PR debug/66728

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> Index: dwarf2out.c
> ===================================================================
> --- dwarf2out.c	(revision 229720)
> +++ dwarf2out.c	(working copy)
> @@ -15593,8 +15593,13 @@
>        return true;
>  
>      case CONST_WIDE_INT:
> -      add_AT_wide (die, DW_AT_const_value,
> -		   std::make_pair (rtl, GET_MODE (rtl)));
> +      {
> +	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
> +	int prec = MIN (wi::min_precision (w1, UNSIGNED),
> +			(unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
> +	wide_int w = wide_int::from (w1, prec, UNSIGNED);
> +	add_AT_wide (die, DW_AT_const_value, w);
> +      }
>        return true;
>  
>      case CONST_DOUBLE:

Setting the precision based on CONST_WIDE_INT_NUNITS means that
we might end up with two different precisions for two values of
the same variable.  E.g. for a 192-bit type, 1<<64 would be given
a precision of 128 (because it needs two HWIs to store) but 1<<128
would be given a precision of 192 (because it needs three HWIs to store).
We could then abort when comparing them for equality, since equality
needs both integers to have the same precision.  E.g. from same_dw_val_p:

    case dw_val_class_wide_int:
      return *v1->v.val_wide == *v2->v.val_wide;

Thanks,
Richard

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 4, 2015, at 12:50 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:

> Mike Stump <mikestump@comcast.net> writes:
>> Index: dwarf2out.c
>> ===================================================================
>> --- dwarf2out.c	(revision 229720)
>> +++ dwarf2out.c	(working copy)
>> @@ -15593,8 +15593,13 @@
>>       return true;
>> 
>>     case CONST_WIDE_INT:
>> -      add_AT_wide (die, DW_AT_const_value,
>> -		   std::make_pair (rtl, GET_MODE (rtl)));
>> +      {
>> +	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
>> +	int prec = MIN (wi::min_precision (w1, UNSIGNED),
>> +			(unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
>> +	wide_int w = wide_int::from (w1, prec, UNSIGNED);
>> +	add_AT_wide (die, DW_AT_const_value, w);
>> +      }
>>       return true;
>> 
>>     case CONST_DOUBLE:
> 
> Setting the precision based on CONST_WIDE_INT_NUNITS means that
> we might end up with two different precisions for two values of
> the same variable.  E.g. for a 192-bit type, 1<<64 would be given
> a precision of 128 (because it needs two HWIs to store) but 1<<128
> would be given a precision of 192 (because it needs three HWIs to store).
> We could then abort when comparing them for equality, since equality
> needs both integers to have the same precision.  E.g. from same_dw_val_p:
> 
>    case dw_val_class_wide_int:
>      return *v1->v.val_wide == *v2->v.val_wide;

Yeah, seems like we should have a v1.prec == v2.prec && on that.  The bad news, there are four of them that are like this.  The good news, 3 of them are location operands, and I don’t think they can hit for a very long time.  I think this is an oversight from the double_int version of the code where we just check the 128 bits for equality.  We can see if Richard wants to weigh in.  I think I’d just pre-approve the change, though, I think a helper to perform mixed equality testing would be the way to go as there are 4 of them, and I pretty sure they should all use the mixed version.  Though, maybe the location list versions are never mixed.  If they aren’t, then there is only 1 client, so, I’d just do the precision test inline.  Anyone able to comment on the location list aspect of this?

Re: [ping] Fix PR debug/66728

[Richard Biener]

On Thu, Nov 5, 2015 at 12:45 AM, Mike Stump <mikestump@comcast.net> wrote:
>
> On Nov 4, 2015, at 12:50 PM, Richard Sandiford <richard.sandiford@arm.com> wrote:
>
>> Mike Stump <mikestump@comcast.net> writes:
>>> Index: dwarf2out.c
>>> ===================================================================
>>> --- dwarf2out.c      (revision 229720)
>>> +++ dwarf2out.c      (working copy)
>>> @@ -15593,8 +15593,13 @@
>>>       return true;
>>>
>>>     case CONST_WIDE_INT:
>>> -      add_AT_wide (die, DW_AT_const_value,
>>> -               std::make_pair (rtl, GET_MODE (rtl)));
>>> +      {
>>> +    wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
>>> +    int prec = MIN (wi::min_precision (w1, UNSIGNED),
>>> +                    (unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
>>> +    wide_int w = wide_int::from (w1, prec, UNSIGNED);
>>> +    add_AT_wide (die, DW_AT_const_value, w);
>>> +      }
>>>       return true;
>>>
>>>     case CONST_DOUBLE:
>>
>> Setting the precision based on CONST_WIDE_INT_NUNITS means that
>> we might end up with two different precisions for two values of
>> the same variable.  E.g. for a 192-bit type, 1<<64 would be given
>> a precision of 128 (because it needs two HWIs to store) but 1<<128
>> would be given a precision of 192 (because it needs three HWIs to store).
>> We could then abort when comparing them for equality, since equality
>> needs both integers to have the same precision.  E.g. from same_dw_val_p:
>>
>>    case dw_val_class_wide_int:
>>      return *v1->v.val_wide == *v2->v.val_wide;
>
> Yeah, seems like we should have a v1.prec == v2.prec && on that.  The bad news, there are four of them that are like this.  The good news, 3 of them are location operands, and I don’t think they can hit for a very long time.  I think this is an oversight from the double_int version of the code where we just check the 128 bits for equality.  We can see if Richard wants to weigh in.  I think I’d just pre-approve the change, though, I think a helper to perform mixed equality testing would be the way to go as there are 4 of them, and I pretty sure they should all use the mixed version.  Though, maybe the location list versions are never mixed.  If they aren’t, then there is only 1 client, so, I’d just do the precision test inline.  Anyone able to comment on the location list aspect of this?

No idea on location lists but maybe this means we should just use the
maximum supported integer mode for CONST_WIDE_INTs?

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 5, 2015, at 4:32 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> No idea on location lists but maybe this means we should just use the
> maximum supported integer mode for CONST_WIDE_INTs?

Ah, yeah, that sounds like a fine idea.  Below is that version.  I snuck in one more change, as it was annoying me, and it is a regression from gcc-4.8.  It has this effect:

@@ -55,7 +55,7 @@ test:
        .long   0x72    # DW_AT_type
        .byte   0x10
        .quad   0       # DW_AT_const_value
-       .quad   0x8000000000000000      # (null)
+       .quad   0x8000000000000000      # 
        .byte   0       # end of children of DIE 0x2d
        .uleb128 0x4    # (DIE (0x6b) DW_TAG_base_type)
        .byte   0x10    # DW_AT_byte_size

This version has the added benefit of reducing all wide_ints to be so shortened.  We do this by changing get_full_len, which changes the world.

If there are no substantial reasons to not check it in now, I’d like to proceed and get it checked in.  People can refine it further in tree if they want.  Any objections?


Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 229720)
+++ dwarf2out.c	(working copy)
@@ -368,12 +368,14 @@
 #endif
 
 /* Get the number of HOST_WIDE_INTs needed to represent the precision
-   of the number.  */
+   of the number.  Some constants have a large uniform precision, so
+   we get the precision needed for the actual value of the number.  */
 
 static unsigned int
 get_full_len (const wide_int &op)
 {
-  return ((op.get_precision () + HOST_BITS_PER_WIDE_INT - 1)
+  int prec = wi::min_precision (op, UNSIGNED);
+  return ((prec + HOST_BITS_PER_WIDE_INT - 1)
 	  / HOST_BITS_PER_WIDE_INT);
 }
 
@@ -9010,7 +9012,7 @@
 		{
 		  dw2_asm_output_data (l, a->dw_attr_val.v.val_wide->elt (i),
 				       "%s", name);
-		  name = NULL;
+		  name = "";
 		}
 	    else
 	      for (i = 0; i < len; ++i)
@@ -9017,7 +9019,7 @@
 		{
 		  dw2_asm_output_data (l, a->dw_attr_val.v.val_wide->elt (i),
 				       "%s", name);
-		  name = NULL;
+		  name = "";
 		}
 	  }
 	  break;
@@ -15593,8 +15595,13 @@
       return true;
 
     case CONST_WIDE_INT:
-      add_AT_wide (die, DW_AT_const_value,
-		   std::make_pair (rtl, GET_MODE (rtl)));
+      {
+	wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
+	unsigned int prec = MIN (wi::min_precision (w1, UNSIGNED),
+				 (unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
+	wide_int w = wi::zext (w1, prec);
+	add_AT_wide (die, DW_AT_const_value, w);
+      }
       return true;
 
     case CONST_DOUBLE:
Index: rtl.h
===================================================================
--- rtl.h	(revision 229720)
+++ rtl.h	(working copy)
@@ -2086,6 +2086,7 @@
 inline unsigned int
 wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
 {
+  gcc_checking_assert (x.second != BLKmode && x.second != VOIDmode);
   return GET_MODE_PRECISION (x.second);
 }
 

Re: [ping] Fix PR debug/66728

[Richard Biener]

On Fri, Nov 6, 2015 at 2:34 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 5, 2015, at 4:32 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> No idea on location lists but maybe this means we should just use the
>> maximum supported integer mode for CONST_WIDE_INTs?
>
> Ah, yeah, that sounds like a fine idea.  Below is that version.  I snuck in one more change, as it was annoying me, and it is a regression from gcc-4.8.  It has this effect:
>
> @@ -55,7 +55,7 @@ test:
>         .long   0x72    # DW_AT_type
>         .byte   0x10
>         .quad   0       # DW_AT_const_value
> -       .quad   0x8000000000000000      # (null)
> +       .quad   0x8000000000000000      #
>         .byte   0       # end of children of DIE 0x2d
>         .uleb128 0x4    # (DIE (0x6b) DW_TAG_base_type)
>         .byte   0x10    # DW_AT_byte_size
>
> This version has the added benefit of reducing all wide_ints to be so shortened.  We do this by changing get_full_len, which changes the world.
>
> If there are no substantial reasons to not check it in now, I’d like to proceed and get it checked in.  People can refine it further in tree if they want.  Any objections?

Ok with a changelog entry and bootstrap/regtest.

Thanks,
Richard.

>
> Index: dwarf2out.c
> ===================================================================
> --- dwarf2out.c (revision 229720)
> +++ dwarf2out.c (working copy)
> @@ -368,12 +368,14 @@
>  #endif
>
>  /* Get the number of HOST_WIDE_INTs needed to represent the precision
> -   of the number.  */
> +   of the number.  Some constants have a large uniform precision, so
> +   we get the precision needed for the actual value of the number.  */
>
>  static unsigned int
>  get_full_len (const wide_int &op)
>  {
> -  return ((op.get_precision () + HOST_BITS_PER_WIDE_INT - 1)
> +  int prec = wi::min_precision (op, UNSIGNED);
> +  return ((prec + HOST_BITS_PER_WIDE_INT - 1)
>           / HOST_BITS_PER_WIDE_INT);
>  }
>
> @@ -9010,7 +9012,7 @@
>                 {
>                   dw2_asm_output_data (l, a->dw_attr_val.v.val_wide->elt (i),
>                                        "%s", name);
> -                 name = NULL;
> +                 name = "";
>                 }
>             else
>               for (i = 0; i < len; ++i)
> @@ -9017,7 +9019,7 @@
>                 {
>                   dw2_asm_output_data (l, a->dw_attr_val.v.val_wide->elt (i),
>                                        "%s", name);
> -                 name = NULL;
> +                 name = "";
>                 }
>           }
>           break;
> @@ -15593,8 +15595,13 @@
>        return true;
>
>      case CONST_WIDE_INT:
> -      add_AT_wide (die, DW_AT_const_value,
> -                  std::make_pair (rtl, GET_MODE (rtl)));
> +      {
> +       wide_int w1 = std::make_pair (rtl, MAX_MODE_INT);
> +       unsigned int prec = MIN (wi::min_precision (w1, UNSIGNED),
> +                                (unsigned int)CONST_WIDE_INT_NUNITS (rtl) * HOST_BITS_PER_WIDE_INT);
> +       wide_int w = wi::zext (w1, prec);
> +       add_AT_wide (die, DW_AT_const_value, w);
> +      }
>        return true;
>
>      case CONST_DOUBLE:
> Index: rtl.h
> ===================================================================
> --- rtl.h       (revision 229720)
> +++ rtl.h       (working copy)
> @@ -2086,6 +2086,7 @@
>  inline unsigned int
>  wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x)
>  {
> +  gcc_checking_assert (x.second != BLKmode && x.second != VOIDmode);
>    return GET_MODE_PRECISION (x.second);
>  }
>

Re: [ping] Fix PR debug/66728

[Mike Stump]

On Nov 6, 2015, at 5:06 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> If there are no substantial reasons to not check it in now, I’d like to proceed and get it checked in.  People can refine it further in tree if they want.  Any objections?
> 
> Ok with a changelog entry and bootstrap/regtest.

Also committed to the release branch after waiting a few days to ensure no issue on trunk after the normal regression test and bootstrap.