On Mon, May 5, 2014 at 10:58 PM, Richard Sandiford <rdsandif...@googlemail.com> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On Mon, May 5, 2014 at 4:51 PM, Richard Sandiford >> <rdsandif...@googlemail.com> wrote: >>> Richard Biener <richard.guent...@gmail.com> writes: >>>> On Mon, May 5, 2014 at 12:54 PM, Richard Sandiford >>>> <rdsandif...@googlemail.com> wrote: >>>>> Richard Biener <richard.guent...@gmail.com> writes: >>>>>> On Fri, May 2, 2014 at 9:19 PM, Richard Sandiford >>>>>> <rdsandif...@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)