Patchwork Fix undefined behavior in IRA

login
register
mail settings
Submitter Marek Polacek
Date March 25, 2014, 4:14 p.m.
Message ID <20140325161421.GS6523@redhat.com>
Download mbox | patch
Permalink /patch/333516/
State New
Headers show

Comments

Marek Polacek - March 25, 2014, 4:14 p.m.
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?

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.


	Marek
Vladimir Makarov - March 25, 2014, 10:38 p.m.
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.
>
>
Marc Glisse - March 26, 2014, 9:27 p.m.
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?
Jakub Jelinek - March 26, 2014, 9:33 p.m.
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

Patch

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;