Re: [PATCH] dwarf2out: Fix up CONST_WIDE_INT handling once more [PR103046]

[Richard Biener <rguenther@suse.de>]

On Wed, 3 Nov 2021, Jakub Jelinek wrote:

> On Wed, Nov 03, 2021 at 09:35:38AM +0100, Jakub Jelinek wrote:
> > Though, thinking more about it, maybe better would be to actually pass
> > machine_mode to add_const_value_attribute from callers, so that we know
> > exactly what precision we want.  Because hypothetically, if say mode is
> > OImode and the CONST_WIDE_INT value fits into 128 bits or 192 bits,
> > we'd emit just those 128 or 192 bits but debug info users would expect
> > 256 bits.
> 
> The other option would be something like untested patch below.
> On
> typedef unsigned __int128 U;
> 
> int
> main ()
> {
>   U a = (U) 1 << 120;
>   U b = 0xffffffffffffffffULL;
>   U c = ((U) 0xffffffff00000000ULL) << 64;
>   return 0;
> }
> vanilla gcc incorrectly emits 0, 0xff00000000000000 for a,
> 0xffffffffffffffff alone (DW_FORM_data8) for b and 0, 0xffffffff00000000
> for c.  gcc with the previously posted PR103046 patch emits
> 0, 0x0100000000000000 for a, 0xffffffffffffffff alone for b and
> 0, 0xffffffff00000000 for c.  And with this patch we emit
> 0, 0x0100000000000000 for a, 0xffffffffffffffff, 0 for b and
> 0, 0xffffffff00000000 for c.
> So, the patch below certainly causes larger debug info (well, 128-bit
> integers are pretty rare), but in this case the question is if it isn't
> more correct, as debug info consumers generally will not know if they
> should sign or zero extend the value in DW_AT_const_value.
> The previous code assumes they will always zero extend it...

So I think the sign-/zero-extension shoud be handled by
->get_len and the extra zero element we eventually get in the
wide_int representation.

I do like passing down the mode, but ..

> 2021-11-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/103046
> 	* dwarf2out.c (add_const_value_attribute): Add MODE argument, use it
> 	in CONST_WIDE_INT handling.  Adjust recursive calls.
> 	(add_location_or_const_value_attribute): Pass DECL_MODE (decl) to
> 	new add_const_value_attribute argument.
> 	(tree_add_const_value_attribute): Pass TYPE_MODE (type) to new
> 	add_const_value_attribute argument.
> 
> --- gcc/dwarf2out.c.jj	2021-10-08 10:52:47.086531820 +0200
> +++ gcc/dwarf2out.c	2021-11-03 09:45:52.160841764 +0100
> @@ -3854,7 +3854,7 @@ static void add_AT_location_description
>  					 dw_loc_list_ref);
>  static void add_data_member_location_attribute (dw_die_ref, tree,
>  						struct vlr_context *);
> -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 unsigned insert_float (const_rtx, unsigned char *);
> @@ -20081,8 +20081,10 @@ insert_float (const_rtx rtl, unsigned ch
>     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)
>  {
> +  scalar_mode int_mode;
> +
>    switch (GET_CODE (rtl))
>      {
>      case CONST_INT:
> @@ -20097,14 +20099,13 @@ add_const_value_attribute (dw_die_ref di
>        return true;
>  
>      case CONST_WIDE_INT:
> -      {
> -	wide_int w1 = rtx_mode_t (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 = wide_int::from (w1, prec, UNSIGNED);
> -	add_AT_wide (die, DW_AT_const_value, w);
> -      }
> +      if (is_int_mode (mode, &int_mode)
> +	  && (GET_MODE_PRECISION (int_mode)
> +	      & (HOST_BITS_PER_WIDE_INT - 1)) == 0)
> +	{
> +	  wide_int w = rtx_mode_t (rtl, int_mode);
> +	  add_AT_wide (die, DW_AT_const_value, w);
> +	}

... this at least deserves a comment why we return true doing
nothing when the mode precision isn't a multiple of 64.  The other
patch simply extended things.

>        return true;
>  
>      case CONST_DOUBLE:
> @@ -20190,7 +20191,7 @@ add_const_value_attribute (dw_die_ref di
>  
>      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))
> @@ -20703,7 +20704,7 @@ add_location_or_const_value_attribute (d
>  
>    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
> @@ -20724,7 +20725,7 @@ add_location_or_const_value_attribute (d
>        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
> @@ -20799,7 +20800,7 @@ tree_add_const_value_attribute (dw_die_r
>       symbols.  */
>    rtl = rtl_for_decl_init (init, type);
>    if (rtl && !early_dwarf)
> -    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.  */
>    if (CHAR_BIT == 8 && BITS_PER_UNIT == 8
>        && initializer_constant_valid_p (init, type))
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)