Patchwork Fix up passing long long in ubsan with -m32 (PR sanitizer/59333)

login
register
mail settings
Submitter Marek Polacek
Date Dec. 5, 2013, 5:14 p.m.
Message ID <20131205171403.GE11710@redhat.com>
Download mbox | patch
Permalink /patch/297209/
State New
Headers show

Comments

Marek Polacek - Dec. 5, 2013, 5:14 p.m.
On Thu, Dec 05, 2013 at 04:44:29PM +0100, Jakub Jelinek wrote:
> On Thu, Dec 05, 2013 at 04:31:20PM +0100, Marek Polacek wrote:
> > > > +	    }
> > > > +	  t = build_fold_addr_expr (var);
> > > > +	  return build2 (COMPOUND_EXPR, TREE_TYPE (t), tem, t);
> > > 
> > > I would expect you want to use this too even if in_expand_p...
> > 
> > Unfortunately, this won't be sufficient -- in expand_expr_real_1 we
> > have 
> >     case COMPOUND_EXPR:
> >       /* Lowered by gimplify.c.  */
> >       gcc_unreachable ();
> > and with the code above we hit that.
> 
> Ah, ok, then perhaps you need to do expand_assignment (var, t, false);
> there and then just build_fold_addr_expr.  The ugly thing is that those
> assignments will be expanded there right away, so you want to move
> the
>   fn = ubsan_build_overflow_builtin (code, gimple_location (stmt),
>                                      TREE_TYPE (arg0), arg0, arg1);
> lines down right above expand_normal (fn);
> Also, you want push_temp_slots (); and pop_temp_slots (); around it.
> So
>   push_temp_slots ();
>   fn = ubsan_build_overflow_builtin (code, gimple_location (stmt),
>                                      TREE_TYPE (arg0), arg0, arg1);
>   expand_normal (fn);
>   pop_temp_slots ();
> That way if you have say multiple assign_stack_temp_for_type slots
> in one spot for one diagnostics, those slots can be reused again
> in another call.

Thanks, that seems to work.  The version below fixes PR59397 reported today 
as well -- we weren't handling ENUMERAL_TYPE in the ubsan_encode_value.

Bootstrapped, ran ubsan testsuite on x86_64-unknown-linux-gnu.
Ok for trunk?

2013-12-05  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/59333
	PR sanitizer/59397
	* ubsan.c: Include rtl.h and expr.h.
	(ubsan_encode_value): Add new parameter.  If expanding, assign
	a stack slot for DECL_RTL of the temporary and call expand_assignment.
	Handle BOOLEAN_TYPE and ENUMERAL_TYPE.
	(ubsan_build_overflow_builtin): Adjust ubsan_encode_value call.
	* ubsan.h (ubsan_encode_value): Adjust declaration.
	* internal-fn.c (ubsan_expand_si_overflow_addsub_check): Move
	ubsan_build_overflow_builtin above expand_normal call.  Surround this call
	with push_temp_slots and pop_temp_slots.
	(ubsan_expand_si_overflow_neg_check): Likewise.
	(ubsan_expand_si_overflow_mul_check): Likewise.
testsuite/
	* c-c++-common/ubsan/pr59333.c: New test.
	* c-c++-common/ubsan/pr59397.c: New test.


	Marek
Jakub Jelinek - Dec. 5, 2013, 5:22 p.m.
On Thu, Dec 05, 2013 at 06:14:03PM +0100, Marek Polacek wrote:
> 2013-12-05  Marek Polacek  <polacek@redhat.com>
> 
> 	PR sanitizer/59333
> 	PR sanitizer/59397
> 	* ubsan.c: Include rtl.h and expr.h.
> 	(ubsan_encode_value): Add new parameter.  If expanding, assign
> 	a stack slot for DECL_RTL of the temporary and call expand_assignment.
> 	Handle BOOLEAN_TYPE and ENUMERAL_TYPE.
> 	(ubsan_build_overflow_builtin): Adjust ubsan_encode_value call.
> 	* ubsan.h (ubsan_encode_value): Adjust declaration.
> 	* internal-fn.c (ubsan_expand_si_overflow_addsub_check): Move
> 	ubsan_build_overflow_builtin above expand_normal call.  Surround this call
> 	with push_temp_slots and pop_temp_slots.
> 	(ubsan_expand_si_overflow_neg_check): Likewise.
> 	(ubsan_expand_si_overflow_mul_check): Likewise.
> testsuite/
> 	* c-c++-common/ubsan/pr59333.c: New test.
> 	* c-c++-common/ubsan/pr59397.c: New test.

Ok, thanks.

> --- gcc/testsuite/c-c++-common/ubsan/pr59333.c.mp	2013-12-05 11:30:36.984759390 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/pr59333.c	2013-12-05 17:28:07.203743004 +0100
> @@ -0,0 +1,19 @@
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=undefined" } */
> +/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */

As a follow-up, please see if all the dg-skip-if -flto
ubsan markings can be removed now.

	Jakub
Marek Polacek - Dec. 5, 2013, 5:55 p.m.
On Thu, Dec 05, 2013 at 06:22:02PM +0100, Jakub Jelinek wrote:
> As a follow-up, please see if all the dg-skip-if -flto
> ubsan markings can be removed now.

Unfortunately, not yet.  With -flto, we fail with
cclAoIBG.o:(.text+0x14): undefined reference to `.Lubsan_data0.2616'^M
collect2: error: ld returned 1 exit status^M
etc.

	Marek

Patch

--- gcc/ubsan.h.mp	2013-12-05 11:25:18.979469651 +0100
+++ gcc/ubsan.h	2013-12-05 12:53:30.970741286 +0100
@@ -41,7 +41,7 @@  extern tree ubsan_instrument_unreachable
 extern tree ubsan_create_data (const char *, location_t,
 			       const struct ubsan_mismatch_data *, ...);
 extern tree ubsan_type_descriptor (tree, bool);
-extern tree ubsan_encode_value (tree);
+extern tree ubsan_encode_value (tree, bool = false);
 extern bool is_ubsan_builtin_p (tree);
 extern tree ubsan_build_overflow_builtin (tree_code, location_t, tree, tree, tree);
 
--- gcc/ubsan.c.mp	2013-12-05 09:40:39.676771966 +0100
+++ gcc/ubsan.c	2013-12-05 17:47:35.409077317 +0100
@@ -27,6 +27,7 @@  along with GCC; see the file COPYING3.
 #include "cgraph.h"
 #include "tree-pass.h"
 #include "tree-ssa-alias.h"
+#include "tree-pretty-print.h"
 #include "internal-fn.h"
 #include "gimple-expr.h"
 #include "gimple.h"
@@ -40,6 +41,8 @@  along with GCC; see the file COPYING3.
 #include "cfgloop.h"
 #include "ubsan.h"
 #include "c-family/c-common.h"
+#include "rtl.h"
+#include "expr.h"
 
 /* Map from a tree to a VAR_DECL tree.  */
 
@@ -102,45 +105,53 @@  decl_for_type_insert (tree type, tree de
 
 /* Helper routine, which encodes a value in the pointer_sized_int_node.
    Arguments with precision <= POINTER_SIZE are passed directly,
-   the rest is passed by reference.  T is a value we are to encode.  */
+   the rest is passed by reference.  T is a value we are to encode.
+   IN_EXPAND_P is true if this function is called during expansion.  */
 
 tree
-ubsan_encode_value (tree t)
+ubsan_encode_value (tree t, bool in_expand_p)
 {
   tree type = TREE_TYPE (t);
-  switch (TREE_CODE (type))
-    {
-    case INTEGER_TYPE:
-      if (TYPE_PRECISION (type) <= POINTER_SIZE)
+  const unsigned int bitsize = GET_MODE_BITSIZE (TYPE_MODE (type));
+  if (bitsize <= POINTER_SIZE)
+    switch (TREE_CODE (type))
+      {
+      case BOOLEAN_TYPE:
+      case ENUMERAL_TYPE:
+      case INTEGER_TYPE:
 	return fold_build1 (NOP_EXPR, pointer_sized_int_node, t);
+      case REAL_TYPE:
+	{
+	  tree itype = build_nonstandard_integer_type (bitsize, true);
+	  t = fold_build1 (VIEW_CONVERT_EXPR, itype, t);
+	  return fold_convert (pointer_sized_int_node, t);
+	}
+      default:
+	gcc_unreachable ();
+      }
+  else
+    {
+      if (!DECL_P (t) || !TREE_ADDRESSABLE (t))
+	{
+	  /* The reason for this is that we don't want to pessimize
+	     code by making vars unnecessarily addressable.  */
+	  tree var = create_tmp_var (type, NULL);
+	  tree tem = build2 (MODIFY_EXPR, void_type_node, var, t);
+	  if (in_expand_p)
+	    {
+	      rtx mem
+		= assign_stack_temp_for_type (TYPE_MODE (type),
+					      GET_MODE_SIZE (TYPE_MODE (type)),
+					      type);
+	      SET_DECL_RTL (var, mem);
+	      expand_assignment (var, t, false);
+	      return build_fold_addr_expr (var);
+	    }
+	  t = build_fold_addr_expr (var);
+	  return build2 (COMPOUND_EXPR, TREE_TYPE (t), tem, t);
+	}
       else
 	return build_fold_addr_expr (t);
-    case REAL_TYPE:
-      {
-	unsigned int bitsize = GET_MODE_BITSIZE (TYPE_MODE (type));
-	if (bitsize <= POINTER_SIZE)
-	  {
-	    tree itype = build_nonstandard_integer_type (bitsize, true);
-	    t = fold_build1 (VIEW_CONVERT_EXPR, itype, t);
-	    return fold_convert (pointer_sized_int_node, t);
-	  }
-	else
-	  {
-	    if (!TREE_ADDRESSABLE (t))
-	      {
-		/* The reason for this is that we don't want to pessimize
-		   code by making vars unnecessarily addressable.  */
-		tree var = create_tmp_var (TREE_TYPE (t), NULL);
-		tree tem = build2 (MODIFY_EXPR, void_type_node, var, t);
-		t = build_fold_addr_expr (var);
-		return build2 (COMPOUND_EXPR, TREE_TYPE (t), tem, t);
-	      }
-	    else
-	      return build_fold_addr_expr (t);
-	  }
-      }
-    default:
-      gcc_unreachable ();
     }
 }
 
@@ -663,8 +674,9 @@  ubsan_build_overflow_builtin (tree_code
   tree fn = builtin_decl_explicit (fn_code);
   return build_call_expr_loc (loc, fn, 2 + (code != NEGATE_EXPR),
 			      build_fold_addr_expr_loc (loc, data),
-			      ubsan_encode_value (op0),
-			      op1 ? ubsan_encode_value (op1) : NULL_TREE);
+			      ubsan_encode_value (op0, true),
+			      op1 ? ubsan_encode_value (op1, true)
+				  : NULL_TREE);
 }
 
 /* Perform the signed integer instrumentation.  GSI is the iterator
--- gcc/internal-fn.c.mp	2013-12-05 16:52:46.402154448 +0100
+++ gcc/internal-fn.c	2013-12-05 17:29:17.483997467 +0100
@@ -171,8 +171,6 @@  ubsan_expand_si_overflow_addsub_check (t
   arg1 = gimple_call_arg (stmt, 1);
   done_label = gen_label_rtx ();
   do_error = gen_label_rtx ();
-  fn = ubsan_build_overflow_builtin (code, gimple_location (stmt),
-				     TREE_TYPE (arg0), arg0, arg1);
   do_pending_stack_adjust ();
   op0 = expand_normal (arg0);
   op1 = expand_normal (arg1);
@@ -237,13 +235,17 @@  ubsan_expand_si_overflow_addsub_check (t
 			       PROB_VERY_LIKELY);
     }
 
-   emit_label (do_error);
-   /* Expand the ubsan builtin call.  */
-   expand_normal (fn);
-   do_pending_stack_adjust ();
+  emit_label (do_error);
+  /* Expand the ubsan builtin call.  */
+  push_temp_slots ();
+  fn = ubsan_build_overflow_builtin (code, gimple_location (stmt),
+				     TREE_TYPE (arg0), arg0, arg1);
+  expand_normal (fn);
+  pop_temp_slots ();
+  do_pending_stack_adjust ();
 
-   /* We're done.  */
-   emit_label (done_label);
+  /* We're done.  */
+  emit_label (done_label);
 
   if (lhs)
     emit_move_insn (target, res);
@@ -262,8 +264,6 @@  ubsan_expand_si_overflow_neg_check (gimp
   arg1 = gimple_call_arg (stmt, 1);
   done_label = gen_label_rtx ();
   do_error = gen_label_rtx ();
-  fn = ubsan_build_overflow_builtin (NEGATE_EXPR, gimple_location (stmt),
-				     TREE_TYPE (arg1), arg1, NULL_TREE);
 
   do_pending_stack_adjust ();
   op1 = expand_normal (arg1);
@@ -313,7 +313,11 @@  ubsan_expand_si_overflow_neg_check (gimp
 
   emit_label (do_error);
   /* Expand the ubsan builtin call.  */
+  push_temp_slots ();
+  fn = ubsan_build_overflow_builtin (NEGATE_EXPR, gimple_location (stmt),
+				     TREE_TYPE (arg1), arg1, NULL_TREE);
   expand_normal (fn);
+  pop_temp_slots ();
   do_pending_stack_adjust ();
 
   /* We're done.  */
@@ -337,8 +341,6 @@  ubsan_expand_si_overflow_mul_check (gimp
   arg1 = gimple_call_arg (stmt, 1);
   done_label = gen_label_rtx ();
   do_error = gen_label_rtx ();
-  fn = ubsan_build_overflow_builtin (MULT_EXPR, gimple_location (stmt),
-				     TREE_TYPE (arg0), arg0, arg1);
 
   do_pending_stack_adjust ();
   op0 = expand_normal (arg0);
@@ -418,7 +420,11 @@  ubsan_expand_si_overflow_mul_check (gimp
 
   emit_label (do_error);
   /* Expand the ubsan builtin call.  */
+  push_temp_slots ();
+  fn = ubsan_build_overflow_builtin (MULT_EXPR, gimple_location (stmt),
+				     TREE_TYPE (arg0), arg0, arg1);
   expand_normal (fn);
+  pop_temp_slots ();
   do_pending_stack_adjust ();
 
   /* We're done.  */
--- gcc/testsuite/c-c++-common/ubsan/pr59397.c.mp	2013-12-05 18:01:53.512193414 +0100
+++ gcc/testsuite/c-c++-common/ubsan/pr59397.c	2013-12-05 18:01:46.074166582 +0100
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=signed-integer-overflow" } */
+
+typedef enum E { A = -1 } e;
+int
+foo (void)
+{
+  e e = A;
+  return e + 1;
+}
--- gcc/testsuite/c-c++-common/ubsan/pr59333.c.mp	2013-12-05 11:30:36.984759390 +0100
+++ gcc/testsuite/c-c++-common/ubsan/pr59333.c	2013-12-05 17:28:07.203743004 +0100
@@ -0,0 +1,19 @@ 
+/* { dg-do run } */
+/* { dg-options "-fsanitize=undefined" } */
+/* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */
+
+long long int __attribute__ ((noinline, noclone))
+foo (long long int i, long long int j)
+{
+  asm ("");
+  return i + j;
+}
+
+int
+main (void)
+{
+  foo (2LL, __LONG_LONG_MAX__);
+  return 0;
+}
+
+/* { dg-output "signed integer overflow: 2 \\+ 9223372036854775807 cannot be represented in type 'long long int'(\n|\r\n|\r)" } */