This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Request to merge Undefined Behavior Sanitizer in (take 2)
- From: Marek Polacek <polacek at redhat dot com>
- To: Jason Merrill <jason at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>, Jeff Law <law at redhat dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>
- Date: Wed, 7 Aug 2013 16:58:03 +0200
- Subject: Re: Request to merge Undefined Behavior Sanitizer in (take 2)
- References: <20130725153227 dot GC32538 at redhat dot com> <51F186E9 dot 9090404 at redhat dot com> <20130731173321 dot GT17022 at redhat dot com> <51F95CF7 dot 8020605 at redhat dot com> <20130801180615 dot GU17022 at redhat dot com> <51FD2EC0 dot 4060607 at redhat dot com> <20130805112452 dot GX17022 at redhat dot com> <52016978 dot 8010807 at redhat dot com>
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