[PATCH] Fix infinite recursion bug in SROA

[James Molloy]

Hi Chandler,

 

OK – sure. Attached is a patch that removes the early exits. Is it OK?

 

Cheers,

 

James

 

From: Chandler Carruth [mailto:chandlerc at google.com] 
Sent: 21 January 2014 10:45
To: James Molloy
Cc: llvm-commits
Subject: Re: [PATCH] Fix infinite recursion bug in SROA

 

I hate that routine. We've been here before, repeatedly, each time with order of iteration bugs.

 

Your patch is certainly correct, but I think its worth setting this question to rest by making this a more direct body of code. Here is my suggestion:

 

- Remove the 'return' from the loop. Every time there is an early exit from this kind of thing, it all goes to shit complexity wise.

- Have the loop pick a best type *and* the best integral type.

- Keep an explicit "no longer viable" state for the basic "best type" case (due to non-load/store or conflicting types)

 

After we look at every slice, return the best type if viable, else the best integral type if viable, else 0.

 

Essentially, Ty and ITy should have the following transitions:

 

Ty: 0 -> <good> -> <bad>

ITy: 0 -> <good>

 

Ty saturates in the 'bad' state.

 

I think that would be more clear than even after your patch.

 

 

On Mon, Jan 20, 2014 at 5:08 AM, James Molloy <james.molloy at arm.com> wrote:

Hi Chandler,

 

The attached patch fixes a bug that could cause SROA to go into infinite recursion (stack overflow in debug mode, infinite loop in release mode).

 

The function findCommonType() attempts to use a type from a load/store inst, and if there are non-load-store instructions or their types do not match it falls back to trying to find an integral type that covers all uses. Otherwise, it should return failure (NULL).

 

The mechanism to fall back to integral types does not reset the “winning” type to NULL. This means that depending on the order in which the input uses are iterated over changes the output of the function. If a non-load-store instruction comes first, it will return NULL (as Ty is never set). If a load/store inst comes first, it will return the Ty of that instruction.

 

There is no testcase as the bug depends on memory layout; I could not force it to fail with opt/llc.

 

Is it OK to commit?

 

Cheers,

 

James

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140121/d082bde0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sroa-findcommontype.diff
Type: application/octet-stream
Size: 2841 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140121/d082bde0/attachment.obj>