This is an archive of the discontinued LLVM Phabricator instance.

Fix SROA regression causing data corruption, fixes PR19250
ClosedPublic

Authored by dotdash on Apr 5 2014, 11:19 AM.

Details

Summary

r199771 accidently broke the logic that makes sure that SROA only splits load
on byte boundaries. If such a split happens, some bits get lost when
reassembling loads of wider types, causing data corruption.

Moving the width check up to reject such splits early fixes the problem.

Diff Detail

Event Timeline

I'd be grateful if somebody could take the time to review this. The regression fixed by this is blocking rust (the language) from using i1 for its bool type.

Thanks!

+Chandler as the Acolyte (wizard? guru? code monkey?) of SROA.

dotdash updated this revision to Diff 10282.Jun 10 2014, 9:26 AM
dotdash updated this object.

Added a comment to explain why the whole if-else block has to be moved after
the early continue. Moved the original test into its own file and added new
tests for the slice order indepence of the chosen type for the alloca.

dotdash updated this revision to Diff 10283.Jun 10 2014, 9:27 AM

Sorry, forgot to remove the old @test25 from the original patch.

dotdash updated this revision to Diff 10463.Jun 16 2014, 3:16 PM

Fix SROA regression causing miscompilation, fixes PR19250

Summary:
r199771 accidently changed the logic that excludes integer types that
are not a byte width multiple from being used as the type used for a
newly formed partition.

This means that we can, for example, end up with a partition that spans
a single byte and has two users, one load with type i1 and another one
with type i32. The latter is considered to be splittable and expands
beyond the partition.

Now in findCommonType, i1 is assigned to Ty before the check that
rejects i1 because its width is not a byte width multiple. And because
it is the only type that spans the whole partition, TyIsCommon remains
true and i1 is returned as the common type.

Since such types are not actually supported, this causes some bits to be
zeroed out in the load of the i32 value, because the first byte gets
truncated to i1 and then zero-extended.

To fix this problem, we must make sure to check for non-byte width types
first and always reject them.

Reviewers: dexonsmith
Cc: aprantl

Differential Revision: http://reviews.llvm.org/D3297

dotdash accepted this revision.Jun 17 2014, 11:43 AM
dotdash added a reviewer: dotdash.
This revision is now accepted and ready to land.Jun 17 2014, 11:43 AM
dotdash closed this revision.Jun 17 2014, 11:44 AM

Committed in r211082