From: Richard Biener <richard.guenther@gmail.com>
To: Richard Biener <richard.guenther@gmail.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Kenneth Zadeck <zadeck@naturalbridge.com>,
Mike Stump <mikestump@comcast.net>,
Richard Sandiford <rdsandiford@googlemail.com>
Subject: Re: [wide-int] Handle zero-precision INTEGER_CSTs again
Date: Tue, 06 May 2014 08:08:00 -0000 [thread overview]
Message-ID: <CAFiYyc293eHVXOFephJhrwXRNu10WQ7_jZkL3hKxp8r8X-FLNA@mail.gmail.com> (raw)
In-Reply-To: <87bnvcx7rz.fsf@talisman.default>
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)
next prev parent reply other threads:[~2014-05-06 8:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-02 19:19 Richard Sandiford
2014-05-03 16:42 ` Kenneth Zadeck
2014-05-03 18:59 ` Richard Sandiford
2014-05-03 19:53 ` Kenneth Zadeck
2014-05-04 19:56 ` Richard Sandiford
2014-05-05 8:31 ` Richard Biener
2014-05-05 10:54 ` Richard Sandiford
2014-05-05 12:16 ` Richard Biener
2014-05-05 14:51 ` Richard Sandiford
2014-05-05 17:32 ` Richard Biener
2014-05-05 18:04 ` Marek Polacek
2014-05-05 20:58 ` Richard Sandiford
2014-05-06 8:08 ` Richard Biener [this message]
2014-05-06 8:11 ` Richard Sandiford
2014-05-06 8:20 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc293eHVXOFephJhrwXRNu10WQ7_jZkL3hKxp8r8X-FLNA@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=mikestump@comcast.net \
--cc=rdsandiford@googlemail.com \
--cc=zadeck@naturalbridge.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).