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

login
register
mail settings
Submitter Marek Polacek
Date Aug. 7, 2013, 2:58 p.m.
Message ID <20130807145803.GE17022@redhat.com>
Download mbox | patch
Permalink /patch/265521/
State New
Headers show

Comments

Marek Polacek - Aug. 7, 2013, 2:58 p.m.
On Tue, Aug 06, 2013 at 05:24:08PM -0400, Jason Merrill wrote:
> 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.

True, in this particular case wrapping x in the SAVE_EXPR isn't
needed, thus the following should work equally:


(It would merit a comment, for sure.) 
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,

	Marek

Patch

--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -10493,9 +10493,12 @@  build_binary_op (location_t location, enum tree_code code,
       && (doing_div_or_mod || doing_shift))
     {
       /* OP0 and/or OP1 might have side-effects.  */
-      op0 = c_save_expr (op0);
+      if (!doing_shift || flag_isoc99)
+        {
+          op0 = c_save_expr (op0);
+          op0 = c_fully_fold (op0, false, NULL);
+       }
       op1 = c_save_expr (op1);
-      op0 = c_fully_fold (op0, false, NULL);
       op1 = c_fully_fold (op1, false, NULL);
       if (doing_div_or_mod)
        instrument_expr = ubsan_instrument_division (location, op0, op1);