On Fri, Apr 07, 2017 at 07:04:08PM +0200, Jakub Jelinek wrote:
> On Fri, Apr 07, 2017 at 04:26:50PM +0200, Martin Liška wrote:
> > Hello.
> >
> > Similar to what was done in Marek's r202113, when op1 is a SAVE_EXPR it must
> > be evaluated before condition, in order to be able to deliver the operand
> > to real shifting. And not just to a BB where ubsan report function is
> > called.
> >
> > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> > Apart from that make check RUNTESTFLAGS="ubsan.exp" works on
> > x86_64-linux-gnu.
> >
> > Ready to be installed?
> > Martin
>
> > >From 2ff2e17d82ee85b09cb5f83afbee70f8b1a84f4f Mon Sep 17 00:00:00 2001
> > From: marxin <mli...@suse.cz>
> > Date: Fri, 7 Apr 2017 12:21:44 +0200
> > Subject: [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR
> > sanitizer/80350).
> >
> > gcc/c-family/ChangeLog:
> >
> > 2017-04-07 Martin Liska <mli...@suse.cz>
> >
> > PR sanitizer/80350
> > * c-ubsan.c (ubsan_instrument_shift): Evaluate RHS before
> > doing an UBSAN check.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2017-04-07 Martin Liska <mli...@suse.cz>
> >
> > PR sanitizer/80350
> > * c-c++-common/ubsan/pr80350.c: New test.
> > ---
> > gcc/c-family/c-ubsan.c | 4 +++-
> > gcc/testsuite/c-c++-common/ubsan/pr80350.c | 17 +++++++++++++++++
> > 2 files changed, 20 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr80350.c
> >
> > diff --git a/gcc/c-family/c-ubsan.c b/gcc/c-family/c-ubsan.c
> > index 91bdef88320..ef45abdd19e 100644
> > --- a/gcc/c-family/c-ubsan.c
> > +++ b/gcc/c-family/c-ubsan.c
> > @@ -171,7 +171,9 @@ ubsan_instrument_shift (location_t loc, enum tree_code
> > code,
> >
> > /* In case we have a SAVE_EXPR in a conditional context, we need to
> > make sure it gets evaluated before the condition. */
> > - t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> > + t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t),
> > + fold_build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> > + unshare_expr (op0), unshare_expr (op1)), t);
>
> For consistency with ubsan_instrument_division and better readability,
> can't you:
> /* In case we have a SAVE_EXPR in a conditional context, we need to
> make sure it gets evaluated before the condition. */
> t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op1), t);
Yeah, I don't have the authority to approve the patch, but I was gonna suggest
this change, too.
Marek
