[PATCH] [Scalarizer] Fix potential for stale data in Scattered across invocations

[Chandler Carruth]

Matt, there are a bunch of problems here.

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.

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.

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.

Matt, please update the review thread when you have added such a test case.

On Fri, Jun 12, 2015 at 3:13 PM Matt Wala <wala at google.com> wrote:

> Scalarizer has two data structures that hold information about changes
> to the function, Gathered and Scattered. These are cleared in finish()
> at the end of runOnFunction() if finish() detects any changes to the
> function.
>
> However, finish() was checking for changes by only checking if
> Gathered was non-empty. The function visitStore() only modifies
> Scattered without touching Gathered. As a result, Scattered could have
> ended up having stale data if Scalarizer only scalarized store
> instructions. Since the data in Scattered is used during the execution
> of the pass, this introduced dangling pointer errors.
>
> The fix is to check whether both Scattered and Gathered are empty
> before deciding what to do in finish().
>
> http://reviews.llvm.org/D10422
>
> Files:
>   lib/Transforms/Scalar/Scalarizer.cpp
>
> Index: lib/Transforms/Scalar/Scalarizer.cpp
> ===================================================================
> --- lib/Transforms/Scalar/Scalarizer.cpp
> +++ lib/Transforms/Scalar/Scalarizer.cpp
> @@ -636,7 +636,9 @@
>  // Delete the instructions that we scalarized.  If a full vector result
>  // is still needed, recreate it using InsertElements.
>  bool Scalarizer::finish() {
> -  if (Gathered.empty())
> +  // The presence of data in Gathered or Scattered indicates changes
> +  // made to the Function.
> +  if (Gathered.empty() && Scattered.empty())
>      return false;
>    for (GatherList::iterator GMI = Gathered.begin(), GME = Gathered.end();
>         GMI != GME; ++GMI) {
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150613/38496687/attachment.html>