[ping] Re: Fix PR debug/66728

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.

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?

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

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

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