This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350).
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Martin Liška <mliska at suse dot cz>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Marek Polacek <polacek at redhat dot com>
- Date: Fri, 7 Apr 2017 19:04:08 +0200
- Subject: Re: [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350).
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jakub at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com ABFF83DBE5
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com ABFF83DBE5
- References: <693e8de8-3fe7-ad42-6f9b-2fd749e7326f@suse.cz>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
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 <mliska@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 <mliska@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 <mliska@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);
Ok with that change.
Jakub