| Message ID | 20140325161421.GS6523@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2014-03-25, 12:14 PM, Marek Polacek wrote: > This is a temporary fix for UB in IRA, where ubsan complains because > there's signed iteger overflow in the multiplication. To shut this > error up, we can perform the multiplication in unsigned and only then > cast the result of the multiplication to int. > > Regtested/bootstrapped on x86_64-linux, ok for trunk? > Ok. Thanks, Marek. > 2014-03-25 Marek Polacek <polacek@redhat.com> > > PR other/59545 > * ira-color.c (update_conflict_hard_regno_costs): Perform the > multiplication in unsigned type. > >
On Tue, 25 Mar 2014, Marek Polacek wrote: > This is a temporary fix for UB in IRA, where ubsan complains because > there's signed iteger overflow in the multiplication. To shut this > error up, we can perform the multiplication in unsigned and only then > cast the result of the multiplication to int. Naive question: why do you want to shut the error up? If modular arithmetic makes sense for costs (sounds doubtful), they should use an unsigned type to begin with. Otherwise, this is making it harder to notice a bug (doesn't sound like an improvement). If you want to cast, doesn't something like long long (HOST_WIDEST_INT?) make more sense than unsigned?
On Wed, Mar 26, 2014 at 10:27:37PM +0100, Marc Glisse wrote: > On Tue, 25 Mar 2014, Marek Polacek wrote: > > >This is a temporary fix for UB in IRA, where ubsan complains because > >there's signed iteger overflow in the multiplication. To shut this > >error up, we can perform the multiplication in unsigned and only then > >cast the result of the multiplication to int. > > Naive question: why do you want to shut the error up? If modular > arithmetic makes sense for costs (sounds doubtful), they should use > an unsigned type to begin with. Otherwise, this is making it harder > to notice a bug (doesn't sound like an improvement). Because it makes bootstrap-ubsan pretty much useless, e.g. in the testsuite almost all tests fail because of this. AFAIK Vlad is aware of this, and if it isn't tracked in some bug, it should be that it should be investigated. In PR59545 I've mentioned also other ira issues with ub, if I remember well it was two other places, but the ira-color.c case has been orders of magnitude more common. Jakub
diff --git gcc/ira-color.c gcc/ira-color.c index c20aaf7..1f4c96e 100644 --- gcc/ira-color.c +++ gcc/ira-color.c @@ -1505,7 +1505,7 @@ update_conflict_hard_regno_costs (int *costs, enum reg_class aclass, index = ira_class_hard_reg_index[aclass][hard_regno]; if (index < 0) continue; - cost = conflict_costs [i] * mult / div; + cost = (int) ((unsigned) conflict_costs [i] * mult) / div; if (cost == 0) continue; cont_p = true;