Page MenuHomePhabricator

Modification of bad code in Reassociate.cpp to avoid potential bug.
Needs ReviewPublic

Authored by alok on Dec 12 2019, 10:02 PM.

Details

Summary

It is evident in the code itself where variable's (type Instruction) user list is accessed without first checking whether it has at least one element.

Code is rearranged to bring check hasOneUse() before user_back().

Diff Detail

Event Timeline

alok created this revision.Dec 12 2019, 10:02 PM

Test?
I'd think Sub->user_back() would simply return nullptr then.

If I understand correctly, the problem is that calling user_back() might access a container with zero elements, yes? This would appear to be an easy fix -- however IMO you should add reviewers who're familiar with the Reassociate pass better [0]. From a couple of minutes examination it would appear ShouldBreakUpSubtract is only callable from OptimizeInst, and that in turn is guarded by isInstructionTriviallyDead, which I would expect to skip such an instruction. There might be something more complicated broken that someone more familiar with reassociate might spot.

If this really does cause a crash, a regression test would be in order too.

[0] Maybe you have already, I don't recognise all the names, sorry