On Wed, 13 Jun 2018, Jakub Jelinek wrote:
> On Wed, Jun 13, 2018 at 10:22:29AM +0200, Jakub Jelinek wrote:
> > Random testcase for -Wduplicated-branches -fsanitize=shift:
> > int
> > foo (int x, int y)
> > {
> > if (x)
> > y = y << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1;
> > else
> > y = y << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1
> > << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1 >> 1 << 1
> > >> 1 << 1 >> 1;
> > return y;
> > }
> >
> > Though it seems we have that problem already in inchash::add_expr. In that
> > case perhaps we could have a pointer to a hashmap in inchash::hash objects,
> > clear it in the ctors and destroy/clear in inchash::hash::end (), though we
> > have the add_commutative that has two separate hash objects.
>
> It isn't specific to just -fsanitize=undefined, even without that there are
> cases we can end up with lots of nested SAVE_EXPRs, like
> -Wduplicated-branches:
> int bar (void);
>
> void
> foo (int x, int *y)
> {
> if (x)
> y[0] += (y[1] += (y[2] += (y[3] += (y[4] += (y[5] += (y[6] += (y[7] +=
> (y[8] +=
> (y[9] += (y[10] += (y[11] += (y[12] += (y[13] += (y[14] += (y[15]
> += (y[16] +=
> (y[17] += (y[18] += (y[19] += (y[20] += (y[21] += (y[22] += (y[23]
> += (y[24] +=
> (y[25] += (y[26] += (y[27] += (y[28] += (y[29] += (y[30] += (y[31]
> += (y[32] +=
> (y[33] += (y[34] += (y[35] += (y[36] += (y[37] += (y[38] += (y[39]
> += (y[40] +=
> (y[41] += (y[42] += (y[43] += (y[44] += (y[45] += (y[46] += (y[47]
> += (y[48] +=
> (y[49] += (y[50] += (y[51] += (y[52] += (y[53] += (y[54] += (y[55]
> += (y[56] +=
> (y[57] += (y[58] += (y[59] += (y[60] += (y[61] += (y[62] += (y[63]
> += (y[64] += bar ()
> ))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))));
> else
> y[0] += (y[1] += (y[2] += (y[3] += (y[4] += (y[5] += (y[6] += (y[7] +=
> (y[8] +=
> (y[9] += (y[10] += (y[11] += (y[12] += (y[13] += (y[14] += (y[15]
> += (y[16] +=
> (y[17] += (y[18] += (y[19] += (y[20] += (y[21] += (y[22] += (y[23]
> += (y[24] +=
> (y[25] += (y[26] += (y[27] += (y[28] += (y[29] += (y[30] += (y[31]
> += (y[32] +=
> (y[33] += (y[34] += (y[35] += (y[36] += (y[37] += (y[38] += (y[39]
> += (y[40] +=
> (y[41] += (y[42] += (y[43] += (y[44] += (y[45] += (y[46] += (y[47]
> += (y[48] +=
> (y[49] += (y[50] += (y[51] += (y[52] += (y[53] += (y[54] += (y[55]
> += (y[56] +=
> (y[57] += (y[58] += (y[59] += (y[60] += (y[61] += (y[62] += (y[63]
> += (y[64] += bar ()
> ))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))));
> }
Your concern is for compile-time, not for correctness, right?
I think that
/* If ARG0 and ARG1 are the same SAVE_EXPR, they are necessarily equal.
We don't care about side effects in that case because the SAVE_EXPR
takes care of that for us. In all other cases, two expressions are
equal if they have no side effects. If we have two identical
expressions with side effects that should be treated the same due
to the only side effects being identical SAVE_EXPR's, that will
be detected in the recursive calls below.
If we are taking an invariant address of two identical objects
they are necessarily equal as well. */
if (arg0 == arg1 && ! (flags & OEP_ONLY_CONST)
&& (TREE_CODE (arg0) == SAVE_EXPR
|| (flags & OEP_MATCH_SIDE_EFFECTS)
|| (! TREE_SIDE_EFFECTS (arg0) && ! TREE_SIDE_EFFECTS (arg1))))
return 1;
is supposed to handle == SAVE_EXPRs and thus the hunk should be
conditionalized on arg0 != arg1 or rather for OEP_LEXICOGRAPHIC
always return true for arg0 == arg1?
I also wondered why this change was necessary for the fix but
it looked safe correctness-wise.
Richard.
