Re: Request to merge Undefined Behavior Sanitizer in (take 2)

[Marek Polacek <polacek at redhat dot com>]

On Sat, Aug 03, 2013 at 12:24:32PM -0400, Jason Merrill wrote:
> Where are the SAVE_EXPRs coming from?  It doesn't seem to me that x
> needs to be wrapped in a SAVE_EXPR at all in this case.  For cases
> where the SAVE_EXPR is needed and not used in the test, you could
> add the SAVE_EXPR before the condition with a COMPOUND_EXPR.

Those SAVE_EXPRs are coming from c-typeck.c in this case:

  if (flag_sanitize & SANITIZE_UNDEFINED
      && current_function_decl != 0
      && (doing_div_or_mod || doing_shift))
    {
      /* OP0 and/or OP1 might have side-effects.  */
      op0 = c_save_expr (op0);
      op1 = c_save_expr (op1);

I've resorted to creating (SAVE_EXPR <x>, ...)  in the condition in case
we're in C89, i.e. creating COMPOUND_EXPR in the condition itself,
it seems to work pretty well.  Firstly, I added explicit (void) cast via
NOP_EXPR (as 'if ((void) x, y)' doesn't trigger -Wunused-value warning),
but that didn't seem to be necessary...

Does that look up to snuff to you?  Thanks,

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

	* c-ubsan.c (ubsan_instrument_shift): Properly evaluate
	SAVE_EXPR even in the C89 mode.

	* gcc.dg/ubsan/save-expr-1.c: New test.

--- gcc/c-family/c-ubsan.c.mp	2013-08-05 13:13:49.245693141 +0200
+++ gcc/c-family/c-ubsan.c	2013-08-05 13:13:53.170709181 +0200
@@ -131,6 +131,12 @@ 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.  */
+  if (!c_dialect_cxx () && !flag_isoc99)
+    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/gcc.dg/ubsan/save-expr-1.c.mp	2013-08-05 13:14:51.057960067 +0200
+++ gcc/testsuite/gcc.dg/ubsan/save-expr-1.c	2013-08-05 13:17:44.582675494 +0200
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=shift -std=c89 -Wall -Werror -O" } */
+
+static int x;
+int
+main (void)
+{
+  int o = 1;
+  int y = x << o;
+  return y;
+}

	Marek