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 01:59:27PM +0100, Richard Biener wrote:
> > >      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.
> 
> Oops, that is obviously a bug.  For non-integral mode or mode with
> precision not a multiple of 64 I meant to punt.  The reason for the latter
> is that if precision is something weird like 123 bits, then the printing
> code will print the whole 64-bit elt which would be wrong for at least
> unsigned numbers with MSB set.  I guess such modes are still very rare to
> non-existing.

Agreed.  The patch looks good to me now.

Thanks,
Richard.

> 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 14:06:22.750387774 +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,15 +20099,15 @@ 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);
> -      }
> -      return true;
> +      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);
> +	  return true;
> +	}
> +      return false;
>  
>      case CONST_DOUBLE:
>        /* Note that a CONST_DOUBLE rtx could represent either an integer or a
> @@ -20190,7 +20192,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 +20705,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 +20726,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 +20801,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)