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 <ja...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
