Re: [wide-int] Handle zero-precision INTEGER_CSTs again

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, May 5, 2014 at 10:58 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Mon, May 5, 2014 at 4:51 PM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> On Mon, May 5, 2014 at 12:54 PM, Richard Sandiford
>>>> <rdsandiford@googlemail.com> wrote:
>>>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>>>> On Fri, May 2, 2014 at 9:19 PM, Richard Sandiford
>>>>>> <rdsandiford@googlemail.com> wrote:
>>>>>>> I'd hoped the days of zero-precision INTEGER_CSTs were behind us after
>>>>>>> Richard's patch to remove min amd max values from zero-width bitfields,
>>>>>>> but a boostrap-ubsan showed otherwise.  One source is in:
>>>>>>>
>>>>>>>   null_pointer_node = build_int_cst (build_pointer_type
>>>>>>> (void_type_node), 0);
>>>>>>>
>>>>>>> if no_target, since the precision of the type comes from ptr_mode,
>>>>>>> which remains the default VOIDmode.  Maybe that's a bug, but setting
>>>>>>> it to an arbitrary nonzero value would also be dangerous.
>>>>>>
>>>>>> Can you explain what 'no_target' should be?  ptr_mode should never be
>>>>>> VOIDmode.
>>>>>
>>>>> Sorry, meant "no_backend" rather than "no_target".  See do_compile.
>>>>
>>>> Ok.  So we do
>>>>
>>>>       /* This must be run always, because it is needed to compute the FP
>>>>          predefined macros, such as __LDBL_MAX__, for targets using non
>>>>          default FP formats.  */
>>>>       init_adjust_machine_modes ();
>>>>
>>>>       /* Set up the back-end if requested.  */
>>>>       if (!no_backend)
>>>>         backend_init ();
>>>>
>>>> where I think that init_adjust_machine_modes should initialize the
>>>> {byte,word,ptr}_mode globals.  Move from init_emit_once.
>
> init_adjust_machine_modes is an auto-generated function so I ended up
> using a new function instead.  Tested on x86_64-linux-gnu.  OK to install?

Ok with also setting double_mode there.

>>>>>> So I don't think we want this patch.  Instead stor-layout should
>>>>>> ICE on zero-precision integer/pointer types.
>>>>>
>>>>> What should happen for void_zero_node?
>>>>
>>>> Not sure what that beast is supposed to be or why it should be
>>>> of INTEGER_CST kind (it's not even initialized in any meaningful
>>>> way).
>>>>
>>>> That said, the wide-int code shouldn't be slowed down by
>>>> precision == 0 checks.  We should never ever reach wide-int
>>>> with such a constant.
>>>
>>> void_zero_node is used for ubsan too, and survives into gimple.
>>> I did hit this in real tests, it wasn't just theoretical.
>>
>> Ugh - for what does it use that ... :/
>>
>> Please remember how to trigger those issues and I'll happily have
>> a look after the merge.
>
> At the time it was just a normal bootstrap-ubsan, but that was
> before the zero-precision patch.  Probably the best way of
> checking for zero-precision tree constants is to put an assert
> for nonzero precisions in:
>
> inline unsigned int
> wi::int_traits <const_tree>::get_precision (const_tree tcst)
> {
>   return TYPE_PRECISION (TREE_TYPE (tcst));
> }
>
> and:
>
> template <int N>
> inline wi::extended_tree <N>::extended_tree (const_tree t)
>   : m_t (t)
> {
>   gcc_checking_assert (TYPE_PRECISION (TREE_TYPE (t)) <= N);
> }
>
> and then bootstrap-ubsan.  But like Marek says, the ubsan code uses
> void_zero_node by name.

I think for the uses it has it can just use NULL_TREE in place of
void_zero_node.

Richard.

> Thanks,
> Richard
>
>
> gcc/
>         * emit-rtl.c (init_derived_machine_modes): New functionm, split
>         out from...
>         (init_emit_once): ...here.
>         * rtl.h (init_derived_machine_modes): Declare.
>         * toplev.c (do_compile): Call it even if no_backend.
>
> Index: gcc/emit-rtl.c
> ===================================================================
> --- gcc/emit-rtl.c      2014-05-03 20:18:49.157107743 +0100
> +++ gcc/emit-rtl.c      2014-05-05 17:44:53.579038259 +0100
> @@ -5620,6 +5620,30 @@ init_emit_regs (void)
>      }
>  }
>
> +/* Initialize global machine_mode variables.  */
> +
> +void
> +init_derived_machine_modes (void)
> +{
> +  byte_mode = VOIDmode;
> +  word_mode = VOIDmode;
> +
> +  for (enum machine_mode mode = GET_CLASS_NARROWEST_MODE (MODE_INT);
> +       mode != VOIDmode;
> +       mode = GET_MODE_WIDER_MODE (mode))
> +    {
> +      if (GET_MODE_BITSIZE (mode) == BITS_PER_UNIT
> +         && byte_mode == VOIDmode)
> +       byte_mode = mode;
> +
> +      if (GET_MODE_BITSIZE (mode) == BITS_PER_WORD
> +         && word_mode == VOIDmode)
> +       word_mode = mode;
> +    }
> +
> +  ptr_mode = mode_for_size (POINTER_SIZE, GET_MODE_CLASS (Pmode), 0);
> +}
> +
>  /* Create some permanent unique rtl objects shared between all functions.  */
>
>  void
> @@ -5643,36 +5667,6 @@ init_emit_once (void)
>    reg_attrs_htab = htab_create_ggc (37, reg_attrs_htab_hash,
>                                     reg_attrs_htab_eq, NULL);
>
> -  /* Compute the word and byte modes.  */
> -
> -  byte_mode = VOIDmode;
> -  word_mode = VOIDmode;
> -  double_mode = VOIDmode;
> -
> -  for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT);
> -       mode != VOIDmode;
> -       mode = GET_MODE_WIDER_MODE (mode))
> -    {
> -      if (GET_MODE_BITSIZE (mode) == BITS_PER_UNIT
> -         && byte_mode == VOIDmode)
> -       byte_mode = mode;
> -
> -      if (GET_MODE_BITSIZE (mode) == BITS_PER_WORD
> -         && word_mode == VOIDmode)
> -       word_mode = mode;
> -    }
> -
> -  for (mode = GET_CLASS_NARROWEST_MODE (MODE_FLOAT);
> -       mode != VOIDmode;
> -       mode = GET_MODE_WIDER_MODE (mode))
> -    {
> -      if (GET_MODE_BITSIZE (mode) == DOUBLE_TYPE_SIZE
> -         && double_mode == VOIDmode)
> -       double_mode = mode;
> -    }
> -
> -  ptr_mode = mode_for_size (POINTER_SIZE, GET_MODE_CLASS (Pmode), 0);
> -
>  #ifdef INIT_EXPANDERS
>    /* This is to initialize {init|mark|free}_machine_status before the first
>       call to push_function_context_to.  This is needed by the Chill front
> @@ -5695,6 +5689,8 @@ init_emit_once (void)
>    else
>      const_true_rtx = gen_rtx_CONST_INT (VOIDmode, STORE_FLAG_VALUE);
>
> +  double_mode = mode_for_size (DOUBLE_TYPE_SIZE, MODE_FLOAT, 0);
> +
>    REAL_VALUE_FROM_INT (dconst0,   0,  0, double_mode);
>    REAL_VALUE_FROM_INT (dconst1,   1,  0, double_mode);
>    REAL_VALUE_FROM_INT (dconst2,   2,  0, double_mode);
> Index: gcc/rtl.h
> ===================================================================
> --- gcc/rtl.h   2014-05-03 20:18:49.157107743 +0100
> +++ gcc/rtl.h   2014-05-05 17:40:26.035785011 +0100
> @@ -2545,6 +2545,7 @@ extern int get_max_insn_count (void);
>  extern int in_sequence_p (void);
>  extern void init_emit (void);
>  extern void init_emit_regs (void);
> +extern void init_derived_machine_modes (void);
>  extern void init_emit_once (void);
>  extern void push_topmost_sequence (void);
>  extern void pop_topmost_sequence (void);
> Index: gcc/toplev.c
> ===================================================================
> --- gcc/toplev.c        2014-04-26 16:05:43.076775028 +0100
> +++ gcc/toplev.c        2014-05-05 17:41:00.168079259 +0100
> @@ -1891,6 +1891,7 @@ do_compile (void)
>          predefined macros, such as __LDBL_MAX__, for targets using non
>          default FP formats.  */
>        init_adjust_machine_modes ();
> +      init_derived_machine_modes ();
>
>        /* Set up the back-end if requested.  */
>        if (!no_backend)