Patchwork Request to merge Undefined Behavior Sanitizer in (take 2)

login
register
mail settings
Submitter Marek Polacek
Date Aug. 15, 2013, 11:04 a.m.
Message ID <20130815110403.GY17022@redhat.com>
Download mbox | patch
Permalink /patch/267351/
State New
Headers show

Comments

Marek Polacek - Aug. 15, 2013, 11:04 a.m.
On Wed, Aug 07, 2013 at 04:58:03PM +0200, Marek Polacek wrote:
> I actually don't know what I prefer more, but if you like this version
> more, I'll go with it.  Maybe this is better because we don't have to
> create SAVE_EXPR and also we avoid one fold_build call.  Thanks,

Not creating the SAVE_EXPRs turned to be a no-go: firstly, the logic
when to create the SAVE_EXPRs and when not is hard-to-follow (depends
on C/C++, depends on various standards and it also depends on whether
the types are signed or not) - both for shift and division.  Moreover,
when we have nested expressions like e.g.

  (i << u) << x

if any of the operands is wrapped in SAVE_EXPR, we get an
-Wuninitialized warning.  So, what I did is to evaluate
the op0 always in the condition, like "if (op0, <cond>)",
which is safe and all the uninitialized warnings are gone,
now the bootstrap with -fsanitize=undefined finally finishes.
(albeit with 
Comparing stages 2 and 3 
warning: gcc/cc1plus-checksum.o differs
warning: gcc/cc1-checksum.o differs
Bootstrap comparison failure!
gcc/combine.o differs
gcc/tree-ssa-loop-ivopts.o differs
gcc/rtlanal.o differs
gcc/calls.o differs
gcc/emit-rtl.o differs
gcc/dbxout.o differs
gcc/function.o differs
gcc/cfgexpand.o differs
gcc/stor-layout.o differs
gcc/tree.o differs
gcc/tree-vect-generic.o differs
gcc/expr.o differs
gcc/fixed-value.o differs
gcc/fold-const.o differs
gcc/i386.o differs
libdecnumber/decimal64.o differs
libdecnumber/decNumber.o differs
make[2]: *** [compare] Error 1
).

Tested x86_64-pc-linux-gnu, applying to the ubsan branch.

2013-08-15  Marek Polacek  <polacek@redhat.com>

	* c-ubsan.c (ubsan_instrument_division): Evaluate SAVE_EXPRs
	before the condition.
	(ubsan_instrument_shift): Likewise.

	* c-c++-common/ubsan/save-expr-1.c: New test.
	* c-c++-common/ubsan/save-expr-2.c: New test.
	* c-c++-common/ubsan/save-expr-3.c: New test.
	* c-c++-common/ubsan/save-expr-4.c: New test.


	Marek
Jason Merrill - Aug. 15, 2013, 3:45 p.m.
On 08/15/2013 07:04 AM, Marek Polacek wrote:
> if any of the operands is wrapped in SAVE_EXPR, we get an
> -Wuninitialized warning.  So, what I did is to evaluate
> the op0 always in the condition, like "if (op0, <cond>)",
> which is safe and all the uninitialized warnings are gone,

That sounds fine.

> now the bootstrap with -fsanitize=undefined finally finishes.
> (albeit with
> Bootstrap comparison failure!

That's not so fine.  :)

Jason

Patch

--- gcc/c-family/c-ubsan.c.mp	2013-08-15 10:44:51.189966522 +0200
+++ gcc/c-family/c-ubsan.c	2013-08-15 12:43:30.454523049 +0200
@@ -73,6 +73,10 @@  ubsan_instrument_division (location_t lo
       x = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, x, tt);
       t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
     }
+
+  /* In case we have a SAVE_EXPR in a conditional context, we need to
+     make sure it gets evaluated before the condition.  */
+  t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
   tree data = ubsan_create_data ("__ubsan_overflow_data",
 				 loc, ubsan_type_descriptor (type),
 				 NULL_TREE);
@@ -133,6 +137,10 @@  ubsan_instrument_shift (location_t loc,
 		       build_int_cst (type0, 0));
       tt = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, x, tt);
     }
+
+  /* In case we have a SAVE_EXPR in a conditional context, we need to
+     make sure it gets evaluated before the condition.  */
+  t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
   tree data = ubsan_create_data ("__ubsan_shift_data",
 				 loc, ubsan_type_descriptor (type0),
 				 ubsan_type_descriptor (type1), NULL_TREE);
--- gcc/testsuite/c-c++-common/ubsan/save-expr-1.c.mp	2013-08-15 10:45:37.377057713 +0200
+++ gcc/testsuite/c-c++-common/ubsan/save-expr-1.c	2013-08-15 10:53:01.601695980 +0200
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */
+
+static int x;
+int
+main (void)
+{
+  int o = 1;
+  int y = x << o;
+  return y;
+}
--- gcc/testsuite/c-c++-common/ubsan/save-expr-2.c.mp	2013-08-15 10:58:37.505938910 +0200
+++ gcc/testsuite/c-c++-common/ubsan/save-expr-2.c	2013-08-15 10:58:30.258913139 +0200
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */
+
+int
+foo (int i, unsigned int u)
+{
+  return u / i;
+}
+
+int
+bar (int i, unsigned int u)
+{
+  return u % i;
+}
--- gcc/testsuite/c-c++-common/ubsan/save-expr-3.c.mp	2013-08-15 11:02:39.111142875 +0200
+++ gcc/testsuite/c-c++-common/ubsan/save-expr-3.c	2013-08-15 11:02:34.874127652 +0200
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */
+
+int x;
+
+int
+foo (int i, int u)
+{
+  return (i << u) << x;
+}
+
+int
+bar (int i, int u)
+{
+  return (i >> u) >> x;
+}
--- gcc/testsuite/c-c++-common/ubsan/save-expr-4.c.mp	2013-08-15 11:09:53.453746629 +0200
+++ gcc/testsuite/c-c++-common/ubsan/save-expr-4.c	2013-08-15 11:09:45.238716728 +0200
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -Wall -Werror -O" } */
+
+int x;
+
+int
+foo (int i, unsigned int u)
+{
+  return (i % u) << (x / u);
+}
+
+int
+bar (int i, unsigned int u)
+{
+  return (((x % u) << (u / i)) >> x);
+}