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

[bjope]

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.

[jdoerfert]

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?

[bjope]

In D57052#1366530, @jdoerfert wrote:

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...).

[jdoerfert]

In D57052#1367472, @bjope wrote:

In D57052#1366530, @jdoerfert wrote:

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]

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]

Updates:

• Fix for VarArgs (need to allow more actual than formal arguments).
• Added check for matching argument types (including a new test case).

[bjope]

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.

[efriedma]

Missing testcase for varargs.

[bjope]

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

[bjope]

In D57052#1373806, @efriedma wrote:

Missing testcase for varargs.

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

[efriedma]

LGTM

llvm/trunk/lib/Transforms/IPO/IPConstantPropagation.cpp

for (Use &U : F.uses()) {
     // Ignore blockaddress uses.
     if (isa<BlockAddress>(UR)) continue;
 
     // If no abstract call site was created we did not understand the use, bail.
     AbstractCallSite ACS(&U);
     if (!ACS)
       return false;
 
+    // Mismatched argument count is undefined behavior. Simply bail out to avoid
+    // handling of such situations below (avoiding asserts/crashes).
+    unsigned NumActualArgs = ACS.getNumArgOperands();
+    if (F.isVarArg() ? ArgumentConstants.size() > NumActualArgs
+                     : ArgumentConstants.size() != NumActualArgs)
+      return false;
+
     // Check out all of the potentially constant arguments.  Note that we don't
     // inspect varargs here.
     Function::arg_iterator Arg = F.arg_begin();
     for (unsigned i = 0, e = ArgumentConstants.size(); i != e; ++i, ++Arg) {
 
       // If this argument is known non-constant, ignore it.
       if (ArgumentConstants[i].second)
         continue;
 
       Value *V = ACS.getCallArgOperand(i);
       Constant *C = dyn_cast_or_null<Constant>(V);
 
+      // Mismatched argument type is undefined behavior. Simply bail out to avoid
+      // handling of such situations below (avoiding asserts/crashes).
+      if (C && Arg->getType() != C->getType())
+        return false;
+
       // We can only propagate thread independent values through callbacks.
       // This is different to direct/indirect call sites because for them we
       // know the thread executing the caller and callee is the same. For
       // callbacks this is not guaranteed, thus a thread dependent value could
       // be different for the caller and callee, making it invalid to propagate.
       if (C && ACS.isCallbackCall() && C->isThreadDependent()) {
         // Argument became non-constant. If all arguments are non-constant now,
         // give up on this function.

llvm/trunk/test/Transforms/IPConstantProp/arg-count-mismatch.ll

+; RUN: opt < %s -ipconstprop -S -o - | FileCheck %s
+
+; The original C source looked like this:
+;
+;   long long a101, b101, e101;
+;   volatile long c101;
+;   int d101;
+;
+;   static inline int bar(p1, p2)
+;   {
+;       return 0;
+;   }
+;
+;   void foo(unsigned p1)
+;   {
+;       long long *f = &b101, *g = &e101;
+;       c101 = 0;
+;       (void)((*f |= a101) - (*g = bar(d101)));
+;       c101 = (*f |= a101 &= p1) == d101;
+;   }
+;
+; When compiled with Clang it gives a warning
+;   warning: too few arguments in call to 'bar'
+;
+; This ll reproducer has been reduced to only include tha call.
+;
+; Note that -lint will report this as UB, but it passes -verify.
+
+; This test is just to verify that we do not crash/assert due to mismatch in
+; argument count between the caller and callee.
+
+define dso_local void @foo(i16 %a) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:    [[CALL:%.*]] = call i16 bitcast (i16 (i16, i16)* @bar to i16 (i16)*)(i16 [[A:%.*]])
+; CHECK-NEXT:    ret void
+;
+  %call = call i16 bitcast (i16 (i16, i16) * @bar to i16 (i16) *)(i16 %a)
+  ret void
+}
+
+define internal i16 @bar(i16 %p1, i16 %p2) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:    ret i16 0
+;
+  ret i16 0
+}
+
+;-------------------------------------------------------------------------------
+; Additional tests to verify that we still optimize when having a mismatch
+; in argument count due to varargs (as long as all non-variadic arguments have
+; been provided),
+
+define dso_local void @vararg_tests(i16 %a) {
+  %call1 = call i16 (i16, ...) @vararg_prop(i16 7, i16 8, i16 %a)
+  %call2 = call i16 bitcast (i16 (i16, i16, ...) * @vararg_no_prop to i16 (i16) *) (i16 7)
+  ret void
+}
+
+define internal i16 @vararg_prop(i16 %p1, ...) {
+; CHECK-LABEL: define internal i16 @vararg_prop(
+; CHECK-NEXT:    ret i16 7
+;
+  ret i16 %p1
+}
+
+define internal i16 @vararg_no_prop(i16 %p1, i16 %p2, ...) {
+; CHECK-LABEL: define internal i16 @vararg_no_prop(
+; CHECK-NEXT:    ret i16 [[P1:%.*]]
+;
+  ret i16 %p1
+}
+

llvm/trunk/test/Transforms/IPConstantProp/arg-type-mismatch.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -ipconstprop -S -o - | FileCheck %s
+
+; This test is just to verify that we do not crash/assert due to mismatch in
+; argument type between the caller and callee.
+
+define dso_local void @foo(i16 %a) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:    [[CALL:%.*]] = call i16 bitcast (i16 (i16, i16)* @bar to i16 (i16, i32)*)(i16 [[A:%.*]], i32 7)
+; CHECK-NEXT:    ret void
+;
+  %call = call i16 bitcast (i16 (i16, i16) * @bar to i16 (i16, i32) *)(i16 %a, i32 7)
+  ret void
+}
+
+define internal i16 @bar(i16 %p1, i16 %p2) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:    ret i16 [[P2:%.*]]
+;
+  ret i16 %p2
+}
+
+