* [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
@ 2017-04-11 20:54 Jakub Jelinek
2017-04-12 5:36 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2017-04-11 20:54 UTC (permalink / raw)
To: Richard Biener, Jeff Law; +Cc: gcc-patches
Hi!
This is another case where we miss needed folding from argN or their
arguments to the expected expression type (type has to be compatible
with opN's type, but argN is after STRIP_NOPS).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2017-04-11 Jakub Jelinek <jakub@redhat.com>
PR sanitizer/80349
* fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
first argument to type.
* g++.dg/ubsan/pr80349.C: New test.
--- gcc/fold-const.c.jj 2017-04-10 22:27:00.000000000 +0200
+++ gcc/fold-const.c 2017-04-11 20:07:02.459839450 +0200
@@ -9916,12 +9916,12 @@ fold_binary_loc (location_t loc,
}
if (c3 != c1)
- return fold_build2_loc (loc, BIT_IOR_EXPR, type,
- fold_build2_loc (loc, BIT_AND_EXPR, type,
- TREE_OPERAND (arg0, 0),
- wide_int_to_tree (type,
- c3)),
- arg1);
+ {
+ tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
+ tem = fold_build2_loc (loc, BIT_AND_EXPR, type, tem,
+ wide_int_to_tree (type, c3));
+ return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
+ }
}
/* See if this can be simplified into a rotate first. If that
--- gcc/testsuite/g++.dg/ubsan/pr80349.C.jj 2017-04-11 20:29:10.154344673 +0200
+++ gcc/testsuite/g++.dg/ubsan/pr80349.C 2017-04-11 20:28:38.000000000 +0200
@@ -0,0 +1,11 @@
+// PR sanitizer/80349
+// { dg-do compile }
+// { dg-options "-fsanitize=undefined" }
+
+extern const long long int v;
+
+void
+foo ()
+{
+ (int)((v & 50 | 051UL) << 0) << 0;
+}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
2017-04-11 20:54 [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349) Jakub Jelinek
@ 2017-04-12 5:36 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2017-04-12 5:36 UTC (permalink / raw)
To: Jakub Jelinek, Jeff Law; +Cc: gcc-patches
On April 11, 2017 10:54:10 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>This is another case where we miss needed folding from argN or their
>arguments to the expected expression type (type has to be compatible
>with opN's type, but argN is after STRIP_NOPS).
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
>2017-04-11 Jakub Jelinek <jakub@redhat.com>
>
> PR sanitizer/80349
> * fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
> first argument to type.
>
> * g++.dg/ubsan/pr80349.C: New test.
>
>--- gcc/fold-const.c.jj 2017-04-10 22:27:00.000000000 +0200
>+++ gcc/fold-const.c 2017-04-11 20:07:02.459839450 +0200
>@@ -9916,12 +9916,12 @@ fold_binary_loc (location_t loc,
> }
>
> if (c3 != c1)
>- return fold_build2_loc (loc, BIT_IOR_EXPR, type,
>- fold_build2_loc (loc, BIT_AND_EXPR, type,
>- TREE_OPERAND (arg0, 0),
>- wide_int_to_tree (type,
>- c3)),
>- arg1);
>+ {
>+ tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
>+ tem = fold_build2_loc (loc, BIT_AND_EXPR, type, tem,
>+ wide_int_to_tree (type, c3));
>+ return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
>+ }
> }
>
> /* See if this can be simplified into a rotate first. If that
>--- gcc/testsuite/g++.dg/ubsan/pr80349.C.jj 2017-04-11
>20:29:10.154344673 +0200
>+++ gcc/testsuite/g++.dg/ubsan/pr80349.C 2017-04-11 20:28:38.000000000
>+0200
>@@ -0,0 +1,11 @@
>+// PR sanitizer/80349
>+// { dg-do compile }
>+// { dg-options "-fsanitize=undefined" }
>+
>+extern const long long int v;
>+
>+void
>+foo ()
>+{
>+ (int)((v & 50 | 051UL) << 0) << 0;
>+}
>
> Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
2017-04-25 16:40 ` Marek Polacek
@ 2017-04-25 16:43 ` Jakub Jelinek
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Jelinek @ 2017-04-25 16:43 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Tue, Apr 25, 2017 at 06:27:01PM +0200, Marek Polacek wrote:
> On Tue, Apr 25, 2017 at 06:09:20PM +0200, Jakub Jelinek wrote:
> > On Tue, Apr 25, 2017 at 06:05:25PM +0200, Marek Polacek wrote:
> > > Here we are crashing because fold_binary_loc produced a BIT_IOR_EXPR with
> > > incompatible operands. Fixed by adding the missing conversion, similarly
> > > to <https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00551.html>.
> > >
> > > Bootstrapped/regtested on x86_64-linux, ok for trunk? And 7.1?
> > >
> > > 2017-04-25 Marek Polacek <polacek@redhat.com>
> > >
> > > PR sanitizer/80349
> > > * fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
> > > first argument to type.
> > >
> > > * g++.dg/ubsan/pr80349-2.C: New test.
> > >
> > > diff --git gcc/fold-const.c gcc/fold-const.c
> > > index f0b8e7a..ce4b2df 100644
> > > --- gcc/fold-const.c
> > > +++ gcc/fold-const.c
> > > @@ -9898,8 +9898,10 @@ fold_binary_loc (location_t loc,
> > >
> > > /* If (C1|C2) == ~0 then (X&C1)|C2 becomes X|C2. */
> > > if (msk.and_not (c1 | c2) == 0)
> > > - return fold_build2_loc (loc, BIT_IOR_EXPR, type,
> > > - TREE_OPERAND (arg0, 0), arg1);
> > > + {
> > > + tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
> > > + return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
> >
> > Shouldn't this use op1 instead of arg1, just in case op1 is a NOP_EXPR
> > of INTEGER_CST or similar unfolded tree?
>
> I didn't think so, because other spots in <case BIT_IOR_EXPR> don't use that
> either, e.g. the if (c3 != c1). But surely I can change it to op1, just the
> place I'm touching or something else too?
For now just commit the patch as is (and to 7.1 if you can immediately, I'd
like to start rc1 rolling in an hour or two).
But if you have time, addressing this in all the places or at least
some would be nice (perhaps in smaller chunks, so that it can be carefully
reviewed, not a patch with 50+ changed spots).
The general rule is that type is the type of op0/op1/op2... (for ops
where all the arguments need to have the same type, say *COND_EXPR or
shifts/rotates are an exception), and arg0/arg1/arg2... can have different
types. So when something is constructing something and expects the
arguments to have type TYPE, then you should be passing opN rather than
argN, and for say argN operands etc. fold_convert to type first.
I think most of the remaining issues left are when argN is INTEGER_CST,
because then the only way you can get that to misbehave is when opN
is not folded (which can still happen e.g. in save_expr until we fix that).
Thanks.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
2017-04-25 16:13 ` Jakub Jelinek
@ 2017-04-25 16:40 ` Marek Polacek
2017-04-25 16:43 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2017-04-25 16:40 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: GCC Patches
On Tue, Apr 25, 2017 at 06:09:20PM +0200, Jakub Jelinek wrote:
> On Tue, Apr 25, 2017 at 06:05:25PM +0200, Marek Polacek wrote:
> > Here we are crashing because fold_binary_loc produced a BIT_IOR_EXPR with
> > incompatible operands. Fixed by adding the missing conversion, similarly
> > to <https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00551.html>.
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk? And 7.1?
> >
> > 2017-04-25 Marek Polacek <polacek@redhat.com>
> >
> > PR sanitizer/80349
> > * fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
> > first argument to type.
> >
> > * g++.dg/ubsan/pr80349-2.C: New test.
> >
> > diff --git gcc/fold-const.c gcc/fold-const.c
> > index f0b8e7a..ce4b2df 100644
> > --- gcc/fold-const.c
> > +++ gcc/fold-const.c
> > @@ -9898,8 +9898,10 @@ fold_binary_loc (location_t loc,
> >
> > /* If (C1|C2) == ~0 then (X&C1)|C2 becomes X|C2. */
> > if (msk.and_not (c1 | c2) == 0)
> > - return fold_build2_loc (loc, BIT_IOR_EXPR, type,
> > - TREE_OPERAND (arg0, 0), arg1);
> > + {
> > + tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
> > + return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
>
> Shouldn't this use op1 instead of arg1, just in case op1 is a NOP_EXPR
> of INTEGER_CST or similar unfolded tree?
I didn't think so, because other spots in <case BIT_IOR_EXPR> don't use that
either, e.g. the if (c3 != c1). But surely I can change it to op1, just the
place I'm touching or something else too?
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
@ 2017-04-25 16:13 Marek Polacek
2017-04-25 16:13 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2017-04-25 16:13 UTC (permalink / raw)
To: GCC Patches
Here we are crashing because fold_binary_loc produced a BIT_IOR_EXPR with
incompatible operands. Fixed by adding the missing conversion, similarly
to <https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00551.html>.
Bootstrapped/regtested on x86_64-linux, ok for trunk? And 7.1?
2017-04-25 Marek Polacek <polacek@redhat.com>
PR sanitizer/80349
* fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
first argument to type.
* g++.dg/ubsan/pr80349-2.C: New test.
diff --git gcc/fold-const.c gcc/fold-const.c
index f0b8e7a..ce4b2df 100644
--- gcc/fold-const.c
+++ gcc/fold-const.c
@@ -9898,8 +9898,10 @@ fold_binary_loc (location_t loc,
/* If (C1|C2) == ~0 then (X&C1)|C2 becomes X|C2. */
if (msk.and_not (c1 | c2) == 0)
- return fold_build2_loc (loc, BIT_IOR_EXPR, type,
- TREE_OPERAND (arg0, 0), arg1);
+ {
+ tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
+ return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
+ }
/* Minimize the number of bits set in C1, i.e. C1 := C1 & ~C2,
unless (C1 & ~C2) | (C2 & C3) for some C3 is a mask of some
diff --git gcc/testsuite/g++.dg/ubsan/pr80349-2.C gcc/testsuite/g++.dg/ubsan/pr80349-2.C
index e69de29..ca5ea4a 100644
--- gcc/testsuite/g++.dg/ubsan/pr80349-2.C
+++ gcc/testsuite/g++.dg/ubsan/pr80349-2.C
@@ -0,0 +1,11 @@
+// PR sanitizer/80349
+// { dg-do compile }
+// { dg-options "-fsanitize=undefined" }
+
+unsigned long int ll;
+
+int
+foo ()
+{
+ return (2036854775807 >> ll & char(207648476159223) | 502810590243120797UL) << 0;
+}
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349)
2017-04-25 16:13 Marek Polacek
@ 2017-04-25 16:13 ` Jakub Jelinek
2017-04-25 16:40 ` Marek Polacek
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2017-04-25 16:13 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Tue, Apr 25, 2017 at 06:05:25PM +0200, Marek Polacek wrote:
> Here we are crashing because fold_binary_loc produced a BIT_IOR_EXPR with
> incompatible operands. Fixed by adding the missing conversion, similarly
> to <https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00551.html>.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk? And 7.1?
>
> 2017-04-25 Marek Polacek <polacek@redhat.com>
>
> PR sanitizer/80349
> * fold-const.c (fold_binary_loc) <case BIT_IOR_EXPR>: Convert arg0's
> first argument to type.
>
> * g++.dg/ubsan/pr80349-2.C: New test.
>
> diff --git gcc/fold-const.c gcc/fold-const.c
> index f0b8e7a..ce4b2df 100644
> --- gcc/fold-const.c
> +++ gcc/fold-const.c
> @@ -9898,8 +9898,10 @@ fold_binary_loc (location_t loc,
>
> /* If (C1|C2) == ~0 then (X&C1)|C2 becomes X|C2. */
> if (msk.and_not (c1 | c2) == 0)
> - return fold_build2_loc (loc, BIT_IOR_EXPR, type,
> - TREE_OPERAND (arg0, 0), arg1);
> + {
> + tem = fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0));
> + return fold_build2_loc (loc, BIT_IOR_EXPR, type, tem, arg1);
Shouldn't this use op1 instead of arg1, just in case op1 is a NOP_EXPR
of INTEGER_CST or similar unfolded tree?
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-25 16:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-11 20:54 [PATCH] Fix fold_binary_loc BIT_IOR_EXPR folding (PR sanitizer/80349) Jakub Jelinek
2017-04-12 5:36 ` Richard Biener
2017-04-25 16:13 Marek Polacek
2017-04-25 16:13 ` Jakub Jelinek
2017-04-25 16:40 ` Marek Polacek
2017-04-25 16:43 ` Jakub Jelinek
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).