This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?

>>>>> 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.

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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]