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:
--- 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);
(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
