Page MenuHomePhabricator

[InferAddressSpaces] Prevent crash on nested ConstantExpr
ClosedPublic

Authored by joey on Feb 14 2019, 2:30 AM.

Details

Reviewers
arsenm
joey
Summary

Seems like everything on PostOrderStack is expected to be of pointer type.

Diff Detail

Event Timeline

joey created this revision.Feb 14 2019, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 2:30 AM
joey added a reviewer: arsenm.Feb 14 2019, 7:03 AM
arsenm added inline comments.Feb 14 2019, 7:35 AM
lib/Transforms/Scalar/InferAddressSpaces.cpp
323–324

I think this is a broader problem with vectors of pointers. This should maybe be checking this is a scalar pointer instead?

test/Transforms/InferAddressSpaces/AMDGPU/infer-address-space.ll
171

I think this is really a vector of pointers bug

joey added a comment.Feb 14 2019, 7:58 AM

Sorry i should have provided more context, this is the original crash:

opt: ../lib/Transforms/Scalar/InferAddressSpaces.cpp:316: void {anonymous}::InferAddressSpaces::appendsFlatAddressExpressionToPostorderStack(llvm::Value*, std::vector<std::pair<llvm::Value*, bool> >&, llvm::DenseSet<llvm::Value*>&) const: Assertion `V->getType()->isPointerTy()' failed.

I changed the test to use a scalar pointer and still got the same crash.

appendsFlatAddressExpressionToPostorderStack gets called with %add.ptr157 as V. Then the it iterates over the operands, and it sees the select and isAddressExpression returns true.

After the select is added to PostorderStack the loop inside collectFlatAddressExpressions calls getPointerOperands. In my test case, this returns i64 73 and then crashes since this is not a pointer.

Maybe isAddressExpression should also check if the result of the select is a pointer?

I misread the test case, this isn't a vector of pointers. If you can simplify the test by removing the vectors, that would be slightly better.

lib/Transforms/Scalar/InferAddressSpaces.cpp
323–324

I think this might be the wrong place. isAddressExpression should probably be doing the type check for the select case, since the other operations imply pointers (except PHI. I imagine you can craft another test case that fails with them)

arsenm added inline comments.Feb 14 2019, 4:50 PM
lib/Transforms/Scalar/InferAddressSpaces.cpp
323–324

This will only happen for constant expressions, so phi isn't an issue. Can you move the type check into the case for select, and assert the phi is a pointer type in isAddressExpression?

joey updated this revision to Diff 187372.Feb 19 2019, 7:52 AM

Add the changes to isAddExpression instead.

arsenm accepted this revision.Feb 19 2019, 7:57 AM

LGTM

This revision is now accepted and ready to land.Feb 19 2019, 7:57 AM
joey accepted this revision.Feb 21 2019, 4:32 AM

Committed revision r354576.

joey closed this revision.Feb 21 2019, 4:32 AM