[PATCH] D25601: DAGCombiner: fix use-after-free when merging consecutive stores

[Nicolai Hähnle via llvm-commits]

On 02.11.2016 19:38, Nicolai Hähnle wrote:
> On 02.11.2016 18:54, Justin Bogner wrote:
>> Nicolai Hähnle <nhaehnle at gmail.com> writes:
>>> nhaehnle updated this revision to Diff 76719.
>>> nhaehnle added a comment.
>>>
>>> Keep returning a boolean.
>>>
>>> Since not all nodes are necessarily merged, the success-path now
>>> needs to
>>> truncate the StoreNodes vector to just those nodes that were actually
>>> removed.
>>
>> Looks pretty close. A couple of comments below.
>
> Thanks for taking a look!
>
> [snip]
>>> -    do {
>>> +    for (;;) {
>>>        // There can be multiple store sequences on the same chain.
>>>        // Keep trying to merge store sequences until we are unable to
>>> do so
>>>        // or until we merge the last store on the chain.
>>> -      bool Changed = MergeConsecutiveStores(ST);
>>> +      SmallVector<MemOpLink, 8> StoreNodes;
>>> +      bool Changed = MergeConsecutiveStores(ST, StoreNodes);
>>
>> What do you think of returning the SmallVector instead of the bool, then
>> just doing `EverChanged |= !StoreNodes.empty()`? It seems a bit simpler
>> to me, and move semantics should mean it's just as efficient.
>
> A SmallVector move cannot be as efficient because it cannot avoid the
> copy in the small case. I don't feel strongly about it though.

Another point to consider is that encoding the size of the SmallVector 
as part of a function signature feels a bit odd, especially in this case 
where it'd affect two functions whose return type must then agree on the 
size of the SmallVector. I'd really prefer not to make that change.

Cheers,
Nicolai

>
>
>>>        EverChanged |= Changed;
>>>        if (!Changed) break;
>>> -    } while (ST->getOpcode() != ISD::DELETED_NODE);
>>> +
>>> +      bool ChangedST = false;
>>> +      for (const auto &Link : StoreNodes) {
>>> +        if (Link.MemNode == ST) {
>>> +          ChangedST = true;
>>> +          break;
>>> +        }
>>> +      }
>>> +      if (ChangedST)
>>> +        break;
>>
>> I think this can be simplified with any_of. Something like this:
>>
>>       if (any_of(StoreNodes,
>>                  [&ST](const MemOpLink &Link) { return Link.MemNode ==
>> ST; }))
>>         return;
>
> Will do.
>
> Cheers,
> Nicolai
>
>>
>>> +    }
>>>
>>>      if (EverChanged)
>>>        return SDValue(N, 0);