This is an archive of the discontinued LLVM Phabricator instance.

[IPCP] Don't crash due to arg count/type mismatch between caller/callee
ClosedPublic

Authored by bjope on Jan 22 2019, 6:22 AM.

Details

Summary

This patch avoids an assert in IPConstantPropagation when
there is a argument count/type mismatch between the caller and
the callee.

While this is actually UB on C-level (clang emits a warning),
the IR verifier seems to accept it. I'm not sure what other
frontends/languages might think about this, so simply bailing out
to avoid hitting an assert (in CallSiteBase<>::getArgOperand or
Value::doRAUW) seems like a simple solution.

The problem is exposed by the fact that AbstractCallSites will look
through a bitcast at the callee position of a call/invoke.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Jan 22 2019, 6:22 AM
jdoerfert accepted this revision.Jan 22 2019, 9:04 AM

I see your point and I acknowledge it is a problem. However, I'm not sure a check in IPCP and consequently later in other IPOs is the best solution.
While I have some ideas on how to deal with this (one is to not create call sites for this situation), I think we can go ahead with your local fix now,
keep the test case and make it more general as needed.

Long story short, LGTM.

One thing, can you mention that this problem was exposed by the fact that AbstractCallSites will look through the bitcast at the callee position of a call/invoke somewhere?

This revision is now accepted and ready to land.Jan 22 2019, 9:04 AM
bjope edited the summary of this revision. (Show Details)Jan 23 2019, 2:08 AM
bjope added a comment.Jan 23 2019, 2:22 AM

I see your point and I acknowledge it is a problem. However, I'm not sure a check in IPCP and consequently later in other IPOs is the best solution.
While I have some ideas on how to deal with this (one is to not create call sites for this situation), I think we can go ahead with your local fix now,
keep the test case and make it more general as needed.

Long story short, LGTM.

One thing, can you mention that this problem was exposed by the fact that AbstractCallSites will look through the bitcast at the callee position of a call/invoke somewhere?

Thanks, I added that info to the summary. Now waiting to get my commit access back (seems like our legal department haven't signed the new license agreement yet... so this can't be landed yet... any day now...).

I see your point and I acknowledge it is a problem. However, I'm not sure a check in IPCP and consequently later in other IPOs is the best solution.
While I have some ideas on how to deal with this (one is to not create call sites for this situation), I think we can go ahead with your local fix now,
keep the test case and make it more general as needed.

Long story short, LGTM.

One thing, can you mention that this problem was exposed by the fact that AbstractCallSites will look through the bitcast at the callee position of a call/invoke somewhere?

Thanks, I added that info to the summary. Now waiting to get my commit access back (seems like our legal department haven't signed the new license agreement yet... so this can't be landed yet... any day now...).

If it is urgent, I can commit it for you. If not, we can just wait.

efriedma added inline comments.
lib/Transforms/IPO/IPConstantPropagation.cpp
72 ↗(On Diff #182903)

Checking the number of arguments probably isn't sufficient here; you also need to check the arguments have the same type.

(I'm not sure the check belongs here, as opposed to the AbstractCallSite constructor, but we can work that out later.)

bjope updated this revision to Diff 183859.Jan 28 2019, 7:42 AM

Updates:

  • Fix for VarArgs (need to allow more actual than formal arguments).
  • Added check for matching argument types (including a new test case).
bjope retitled this revision from [IPCP] Don't crash due to arg count mismath between caller/callee to [IPCP] Don't crash due to arg count/type mismatch between caller/callee.Jan 28 2019, 7:45 AM
bjope edited the summary of this revision. (Show Details)
bjope requested review of this revision.Jan 28 2019, 7:50 AM
bjope added a subscriber: eli.friedman.

Please have another look.

I added a check for mismatched types which also would result in an assert (as mentioned by @eli.friedman). Not sure how to reproduce that from C/C++ though, but it was easy to write an LLVM IR reproducer.
I also made some adjustments for varargs that was not there in the earlier reviewed changeset.

Missing testcase for varargs.

bjope updated this revision to Diff 183904.Jan 28 2019, 10:20 AM

Added test cases to verify that we still optmize for VLA (as long as all non-variadic arguments are provided).

Missing testcase for varargs.

Thank for reminding me! I've added both a positive and negative test case using varargs in the latest changeset.

This revision is now accepted and ready to land.Jan 28 2019, 1:14 PM
This revision was automatically updated to reflect the committed changes.