This is an archive of the discontinued LLVM Phabricator instance.

[Lint] Avoid failed assertion by fetching the proper pointer type
ClosedPublic

Authored by uabelho on Sep 15 2017, 1:32 AM.

Details

Summary

When checking if a constant expression is a noop cast we fetched the
IntPtrType by doing DL->getIntPtrType(V->getType())). However, there can
be cases where V doesn't return a pointer, and then getIntPtrType()
triggers an assertion.

Now we pass DataLayout to isNoopCast so the method itself can determine
what the IntPtrType is.

Diff Detail

Event Timeline

uabelho created this revision.Sep 15 2017, 1:32 AM

Picked you as reviewer Matt since you added the CastInst::isNoopCast(const DataLayout *DL) method
which I stole the fix from.

I've run into two different cases where Lint crashes due to the problem this patch tries to solve,
added them both in the new testcase.

arsenm added inline comments.Sep 15 2017, 9:42 AM
lib/Analysis/Lint.cpp
684–693

This is a convoluted way to go about this. This looks like some weird API artifact from before the DataLayout was mandatory. We should change isNoopCast to take the DataLayout instead (it looks like the instruction method version already does this)

uabelho updated this revision to Diff 115614.Sep 18 2017, 2:52 AM
uabelho edited the summary of this revision. (Show Details)

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

arsenm edited edge metadata.Oct 2 2017, 12:10 PM

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

arsenm accepted this revision.Oct 2 2017, 12:10 PM

LGTM

This revision is now accepted and ready to land.Oct 2 2017, 12:10 PM
This revision was automatically updated to reflect the committed changes.

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

Alright I'll do that.
Thanks!

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

Alright I'll do that.
Thanks!

Here we go:
https://reviews.llvm.org/D38497