| Message ID | 87bnvcx7rz.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
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)
Richard Biener <richard.guenther@gmail.com> writes: > 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. double_mode is just a local variable though, it's only used to set up dconstN. Thanks, Richard
On Tue, May 6, 2014 at 10:11 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> 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. > > double_mode is just a local variable though, it's only used to set up > dconstN. Ah, I see. Fine then as-is. Thanks, Richard. > Thanks, > Richard
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)