Page MenuHomePhabricator

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

Authored by wala on Jun 15 2015, 1:48 PM.

Details

Summary

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(). This also fixes a problem
where the Function can be modified although the pass returns false.

Diff Detail

Event Timeline

wala updated this revision to Diff 27715.Jun 15 2015, 1:48 PM
wala retitled this revision from to [Scalarizer] Fix potential for stale data in Scattered across invocations.
wala updated this object.
wala edited the test plan for this revision. (Show Details)
wala added a subscriber: Unknown Object (MLST).
rnk added a subscriber: rnk.Jul 23 2015, 1:22 PM

Nice catch!

test/Transforms/Scalarizer/store-bug.ll
5–8

I'd prefer it if you wrote some CHECK lines for four loads and stores.

wala updated this revision to Diff 30522.Jul 23 2015, 1:39 PM

Add CHECK lines.

wala marked an inline comment as done.Jul 23 2015, 1:40 PM

Added check lines for the stores (no loads in here).

rnk accepted this revision.Jul 23 2015, 1:41 PM
rnk added a reviewer: rnk.

lgtm

This revision is now accepted and ready to land.Jul 23 2015, 1:41 PM
wala closed this revision.Jul 23 2015, 1:54 PM