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

[Nirav Dave via llvm-commits]

niravd added a comment.

Do you have a test case for this?

The moving of StoreNodes to the local context seems like it could be a real fix to a nasty problem. We never seem to clear it and that may cause incorrect merges (though i have no example).

Otherwise this seems like a small worthwhile cleanup. As I understand it, when we merge stores we look for candidate nodes that match St but do not require that St is one of the merged nodes which makes it possible for a merge to leave the original store without marking it to be checked again. This may leave some stores that were merge candidates but were not chosen untouched. 
I think a comment explaining this would be helpful.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11694
   // Save the StoreSDNodes that we find in the chain.
-  SmallVector<MemOpLink, 8> StoreNodes;
-
----------------
Please  kill the comment on the preceding line.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12313
+      bool ChangedST = false;
+      for (unsigned i = 0; i < NumChanged; ++i) {
+        if (StoreNodes[i].MemNode == ST) {
----------------
Use StoreNodes.size() instead of NumChanged (or better yet an iterator). This lets you keep the return type above in MergeConsecutiveStores.


https://reviews.llvm.org/D25601