* [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350).
@ 2017-04-07 14:26 Martin Liška
2017-04-07 17:04 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Martin Liška @ 2017-04-07 14:26 UTC (permalink / raw)
To: GCC Patches; +Cc: Marek Polacek
[-- Attachment #1: Type: text/plain, Size: 423 bytes --]
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
[-- Attachment #2: 0001-Evaluate-a-SAVE_EXPR-before-an-UBSAN-check-PR-saniti.patch --]
[-- Type: text/x-patch, Size: 1996 bytes --]
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);
enum sanitize_code recover_kind = SANITIZE_SHIFT_EXPONENT;
tree else_t = void_node;
diff --git a/gcc/testsuite/c-c++-common/ubsan/pr80350.c b/gcc/testsuite/c-c++-common/ubsan/pr80350.c
new file mode 100644
index 00000000000..317d2facaf7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/pr80350.c
@@ -0,0 +1,17 @@
+/* PR target/80310 */
+/* { dg-do run } */
+/* { dg-options "-O0 -fsanitize=shift -fno-sanitize-recover=shift" } */
+
+unsigned int x = 1;
+unsigned int y = 0;
+
+void foo() {
+ y = 1 >> (!x * 1111);
+}
+
+int main () {
+ foo ();
+ if (y != 1)
+ __builtin_abort ();
+ return 0;
+}
--
2.12.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350).
2017-04-07 14:26 [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350) Martin Liška
@ 2017-04-07 17:04 ` Jakub Jelinek
2017-04-07 17:07 ` Marek Polacek
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2017-04-07 17:04 UTC (permalink / raw)
To: Martin Liška; +Cc: GCC Patches, Marek Polacek
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350).
2017-04-07 17:04 ` Jakub Jelinek
@ 2017-04-07 17:07 ` Marek Polacek
0 siblings, 0 replies; 3+ messages in thread
From: Marek Polacek @ 2017-04-07 17:07 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Martin Liška, GCC Patches
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 <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);
Yeah, I don't have the authority to approve the patch, but I was gonna suggest
this change, too.
Marek
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-07 17:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 14:26 [PATCH] Evaluate a SAVE_EXPR before an UBSAN check (PR sanitizer/80350) Martin Liška
2017-04-07 17:04 ` Jakub Jelinek
2017-04-07 17:07 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).