Message ID | 20130805112452.GX17022@redhat.com |
---|---|
State | New |
Headers | show |
On 08/05/2013 07:24 AM, Marek Polacek wrote: > 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); Yes, but why do we need to wrap op0 in a save_expr? That's only necessary if we're going to be evaluating it twice on the same path, but here it's only evaluated once on each path. Jason
--- 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; +}