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 <pola...@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
