<div dir="ltr">Matt, there are a bunch of problems here.<br><br>First, Steve Hines accepted this revision in Phabricator, but didn't actually send any LGTM or other comment to the mailing list, so it didn't look like *any* review happened.<div><br></div><div>Second, Steve, I'm not sure how familiar with this code you are as I've never seen you really contribute to it, so it might be better to let others review the patch.</div><div><br></div><div>Third, making the point that sufficient review had not happened, this is a bugfix patch which does not contain a test case. I feel confident that a dangling pointer style bug could be exercised with a good test case and ASan. We have ASan bots, so having the test case in the tree is really useful.</div><div><br></div><div>Matt, please update the review thread when you have added such a test case.</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 12, 2015 at 3:13 PM Matt Wala <<a href="mailto:wala@google.com">wala@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Scalarizer has two data structures that hold information about changes<br>
to the function, Gathered and Scattered. These are cleared in finish()<br>
at the end of runOnFunction() if finish() detects any changes to the<br>
function.<br>
<br>
However, finish() was checking for changes by only checking if<br>
Gathered was non-empty. The function visitStore() only modifies<br>
Scattered without touching Gathered. As a result, Scattered could have<br>
ended up having stale data if Scalarizer only scalarized store<br>
instructions. Since the data in Scattered is used during the execution<br>
of the pass, this introduced dangling pointer errors.<br>
<br>
The fix is to check whether both Scattered and Gathered are empty<br>
before deciding what to do in finish().<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10422&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=VDr7hsZmBQ62wCKBDKSGJMMXpfC2s49Go9dIXNzrJMs&s=T4waY9aTdBPdqTpEdBpK_5HL4Z3DGXqrYzus2CuHPOQ&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10422</a><br>
<br>
Files:<br>
  lib/Transforms/Scalar/Scalarizer.cpp<br>
<br>
Index: lib/Transforms/Scalar/Scalarizer.cpp<br>
===================================================================<br>
--- lib/Transforms/Scalar/Scalarizer.cpp<br>
+++ lib/Transforms/Scalar/Scalarizer.cpp<br>
@@ -636,7 +636,9 @@<br>
 // Delete the instructions that we scalarized.  If a full vector result<br>
 // is still needed, recreate it using InsertElements.<br>
 bool Scalarizer::finish() {<br>
-  if (Gathered.empty())<br>
+  // The presence of data in Gathered or Scattered indicates changes<br>
+  // made to the Function.<br>
+  if (Gathered.empty() && Scattered.empty())<br>
     return false;<br>
   for (GatherList::iterator GMI = Gathered.begin(), GME = Gathered.end();<br>
        GMI != GME; ++GMI) {<br>
<br>
EMAIL PREFERENCES<br>
  <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=VDr7hsZmBQ62wCKBDKSGJMMXpfC2s49Go9dIXNzrJMs&s=lvsOl829W1VMw-W1SnLqqhBWleHf7eL0fF2uuRkbcFs&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>